diff mbox series

btrfs: fix backref walking not returning all inode refs

Message ID 77994dd9ede2084d45dd0a36938c67de70d8e859.1683123587.git.fdmanana@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: fix backref walking not returning all inode refs | expand

Commit Message

Filipe Manana May 3, 2023, 2:27 p.m. UTC 
From: Filipe Manana <fdmanana@suse.com>

When using the logical to ino ioctl v2, if the flag to ignore offsets of
file extent items (BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET) is given, the
backref walking code ends up not returning references for all file offsets
of an inode that point to the given logical bytenr. This happens since
kernel 6.2, commit 6ce6ba534418 ("btrfs: use a single argument for extent
offset in backref walking functions"), as it mistakenly skipped the search
for file extent items in a leaf that point to the target extent if that
flag is given. Instead it should only skip the filtering done by
check_extent_in_eb() - that is, it should not avoid the calls to that
function.

So fix this by always calling check_extent_in_eb() and have this function
do the filtering only if an extent offset is given and the flag to ignore
offsets is not set.

Fixes: 6ce6ba534418 ("btrfs: use a single argument for extent offset in backref walking functions")
Reported-by: Vladimir Panteleev <git@vladimir.panteleev.md>
Link: https://lore.kernel.org/linux-btrfs/CAHhfkvwo=nmzrJSqZ2qMfF-rZB-ab6ahHnCD_sq9h4o8v+M7QQ@mail.gmail.com/
Tested-by: Vladimir Panteleev <git@vladimir.panteleev.md>
CC: stable@vger.kernel.org # 6.2+
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/backref.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

Comments

Wang Yugui May 4, 2023, 9:41 a.m. UTC | #1 
Hi,

> From: Filipe Manana <fdmanana@suse.com>
> 
> When using the logical to ino ioctl v2, if the flag to ignore offsets of
> file extent items (BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET) is given, the
> backref walking code ends up not returning references for all file offsets
> of an inode that point to the given logical bytenr. This happens since
> kernel 6.2, commit 6ce6ba534418 ("btrfs: use a single argument for extent
> offset in backref walking functions"), as it mistakenly skipped the search
> for file extent items in a leaf that point to the target extent if that
> flag is given. Instead it should only skip the filtering done by
> check_extent_in_eb() - that is, it should not avoid the calls to that
> function.
> 
> So fix this by always calling check_extent_in_eb() and have this function
> do the filtering only if an extent offset is given and the flag to ignore
> offsets is not set.
> Fixes: 6ce6ba534418 ("btrfs: use a single argument for extent offset in backref walking functions")
> Reported-by: Vladimir Panteleev <git@vladimir.panteleev.md>
> Link: https://lore.kernel.org/linux-btrfs/CAHhfkvwo=nmzrJSqZ2qMfF-rZB-ab6ahHnCD_sq9h4o8v+M7QQ@mail.gmail.com/
> Tested-by: Vladimir Panteleev <git@vladimir.panteleev.md>
> CC: stable@vger.kernel.org # 6.2+
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

fstests(btrfs/299) will fail with this patch.

Best Regards
Wang Yugui (wangyugui@e16-tech.com)
2023/05/04

> ---
>  fs/btrfs/backref.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index e54f0884802a..8e61be3fe9a8 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -45,7 +45,9 @@ 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 &&
> +	    ctx->extent_item_pos > 0 &&
> +	    !btrfs_file_extent_compression(eb, fi) &&
>  	    !btrfs_file_extent_encryption(eb, fi) &&
>  	    !btrfs_file_extent_other_encoding(eb, fi)) {
>  		u64 data_offset;
> @@ -552,13 +554,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);
> -- 
> 2.35.1
Filipe Manana May 4, 2023, 10:13 a.m. UTC | #2 
On Thu, May 4, 2023 at 10:41 AM Wang Yugui <wangyugui@e16-tech.com> wrote:
>
> Hi,
>
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > When using the logical to ino ioctl v2, if the flag to ignore offsets of
> > file extent items (BTRFS_LOGICAL_INO_ARGS_IGNORE_OFFSET) is given, the
> > backref walking code ends up not returning references for all file offsets
> > of an inode that point to the given logical bytenr. This happens since
> > kernel 6.2, commit 6ce6ba534418 ("btrfs: use a single argument for extent
> > offset in backref walking functions"), as it mistakenly skipped the search
> > for file extent items in a leaf that point to the target extent if that
> > flag is given. Instead it should only skip the filtering done by
> > check_extent_in_eb() - that is, it should not avoid the calls to that
> > function.
> >
> > So fix this by always calling check_extent_in_eb() and have this function
> > do the filtering only if an extent offset is given and the flag to ignore
> > offsets is not set.
> > Fixes: 6ce6ba534418 ("btrfs: use a single argument for extent offset in backref walking functions")
> > Reported-by: Vladimir Panteleev <git@vladimir.panteleev.md>
> > Link: https://lore.kernel.org/linux-btrfs/CAHhfkvwo=nmzrJSqZ2qMfF-rZB-ab6ahHnCD_sq9h4o8v+M7QQ@mail.gmail.com/
> > Tested-by: Vladimir Panteleev <git@vladimir.panteleev.md>
> > CC: stable@vger.kernel.org # 6.2+
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
>
> fstests(btrfs/299) will fail with this patch.

Yes, I noticed it later, and it is fixed in v2.
Thanks.

>
> Best Regards
> Wang Yugui (wangyugui@e16-tech.com)
> 2023/05/04
>
> > ---
> >  fs/btrfs/backref.c | 15 +++++++--------
> >  1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> > index e54f0884802a..8e61be3fe9a8 100644
> > --- a/fs/btrfs/backref.c
> > +++ b/fs/btrfs/backref.c
> > @@ -45,7 +45,9 @@ 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 &&
> > +         ctx->extent_item_pos > 0 &&
> > +         !btrfs_file_extent_compression(eb, fi) &&
> >           !btrfs_file_extent_encryption(eb, fi) &&
> >           !btrfs_file_extent_other_encoding(eb, fi)) {
> >               u64 data_offset;
> > @@ -552,13 +554,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);
> > --
> > 2.35.1
>
>
diff mbox series

Patch

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index e54f0884802a..8e61be3fe9a8 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -45,7 +45,9 @@  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 &&
+	    ctx->extent_item_pos > 0 &&
+	    !btrfs_file_extent_compression(eb, fi) &&
 	    !btrfs_file_extent_encryption(eb, fi) &&
 	    !btrfs_file_extent_other_encoding(eb, fi)) {
 		u64 data_offset;
@@ -552,13 +554,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);