diff mbox series

btrfs: relocation: fix wrong file extent type check to avoid false -ENOENT error

Message ID 20201229132934.117325-1-wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: relocation: fix wrong file extent type check to avoid false -ENOENT error | expand

Commit Message

Qu Wenruo Dec. 29, 2020, 1:29 p.m. UTC
[BUG]
There are several bug reports about recent kernel unable to relocate
certain data block groups.

Sometimes the error just go away, but there is one reporter who can
reproduce it reliably.

The dmesg would look like:
[  438.260483] BTRFS info (device dm-10): balance: start -dvrange=34625344765952..34625344765953
[  438.269018] BTRFS info (device dm-10): relocating block group 34625344765952 flags data|raid1
[  450.439609] BTRFS info (device dm-10): found 167 extents, stage: move data extents
[  463.501781] BTRFS info (device dm-10): balance: ended with status: -2

[CAUSE]
The -ENOENT error is returned from the following chall chain:

add_data_references()
|- delete_v1_space_cache();
   |- if (!found)
         return -ENOENT;

The variable @found is set to true if we find a data extent whose
disk bytenr matches parameter @data_bytes.

With extra debug, the offending tree block looks like this:
  leaf bytenr = 42676709441536, data_bytenr = 34626327621632

                ctime 1567904822.739884119 (2019-09-08 03:07:02)
                mtime 0.0 (1970-01-01 01:00:00)
                otime 0.0 (1970-01-01 01:00:00)
        item 27 key (51933 EXTENT_DATA 0) itemoff 9854 itemsize 53
                generation 1517381 type 2 (prealloc)
                prealloc data disk byte 34626327621632 nr 262144 <<<
                prealloc data offset 0 nr 262144
        item 28 key (52262 ROOT_ITEM 0) itemoff 9415 itemsize 439
                generation 2618893 root_dirid 256 bytenr 42677048360960 level 3 refs 1
                lastsnap 2618893 byte_limit 0 bytes_used 5557338112 flags 0x0(none)
                uuid d0d4361f-d231-6d40-8901-fe506e4b2b53

Although item 27 has disk bytenr 34626327621632, which matches the
data_bytenr, its type is prealloc, not reg.
This makes the existing code skip that item, and return -ENOENT.

[FIX]
The code is modified in commit  19b546d7a1b2 ("btrfs: relocation: Use
btrfs_find_all_leafs to locate data extent parent tree leaves"), before
that commit, we use something like
"if (type == BTRFS_FILE_EXTENT_INLINE) continue;".

But in that offending commit, we use (type == BTRFS_FILE_EXTENT_REG),
ignoring BTRFS_FILE_EXTENT_PREALLOC.

Fix it by also checking BTRFS_FILE_EXTENT_PREALLOC.

Reported-by: Stéphane Lesimple <stephane_btrfs2@lesimple.fr>
Fixes: 19b546d7a1b2 ("btrfs: relocation: Use btrfs_find_all_leafs to locate data extent parent tree leaves")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/relocation.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Su Yue Dec. 30, 2020, 7:57 a.m. UTC | #1
On Tue 29 Dec 2020 at 21:29, Qu Wenruo <wqu@suse.com> wrote:

> [BUG]
> There are several bug reports about recent kernel unable to 
> relocate
> certain data block groups.
>
> Sometimes the error just go away, but there is one reporter who 
> can
> reproduce it reliably.
>
> The dmesg would look like:
> [  438.260483] BTRFS info (device dm-10): balance: start 
> -dvrange=34625344765952..34625344765953
> [  438.269018] BTRFS info (device dm-10): relocating block group 
> 34625344765952 flags data|raid1
> [  450.439609] BTRFS info (device dm-10): found 167 extents, 
> stage: move data extents
> [  463.501781] BTRFS info (device dm-10): balance: ended with 
> status: -2
>
> [CAUSE]
> The -ENOENT error is returned from the following chall chain:
>
> add_data_references()
> |- delete_v1_space_cache();
>    |- if (!found)
>          return -ENOENT;
>
> The variable @found is set to true if we find a data extent 
> whose
> disk bytenr matches parameter @data_bytes.
>
> With extra debug, the offending tree block looks like this:
>   leaf bytenr = 42676709441536, data_bytenr = 34626327621632
>
>                 ctime 1567904822.739884119 (2019-09-08 03:07:02)
>                 mtime 0.0 (1970-01-01 01:00:00)
>                 otime 0.0 (1970-01-01 01:00:00)
>         item 27 key (51933 EXTENT_DATA 0) itemoff 9854 itemsize 
>         53
>                 generation 1517381 type 2 (prealloc)
>                 prealloc data disk byte 34626327621632 nr 262144 
>                 <<<
>                 prealloc data offset 0 nr 262144
>         item 28 key (52262 ROOT_ITEM 0) itemoff 9415 itemsize 
>         439
>                 generation 2618893 root_dirid 256 bytenr 
>                 42677048360960 level 3 refs 1
>                 lastsnap 2618893 byte_limit 0 bytes_used 
>                 5557338112 flags 0x0(none)
>                 uuid d0d4361f-d231-6d40-8901-fe506e4b2b53
>
> Although item 27 has disk bytenr 34626327621632, which matches 
> the
> data_bytenr, its type is prealloc, not reg.
> This makes the existing code skip that item, and return -ENOENT.
>
> [FIX]
> The code is modified in commit  19b546d7a1b2 ("btrfs: 
> relocation: Use
> btrfs_find_all_leafs to locate data extent parent tree leaves"), 
> before
> that commit, we use something like
> "if (type == BTRFS_FILE_EXTENT_INLINE) continue;".
>
> But in that offending commit, we use (type == 
> BTRFS_FILE_EXTENT_REG),
> ignoring BTRFS_FILE_EXTENT_PREALLOC.
>
> Fix it by also checking BTRFS_FILE_EXTENT_PREALLOC.
>
> Reported-by: Stéphane Lesimple <stephane_btrfs2@lesimple.fr>
> Fixes: 19b546d7a1b2 ("btrfs: relocation: Use 
> btrfs_find_all_leafs to locate data extent parent tree leaves")
> Signed-off-by: Qu Wenruo <wqu@suse.com>
>

Reviewed-by: Su Yue <l@damenly.su>

> ---
>  fs/btrfs/relocation.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 19b7db8b2117..df63ef64c5c0 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -2975,11 +2975,16 @@ static int delete_v1_space_cache(struct 
> extent_buffer *leaf,
>  		return 0;
>
>  	for (i = 0; i < btrfs_header_nritems(leaf); i++) {
> +		u8 type;
> +
>  		btrfs_item_key_to_cpu(leaf, &key, i);
>  		if (key.type != BTRFS_EXTENT_DATA_KEY)
>  			continue;
>  		ei = btrfs_item_ptr(leaf, i, struct 
>  btrfs_file_extent_item);
> -		if (btrfs_file_extent_type(leaf, ei) == 
> BTRFS_FILE_EXTENT_REG &&
> +		type = btrfs_file_extent_type(leaf, ei);
> +
> +		if ((type == BTRFS_FILE_EXTENT_REG ||
> +		     type == BTRFS_FILE_EXTENT_PREALLOC) &&
>  		    btrfs_file_extent_disk_bytenr(leaf, ei) == 
>  data_bytenr) {
>  			found = true;
>  			space_cache_ino = key.objectid;
David Sterba Jan. 4, 2021, 4:16 p.m. UTC | #2
On Tue, Dec 29, 2020 at 09:29:34PM +0800, Qu Wenruo wrote:
> [BUG]
> There are several bug reports about recent kernel unable to relocate
> certain data block groups.
> 
> Sometimes the error just go away, but there is one reporter who can
> reproduce it reliably.
> 
> The dmesg would look like:
> [  438.260483] BTRFS info (device dm-10): balance: start -dvrange=34625344765952..34625344765953
> [  438.269018] BTRFS info (device dm-10): relocating block group 34625344765952 flags data|raid1
> [  450.439609] BTRFS info (device dm-10): found 167 extents, stage: move data extents
> [  463.501781] BTRFS info (device dm-10): balance: ended with status: -2
> 
> [CAUSE]
> The -ENOENT error is returned from the following chall chain:
> 
> add_data_references()
> |- delete_v1_space_cache();
>    |- if (!found)
>          return -ENOENT;
> 
> The variable @found is set to true if we find a data extent whose
> disk bytenr matches parameter @data_bytes.
> 
> With extra debug, the offending tree block looks like this:
>   leaf bytenr = 42676709441536, data_bytenr = 34626327621632
> 
>                 ctime 1567904822.739884119 (2019-09-08 03:07:02)
>                 mtime 0.0 (1970-01-01 01:00:00)
>                 otime 0.0 (1970-01-01 01:00:00)
>         item 27 key (51933 EXTENT_DATA 0) itemoff 9854 itemsize 53
>                 generation 1517381 type 2 (prealloc)
>                 prealloc data disk byte 34626327621632 nr 262144 <<<
>                 prealloc data offset 0 nr 262144
>         item 28 key (52262 ROOT_ITEM 0) itemoff 9415 itemsize 439
>                 generation 2618893 root_dirid 256 bytenr 42677048360960 level 3 refs 1
>                 lastsnap 2618893 byte_limit 0 bytes_used 5557338112 flags 0x0(none)
>                 uuid d0d4361f-d231-6d40-8901-fe506e4b2b53
> 
> Although item 27 has disk bytenr 34626327621632, which matches the
> data_bytenr, its type is prealloc, not reg.
> This makes the existing code skip that item, and return -ENOENT.
> 
> [FIX]
> The code is modified in commit  19b546d7a1b2 ("btrfs: relocation: Use
> btrfs_find_all_leafs to locate data extent parent tree leaves"), before
> that commit, we use something like
> "if (type == BTRFS_FILE_EXTENT_INLINE) continue;".
> 
> But in that offending commit, we use (type == BTRFS_FILE_EXTENT_REG),
> ignoring BTRFS_FILE_EXTENT_PREALLOC.
> 
> Fix it by also checking BTRFS_FILE_EXTENT_PREALLOC.
> 
> Reported-by: Stéphane Lesimple <stephane_btrfs2@lesimple.fr>
> Fixes: 19b546d7a1b2 ("btrfs: relocation: Use btrfs_find_all_leafs to locate data extent parent tree leaves")
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Thank you all for tracking down the bug, added to misc-next.
Stéphane Lesimple Jan. 5, 2021, 6:24 p.m. UTC | #3
FWIW, just recompiled with the patch to be 100% sure, as I still had
the problematic FS around untouched:

[  199.722122] BTRFS info (device dm-10): balance: start -dvrange=34625344765952..34625344765953
[  199.730267] BTRFS info (device dm-10): relocating block group 34625344765952 flags data|raid1
[  212.232222] BTRFS info (device dm-10): found 167 extents, stage: move data extents
[  236.124541] BTRFS info (device dm-10): found 167 extents, stage: update data pointers
[  248.011778] BTRFS info (device dm-10): balance: ended with status: 0

As expected, all is good now!

Tested-By: Stéphane Lesimple <stephane_btrfs2@lesimple.fr>
David Sterba Jan. 6, 2021, 4:34 p.m. UTC | #4
On Tue, Jan 05, 2021 at 06:24:24PM +0000, Stéphane Lesimple wrote:
> FWIW, just recompiled with the patch to be 100% sure, as I still had
> the problematic FS around untouched:
> 
> [  199.722122] BTRFS info (device dm-10): balance: start -dvrange=34625344765952..34625344765953
> [  199.730267] BTRFS info (device dm-10): relocating block group 34625344765952 flags data|raid1
> [  212.232222] BTRFS info (device dm-10): found 167 extents, stage: move data extents
> [  236.124541] BTRFS info (device dm-10): found 167 extents, stage: update data pointers
> [  248.011778] BTRFS info (device dm-10): balance: ended with status: 0
> 
> As expected, all is good now!
> 
> Tested-By: Stéphane Lesimple <stephane_btrfs2@lesimple.fr>

Thanks for testing, tag added to the patch.
diff mbox series

Patch

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 19b7db8b2117..df63ef64c5c0 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2975,11 +2975,16 @@  static int delete_v1_space_cache(struct extent_buffer *leaf,
 		return 0;
 
 	for (i = 0; i < btrfs_header_nritems(leaf); i++) {
+		u8 type;
+
 		btrfs_item_key_to_cpu(leaf, &key, i);
 		if (key.type != BTRFS_EXTENT_DATA_KEY)
 			continue;
 		ei = btrfs_item_ptr(leaf, i, struct btrfs_file_extent_item);
-		if (btrfs_file_extent_type(leaf, ei) == BTRFS_FILE_EXTENT_REG &&
+		type = btrfs_file_extent_type(leaf, ei);
+
+		if ((type == BTRFS_FILE_EXTENT_REG ||
+		     type == BTRFS_FILE_EXTENT_PREALLOC) &&
 		    btrfs_file_extent_disk_bytenr(leaf, ei) == data_bytenr) {
 			found = true;
 			space_cache_ino = key.objectid;