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

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

[Luca Stefani]

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/
Original discussion: https://lore.kernel.org/lkml/20240822164908.4957-1-luca.stefani.ge1@gmail.com/

Luca Stefani (3):
  block: Export bio_discard_limit
  btrfs: Split remaining space to discard in chunks
  btrfs: Don't block system suspend during fstrim

 block/blk-lib.c        |  3 ++-
 fs/btrfs/extent-tree.c | 40 +++++++++++++++++++++++++++++++++-------
 include/linux/blkdev.h |  1 +
 3 files changed, 36 insertions(+), 8 deletions(-)

-- 
2.46.0

[PATCH v2 1/3] block: Export bio_discard_limit

[Luca Stefani]

It can be used to calculate the sector size limit of each
discard call allowing filesystem to implement their own
chunked discard logic with customized behavior, for example
cancellation due to signals.

Signed-off-by: Luca Stefani <luca.stefani.ge1@gmail.com>
---
 block/blk-lib.c        | 3 ++-
 include/linux/blkdev.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 4c9f20a689f7..501cbfc4f112 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -10,7 +10,7 @@
 
 #include "blk.h"
 
-static sector_t bio_discard_limit(struct block_device *bdev, sector_t sector)
+sector_t bio_discard_limit(struct block_device *bdev, sector_t sector)
 {
 	unsigned int discard_granularity = bdev_discard_granularity(bdev);
 	sector_t granularity_aligned_sector;
@@ -34,6 +34,7 @@ static sector_t bio_discard_limit(struct block_device *bdev, sector_t sector)
 	 */
 	return round_down(UINT_MAX, discard_granularity) >> SECTOR_SHIFT;
 }
+EXPORT_SYMBOL(bio_discard_limit);
 
 struct bio *blk_alloc_discard_bio(struct block_device *bdev,
 		sector_t *sector, sector_t *nr_sects, gfp_t gfp_mask)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index b7664d593486..42d63400025b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1088,6 +1088,7 @@ static inline long nr_blockdev_pages(void)
 
 extern void blk_io_schedule(void);
 
+sector_t bio_discard_limit(struct block_device *bdev, sector_t sector);
 int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask);
 int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
-- 
2.46.0

Re: [PATCH v2 1/3] block: Export bio_discard_limit

[David Sterba]

On Mon, Sep 02, 2024 at 10:56:10PM +0200, Luca Stefani wrote:
> It can be used to calculate the sector size limit of each
> discard call allowing filesystem to implement their own
> chunked discard logic with customized behavior, for example
> cancellation due to signals.

Maybe to add context for block layer people why we want to export this:

The fs trim loops over ranges and sends discard requests, some ranges
can be large so it's all transparently handled by blkdev_issue_discard()
and processed in smaller chunks.

We need to insert checks for cancellation (or suspend) requests into the
the loop. Rather than setting an arbitrary chunk length on the
filesystem level I've suggested to use bio_discard_limit() assuming it
will do optimal number of IO requests. Then we don't have to guess
whether 1G or 10G is the right value, unnecessarily increasing the
number of requests when the device could handle larger ranges.

Re: [PATCH v2 1/3] block: Export bio_discard_limit

[Christoph Hellwig]

On Mon, Sep 02, 2024 at 11:57:05PM +0200, David Sterba wrote:
> On Mon, Sep 02, 2024 at 10:56:10PM +0200, Luca Stefani wrote:
> > It can be used to calculate the sector size limit of each
> > discard call allowing filesystem to implement their own
> > chunked discard logic with customized behavior, for example
> > cancellation due to signals.
> 
> Maybe to add context for block layer people why we want to export this:
> 
> The fs trim loops over ranges and sends discard requests, some ranges
> can be large so it's all transparently handled by blkdev_issue_discard()
> and processed in smaller chunks.

Then don't use blkdev_issue_discard but use blk_alloc_discard_bio
directly.

NAK to the export of bio_discard_limit.

[PATCH v2 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 the block discard limit
in preparation of introduction of cancellation signals handling.

Reported-by: Qu Wenruo <wqu@suse.com>
Closes: https://lore.kernel.org/lkml/2e15214b-7e95-4e64-a899-725de12c9037@gmail.com/T/#mdfef1d8b36334a15c54cd009f6aadf49e260e105
Signed-off-by: Luca Stefani <luca.stefani.ge1@gmail.com>
---
 fs/btrfs/extent-tree.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index feec49e6f9c8..894684f4f497 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -1239,8 +1239,9 @@ 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);
+	sector_t sector, nr_sects, bio_sects;
 
 	/* Adjust the range to be aligned to 512B sectors if necessary. */
 	if (start != aligned_start) {
@@ -1300,13 +1301,23 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
 		bytes_left = end - start;
 	}
 
-	if (bytes_left) {
-		ret = blkdev_issue_discard(bdev, start >> SECTOR_SHIFT,
-					   bytes_left >> SECTOR_SHIFT,
-					   GFP_NOFS);
+	while (bytes_left) {
+		sector = start >> SECTOR_SHIFT;
+		nr_sects = bytes_left >> SECTOR_SHIFT;
+		bio_sects = min(nr_sects, bio_discard_limit(bdev, sector));
+		bytes_to_discard = bio_sects << SECTOR_SHIFT;
+
+		ret = blkdev_issue_discard(bdev, sector, bio_sects, GFP_NOFS);
+
 		if (!ret)
-			*discarded_bytes += bytes_left;
+			*discarded_bytes += bytes_to_discard;
+		else if (ret != -EOPNOTSUPP)
+			return ret;
+
+		start += bytes_to_discard;
+		bytes_left -= bytes_to_discard;
 	}
+
 	return ret;
 }
 
-- 
2.46.0

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

[Qu Wenruo]

在 2024/9/3 06:26, 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 the block discard limit
> in preparation of introduction of cancellation signals handling.
> 
> Reported-by: Qu Wenruo <wqu@suse.com>

Well, I'd say who ever reported the original fstrim and suspension 
failure should be the reporter, not me.

And David's advice is indeed pretty good for the new split according to 
the discard limit.

> Closes: https://lore.kernel.org/lkml/2e15214b-7e95-4e64-a899-725de12c9037@gmail.com/T/#mdfef1d8b36334a15c54cd009f6aadf49e260e105
> Signed-off-by: Luca Stefani <luca.stefani.ge1@gmail.com>
> ---
>   fs/btrfs/extent-tree.c | 23 +++++++++++++++++------
>   1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index feec49e6f9c8..894684f4f497 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -1239,8 +1239,9 @@ 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;

You can calculate the discard limit here, no need to recalculate inside 
the loop.

For the other variables only utilized inside the loop, you can always 
declare them inside the loop.

Otherwise looks good to me.

Thanks,
Qu
>   	u64 aligned_start = ALIGN(start, 1 << SECTOR_SHIFT);
> +	sector_t sector, nr_sects, bio_sects;
>   
>   	/* Adjust the range to be aligned to 512B sectors if necessary. */
>   	if (start != aligned_start) {
> @@ -1300,13 +1301,23 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
>   		bytes_left = end - start;
>   	}
>   
> -	if (bytes_left) {
> -		ret = blkdev_issue_discard(bdev, start >> SECTOR_SHIFT,
> -					   bytes_left >> SECTOR_SHIFT,
> -					   GFP_NOFS);
> +	while (bytes_left) {
> +		sector = start >> SECTOR_SHIFT;
> +		nr_sects = bytes_left >> SECTOR_SHIFT;
> +		bio_sects = min(nr_sects, bio_discard_limit(bdev, sector));
> +		bytes_to_discard = bio_sects << SECTOR_SHIFT;
> +
> +		ret = blkdev_issue_discard(bdev, sector, bio_sects, GFP_NOFS);
> +
>   		if (!ret)
> -			*discarded_bytes += bytes_left;
> +			*discarded_bytes += bytes_to_discard;
> +		else if (ret != -EOPNOTSUPP)
> +			return ret;
> +
> +		start += bytes_to_discard;
> +		bytes_left -= bytes_to_discard;
>   	}
> +
>   	return ret;
>   }
>   

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

[David Sterba]

On Tue, Sep 03, 2024 at 08:01:16AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2024/9/3 06:26, 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 the block discard limit
> > in preparation of introduction of cancellation signals handling.
> > 
> > Reported-by: Qu Wenruo <wqu@suse.com>
> 
> Well, I'd say who ever reported the original fstrim and suspension 
> failure should be the reporter, not me.

Once the patch is finalized I'll add the links to reports but yeah if
they're added when the patches are submitted it's helpful.

[PATCH v2 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 | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 894684f4f497..7c78ed4044db 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)
 {
@@ -1302,6 +1308,9 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
 	}
 
 	while (bytes_left) {
+		if (btrfs_trim_interrupted())
+			break;
+
 		sector = start >> SECTOR_SHIFT;
 		nr_sects = bytes_left >> SECTOR_SHIFT;
 		bio_sects = min(nr_sects, bio_discard_limit(bdev, sector));
@@ -6470,7 +6479,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;
 		}
@@ -6519,6 +6528,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;
@@ -6558,6 +6570,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 v2 3/3] btrfs: Don't block system suspend during fstrim

[Qu Wenruo]

在 2024/9/3 06:26, 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 | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 894684f4f497..7c78ed4044db 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)
>   {
> @@ -1302,6 +1308,9 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
>   	}
>   
>   	while (bytes_left) {
> +		if (btrfs_trim_interrupted())
> +			break;
> +

Although with this change, the check should be more than enough to cover 
everything, better than the existing per-block-group check and the 
possibly large discard for the free extents.

Thus just this check would be enough.

The other thing is the old return value, I guess you can just add "ret = 
-EERESTARTSYS;" before the break.

Thanks,
Qu
>   		sector = start >> SECTOR_SHIFT;
>   		nr_sects = bytes_left >> SECTOR_SHIFT;
>   		bio_sects = min(nr_sects, bio_discard_limit(bdev, sector));
> @@ -6470,7 +6479,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;
>   		}
> @@ -6519,6 +6528,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;
> @@ -6558,6 +6570,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;
>