Re: 6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

[Zygo Blaxell <ce3g8jdj@umail.furryterror.org>]

On Wed, May 03, 2023 at 04:56:39PM +0100, Filipe Manana wrote:
> On Wed, May 3, 2023 at 4:40 PM Zygo Blaxell
> <ce3g8jdj@umail.furryterror.org> wrote:
> >
> > On Wed, May 03, 2023 at 01:49:16PM +0100, Filipe Manana wrote:
> > > On Wed, May 3, 2023 at 1:37 PM Filipe Manana <fdmanana@kernel.org> wrote:
> > > >
> > > > On Wed, May 3, 2023 at 1:33 PM Vladimir Panteleev
> > > > <git@vladimir.panteleev.md> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > Commit 6ce6ba534418132f4c727d5707fe2794c797299c appears to have broken
> > > > > the BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET flag to
> > > > > BTRFS_IOC_LOGICAL_INO_V2. The ioctl now always seems to return zero
> > > > > inodes with the flag, if the same happened without the flag, thus
> > > > > making it not very useful.
> > > > >
> > > > > Context: I maintain btdu, a disk usage profiler for btrfs. It uses
> > > > > BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET to help users estimate the amount
> > > > > of space wasted by bookend extents, and identify files / applications
> > > > > / IO patterns which create excessive amounts of them.
> > > >
> > > > Are you able to apply and test a kernel patch?
> > > >
> > > > If so, try the following one (also at:
> > > > https://gist.github.com/fdmanana/9ae7f6c62779aacf4bfd3b155d175792)
> > > >
> > > > diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> > > > index e54f0884802a..c4c5784e897a 100644
> > > > --- a/fs/btrfs/backref.c
> > > > +++ b/fs/btrfs/backref.c
> > > > @@ -45,7 +45,8 @@ static int check_extent_in_eb(struct
> > > > btrfs_backref_walk_ctx *ctx,
> > > >         int root_count;
> > > >         bool cached;
> > > >
> > > > -       if (!btrfs_file_extent_compression(eb, fi) &&
> > > > +       if (!ctx->ignore_extent_item_pos &&
> > >
> > > This misses a:
> > >
> > > .. && ctx->extent_item_pos > 0 &&
> >
> > Ummm...why?
> 
> Because if it's 0 it will trigger an underflow at check_extent_in_eb():
> 
> offset += ctx->extent_item_pos - data_offset;
> 
> if the file extent item's data offset is > 0.
> So the filtering must happen only if the offset is not zero, and it was computed
> earlier at iterate_inodes_from_logical().
> 
> > With IGNORE_OFFSET set, we want to ignore the offset on the candidate
> > matching extent and on the original search bytenr, so we get matches in
> > cases where the search bytenr happens to be at offset 0 in the extent.
> 
> And that's what happens with the patch.

That's true, but the real reason why it works for the IGNORE_OFFSET case
is that I misread the condition:

	if (!ctx->ignore_extent_item_pos && 
		// nothing else matters 
		// because !ctx->ignore_extent_item_pos is false,
		// so the rest of the && aren't evaluated, or the
		// if true branch

so the rest of the code carries on using the extent's bytenr as 'offset'.

The true branch of the 'if' is only relevant if we are _not_ ignoring
the offset, since that is where the offset gets checked against the
file extent item.  In that case, the target block could be on either
side of the file extent item's reference range (below data_offset,
or above data_offset + data_length) as well as inside the range.
The outside-the-range case is handled here:

                data_offset = btrfs_file_extent_offset(eb, fi);

                if (ctx->extent_item_pos < data_offset ||
                    ctx->extent_item_pos >= data_offset + data_len)
                        return 1;

so we don't get to the line you mentioned:

	offset += ctx->extent_item_pos - data_offset;

ctx->extent_item_pos >= data_offset and
ctx->extent_item_pos < data_offset + data_len, so
offset + ctx->extent_item_pos - data_offset must be within the extent
(assuming the referencing file_extent_item isn't busted).

> Any file extent item that points to the target bytenr, will be
> considered when the ignore flag is given.
> 
> > I think your first patch was the better one.  What am I missing?
> 
> I think the above explains it.
> 
> >
> > > I've updated the gist with it:
> > > https://gist.githubusercontent.com/fdmanana/9ae7f6c62779aacf4bfd3b155d175792/raw/3f41c8486eb73a038f026c8bfe767bd763a016c9/logical_ino2_fix.patch
> > >
> > > Thanks.
> > >
> > > > +           !btrfs_file_extent_compression(eb, fi) &&
> > > >             !btrfs_file_extent_encryption(eb, fi) &&
> > > >             !btrfs_file_extent_other_encoding(eb, fi)) {
> > > >                 u64 data_offset;
> > > > @@ -552,13 +553,10 @@ static int add_all_parents(struct
> > > > btrfs_backref_walk_ctx *ctx,
> > > >                                 count++;
> > > >                         else
> > > >                                 goto next;
> > > > -                       if (!ctx->ignore_extent_item_pos) {
> > > > -                               ret = check_extent_in_eb(ctx, &key,
> > > > eb, fi, &eie);
> > > > -                               if (ret == BTRFS_ITERATE_EXTENT_INODES_STOP ||
> > > > -                                   ret < 0)
> > > > -                                       break;
> > > > -                       }
> > > > -                       if (ret > 0)
> > > > +                       ret = check_extent_in_eb(ctx, &key, eb, fi, &eie);
> > > > +                       if (ret == BTRFS_ITERATE_EXTENT_INODES_STOP || ret < 0)
> > > > +                               break;
> > > > +                       else if (ret > 0)
> > > >                                 goto next;
> > > >                         ret = ulist_add_merge_ptr(parents, eb->start,
> > > >                                                   eie, (void **)&old, GFP_NOFS);
> > > >
> > > > Thanks.
> > > > >
> > > > > Thanks!