Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <2a56b6d4-5f24-9738-ec83-cefb20998c8c@kernel.dk>
Date:   Tue, 30 May 2023 10:50:55 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH 0/7] block layer patches for bcachefs
To:     Kent Overstreet <kent.overstreet@linux.dev>
Cc:     linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
References: <20230525214822.2725616-1-kent.overstreet@linux.dev>
 <ee03b7ce-8257-17f9-f83e-bea2c64aff16@kernel.dk>
 <ZHEaKQH22Uxk9jPK@moria.home.lan>
 <8e874109-db4a-82e3-4020-0596eeabbadf@kernel.dk>
 <ZHYfGvPJFONm58dA@moria.home.lan>
Content-Language: en-US
From:   Jens Axboe <axboe@kernel.dk>
In-Reply-To: <ZHYfGvPJFONm58dA@moria.home.lan>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/30/23 10:06?AM, Kent Overstreet wrote:
> On Tue, May 30, 2023 at 08:22:50AM -0600, Jens Axboe wrote:
>> On 5/26/23 2:44?PM, Kent Overstreet wrote:
>>> On Fri, May 26, 2023 at 08:35:23AM -0600, Jens Axboe wrote:
>>>> On 5/25/23 3:48?PM, Kent Overstreet wrote:
>>>>> Jens, here's the full series of block layer patches needed for bcachefs:
>>>>>
>>>>> Some of these (added exports, zero_fill_bio_iter?) can probably go with
>>>>> the bcachefs pull and I'm just including here for completeness. The main
>>>>> ones are the bio_iter patches, and the __invalidate_super() patch.
>>>>>
>>>>> The bio_iter series has a new documentation patch.
>>>>>
>>>>> I would still like the __invalidate_super() patch to get some review
>>>>> (from VFS people? unclear who owns this).
>>>>
>>>> I wanted to check the code generation for patches 4 and 5, but the
>>>> series doesn't seem to apply to current -git nor my for-6.5/block.
>>>> There's no base commit in this cover letter either, so what is this
>>>> against?
>>>>
>>>> Please send one that applies to for-6.5/block so it's a bit easier
>>>> to take a closer look at this.
>>>
>>> Here you go:
>>> git pull https://evilpiepirate.org/git/bcachefs.git block-for-bcachefs
>>
>> Thanks
>>
>> The re-exporting of helpers is somewhat odd - why is bcachefs special
>> here and needs these, while others do not?
> 
> It's not iomap based.
> 
>> But the main issue for me are the iterator changes, which mostly just
>> seems like unnecessary churn. What's the justification for these? The
>> commit messages don;t really have any. Doesn't seem like much of a
>> simplification, and in fact it's more code than before and obviously
>> more stack usage as well.
> 
> I need bio_for_each_folio().
> 
> The approach taken by the bcachefs IO paths is to first build up bios,
> then walk the extents btree to determine where to send them, splitting
> as needed.
> 
> For reading into the page cache we additionally need to initialize our
> private state based on what we're reading from that says what's on
> disk (unallocated, reservation, or normal allocation) and how many
> replicas. This is used for both i_blocks accounting and for deciding
> when we need to get a disk reservation. Since we're doing this post
> split, it needs bio_for_each_folio, not the _all variant.
> 
> Yes, the iterator changes are a bit more code - but it's split up into
> better helpers now, the pointer arithmetic before was a bit dense; I
> found the result to be more readable. I'm surprised at more stack
> usage; I would have expected _less_ for bio_for_each_page_all() since
> it gets rid of a pointer into the bvec_iter_all. How did you measure
> that?

Sorry typo, I meant text. Just checked stack and it looks identical, but
things like blk-map grows ~6% more text, and bio ~3%. Didn't check all
of them, but at least those two are consistent across x86-64 and
aarch64. Ditto on the data front. Need to take a closer look at where
exactly that is coming from, and what that looks like.

-- 
Jens Axboe