6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

[Vladimir Panteleev]

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.

Thanks!

Re: 6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

[Filipe Manana]

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 &&
+           !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!

Re: 6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

[Filipe Manana]

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 &&

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!

Re: 6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

[Vladimir Panteleev]

On Wed, 3 May 2023 at 12:49, Filipe Manana <fdmanana@kernel.org> wrote:
>
> On Wed, May 3, 2023 at 1:37 PM Filipe Manana <fdmanana@kernel.org> wrote:
> > 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 &&
>
> I've updated the gist with it:
> https://gist.githubusercontent.com/fdmanana/9ae7f6c62779aacf4bfd3b155d175792/raw/3f41c8486eb73a038f026c8bfe767bd763a016c9/logical_ino2_fix.patch
>
> Thanks.

Yes, it works!

The first version of the patch seemed to work too.

Re: 6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

[Filipe Manana]

On Wed, May 3, 2023 at 2:01 PM Vladimir Panteleev
<git@vladimir.panteleev.md> wrote:
>
> On Wed, 3 May 2023 at 12:49, Filipe Manana <fdmanana@kernel.org> wrote:
> >
> > On Wed, May 3, 2023 at 1:37 PM Filipe Manana <fdmanana@kernel.org> wrote:
> > > 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 &&
> >
> > I've updated the gist with it:
> > https://gist.githubusercontent.com/fdmanana/9ae7f6c62779aacf4bfd3b155d175792/raw/3f41c8486eb73a038f026c8bfe767bd763a016c9/logical_ino2_fix.patch
> >
> > Thanks.
>
> Yes, it works!

Ok, great.
I'll add a changelog and send it to the list.

Thanks for the testing and report.

>
> The first version of the patch seemed to work too.

Re: 6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

[Vladimir Panteleev]

On Wed, 3 May 2023 at 13:06, Filipe Manana <fdmanana@kernel.org> wrote:
> Ok, great.
> I'll add a changelog and send it to the list.
>
> Thanks for the testing and report.

Hi Filipe,

I have done some more testing on my laptop's real filesystem. Good
news: the patch is 85% correct!

Unfortunately however, out of all randomly chosen logical addresses
that previously returned non-zero inodes with
BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET, about 15% still return zero
inodes with the patch (for whatever reason, almost entirely confined
to files in snapshots).

This small change inspired by your patch gets it to 100% for me:

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 799668b35b3c..a59d854db372 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -1604,8 +1604,7 @@ static int find_parent_nodes(struct
btrfs_backref_walk_ctx *ctx,
                 goto out;
         }
         if (ref->count && ref->parent) {
-            if (!ctx->ignore_extent_item_pos && !ref->inode_list &&
-                ref->level == 0) {
+            if (!ref->inode_list && ref->level == 0) {
                 struct btrfs_tree_parent_check check = { 0 };
                 struct extent_buffer *eb;

Hope this helps!

Re: 6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

[Filipe Manana]

On Wed, May 3, 2023 at 10:33 PM Vladimir Panteleev
<git@vladimir.panteleev.md> wrote:
>
> On Wed, 3 May 2023 at 13:06, Filipe Manana <fdmanana@kernel.org> wrote:
> > Ok, great.
> > I'll add a changelog and send it to the list.
> >
> > Thanks for the testing and report.
>
> Hi Filipe,
>
> I have done some more testing on my laptop's real filesystem. Good
> news: the patch is 85% correct!
>
> Unfortunately however, out of all randomly chosen logical addresses
> that previously returned non-zero inodes with
> BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET, about 15% still return zero
> inodes with the patch (for whatever reason, almost entirely confined
> to files in snapshots).
>
> This small change inspired by your patch gets it to 100% for me:

Yes, it's correct. I've integrated it into v2 of the patch (coming
soon), as well as
another update there.

Thanks!

>
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index 799668b35b3c..a59d854db372 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -1604,8 +1604,7 @@ static int find_parent_nodes(struct
> btrfs_backref_walk_ctx *ctx,
>                  goto out;
>          }
>          if (ref->count && ref->parent) {
> -            if (!ctx->ignore_extent_item_pos && !ref->inode_list &&
> -                ref->level == 0) {
> +            if (!ref->inode_list && ref->level == 0) {
>                  struct btrfs_tree_parent_check check = { 0 };
>                  struct extent_buffer *eb;
>
> Hope this helps!

Re: 6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

[Zygo Blaxell]

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?

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.

I think your first patch was the better one.  What am I missing?

> 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!

Re: 6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

[Filipe Manana]

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.
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!

Re: 6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

[Zygo Blaxell]

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!

Re: 6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

[Filipe Manana]

On Wed, May 3, 2023 at 10:57 PM Zygo Blaxell
<ce3g8jdj@umail.furryterror.org> wrote:
>
> 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;

Yes.
I realized that later and left tests running overnight after removing
that and integrating Vladimir's patch.

I think I was originally confused by looking at the old code where
there was an "extent_item_pos" which was a u64 pointer in some
functions and a plain u64 in others.

I removed it from v2.

I'm also writing a test case for fstests to exercise the logical to
ino ioctl, to cover these cases and many others, as unfortunately we
don't have any coverage for it in fstests.

Thanks.

>
> 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!

Re: 6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

[Zygo Blaxell]

On Thu, May 04, 2023 at 11:11:43AM +0100, Filipe Manana wrote:
> On Wed, May 3, 2023 at 10:57 PM Zygo Blaxell
> <ce3g8jdj@umail.furryterror.org> wrote:
> >
> > 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;
> 
> Yes.
> I realized that later and left tests running overnight after removing
> that and integrating Vladimir's patch.
> 
> I think I was originally confused by looking at the old code where
> there was an "extent_item_pos" which was a u64 pointer in some
> functions and a plain u64 in others.
> 
> I removed it from v2.
> 
> I'm also writing a test case for fstests to exercise the logical to
> ino ioctl, to cover these cases and many others, as unfortunately we
> don't have any coverage for it in fstests.

Cool!

Thanks for giving it your attention.

> Thanks.
> 
> >
> > 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!

Re: 6.2 regression: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken

[Linux regression tracking #adding (Thorsten Leemhuis)]

[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]

[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]

On 03.05.23 14:27, Vladimir Panteleev wrote:
> 
> 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.

Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:

#regzbot ^introduced 6ce6ba534418132f4c727d5707fe2794c79729
#regzbot title btrfs: BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET broken
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.