On Tue, May 09, 2023 at 12:50:02PM +0100, fdmanana@xxxxxxxxxx wrote:
> From: Filipe Manana <fdmanana@xxxxxxxx>
>
> 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") because:
>
> 1) 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).
>
> 2) It was also not building a list of inode extent elements (struct
> extent_inode_elem) if we have multiple inode references for an extent
> when the ignore offset flag is given to the logical to ino ioctl - it
> would leave a single element, only the last one that was found.
>
> These stem from the confusing old interface for backref walking functions
> where we had an extent item offset argument that was a pointer to a u64
> and another boolean argument that indicated if the offset should be
> ignored, but the pointer could be NULL. That NULL case is used by
> relocation, qgroup extent accounting and fiemap, simply to avoid building
> the inode extent list for each reference, as it's not necessary for those
> use cases and therefore avoids memory allocations and some computations.
>
> Fix this by adding a boolean argument to the backref walk context
> structure to indicate that the inode extent list should not be built,
> make relocation set that argument to true and fix the backref walking
> logic to skip the calls to check_extent_in_eb() and find_extent_in_eb()
> only if this new argument is true, instead of 'ignore_extent_item_pos'
> being true.
>
> A test case for fstests will be added soon, to provide cover not only
> for these cases but to the logical to ino ioctl in general as well, as
> currently we do not have a test case for it.
>
> Fixes: 6ce6ba534418 ("btrfs: use a single argument for extent offset in backref walking functions")
> Reported-by: Vladimir Panteleev <git@xxxxxxxxxxxxxxxxxxxxx>
> Link: https://lore.kernel.org/linux-btrfs/CAHhfkvwo=nmzrJSqZ2qMfF-rZB-ab6ahHnCD_sq9h4o8v+M7QQ@xxxxxxxxxxxxxx/
> Tested-by: Vladimir Panteleev <git@xxxxxxxxxxxxxxxxxxxxx>
> CC: stable@xxxxxxxxxxxxxxx # 6.2+
> Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
> ---
>
> 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.
>
> V3: Also fix the condition to decide if we should add an inode element to the
> list of inode elements built before. Also rework the fix based on that
> missing part.
Added to misc-next. As this is a user visible bug I'll submit it in
another batch for rc2, but there will be a few days before that in case
you'd need another update. Thanks.
