Re: [PATCH] btrfs: relocation: output warning message for leftover v1 space cache before aborting current data balance

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2020/12/29 下午5:27, Stéphane Lesimple wrote:
> December 29, 2020 1:38 AM, "Qu Wenruo" <wqu@suse.com> wrote:
>
>> In delete_v1_space_cache(), if we find a leaf whose owner is tree root,
>> and we can't grab the free space cache inode, then we return -ENOENT.
>>
>> However this would make the caller, add_data_references(), to consider
>> this as a critical error, and abort current data balance.
>>
>> This happens for fs using free space cache v2, while still has v1 data
>> left.
>>
>> For v2 free space cache, we no longer load v1 data, making btrfs_igrab()
>> no longer work for root tree to grab v1 free space cache inodes.
>>
>> The proper fix for the problem is to delete v1 space cache completely
>> during v2 convert.
>>
>> We can't just ignore the -ENOENT error, as for root tree we don't use
>> reloc tree to replace its data references, but rely on COW.
>> This means, we have no way to relocate the leftover v1 data, and block
>> the relocation.
>>
>> This patch will just workaround it by outputting a warning message,
>> showing the user how to manually solve it.
>>
>> Reported-by: Stéphane Lesimple <stephane_btrfs2@lesimple.fr>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> Your analysis seems correct, as this FS is quite old (several years),
> and has seen quite a number of kernel versions! I converted it to
> space_cache v2 ~6-12 months ago I think. It does has v1 leftovers:
>
> # btrfs ins dump-tree -t root /dev/mapper/luks-tank-mdata | grep EXTENT_DA
> item 27 key (51933 EXTENT_DATA 0) itemoff 9854 itemsize 53
> item 12 key (72271 EXTENT_DATA 0) itemoff 14310 itemsize 53
> item 25 key (74907 EXTENT_DATA 0) itemoff 12230 itemsize 53

Mind to dump all those related inodes?

E.g:

$ btrfs ins dump-tree -t root <dev> | gerp 51933 -C 10

The point here is to show if we have INODE_ITEM here for the inode number.

If there is really no INODE_ITEM, but have EXTENT_DATA left, then there
is something wrong with the v1 cache deletion.
That would explain the problem you're hitting.

Either caused by kernel or btrfs-progs, we need to further pin down the
cause of course.
For any reason, this should be considered as a corruption, which is
pretty dangerous (deleting inode item without deleting all its content,
definitely a big NO-NO).

>
> What's interesting also is that a FS I created only a few weeks ago,
> under kernel 5.6.17, also has v1 leftovers, as per the above command.
> So the issue might be more common than we think (not just years-old FS).
>
> Before fixing the FS I can't balance, I wanted to test your patch,
> even if it's pretty straightforward, just to be sure:
>
> # btrfs bal start -dvrange=34625344765952..34625344765953 /tank
> ERROR: error during balancing '/tank': No such file or directory
> There may be more info in syslog - try dmesg | tail
>
> [   76.114187] BTRFS info (device dm-10): balance: start -dvrange=34625344765952..34625344765953
> [   76.122792] BTRFS info (device dm-10): relocating block group 34625344765952 flags data|raid1
> [   87.065468] BTRFS info (device dm-10): found 167 extents, stage: move data extents
> [   87.685571] BTRFS warning (device dm-10): leftover v1 space cache found, please use btrfs-check --clear-space-cache v1 to clean it up

Although above ins dump should work, I just want to be extra sure about
where the -ENOENT comes from.

Would you mind to test the following debug output?

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 19b7db8b2117..f7f3682ce017 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2932,8 +2932,10 @@ static int delete_block_group_cache(struct
btrfs_fs_info *fs_info,
                 goto truncate;

         inode = btrfs_iget(fs_info->sb, ino, root);
-       if (IS_ERR(inode))
+       if (IS_ERR(inode)) {
+               pr_info("%s: no inode item found for ino %llu\n",
__func__, ino);
                 return -ENOENT;
+       }

  truncate:
         ret = btrfs_check_trunc_cache_free_space(fs_info,
@@ -2986,8 +2988,11 @@ static int delete_v1_space_cache(struct
extent_buffer *leaf,
                         break;
                 }
         }
-       if (!found)
+       if (!found) {
+               pr_info("%s: no FILE_EXTENT found, leaf start=%llu
data_bytenr=%llu\n",
+                       __func__, leaf->start, data_bytenr);
                 return -ENOENT;
+       }
         ret = delete_block_group_cache(leaf->fs_info, block_group, NULL,
                                         space_cache_ino);
         return ret;


My guess is, the dump would show no INODE_ITEM, and the debug output
would print something like "delete_block_group_cache: no inode item
found for XXXXXX".

Thanks,
Qu
> [  100.018692] BTRFS info (device dm-10): balance: ended with status: -2
>
> So, it works. You can add my Tested-By.
>
> Regards,
>
> Stéphane.
>