btrfs: Enhance btrfs_trim_fs function to handle error better
PATCH v2 1/2 btrfs: Enhance btrfs_trim_fs function to handle error better
Function btrfs_trim_fs() doesn't handle errors in a consistent way, if error happens when trimming existing block groups, it will skip the remaining blocks and continue to trim unallocated space for each device.
And the return value will only reflect the final error from device trimming.
This patch will fix such behavior by:
1) Recording first error from block group or device trimming So return value will also reflect any error found when trimming. Make developer more aware of the problem.
2) Outputting debug message for total trimming failure Since trimming failure for device and block group is not fatal, put the message level to debug and only output the total failure number along with the info for first block group/device.
3) Continuing trimming if we can
If we failed to trim one block group or device, we could still try next block group or device.
Such behavior can avoid confusion for case like failure to trim the first block group and then only unallocated space is trimmed.
---
changelog:
v2:
Only report total number of errors and first errno to make it less noisy.
Change message level from warning to debug
--- fs/btrfs/extent-tree.c | 75 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 58 insertions(+), 17 deletions(-)
fs/btrfs/extent-tree.c +58 -17
@@ -10995,6 +10995,16 @@ static int btrfs_trim_free_extents(struct btrfs_device *device,return ret; } +/* +* Trim the whole fs, by: +* 1) Trimming free space in each block group +* 2) Trimming unallocated space in each device +* +* Will try to continue trimming even if we failed to trim one block group or +* device. +* The return value will be the error return value of the first error. +* Or 0 if nothing wrong happened. +*/ int btrfs_trim_fs (struct btrfs_fs_info *fs_info,struct fstrim_range *range) {struct btrfs_block_group_cache *cache =NULL ;
@@ -11005,6 +11015,12 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)u64 end;u64 trimmed =0 ;u64 total_bytes =btrfs_super_total_bytes (fs_info->super_copy); +u64 first_bg_failed =0 ; +u64 first_dev_failed =0 ; +int bg_failed =0 ; +int dev_failed =0 ; +int bg_ret =0 ; +int dev_ret =0 ;int ret =0 ;/*
@@ -11015,7 +11031,7 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)else cache =btrfs_lookup_block_group (fs_info, range->start); -while (cache) { +for (; cache; cache =next_block_group (fs_info, cache)) {if (cache->key.objectid >= (range->start + range->len)) {btrfs_put_block_group (cache);break ;
@@ -11029,45 +11045,70 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)if (!block_group_cache_done (cache)) { ret =cache_block_group (cache,0 );if (ret) { -btrfs_put_block_group (cache); -break ; +if (!bg_ret) { + bg_ret = ret; + first_bg_failed = + cache->key.objectid; + } + bg_failed++; +continue ; } ret =wait_block_group_cache_done (cache);if (ret) { -btrfs_put_block_group (cache); -break ; +if (!bg_ret) { + bg_ret = ret; + first_bg_failed = + cache->key.objectid; + } + bg_failed++; +continue ; } } - ret =btrfs_trim_block_group (cache, - &group_trimmed, - start, - end, - range->minlen); + ret =btrfs_trim_block_group (cache, &group_trimmed, + start, end, range->minlen); trimmed += group_trimmed;if (ret) { -btrfs_put_block_group (cache); -break ; +if (!bg_ret) { + bg_ret = ret; + first_bg_failed = cache->key.objectid; + } + bg_failed++; +continue ; } } - - cache =next_block_group (fs_info, cache); } +if (bg_ret) +btrfs_debug (fs_info, +"failed to trim %d block groups, first error occurs for block group %llu ret %d" , + bg_failed, first_bg_failed, bg_ret); +mutex_lock (&fs_info->fs_devices->device_list_mutex); devices = &fs_info->fs_devices->alloc_list;list_for_each_entry (device, devices, dev_alloc_list) { ret =btrfs_trim_free_extents (device, range->minlen, &group_trimmed); -if (ret) -break ; +if (ret) { +if (!dev_ret) { + dev_ret = ret; + first_dev_failed = device->devid; + } + dev_failed++; + } trimmed += group_trimmed; }mutex_unlock (&fs_info->fs_devices->device_list_mutex); +if (dev_ret) +btrfs_debug (fs_info, +"failed to trim %d devices, first error occurs for devid %llu ret %d" , + dev_failed, first_dev_failed, dev_ret); range->len = trimmed; -return ret; +if (bg_ret) +return bg_ret; +return dev_ret; }/*
Signature · 1 line
2.15.0
PATCH 2/2 btrfs: extent-tree: Ensure btrfs_trim_fs can trim the whole fs
[BUG]
fstrim on some btrfs only trims the unallocated space, not trimming any space in existing block groups.
[CAUSE]fstrim_range passed in by default fstrim will be:
range->start = 0range->len = fs_size (which equals with super->total_bytes) range->min_len = 512
However btrfs_trim_fs() following above parameter to search block groups to trim.
While it's quite possible that all chunks start beyond super->total_bytes if the fs is balanced several times.
In that case, btrfs will skip trimming block groups and only trim the unallocated space of each device.
[FIX]
For common full fs trimming range passed in, extent its len to (u64)-1 so we will iterate all block groups.
And for custom fs trimming range, due to the fact that the range will always be truncated by range [0, super->total_bytes), making custom fs trimming range useless.
Just return -ENOTTY for custom fs trimming range.
--- fs/btrfs/extent-tree.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)
fs/btrfs/extent-tree.c +24 -5
@@ -11024,12 +11024,31 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)int ret =0 ;/* -* try to trim all FS space, our block group may start from non-zero. +* NOTE: Btrfs uses its own logical address space, where its first +* chunk can start anywhere if it wants. +* If we follow common start = 0 and len = fs_size from @range, we +* can end up without trimming any block groups, since it's highly +* possible all chunks start beyond that range. +* +* So if we want to trim the whole fs, extent the len to (u64)-1 to trim +* all block groups. +* +* Also, since @range will always be truncated to fs size, manually +* passing range to trim specified range doesn't make much sense. +* (No mean to trim any block group whose bytenr starts beyond +* @total_bytes) +* So in that case, return -ENOTTY directly to prevent any custom trim +* request. */ -if (range->len == total_bytes) - cache =btrfs_lookup_first_block_group (fs_info, range->start); -else - cache =btrfs_lookup_block_group (fs_info, range->start); +if (range->start ==0 && range->len == total_bytes) { + range->len = (u64)-1 ; + }else { +btrfs_info (fs_info, +"trimming custom range is not supported due to the limitation of fstrim_range" ); +return -ENOTTY ; + } + + cache =btrfs_lookup_first_block_group (fs_info, range->start);for (; cache; cache =next_block_group (fs_info, cache)) {if (cache->key.objectid >= (range->start + range->len)) {
Signature · 1 line
2.15.0
Re: [PATCH 2/2] btrfs: extent-tree: Ensure btrfs_trim_fs can trim the whole fs
Quoted 24 lines · Qu Wenruo
[BUG]
fstrim on some btrfs only trims the unallocated space, not trimming any space in existing block groups.[CAUSE]
fstrim_rangepassed in by default fstrim will be:
range->start= 0range->len=fs_size(which equals withsuper->total_bytes)range->min_len= 512However
btrfs_trim_fs()following above parameter to search block groups to trim.While it's quite possible that all chunks start beyond
super->total_bytesif the fs is balanced several times.In that case, btrfs will skip trimming block groups and only trim the unallocated space of each device.
[FIX]
For common full fs trimming range passed in, extent its len to (u64)-1 so we will iterate all block groups.
Quoted 8 lines
And for custom fs trimming range, due to the fact that the range will always be truncated by range [0,
super->total_bytes), making custom fs trimming range useless.Just return
-ENOTTYfor custom fs trimming range.Reported-by Chris Murphy <[email protected]>Signed-off-by Qu Wenruo <[email protected]>
I think we want this tagged for stable ?
Quoted 46 lines
--- fs/btrfs/extent-tree.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)fs/btrfs/extent-tree.c +24
-5@@ -11024,12 +11024,31 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)int ret =0 ;/* -* try to trim all FS space, our block group may start from non-zero. +* NOTE: Btrfs uses its own logical address space, where its first +* chunk can start anywhere if it wants. +* If we follow common start = 0 and len = fs_size from @range, we +* can end up without trimming any block groups, since it's highly +* possible all chunks start beyond that range. +* +* So if we want to trim the whole fs, extent the len to (u64)-1 to trim +* all block groups. +* +* Also, since @range will always be truncated to fs size, manually +* passing range to trim specified range doesn't make much sense. +* (No mean to trim any block group whose bytenr starts beyond +* @total_bytes) +* So in that case, return -ENOTTY directly to prevent any custom trim +* request. */ -if (range->len == total_bytes) - cache =btrfs_lookup_first_block_group (fs_info, range->start); -else - cache =btrfs_lookup_block_group (fs_info, range->start); +if (range->start ==0 && range->len == total_bytes) { + range->len = (u64)-1 ; + }else { +btrfs_info (fs_info, +"trimming custom range is not supported due to the limitation of fstrim_range" ); +return -ENOTTY ; + } + + cache =btrfs_lookup_first_block_group (fs_info, range->start);for (; cache; cache =next_block_group (fs_info, cache)) {if (cache->key.objectid >= (range->start + range->len)) {
Re: [PATCH 2/2] btrfs: extent-tree: Ensure btrfs_trim_fs can trim the whole fs
Quoted 31 lines · Nikolay Borisov
Quoted 28 lines · Qu Wenruo
[BUG]
fstrim on some btrfs only trims the unallocated space, not trimming any space in existing block groups.[CAUSE]
fstrim_rangepassed in by default fstrim will be:
range->start= 0range->len=fs_size(which equals withsuper->total_bytes)range->min_len= 512However
btrfs_trim_fs()following above parameter to search block groups to trim.While it's quite possible that all chunks start beyond
super->total_bytesif the fs is balanced several times.In that case, btrfs will skip trimming block groups and only trim the unallocated space of each device.
[FIX]
For common full fs trimming range passed in, extent its len to (u64)-1 so we will iterate all block groups.And for custom fs trimming range, due to the fact that the range will always be truncated by range [0,
super->total_bytes), making custom fs trimming range useless.
Quoted 6 lines
Quoted 4 lines
Just return
-ENOTTYfor custom fs trimming range.Reported-by Chris Murphy <[email protected]>Signed-off-by Qu Wenruo <[email protected]>I think we want this tagged for stable ?
Sounds pretty good.
Although I'm a little concerned about the ENOTTY return branch, not sure if it will break script of some guys.
Thanks,
Qu
Quoted 53 lines
Quoted 38 lines
--- fs/btrfs/extent-tree.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)fs/btrfs/extent-tree.c +17
-5@@ -11024,12 +11024,31 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)int ret =0 ;/* -* try to trim all FS space, our block group may start from non-zero. +* NOTE: Btrfs uses its own logical address space, where its first +* chunk can start anywhere if it wants. +* If we follow common start = 0 and len = fs_size from @range, we +* can end up without trimming any block groups, since it's highly +* possible all chunks start beyond that range. +* +* So if we want to trim the whole fs, extent the len to (u64)-1 to trim +* all block groups. +* +* Also, since @range will always be truncated to fs size, manually +* passing range to trim specified range doesn't make much sense. +* (No mean to trim any block group whose bytenr starts beyond +* @total_bytes) +* So in that case, return -ENOTTY directly to prevent any custom trim +* request. */ -if (range->len == total_bytes) - cache =btrfs_lookup_first_block_group (fs_info, range->start); -else - cache =btrfs_lookup_block_group (fs_info, range->start); +if (range->start ==0 && range->len == total_bytes) { + range->len = (u64)-1 ;fs/btrfs/extent-tree.c int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range) · line 11054
Quoted 10 lines
fs/btrfs/extent-tree.c +7
-0+ }else { +btrfs_info (fs_info, +"trimming custom range is not supported due to the limitation of fstrim_range" ); +return -ENOTTY ; + } + + cache =btrfs_lookup_first_block_group (fs_info, range->start);for (; cache; cache =next_block_group (fs_info, cache)) {if (cache->key.objectid >= (range->start + range->len)) {--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] btrfs: extent-tree: Ensure btrfs_trim_fs can trim the whole fs
In reply to Qu WenruoQuoted 57 lines · Qu Wenruo
[BUG]
fstrim on some btrfs only trims the unallocated space, not trimming any space in existing block groups.[CAUSE]
fstrim_rangepassed in by default fstrim will be:
range->start= 0range->len=fs_size(which equals withsuper->total_bytes)range->min_len= 512However
btrfs_trim_fs()following above parameter to search block groups to trim.While it's quite possible that all chunks start beyond
super->total_bytesif the fs is balanced several times.In that case, btrfs will skip trimming block groups and only trim the unallocated space of each device.
[FIX]
For common full fs trimming range passed in, extent its len to (u64)-1 so we will iterate all block groups.And for custom fs trimming range, due to the fact that the range will always be truncated by range [0,
super->total_bytes), making custom fs trimming range useless.Just return
-ENOTTYfor custom fs trimming range.Reported-by Chris Murphy <[email protected]>Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/extent-tree.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)fs/btrfs/extent-tree.c +12
-1@@ -11024,12 +11024,31 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)int ret =0 ;/* -* try to trim all FS space, our block group may start from non-zero. +* NOTE: Btrfs uses its own logical address space, where its first +* chunk can start anywhere if it wants. +* If we follow common start = 0 and len = fs_size from @range, we +* can end up without trimming any block groups, since it's highly +* possible all chunks start beyond that range. +* +* So if we want to trim the whole fs, extent the len to (u64)-1 to trim +* all block groups. +* +* Also, since @range will always be truncated to fs size, manually +* passing range to trim specified range doesn't make much sense. +* (No mean to trim any block group whose bytenr starts beyond
fs/btrfs/extent-tree.c int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range) · line 11044
Quoted 10 lines
fs/btrfs/extent-tree.c +5
-4+* @total_bytes) +* So in that case, return -ENOTTY directly to prevent any custom trim +* request. */ -if (range->len == total_bytes) - cache =btrfs_lookup_first_block_group (fs_info, range->start); -else - cache =btrfs_lookup_block_group (fs_info, range->start); +if (range->start ==0 && range->len == total_bytes) { + range->len = (u64)-1 ;
After the fs_trim program gets the value for the range's length and before it invokes the trim ioctl, the value might have changed, resulting in returning the enotty error below.
fs/btrfs/extent-tree.c int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range) · line 11047
Quoted 3 lines
fs/btrfs/extent-tree.c +3
-0+ }else { +btrfs_info (fs_info, +"trimming custom range is not supported due to the limitation of fstrim_range" );
I can't understand this message, and I doubt the average user/admin can.
To me it seems this can be a lot more simple by ignoring the range, that is, always considering [0, (u64)-1[. After all, due to the way btrfs organizes space, the range does not make any sense and I doubt users/programs will have all the necessary knowledge and willing to compute a range that makes sense to btrfs based on the current block group layout of the fs...
Quoted 14 lines
fs/btrfs/extent-tree.c +4
-1+return -ENOTTY ; + } + + cache =btrfs_lookup_first_block_group (fs_info, range->start);for (; cache; cache =next_block_group (fs_info, cache)) {if (cache->key.objectid >= (range->start + range->len)) { --2.15.0
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Filipe David Manana, “Whether you think you can, or you think you can't — you're right.”
Re: [PATCH 2/2] btrfs: extent-tree: Ensure btrfs_trim_fs can trim the whole fs
Quoted 77 lines · Filipe Manana
Quoted 57 lines · Qu Wenruo
[BUG]
fstrim on some btrfs only trims the unallocated space, not trimming any space in existing block groups.[CAUSE]
fstrim_rangepassed in by default fstrim will be:
range->start= 0range->len=fs_size(which equals withsuper->total_bytes)range->min_len= 512However
btrfs_trim_fs()following above parameter to search block groups to trim.While it's quite possible that all chunks start beyond
super->total_bytesif the fs is balanced several times.In that case, btrfs will skip trimming block groups and only trim the unallocated space of each device.
[FIX]
For common full fs trimming range passed in, extent its len to (u64)-1 so we will iterate all block groups.And for custom fs trimming range, due to the fact that the range will always be truncated by range [0,
super->total_bytes), making custom fs trimming range useless.Just return
-ENOTTYfor custom fs trimming range.Reported-by Chris Murphy <[email protected]>Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/extent-tree.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)fs/btrfs/extent-tree.c +12
-1@@ -11024,12 +11024,31 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)int ret =0 ;/* -* try to trim all FS space, our block group may start from non-zero. +* NOTE: Btrfs uses its own logical address space, where its first +* chunk can start anywhere if it wants. +* If we follow common start = 0 and len = fs_size from @range, we +* can end up without trimming any block groups, since it's highly +* possible all chunks start beyond that range. +* +* So if we want to trim the whole fs, extent the len to (u64)-1 to trim +* all block groups. +* +* Also, since @range will always be truncated to fs size, manually +* passing range to trim specified range doesn't make much sense. +* (No mean to trim any block group whose bytenr starts beyond fs/btrfs/extent-tree.c int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range) · line 11044
Quoted 10 lines
fs/btrfs/extent-tree.c +5
-4+* @total_bytes) +* So in that case, return -ENOTTY directly to prevent any custom trim +* request. */ -if (range->len == total_bytes) - cache =btrfs_lookup_first_block_group (fs_info, range->start); -else - cache =btrfs_lookup_block_group (fs_info, range->start); +if (range->start ==0 && range->len == total_bytes) { + range->len = (u64)-1 ;After the
fs_trimprogram gets the value for the range's length and before it invokes the trim ioctl, the value might have changed, resulting in returning the enotty error below.fs/btrfs/extent-tree.c int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range) · line 11047
Quoted 3 lines
fs/btrfs/extent-tree.c +3
-0+ }else { +btrfs_info (fs_info, +"trimming custom range is not supported due to the limitation of fstrim_range" );
Quoted 8 lines
I can't understand this message, and I doubt the average user/admin can.
To me it seems this can be a lot more simple by ignoring the range, that is, always considering [0, (u64)-1[. After all, due to the way btrfs organizes space, the range does not make any sense and I doubt users/programs will have all the necessary knowledge and willing to compute a range that makes sense to btrfs based on the current block group layout of the fs...
Yes, just ignoring the range at all is the simplest solution.
Although I'm a little worried about false bug report about wrong trimmed bytes later.
Current bug is exposed by Chris Murphy who takes extra care about the trimmed bytes.
If we change the behavior to always trim the whole fs, maybe some other careful guy trying to trim some strange range will report bug about this.
Maybe put some note in btrfs(5) and always trim the whole fs will be a better solution?
Thanks,
Qu
Quoted 18 lines
Quoted 15 lines
fs/btrfs/extent-tree.c +4
-1+return -ENOTTY ; + } + + cache =btrfs_lookup_first_block_group (fs_info, range->start);for (; cache; cache =next_block_group (fs_info, cache)) {if (cache->key.objectid >= (range->start + range->len)) { --2.15.0
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html