diff mbox series

[v2] btrfs: fix backref walking not returning all inode refs

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

Commit Message

Filipe Manana May 4, 2023, 10:12 a.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 (or find_extent_in_eb(), which uses it).

So fix this by always calling check_extent_in_eb() and find_extent_in_eb()
and have check_extent_in_eb() do the filtering only if the flag to ignore
offsets is 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>
---

V2: Remove wrong check for a non-zero extent item offset.
    Apply the same logic at find_parent_nodes(), that is, search for file
    extent items on a leaf if the ignore flag is given - the filtering
    will be done later at check_extent_in_eb(). Spotted by Vladimir Panteleev
    in the thread mentioned above.

 fs/btrfs/backref.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

Comments

David Sterba May 5, 2023, 10:03 a.m. UTC | #1 
On Thu, May 04, 2023 at 11:12:03AM +0100, fdmanana@kernel.org wrote:
> 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 (or find_extent_in_eb(), which uses it).
> 
> So fix this by always calling check_extent_in_eb() and find_extent_in_eb()
> and have check_extent_in_eb() do the filtering only if the flag to ignore
> offsets is 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>
> ---
> 
> V2: Remove wrong check for a non-zero extent item offset.
>     Apply the same logic at find_parent_nodes(), that is, search for file
>     extent items on a leaf if the ignore flag is given - the filtering
>     will be done later at check_extent_in_eb(). Spotted by Vladimir Panteleev
>     in the thread mentioned above.

Replaced in misc-next, thanks for the quick fix.
Filipe Manana May 8, 2023, 7:51 p.m. UTC | #2 
On Fri, May 5, 2023 at 11:09 AM David Sterba <dsterba@suse.cz> wrote:
>
> On Thu, May 04, 2023 at 11:12:03AM +0100, fdmanana@kernel.org wrote:
> > 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 (or find_extent_in_eb(), which uses it).
> >
> > So fix this by always calling check_extent_in_eb() and find_extent_in_eb()
> > and have check_extent_in_eb() do the filtering only if the flag to ignore
> > offsets is 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>
> > ---
> >
> > V2: Remove wrong check for a non-zero extent item offset.
> >     Apply the same logic at find_parent_nodes(), that is, search for file
> >     extent items on a leaf if the ignore flag is given - the filtering
> >     will be done later at check_extent_in_eb(). Spotted by Vladimir Panteleev
> >     in the thread mentioned above.
>
> Replaced in misc-next, thanks for the quick fix.

Can you please remove it in the meanwhile?
I noticed this isn't quite right and there's still two cases not
working as they should be.
I'll send a v3 after finishing some more tests, probably tomorrow if
everything goes fine.

Thanks.
David Sterba May 9, 2023, 10:47 a.m. UTC | #3 
On Mon, May 08, 2023 at 08:51:06PM +0100, Filipe Manana wrote:
> On Fri, May 5, 2023 at 11:09 AM David Sterba <dsterba@suse.cz> wrote:
> >
> > On Thu, May 04, 2023 at 11:12:03AM +0100, fdmanana@kernel.org wrote:
> > > 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 (or find_extent_in_eb(), which uses it).
> > >
> > > So fix this by always calling check_extent_in_eb() and find_extent_in_eb()
> > > and have check_extent_in_eb() do the filtering only if the flag to ignore
> > > offsets is 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>
> > > ---
> > >
> > > V2: Remove wrong check for a non-zero extent item offset.
> > >     Apply the same logic at find_parent_nodes(), that is, search for file
> > >     extent items on a leaf if the ignore flag is given - the filtering
> > >     will be done later at check_extent_in_eb(). Spotted by Vladimir Panteleev
> > >     in the thread mentioned above.
> >
> > Replaced in misc-next, thanks for the quick fix.
> 
> Can you please remove it in the meanwhile?
> I noticed this isn't quite right and there's still two cases not
> working as they should be.
> I'll send a v3 after finishing some more tests, probably tomorrow if
> everything goes fine.

Ok, removed and pushed.
diff mbox series

Patch

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index e54f0884802a..787417f9893c 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);
@@ -1606,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;