btrfs-progs: free-space-cache: Enhance free space cache free space check
PATCH v2 1/1 btrfs-progs: free-space-cache: Enhance free space cache free space check
When we found free space difference between free space cache and block group item, we just discard this free space cache.
Normally such difference is caused by btrfs_reserve_extent() called by delalloc which is out of a transaction.
And since all btrfs_release_extent() is called with a transaction, under heavy race free space cache can have less free space than block group item.
Normally kernel will detect such difference and just discard that cache.
However we must be more careful if free space cache has more free space cache, and if that happens, paried with above race one invalid free space cache can be loaded into kernel.
So if we find any free space cache who has more free space then block group item, we report it as an error other than ignoring it.
---
v2:
Fix the timming of free space output.
--- check/main.c | 4 +++- free-space-cache.c | 32 ++++++++++++++++++++++++-------- 2 files changed, 27 insertions(+), 9 deletions(-)
check/main.c +3 -1
@@ -5339,7 +5339,9 @@ static int check_space_cache(struct btrfs_root *root) error += ret; }else { ret =load_free_space_cache (root->fs_info, cache); -if (!ret) +if (ret <0 ) + error++; +if (ret <=0 )continue ; }
free-space-cache.c +24 -8
@@ -438,7 +438,8 @@ int load_free_space_cache(struct btrfs_fs_info *fs_info,struct btrfs_path *path;u64 used =btrfs_block_group_used (&block_group->item);int ret =0 ; -int matched; +u64 bg_free; +s64 diff; path =btrfs_alloc_path ();if (!path)
@@ -448,18 +449,33 @@ int load_free_space_cache(struct btrfs_fs_info *fs_info, block_group->key.objectid);btrfs_free_path (path); - matched = (ctl->free_space == (block_group->key.offset - used - - block_group->bytes_super)); -if (ret ==1 && !matched) { -__btrfs_remove_free_space_cache (ctl); + bg_free = block_group->key.offset - used - block_group->bytes_super; + diff = ctl->free_space - bg_free; +if (ret ==1 && diff) {fprintf (stderr, -"block group %llu has wrong amount of free space\n" , - block_group->key.objectid); +"block group %llu has wrong amount of free space, free space cache has %llu block group has %llu\n" , + block_group->key.objectid, ctl->free_space, bg_free); +__btrfs_remove_free_space_cache (ctl); +/* +* Due to btrfs_reserve_extent() can happen out of a +* transaction, but all btrfs_release_extent() happens inside +* a transaction, so under heavy race it's possible that free +* space cache has less free space, and both kernel just discard +* such cache. But if we find some case where free space cache +* has more free space, this means under certain case such +* cache can be loaded and cause double allocate. +* +* Detect such possibility here. +*/ +if (diff >0 ) +error ( +"free space cache has more free space than block group item, this could leads to serious corruption, please contact btrfs developers" ); ret =-1 ; }if (ret <0 ) { - ret =0 ; +if (diff <=0 ) + ret =0 ; fprintf(stderr, "failed to load free space cache for block group %llu\n",
Signature · 1 line
2.16.2
Quoted 61 lines · Qu Wenruo
When we found free space difference between free space cache and block group item, we just discard this free space cache.
Normally such difference is caused by
btrfs_reserve_extent()called by delalloc which is out of a transaction.
And since allbtrfs_release_extent()is called with a transaction, under heavy race free space cache can have less free space than block group item.Normally kernel will detect such difference and just discard that cache.
However we must be more careful if free space cache has more free space cache, and if that happens, paried with above race one invalid free space cache can be loaded into kernel.
So if we find any free space cache who has more free space then block group item, we report it as an error other than ignoring it.
Signed-off-by Qu Wenruo <[email protected]>---
v2:
Fix the timming of free space output.--- check/main.c | 4 +++- free-space-cache.c | 32 ++++++++++++++++++++++++-------- 2 files changed, 27 insertions(+), 9 deletions(-)check/main.c +3
-1@@ -5339,7 +5339,9 @@ static int check_space_cache(struct btrfs_root *root) error += ret; }else { ret =load_free_space_cache (root->fs_info, cache); -if (!ret) +if (ret <0 ) + error++; +if (ret <=0 )continue ; }free-space-cache.c +2
-2@@ -438,7 +438,8 @@ int load_free_space_cache(struct btrfs_fs_info *fs_info,struct btrfs_path *path;u64 used =btrfs_block_group_used (&block_group->item);int ret =0 ; -int matched; +u64 bg_free; +s64 diff; path =btrfs_alloc_path ();if (!path)@@ -448,18 +449,33 @@ int load_free_space_cache(struct btrfs_fs_info *fs_info, block_group->key.objectid);btrfs_free_path (path); - matched = (ctl->free_space == (block_group->key.offset - used -
free-space-cache.c int load_free_space_cache(struct btrfs_fs_info *fs_info, · line 456
Quoted 10 lines
free-space-cache.c +4
-5- block_group->bytes_super)); -if (ret ==1 && !matched) { -__btrfs_remove_free_space_cache (ctl); + bg_free = block_group->key.offset - used - block_group->bytes_super; + diff = ctl->free_space - bg_free; +if (ret ==1 && diff) {fprintf (stderr, -"block group %llu has wrong amount of free space\n" , - block_group->key.objectid); + "block group %llu has wrong amount of free space, free space cache has %llu block group has %llu\n",nit: Always put units when printing numbers. In this case we are talking
about bytes.
Quoted 5 lines
free-space-cache.c +5
-0+ block_group->key.objectid, ctl->free_space, bg_free); +__btrfs_remove_free_space_cache (ctl); +/* +* Due to btrfs_reserve_extent() can happen out of > + * transaction, but all btrfs_release_extent() happens inside +* a transaction, so under heavy race it's possible that free
free-space-cache.c int load_free_space_cache(struct btrfs_fs_info *fs_info, · line 472
Quoted 10 lines
free-space-cache.c +10
-0+* space cache has less free space, and both kernel just discard +* such cache. But if we find some case where free space cache +* has more free space, this means under certain case such +* cache can be loaded and cause double allocate. +* +* Detect such possibility here. +*/ +if (diff >0 ) +error ( +"free space cache has more free space than block group item, this could leads to serious corruption, please contact btrfs developers" );
I'm not entirely happy with this message. So they will post to the mailing list saying something along the lines of "I got this message what do I do no, please help". Better to output actionable data so that the user can post it immediately.
Quoted 11 lines
free-space-cache.c +2
-1ret =-1 ; }if (ret <0 ) { - ret =0 ; +if (diff <=0 ) + ret =0 ; fprintf(stderr, "failed to load free space cache for block group %llu\n",
Quoted 87 lines · Nikolay Borisov
Quoted 61 lines · Qu Wenruo
When we found free space difference between free space cache and block group item, we just discard this free space cache.
Normally such difference is caused by
btrfs_reserve_extent()called by delalloc which is out of a transaction.
And since allbtrfs_release_extent()is called with a transaction, under heavy race free space cache can have less free space than block group item.Normally kernel will detect such difference and just discard that cache.
However we must be more careful if free space cache has more free space cache, and if that happens, paried with above race one invalid free space cache can be loaded into kernel.
So if we find any free space cache who has more free space then block group item, we report it as an error other than ignoring it.
Signed-off-by Qu Wenruo <[email protected]>---
v2:
Fix the timming of free space output.--- check/main.c | 4 +++- free-space-cache.c | 32 ++++++++++++++++++++++++-------- 2 files changed, 27 insertions(+), 9 deletions(-)check/main.c +3
-1@@ -5339,7 +5339,9 @@ static int check_space_cache(struct btrfs_root *root) error += ret; }else { ret =load_free_space_cache (root->fs_info, cache); -if (!ret) +if (ret <0 ) + error++; +if (ret <=0 )continue ; }free-space-cache.c +2
-2@@ -438,7 +438,8 @@ int load_free_space_cache(struct btrfs_fs_info *fs_info,struct btrfs_path *path;u64 used =btrfs_block_group_used (&block_group->item);int ret =0 ; -int matched; +u64 bg_free; +s64 diff; path =btrfs_alloc_path ();if (!path)@@ -448,18 +449,33 @@ int load_free_space_cache(struct btrfs_fs_info *fs_info, block_group->key.objectid);btrfs_free_path (path); - matched = (ctl->free_space == (block_group->key.offset - used -free-space-cache.c int load_free_space_cache(struct btrfs_fs_info *fs_info, · line 456
Quoted 10 lines
free-space-cache.c +4
-5- block_group->bytes_super)); -if (ret ==1 && !matched) { -__btrfs_remove_free_space_cache (ctl); + bg_free = block_group->key.offset - used - block_group->bytes_super; + diff = ctl->free_space - bg_free; +if (ret ==1 && diff) {fprintf (stderr, -"block group %llu has wrong amount of free space\n" , - block_group->key.objectid); + "block group %llu has wrong amount of free space, free space cache has %llu block group has %llu\n",nit: Always put units when printing numbers. In this case we are talkingabout bytes.
Quoted 12 lines
free-space-cache.c +12
-0+ block_group->key.objectid, ctl->free_space, bg_free); +__btrfs_remove_free_space_cache (ctl); +/* +* Due to btrfs_reserve_extent() can happen out of > + * transaction, but all btrfs_release_extent() happens inside +* a transaction, so under heavy race it's possible that free +* space cache has less free space, and both kernel just discard +* such cache. But if we find some case where free space cache +* has more free space, this means under certain case such +* cache can be loaded and cause double allocate. +* +* Detect such possibility here. +*/
Quoted 8 lines
free-space-cache.c int load_free_space_cache(struct btrfs_fs_info *fs_info, · line 472
Quoted 3 lines
free-space-cache.c +3
-0+if (diff >0 ) +error ( +"free space cache has more free space than block group item, this could leads to serious corruption, please contact btrfs developers" );I'm not entirely happy with this message. So they will post to the mailing list saying something along the lines of "I got this message what do I do no, please help". Better to output actionable data so that the user can post it immediately.
Unfortunately, this is already the situation we don't expect to see.
What we really need is to know this could happen, and if possible some info about the situation.
There is not much actionable data here.
Thanks,
Qu
Quoted 12 lines
Quoted 12 lines
free-space-cache.c +2
-1ret =-1 ; }if (ret <0 ) { - ret =0 ; +if (diff <=0 ) + ret =0 ; fprintf(stderr, "failed to load free space cache for block group %llu\n",
Quoted 99 lines · Qu Wenruo
Quoted 87 lines · Nikolay Borisov
Quoted 61 lines · Qu Wenruo
When we found free space difference between free space cache and block group item, we just discard this free space cache.
Normally such difference is caused by
btrfs_reserve_extent()called by delalloc which is out of a transaction.
And since allbtrfs_release_extent()is called with a transaction, under heavy race free space cache can have less free space than block group item.Normally kernel will detect such difference and just discard that cache.
However we must be more careful if free space cache has more free space cache, and if that happens, paried with above race one invalid free space cache can be loaded into kernel.
So if we find any free space cache who has more free space then block group item, we report it as an error other than ignoring it.
Signed-off-by Qu Wenruo <[email protected]>---
v2:
Fix the timming of free space output.--- check/main.c | 4 +++- free-space-cache.c | 32 ++++++++++++++++++++++++-------- 2 files changed, 27 insertions(+), 9 deletions(-)check/main.c +3
-1@@ -5339,7 +5339,9 @@ static int check_space_cache(struct btrfs_root *root) error += ret; }else { ret =load_free_space_cache (root->fs_info, cache); -if (!ret) +if (ret <0 ) + error++; +if (ret <=0 )continue ; }free-space-cache.c +2
-2@@ -438,7 +438,8 @@ int load_free_space_cache(struct btrfs_fs_info *fs_info,struct btrfs_path *path;u64 used =btrfs_block_group_used (&block_group->item);int ret =0 ; -int matched; +u64 bg_free; +s64 diff; path =btrfs_alloc_path ();if (!path)@@ -448,18 +449,33 @@ int load_free_space_cache(struct btrfs_fs_info *fs_info, block_group->key.objectid);btrfs_free_path (path); - matched = (ctl->free_space == (block_group->key.offset - used -free-space-cache.c int load_free_space_cache(struct btrfs_fs_info *fs_info, · line 456
Quoted 10 lines
free-space-cache.c +4
-5- block_group->bytes_super)); -if (ret ==1 && !matched) { -__btrfs_remove_free_space_cache (ctl); + bg_free = block_group->key.offset - used - block_group->bytes_super; + diff = ctl->free_space - bg_free; +if (ret ==1 && diff) {fprintf (stderr, -"block group %llu has wrong amount of free space\n" , - block_group->key.objectid); + "block group %llu has wrong amount of free space, free space cache has %llu block group has %llu\n",nit: Always put units when printing numbers. In this case we are talkingabout bytes.
Quoted 12 lines
free-space-cache.c +12
-0+ block_group->key.objectid, ctl->free_space, bg_free); +__btrfs_remove_free_space_cache (ctl); +/* +* Due to btrfs_reserve_extent() can happen out of > + * transaction, but all btrfs_release_extent() happens inside +* a transaction, so under heavy race it's possible that free +* space cache has less free space, and both kernel just discard +* such cache. But if we find some case where free space cache +* has more free space, this means under certain case such +* cache can be loaded and cause double allocate. +* +* Detect such possibility here. +*/ Quoted 8 lines
free-space-cache.c int load_free_space_cache(struct btrfs_fs_info *fs_info, · line 472
Quoted 3 lines
free-space-cache.c +3
-0+if (diff >0 ) +error ( +"free space cache has more free space than block group item, this could leads to serious corruption, please contact btrfs developers" );I'm not entirely happy with this message. So they will post to the mailing list saying something along the lines of "I got this message what do I do no, please help". Better to output actionable data so that the user can post it immediately.
Quoted 5 lines
Unfortunately, this is already the situation we don't expect to see.
What we really need is to know this could happen, and if possible some info about the situation.
There is not much actionable data here.
Fair enough, at the very least I think we should put information how to contact btrfs developers. So put the address of the mailing list, I don't think it's safe to assume people will be aware of it .
Quoted 17 lines
Thanks,
QuQuoted 12 lines
Quoted 12 lines
free-space-cache.c +2
-1ret =-1 ; }if (ret <0 ) { - ret =0 ; +if (diff <=0 ) + ret =0 ; fprintf(stderr, "failed to load free space cache for block group %llu\n",
Quoted 18 lines · Nikolay Borisov
Quoted 18 lines
Quoted 7 lines
Quoted 7 lines
free-space-cache.c +7
-0+* space cache has less free space, and both kernel just discard +* such cache. But if we find some case where free space cache +* has more free space, this means under certain case such +* cache can be loaded and cause double allocate. +* +* Detect such possibility here. +*/ Quoted 8 lines
free-space-cache.c int load_free_space_cache(struct btrfs_fs_info *fs_info, · line 472
Quoted 3 lines
free-space-cache.c +3
-0+if (diff >0 ) +error ( +"free space cache has more free space than block group item, this could leads to serious corruption, please contact btrfs developers" );I'm not entirely happy with this message. So they will post to the mailing list saying something along the lines of "I got this message what do I do no, please help". Better to output actionable data so that the user can post it immediately.
Unfortunately, this is already the situation we don't expect to see.
Quoted 7 lines
Quoted 3 lines
What we really need is to know this could happen, and if possible some info about the situation.
There is not much actionable data here.Fair enough, at the very least I think we should put information how to contact btrfs developers. So put the address of the mailing list, I don't think it's safe to assume people will be aware of it .
This should go to the manual page or help string, and reference it from here.
Quoted 15 lines · Qu Wenruo
When we found free space difference between free space cache and block group item, we just discard this free space cache.
Normally such difference is caused by
btrfs_reserve_extent()called by delalloc which is out of a transaction.
And since allbtrfs_release_extent()is called with a transaction, under heavy race free space cache can have less free space than block group item.Normally kernel will detect such difference and just discard that cache.
However we must be more careful if free space cache has more free space cache, and if that happens, paried with above race one invalid free space cache can be loaded into kernel.
Quoted 7 lines
So if we find any free space cache who has more free space then block group item, we report it as an error other than ignoring it.
Signed-off-by Qu Wenruo <[email protected]>---
v2:
Fix the timming of free space output.
I'll apply the v2 patch, thanks. The message can be updated with manual page together.