[PATCH] btrfs: set start on clone before calling copy_extent_buffer_full

[PATCH] btrfs: set start on clone before calling copy_extent_buffer_full

[Josef Bacik]

Our subpage testing started hanging on generic/560 and I bisected it
down to Fixes: 1cab1375ba6d ("btrfs: reuse cloned extent buffer during
fiemap to avoid re-allocations").  This is subtle because we use
eb->start to figure out where in the folio we're copying to when we're
subpage, as our ->start may refer to an area inside of the folio.

We were copying with ->start set to the previous value, and then
re-setting ->start in order to be used later on by fiemap.  However this
changed the offset into the eb that we would read from, which would
cause us to not emit EOF sometimes for fiemap.  Thanks to a bug in the
duperemove that the CI vms are using this manifested as a hung test.

Fix this by setting start before we co copy_extent_buffer_full to make
sure that we're copying into the same offset inside of the folio that we
will read from later.

With this fix we now pass generic/560 on our subpage tests.

Fixes: 1cab1375ba6d ("btrfs: reuse cloned extent buffer during fiemap toavoid re-allocations")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent_io.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 49f7161a6578..a3d0befaa461 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2809,13 +2809,17 @@ static int fiemap_next_leaf_item(struct btrfs_inode *inode, struct btrfs_path *p
 		goto out;
 	}
 
-	/* See the comment at fiemap_search_slot() about why we clone. */
-	copy_extent_buffer_full(clone, path->nodes[0]);
 	/*
 	 * Important to preserve the start field, for the optimizations when
 	 * checking if extents are shared (see extent_fiemap()).
+	 *
+	 * Additionally it needs to be set before we call
+	 * copy_extent_buffer_full because for subpagesize we need to make sure
+	 * we have the correctly calculated offset.
 	 */
 	clone->start = path->nodes[0]->start;
+	/* See the comment at fiemap_search_slot() about why we clone. */
+	copy_extent_buffer_full(clone, path->nodes[0]);
 
 	slot = path->slots[0];
 	btrfs_release_path(path);
-- 
2.43.0

Re: [PATCH] btrfs: set start on clone before calling copy_extent_buffer_full

[Filipe Manana]

On Sun, Apr 14, 2024 at 6:50 AM Josef Bacik <josef@toxicpanda.com> wrote:
>
> Our subpage testing started hanging on generic/560 and I bisected it
> down to Fixes: 1cab1375ba6d ("btrfs: reuse cloned extent buffer during

The "Fixes:" should be here, seems like a copy paste mistake from the
bottom of the change log.

> fiemap to avoid re-allocations").  This is subtle because we use
> eb->start to figure out where in the folio we're copying to when we're
> subpage, as our ->start may refer to an area inside of the folio.

Where is that exactly? Is it at get_eb_offset_in_folio()? If it's that
I don't see why it's subpage specific.
Can you mention where exactly in the change log?

>
> We were copying with ->start set to the previous value, and then
> re-setting ->start in order to be used later on by fiemap.  However this
> changed the offset into the eb that we would read from, which would

I don't understand this part that says: "we would read from".
We would read what and where? Are you saying we need to read from the
destination eb during copy_full_extent_buffer()?
Where is that?

Does this only affect copy_extent_buffer_full()? Doesn't it affect
copy_extent_buffer() too? If not, why?
Can you please give all these details in the changelog?

> cause us to not emit EOF sometimes for fiemap.  Thanks to a bug in the
> duperemove that the CI vms are using this manifested as a hung test.
>
> Fix this by setting start before we co copy_extent_buffer_full to make
> sure that we're copying into the same offset inside of the folio that we
> will read from later.
>
> With this fix we now pass generic/560 on our subpage tests.
>
> Fixes: 1cab1375ba6d ("btrfs: reuse cloned extent buffer during fiemap toavoid re-allocations")

Missing a space at "toavoid", in the commit's subject there's actually a space.

> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/extent_io.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 49f7161a6578..a3d0befaa461 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2809,13 +2809,17 @@ static int fiemap_next_leaf_item(struct btrfs_inode *inode, struct btrfs_path *p
>                 goto out;
>         }
>
> -       /* See the comment at fiemap_search_slot() about why we clone. */
> -       copy_extent_buffer_full(clone, path->nodes[0]);
>         /*
>          * Important to preserve the start field, for the optimizations when
>          * checking if extents are shared (see extent_fiemap()).
> +        *
> +        * Additionally it needs to be set before we call
> +        * copy_extent_buffer_full because for subpagesize we need to make sure

Can we get the () in front of the function name, so that it's
consistent with the rest of the comment (paragraph above)?

> +        * we have the correctly calculated offset.
>          */
>         clone->start = path->nodes[0]->start;
> +       /* See the comment at fiemap_search_slot() about why we clone. */
> +       copy_extent_buffer_full(clone, path->nodes[0]);

Ok so this is a landmine and I doubt people will remember to do this
when using copy_extent_buffer_full() in the future.
If this is really needed, why not do that at copy_extent_buffer_full()
itself? This would be more future proof.

Why wouldn't we need this in other places that use
copy_extent_buffer_full() too?

For example in the tree log code where we use a dummy eb and we don't
update the eb's ->start because we don't care about it. Why is it not
a problem there? Or you missed it?

Or another example at btrfs_copy_root(). where we can't obviously set
the destination eb's ->start to that of the source eb. Why don't we
run into any problem there?

I'm puzzled at why we need to this ->start update only in this place,
why the ->start of the destination eb of copy_extent_buffer_full() is
used or why it causes a problem, why is it subpage specific

Thanks.


>
>         slot = path->slots[0];
>         btrfs_release_path(path);
> --
> 2.43.0
>
>

Re: [PATCH] btrfs: set start on clone before calling copy_extent_buffer_full

[Josef Bacik]

On Sun, Apr 14, 2024 at 11:22:17AM +0100, Filipe Manana wrote:
> On Sun, Apr 14, 2024 at 6:50 AM Josef Bacik <josef@toxicpanda.com> wrote:
> >
> > Our subpage testing started hanging on generic/560 and I bisected it
> > down to Fixes: 1cab1375ba6d ("btrfs: reuse cloned extent buffer during
> 
> The "Fixes:" should be here, seems like a copy paste mistake from the
> bottom of the change log.
> 
> > fiemap to avoid re-allocations").  This is subtle because we use
> > eb->start to figure out where in the folio we're copying to when we're
> > subpage, as our ->start may refer to an area inside of the folio.
> 
> Where is that exactly? Is it at get_eb_offset_in_folio()? If it's that
> I don't see why it's subpage specific.
> Can you mention where exactly in the change log?
> 
> >
> > We were copying with ->start set to the previous value, and then
> > re-setting ->start in order to be used later on by fiemap.  However this
> > changed the offset into the eb that we would read from, which would
> 
> I don't understand this part that says: "we would read from".
> We would read what and where? Are you saying we need to read from the
> destination eb during copy_full_extent_buffer()?
> Where is that?
> 
> Does this only affect copy_extent_buffer_full()? Doesn't it affect
> copy_extent_buffer() too? If not, why?
> Can you please give all these details in the changelog?
> 
> > cause us to not emit EOF sometimes for fiemap.  Thanks to a bug in the
> > duperemove that the CI vms are using this manifested as a hung test.
> >
> > Fix this by setting start before we co copy_extent_buffer_full to make
> > sure that we're copying into the same offset inside of the folio that we
> > will read from later.
> >
> > With this fix we now pass generic/560 on our subpage tests.
> >
> > Fixes: 1cab1375ba6d ("btrfs: reuse cloned extent buffer during fiemap toavoid re-allocations")
> 
> Missing a space at "toavoid", in the commit's subject there's actually a space.
> 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/btrfs/extent_io.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 49f7161a6578..a3d0befaa461 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -2809,13 +2809,17 @@ static int fiemap_next_leaf_item(struct btrfs_inode *inode, struct btrfs_path *p
> >                 goto out;
> >         }
> >
> > -       /* See the comment at fiemap_search_slot() about why we clone. */
> > -       copy_extent_buffer_full(clone, path->nodes[0]);
> >         /*
> >          * Important to preserve the start field, for the optimizations when
> >          * checking if extents are shared (see extent_fiemap()).
> > +        *
> > +        * Additionally it needs to be set before we call
> > +        * copy_extent_buffer_full because for subpagesize we need to make sure
> 
> Can we get the () in front of the function name, so that it's
> consistent with the rest of the comment (paragraph above)?
> 
> > +        * we have the correctly calculated offset.
> >          */
> >         clone->start = path->nodes[0]->start;
> > +       /* See the comment at fiemap_search_slot() about why we clone. */
> > +       copy_extent_buffer_full(clone, path->nodes[0]);
> 
> Ok so this is a landmine and I doubt people will remember to do this
> when using copy_extent_buffer_full() in the future.
> If this is really needed, why not do that at copy_extent_buffer_full()
> itself? This would be more future proof.
> 
> Why wouldn't we need this in other places that use
> copy_extent_buffer_full() too?
> 
> For example in the tree log code where we use a dummy eb and we don't
> update the eb's ->start because we don't care about it. Why is it not
> a problem there? Or you missed it?
> 
> Or another example at btrfs_copy_root(). where we can't obviously set
> the destination eb's ->start to that of the source eb. Why don't we
> run into any problem there?
> 
> I'm puzzled at why we need to this ->start update only in this place,
> why the ->start of the destination eb of copy_extent_buffer_full() is
> used or why it causes a problem, why is it subpage specific

Sorry, 2am changelogs aren't great.

In __write_extent_buffer() we do

offset = get_eb_offset_in_folio(eb, start);

start is the logical offset into the eb we're copying to, in this case it's 0
because we're doing copy_extent_buffer_full().  get_eb_offset_in_folio() does
this

        return offset_in_folio(eb->folios[0], offset + eb->start);

and offset_in_folio() does this

        return start & (eb->folio_size - 1);

so in this case offset is 0, so we're just passing in eb->start.

With subpage ->start can be not folio_size aligned, so this would be some
arbitrary offset into the folio.

The other places aren't a problem because we don't change eb->start after we
do the copy.  This is a problem because we're re-using a cloned eb, so we do
this

// start is 4k on a 16k pagesize fs, so we're at the 4k offset into the folio
copy_extent_buffer_full();

// now start is at 8k offset into the folio
cloned->start = 8k;

Any subsequent reads from the eb, by which I mean things like
btrfs_item_key_to_cpu(), anything we read the members out of the leaf are going
to use the same get_eb_offset_in_folio() helper and now come up with a different
answer than where we copied into.

All other places are fine, because we don't change ->start after we've copied
data into it.  I'll update the changelog to include this information, thanks,

Josef