| Message ID | 20230525214822.2725616-1-kent.overstreet@linux.dev (mailing list archive) |
|---|---|
| Headers | show |
| Series | block layer patches for bcachefs | expand |
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.
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 changed folio_vec to folio_seg, build tested it and just pointed my CI at it, results will be showing up for xfs at https://evilpiepirate.org/~testdashboard/ci?branch=block-for-bcachefs
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? 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.
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?
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.
On Tue, May 30, 2023 at 10:50:55AM -0600, Jens Axboe wrote: > 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. Weird - when I looked kernel text size was unchanged, it was data that increased with the earlier version of the patch due to a new WARN_ON(). I'll have another look.
On Tue, May 30, 2023 at 10:50:55AM -0600, Jens Axboe wrote: > 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. A good chunk of that is because I added warnings and assertions for e.g. running past the end of the bvec array. These bugs are rare and shouldn't happen with normal iterator usage (e.g. the bio_for_each_* macros), but I'd like to keep them as a debug mode thing. But we don't yet have CONFIG_BLOCK_DEBUG - perhaps we should. With those out, I see a code size decrease in bio.c, which makes sense - gcc ought to be able to generate slightly better code when it's dealing with pure values, provided everything is inlined and there's no aliasing considerations. Onto blk-map.c: bio_copy_kern_endio_read() increases in code size, but if I change memcpy_from_bvec() to take the bvec by val instead of by ref it's basically the same code size. There's no disadvantage to changing memcpy_from_bvec() to pass by val. bio_copy_(to|from)_iter() is a wtf, though - gcc is now spilling the constructed bvec to the stack; my best guess is it's a register pressure thing (but we shouldn't be short registers here!). So, since the fastpath stuff in bio.c gets smaller and blk-map.c is not exactly fastpath stuff I'm not inclined to fight with gcc on this one - let me know if that works for you. Branch is updated - I split out the new assertions into a separate patch that adds CONFIG_BLK_DEBUG, and another patch for mempcy_(to|from)_bio() for a small code size decrease. https://evilpiepirate.org/git/bcachefs.git/log/?h=block-for-bcachefs or git pull http://evilpiepirate.org/git/bcachefs.git block-for-bcachefs
On 6/4/23 5:38?PM, Kent Overstreet wrote: > On Tue, May 30, 2023 at 10:50:55AM -0600, Jens Axboe wrote: >> 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. > > A good chunk of that is because I added warnings and assertions for > e.g. running past the end of the bvec array. These bugs are rare and > shouldn't happen with normal iterator usage (e.g. the bio_for_each_* > macros), but I'd like to keep them as a debug mode thing. > > But we don't yet have CONFIG_BLOCK_DEBUG - perhaps we should. Let's split those out then, especially as we don't have a BLOCK_DEBUG option right now. > With those out, I see a code size decrease in bio.c, which makes sense - > gcc ought to be able to generate slightly better code when it's dealing > with pure values, provided everything is inlined and there's no aliasing > considerations. > > Onto blk-map.c: > > bio_copy_kern_endio_read() increases in code size, but if I change > memcpy_from_bvec() to take the bvec by val instead of by ref it's > basically the same code size. There's no disadvantage to changing > memcpy_from_bvec() to pass by val. > > bio_copy_(to|from)_iter() is a wtf, though - gcc is now spilling the > constructed bvec to the stack; my best guess is it's a register pressure > thing (but we shouldn't be short registers here!). > > So, since the fastpath stuff in bio.c gets smaller and blk-map.c is not > exactly fastpath stuff I'm not inclined to fight with gcc on this one - > let me know if that works for you. > > Branch is updated - I split out the new assertions into a separate patch > that adds CONFIG_BLK_DEBUG, and another patch for mempcy_(to|from)_bio() > for a small code size decrease. > > https://evilpiepirate.org/git/bcachefs.git/log/?h=block-for-bcachefs > or > git pull http://evilpiepirate.org/git/bcachefs.git block-for-bcachefs Cn you resend just the iterator changes in their current form? The various re-exports are a separate discussion, I think we should focus on the iterator bits first.
On Mon, Jun 05, 2023 at 10:49:37AM -0600, Jens Axboe wrote: > On 6/4/23 5:38?PM, Kent Overstreet wrote: > > On Tue, May 30, 2023 at 10:50:55AM -0600, Jens Axboe wrote: > >> 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. > > > > A good chunk of that is because I added warnings and assertions for > > e.g. running past the end of the bvec array. These bugs are rare and > > shouldn't happen with normal iterator usage (e.g. the bio_for_each_* > > macros), but I'd like to keep them as a debug mode thing. > > > > But we don't yet have CONFIG_BLOCK_DEBUG - perhaps we should. > > Let's split those out then, especially as we don't have a BLOCK_DEBUG > option right now. Already did that; there's a patch in the branch that adds CONFIG_BLK_DEBUG with the new assertions. > Cn you resend just the iterator changes in their current form? The > various re-exports are a separate discussion, I think we should focus on > the iterator bits first. They're up in that branch with the iterator changes first now; I'll mail them out too.