Re: [PATCH 0/7] block layer patches for bcachefs

From: Kent Overstreet <kent.overstreet@linux.dev>
To: Jens Axboe <axboe@kernel.dk>
Cc: linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 0/7] block layer patches for bcachefs
Date: Tue, 30 May 2023 12:06:50 -0400	[thread overview]
Message-ID: <ZHYfGvPJFONm58dA@moria.home.lan> (raw)
In-Reply-To: <8e874109-db4a-82e3-4020-0596eeabbadf@kernel.dk>

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?