[PATCH v4 0/3] btrfs: Don't block system suspend during fstrim

[PATCH v4 0/3] btrfs: Don't block system suspend during fstrim

[Luca Stefani]

Changes since v3:
* Went back to manual chunk size

Changes since v2:
* Use blk_alloc_discard_bio directly
* Reset ret to ERESTARTSYS

Changes since v1:
* Use bio_discard_limit to calculate chunk size
* Makes use of the split chunks

v1: https://lore.kernel.org/lkml/20240902114303.922472-1-luca.stefani.ge1@gmail.com/
v2: https://lore.kernel.org/lkml/20240902205828.943155-1-luca.stefani.ge1@gmail.com/
v3: https://lore.kernel.org/lkml/20240903071625.957275-4-luca.stefani.ge1@gmail.com/
Original discussion: https://lore.kernel.org/lkml/20240822164908.4957-1-luca.stefani.ge1@gmail.com/

Luca Stefani (3):
  btrfs: Always update fstrim_range on failure
  btrfs: Split remaining space to discard in chunks
  btrfs: Don't block system suspend during fstrim

 fs/btrfs/extent-tree.c | 47 +++++++++++++++++++++++++++++++++++-------
 fs/btrfs/ioctl.c       |  4 +---
 2 files changed, 40 insertions(+), 11 deletions(-)

-- 
2.46.0

[PATCH v4 1/3] btrfs: Always update fstrim_range on failure

[Luca Stefani]

Even in case of failure we could've discarded
some data and userspace should be made aware of it,
so copy fstrim_range to userspace regardless.

Also make sure to update the trimmed bytes amount
even if btrfs_trim_free_extents fails.

Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Luca Stefani <luca.stefani.ge1@gmail.com>
---
 fs/btrfs/extent-tree.c | 4 ++--
 fs/btrfs/ioctl.c       | 4 +---
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index feec49e6f9c8..a5966324607d 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6551,13 +6551,13 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)
 			continue;
 
 		ret = btrfs_trim_free_extents(device, &group_trimmed);
+
+		trimmed += group_trimmed;
 		if (ret) {
 			dev_failed++;
 			dev_ret = ret;
 			break;
 		}
-
-		trimmed += group_trimmed;
 	}
 	mutex_unlock(&fs_devices->device_list_mutex);
 
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index e0a664b8a46a..94d8f29b04c5 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -543,13 +543,11 @@ static noinline int btrfs_ioctl_fitrim(struct btrfs_fs_info *fs_info,
 
 	range.minlen = max(range.minlen, minlen);
 	ret = btrfs_trim_fs(fs_info, &range);
-	if (ret < 0)
-		return ret;
 
 	if (copy_to_user(arg, &range, sizeof(range)))
 		return -EFAULT;
 
-	return 0;
+	return ret;
 }
 
 int __pure btrfs_is_empty_uuid(const u8 *uuid)
-- 
2.46.0

[PATCH v4 2/3] btrfs: Split remaining space to discard in chunks

[Luca Stefani]

Per Qu Wenruo in case we have a very large disk, e.g. 8TiB device,
mostly empty although we will do the split according to our super block
locations, the last super block ends at 256G, we can submit a huge
discard for the range [256G, 8T), causing a super large delay.

We now split the space left to discard based on BTRFS_MAX_DATA_CHUNK_SIZE
in preparation of introduction of cancellation signals handling.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=219180
Link: https://bugzilla.suse.com/show_bug.cgi?id=1229737
Signed-off-by: Luca Stefani <luca.stefani.ge1@gmail.com>
---
 fs/btrfs/extent-tree.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a5966324607d..cbe66d0acff8 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -1239,7 +1239,7 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
 			       u64 *discarded_bytes)
 {
 	int j, ret = 0;
-	u64 bytes_left, end;
+	u64 bytes_left, bytes_to_discard, end;
 	u64 aligned_start = ALIGN(start, 1 << SECTOR_SHIFT);
 
 	/* Adjust the range to be aligned to 512B sectors if necessary. */
@@ -1300,13 +1300,27 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
 		bytes_left = end - start;
 	}
 
-	if (bytes_left) {
+	while (bytes_left) {
+		if (bytes_left > BTRFS_MAX_DATA_CHUNK_SIZE)
+			bytes_to_discard = BTRFS_MAX_DATA_CHUNK_SIZE;
+		else
+			bytes_to_discard = bytes_left;
+
 		ret = blkdev_issue_discard(bdev, start >> SECTOR_SHIFT,
-					   bytes_left >> SECTOR_SHIFT,
+					   bytes_to_discard >> SECTOR_SHIFT,
 					   GFP_NOFS);
-		if (!ret)
-			*discarded_bytes += bytes_left;
+
+		if (ret) {
+			if (ret != -EOPNOTSUPP)
+				break;
+			continue;
+		}
+
+		start += bytes_to_discard;
+		bytes_left -= bytes_to_discard;
+		*discarded_bytes += bytes_to_discard;
 	}
+
 	return ret;
 }
 
-- 
2.46.0

Re: [PATCH v4 2/3] btrfs: Split remaining space to discard in chunks

[Qu Wenruo]

在 2024/9/16 19:46, Luca Stefani 写道:
> Per Qu Wenruo in case we have a very large disk, e.g. 8TiB device,
> mostly empty although we will do the split according to our super block
> locations, the last super block ends at 256G, we can submit a huge
> discard for the range [256G, 8T), causing a super large delay.
> 
> We now split the space left to discard based on BTRFS_MAX_DATA_CHUNK_SIZE
> in preparation of introduction of cancellation signals handling.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=219180
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1229737
> Signed-off-by: Luca Stefani <luca.stefani.ge1@gmail.com>
> ---
>   fs/btrfs/extent-tree.c | 24 +++++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index a5966324607d..cbe66d0acff8 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -1239,7 +1239,7 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
>   			       u64 *discarded_bytes)
>   {
>   	int j, ret = 0;
> -	u64 bytes_left, end;
> +	u64 bytes_left, bytes_to_discard, end;
>   	u64 aligned_start = ALIGN(start, 1 << SECTOR_SHIFT);
>   
>   	/* Adjust the range to be aligned to 512B sectors if necessary. */
> @@ -1300,13 +1300,27 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
>   		bytes_left = end - start;
>   	}
>   
> -	if (bytes_left) {
> +	while (bytes_left) {
> +		if (bytes_left > BTRFS_MAX_DATA_CHUNK_SIZE)
> +			bytes_to_discard = BTRFS_MAX_DATA_CHUNK_SIZE;

That MAX_DATA_CHUNK_SIZE is only possible for RAID0/RAID10/RAID5/RAID6, 
by spanning the device extents across multiple devices.

For each device, the maximum size is limited to 1G (check 
init_alloc_chunk_ctl_policy_regular()).

So you can just limit it to 1G instead.
(If you want, you can also extract that into a macro as a cleanup).

Furthermore, you can use min() instead of a if ().

So you only need:

		bytes_to_discard = min(SZ_1G, bytes_left);

Otherwise this looks good enough to me.
If the 1G size is not good enough, we can later tune it to smaller values.

Personally speaking I think 1G would be enough.

Thanks,
Qu
> +		else
> +			bytes_to_discard = bytes_left;
> +
>   		ret = blkdev_issue_discard(bdev, start >> SECTOR_SHIFT,
> -					   bytes_left >> SECTOR_SHIFT,
> +					   bytes_to_discard >> SECTOR_SHIFT,
>   					   GFP_NOFS);
> -		if (!ret)
> -			*discarded_bytes += bytes_left;
> +
> +		if (ret) {
> +			if (ret != -EOPNOTSUPP)
> +				break;
> +			continue;
> +		}
> +
> +		start += bytes_to_discard;
> +		bytes_left -= bytes_to_discard;
> +		*discarded_bytes += bytes_to_discard;
>   	}
> +
>   	return ret;
>   }
>   

Re: [PATCH v4 2/3] btrfs: Split remaining space to discard in chunks

[Luca Stefani]

On 16/09/24 12:39, Qu Wenruo wrote:
> 
> 
> 在 2024/9/16 19:46, Luca Stefani 写道:
>> Per Qu Wenruo in case we have a very large disk, e.g. 8TiB device,
>> mostly empty although we will do the split according to our super block
>> locations, the last super block ends at 256G, we can submit a huge
>> discard for the range [256G, 8T), causing a super large delay.
>>
>> We now split the space left to discard based on BTRFS_MAX_DATA_CHUNK_SIZE
>> in preparation of introduction of cancellation signals handling.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=219180
>> Link: https://bugzilla.suse.com/show_bug.cgi?id=1229737
>> Signed-off-by: Luca Stefani <luca.stefani.ge1@gmail.com>
>> ---
>>   fs/btrfs/extent-tree.c | 24 +++++++++++++++++++-----
>>   1 file changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index a5966324607d..cbe66d0acff8 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -1239,7 +1239,7 @@ static int btrfs_issue_discard(struct 
>> block_device *bdev, u64 start, u64 len,
>>                      u64 *discarded_bytes)
>>   {
>>       int j, ret = 0;
>> -    u64 bytes_left, end;
>> +    u64 bytes_left, bytes_to_discard, end;
>>       u64 aligned_start = ALIGN(start, 1 << SECTOR_SHIFT);
>>       /* Adjust the range to be aligned to 512B sectors if necessary. */
>> @@ -1300,13 +1300,27 @@ static int btrfs_issue_discard(struct 
>> block_device *bdev, u64 start, u64 len,
>>           bytes_left = end - start;
>>       }
>> -    if (bytes_left) {
>> +    while (bytes_left) {
>> +        if (bytes_left > BTRFS_MAX_DATA_CHUNK_SIZE)
>> +            bytes_to_discard = BTRFS_MAX_DATA_CHUNK_SIZE;
> 
> That MAX_DATA_CHUNK_SIZE is only possible for RAID0/RAID10/RAID5/RAID6, 
> by spanning the device extents across multiple devices.
> 
> For each device, the maximum size is limited to 1G (check 
> init_alloc_chunk_ctl_policy_regular()).
> 
> So you can just limit it to 1G instead.
> (If you want, you can also extract that into a macro as a cleanup).
I think SZ_1G is enough for now.
> 
> Furthermore, you can use min() instead of a if ().
> 
> So you only need:
> 
>          bytes_to_discard = min(SZ_1G, bytes_left);
> 
> Otherwise this looks good enough to me.
> If the 1G size is not good enough, we can later tune it to smaller values.
> 
> Personally speaking I think 1G would be enough.
> 
> Thanks,
> Qu
Ack, done in v5
>> +        else
>> +            bytes_to_discard = bytes_left;
>> +
>>           ret = blkdev_issue_discard(bdev, start >> SECTOR_SHIFT,
>> -                       bytes_left >> SECTOR_SHIFT,
>> +                       bytes_to_discard >> SECTOR_SHIFT,
>>                          GFP_NOFS);
>> -        if (!ret)
>> -            *discarded_bytes += bytes_left;
>> +
>> +        if (ret) {
>> +            if (ret != -EOPNOTSUPP)
>> +                break;
>> +            continue;
>> +        }
>> +
>> +        start += bytes_to_discard;
>> +        bytes_left -= bytes_to_discard;
>> +        *discarded_bytes += bytes_to_discard;
>>       }
>> +
>>       return ret;
>>   }

[PATCH v4 3/3] btrfs: Don't block system suspend during fstrim

[Luca Stefani]

Sometimes the system isn't able to suspend because the task
responsible for trimming the device isn't able to finish in
time, especially since we have a free extent discarding phase,
which can trim a lot of unallocated space, and there is no
limits on the trim size (unlike the block group part).

Since discard isn't a critical call it can be interrupted
at any time, in such cases we stop the trim, report the amount
of discarded bytes and return failure.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=219180
Link: https://bugzilla.suse.com/show_bug.cgi?id=1229737
Signed-off-by: Luca Stefani <luca.stefani.ge1@gmail.com>
---
 fs/btrfs/extent-tree.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index cbe66d0acff8..ab2e5d366a3a 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -16,6 +16,7 @@
 #include <linux/percpu_counter.h>
 #include <linux/lockdep.h>
 #include <linux/crc32c.h>
+#include <linux/freezer.h>
 #include "ctree.h"
 #include "extent-tree.h"
 #include "transaction.h"
@@ -1235,6 +1236,11 @@ static int remove_extent_backref(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
+static bool btrfs_trim_interrupted(void)
+{
+	return fatal_signal_pending(current) || freezing(current);
+}
+
 static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
 			       u64 *discarded_bytes)
 {
@@ -1319,6 +1325,11 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
 		start += bytes_to_discard;
 		bytes_left -= bytes_to_discard;
 		*discarded_bytes += bytes_to_discard;
+
+		if (btrfs_trim_interrupted()) {
+			ret = -ERESTARTSYS;
+			break;
+		}
 	}
 
 	return ret;
@@ -6473,7 +6484,7 @@ static int btrfs_trim_free_extents(struct btrfs_device *device, u64 *trimmed)
 		start += len;
 		*trimmed += bytes;
 
-		if (fatal_signal_pending(current)) {
+		if (btrfs_trim_interrupted()) {
 			ret = -ERESTARTSYS;
 			break;
 		}
@@ -6522,6 +6533,9 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)
 
 	cache = btrfs_lookup_first_block_group(fs_info, range->start);
 	for (; cache; cache = btrfs_next_block_group(cache)) {
+		if (btrfs_trim_interrupted())
+			break;
+
 		if (cache->start >= range_end) {
 			btrfs_put_block_group(cache);
 			break;
@@ -6561,6 +6575,9 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)
 
 	mutex_lock(&fs_devices->device_list_mutex);
 	list_for_each_entry(device, &fs_devices->devices, dev_list) {
+		if (btrfs_trim_interrupted())
+			break;
+
 		if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state))
 			continue;
 
-- 
2.46.0

Re: [PATCH v4 3/3] btrfs: Don't block system suspend during fstrim

[Qu Wenruo]

在 2024/9/16 19:46, Luca Stefani 写道:
> Sometimes the system isn't able to suspend because the task
> responsible for trimming the device isn't able to finish in
> time, especially since we have a free extent discarding phase,
> which can trim a lot of unallocated space, and there is no
> limits on the trim size (unlike the block group part).
> 
> Since discard isn't a critical call it can be interrupted
> at any time, in such cases we stop the trim, report the amount
> of discarded bytes and return failure.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=219180
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1229737
> Signed-off-by: Luca Stefani <luca.stefani.ge1@gmail.com>
> ---
>   fs/btrfs/extent-tree.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index cbe66d0acff8..ab2e5d366a3a 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -16,6 +16,7 @@
>   #include <linux/percpu_counter.h>
>   #include <linux/lockdep.h>
>   #include <linux/crc32c.h>
> +#include <linux/freezer.h>
>   #include "ctree.h"
>   #include "extent-tree.h"
>   #include "transaction.h"
> @@ -1235,6 +1236,11 @@ static int remove_extent_backref(struct btrfs_trans_handle *trans,
>   	return ret;
>   }
>   
> +static bool btrfs_trim_interrupted(void)
> +{
> +	return fatal_signal_pending(current) || freezing(current);
> +}
> +
>   static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
>   			       u64 *discarded_bytes)
>   {
> @@ -1319,6 +1325,11 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
>   		start += bytes_to_discard;
>   		bytes_left -= bytes_to_discard;
>   		*discarded_bytes += bytes_to_discard;
> +
> +		if (btrfs_trim_interrupted()) {
> +			ret = -ERESTARTSYS;
> +			break;
> +		}
>   	}
>   
>   	return ret;
> @@ -6473,7 +6484,7 @@ static int btrfs_trim_free_extents(struct btrfs_device *device, u64 *trimmed)
>   		start += len;
>   		*trimmed += bytes;
>   
> -		if (fatal_signal_pending(current)) {
> +		if (btrfs_trim_interrupted()) {
>   			ret = -ERESTARTSYS;
>   			break;
>   		}
> @@ -6522,6 +6533,9 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)
>   
>   	cache = btrfs_lookup_first_block_group(fs_info, range->start);
>   	for (; cache; cache = btrfs_next_block_group(cache)) {
> +		if (btrfs_trim_interrupted())
> +			break;
> +

Please update @bg_ret return value.

>   		if (cache->start >= range_end) {
>   			btrfs_put_block_group(cache);
>   			break;
> @@ -6561,6 +6575,9 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)
>   
>   	mutex_lock(&fs_devices->device_list_mutex);
>   	list_for_each_entry(device, &fs_devices->devices, dev_list) {
> +		if (btrfs_trim_interrupted())
> +			break;
> +

The same here, please update @dev_ret.

Thanks,
Qu
>   		if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state))
>   			continue;
>   

Re: [PATCH v4 3/3] btrfs: Don't block system suspend during fstrim

[Luca Stefani]

On 16/09/24 12:41, Qu Wenruo wrote:
> 
> 
> 在 2024/9/16 19:46, Luca Stefani 写道:
>> Sometimes the system isn't able to suspend because the task
>> responsible for trimming the device isn't able to finish in
>> time, especially since we have a free extent discarding phase,
>> which can trim a lot of unallocated space, and there is no
>> limits on the trim size (unlike the block group part).
>>
>> Since discard isn't a critical call it can be interrupted
>> at any time, in such cases we stop the trim, report the amount
>> of discarded bytes and return failure.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=219180
>> Link: https://bugzilla.suse.com/show_bug.cgi?id=1229737
>> Signed-off-by: Luca Stefani <luca.stefani.ge1@gmail.com>
>> ---
>>   fs/btrfs/extent-tree.c | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index cbe66d0acff8..ab2e5d366a3a 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -16,6 +16,7 @@
>>   #include <linux/percpu_counter.h>
>>   #include <linux/lockdep.h>
>>   #include <linux/crc32c.h>
>> +#include <linux/freezer.h>
>>   #include "ctree.h"
>>   #include "extent-tree.h"
>>   #include "transaction.h"
>> @@ -1235,6 +1236,11 @@ static int remove_extent_backref(struct 
>> btrfs_trans_handle *trans,
>>       return ret;
>>   }
>> +static bool btrfs_trim_interrupted(void)
>> +{
>> +    return fatal_signal_pending(current) || freezing(current);
>> +}
>> +
>>   static int btrfs_issue_discard(struct block_device *bdev, u64 start, 
>> u64 len,
>>                      u64 *discarded_bytes)
>>   {
>> @@ -1319,6 +1325,11 @@ static int btrfs_issue_discard(struct 
>> block_device *bdev, u64 start, u64 len,
>>           start += bytes_to_discard;
>>           bytes_left -= bytes_to_discard;
>>           *discarded_bytes += bytes_to_discard;
>> +
>> +        if (btrfs_trim_interrupted()) {
>> +            ret = -ERESTARTSYS;
>> +            break;
>> +        }
>>       }
>>       return ret;
>> @@ -6473,7 +6484,7 @@ static int btrfs_trim_free_extents(struct 
>> btrfs_device *device, u64 *trimmed)
>>           start += len;
>>           *trimmed += bytes;
>> -        if (fatal_signal_pending(current)) {
>> +        if (btrfs_trim_interrupted()) {
>>               ret = -ERESTARTSYS;
>>               break;
>>           }
>> @@ -6522,6 +6533,9 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, 
>> struct fstrim_range *range)
>>       cache = btrfs_lookup_first_block_group(fs_info, range->start);
>>       for (; cache; cache = btrfs_next_block_group(cache)) {
>> +        if (btrfs_trim_interrupted())
>> +            break;
>> +
> 
> Please update @bg_ret return value.
Done in v5.
> 
>>           if (cache->start >= range_end) {
>>               btrfs_put_block_group(cache);
>>               break;
>> @@ -6561,6 +6575,9 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, 
>> struct fstrim_range *range)
>>       mutex_lock(&fs_devices->device_list_mutex);
>>       list_for_each_entry(device, &fs_devices->devices, dev_list) {
>> +        if (btrfs_trim_interrupted())
>> +            break;
>> +
> 
> The same here, please update @dev_ret.
ditto.
> 
> Thanks,
> Qu
>>           if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state))
>>               continue;