[PATCHv3 0/4] block: make long runnint operations killable

[PATCHv3 0/4] block: make long runnint operations killable

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Changes from v2:

  Wait for chained bio's to complete before returning from kill signal.

  Miscellaneous cleanup patches for style and consistency.

Keith Busch (4):
  block: blkdev_issue_secure_erase loop style
  block: cleanup __blkdev_issue_write_zeroes
  block: io wait hang check helper
  blk-lib: check for kill signal

 block/bio.c     | 12 +----------
 block/blk-lib.c | 57 ++++++++++++++++++++++++++++++++++++++-----------
 block/blk-mq.c  | 19 +++--------------
 block/blk.h     | 13 +++++++++++
 4 files changed, 61 insertions(+), 40 deletions(-)

-- 
2.34.1

[PATCHv3 1/4] block: blkdev_issue_secure_erase loop style

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Use consistent coding style in this file. All the other loops for the
same purpose use "while (nr_sects)", so they win.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-lib.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index e59c3069e8351..91770da2239f2 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -322,7 +322,7 @@ int blkdev_issue_secure_erase(struct block_device *bdev, sector_t sector,
 		return -EPERM;
 
 	blk_start_plug(&plug);
-	for (;;) {
+	while (nr_sects) {
 		unsigned int len = min_t(sector_t, nr_sects, max_sectors);
 
 		bio = blk_next_bio(bio, bdev, 0, REQ_OP_SECURE_ERASE, gfp);
@@ -331,13 +331,12 @@ int blkdev_issue_secure_erase(struct block_device *bdev, sector_t sector,
 
 		sector += len;
 		nr_sects -= len;
-		if (!nr_sects) {
-			ret = submit_bio_wait(bio);
-			bio_put(bio);
-			break;
-		}
 		cond_resched();
 	}
+	if (bio) {
+		ret = submit_bio_wait(bio);
+		bio_put(bio);
+	}
 	blk_finish_plug(&plug);
 
 	return ret;
-- 
2.34.1

Re: [PATCHv3 1/4] block: blkdev_issue_secure_erase loop style

[Ming Lei]

On Fri, Feb 23, 2024 at 3:21 AM Keith Busch <kbusch@meta.com> wrote:
>
> From: Keith Busch <kbusch@kernel.org>
>
> Use consistent coding style in this file. All the other loops for the
> same purpose use "while (nr_sects)", so they win.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Re: [PATCHv3 1/4] block: blkdev_issue_secure_erase loop style

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCHv3 2/4] block: __blkdev_issue_write_zeroes cleanup

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Use min to calculate the next number of sectors like everyone else.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-lib.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 91770da2239f2..d4c476cf3784a 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -132,19 +132,16 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev,
 		return -EOPNOTSUPP;
 
 	while (nr_sects) {
+		unsigned int len = min_t(sector_t, nr_sects, max_write_zeroes_sectors);
+
 		bio = blk_next_bio(bio, bdev, 0, REQ_OP_WRITE_ZEROES, gfp_mask);
 		bio->bi_iter.bi_sector = sector;
 		if (flags & BLKDEV_ZERO_NOUNMAP)
 			bio->bi_opf |= REQ_NOUNMAP;
 
-		if (nr_sects > max_write_zeroes_sectors) {
-			bio->bi_iter.bi_size = max_write_zeroes_sectors << 9;
-			nr_sects -= max_write_zeroes_sectors;
-			sector += max_write_zeroes_sectors;
-		} else {
-			bio->bi_iter.bi_size = nr_sects << 9;
-			nr_sects = 0;
-		}
+		bio->bi_iter.bi_size = len << 9;
+		nr_sects -= len;
+		sector += len;
 		cond_resched();
 	}
 
-- 
2.34.1

Re: [PATCHv3 2/4] block: __blkdev_issue_write_zeroes cleanup

[Ming Lei]

On Fri, Feb 23, 2024 at 3:20 AM Keith Busch <kbusch@meta.com> wrote:
>
> From: Keith Busch <kbusch@kernel.org>
>
> Use min to calculate the next number of sectors like everyone else.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>

On Fri, Feb 23, 2024 at 3:21 AM Keith Busch <kbusch@meta.com> wrote:
>
> From: Keith Busch <kbusch@kernel.org>
>
> Use consistent coding style in this file. All the other loops for the
> same purpose use "while (nr_sects)", so they win.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Re: [PATCHv3 2/4] block: __blkdev_issue_write_zeroes cleanup

[Christoph Hellwig]

On Thu, Feb 22, 2024 at 11:19:20AM -0800, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Use min to calculate the next number of sectors like everyone else.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  block/blk-lib.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 91770da2239f2..d4c476cf3784a 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -132,19 +132,16 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev,
>  		return -EOPNOTSUPP;
>  
>  	while (nr_sects) {
> +		unsigned int len = min_t(sector_t, nr_sects, max_write_zeroes_sectors);

Please avoid the overly long line.

> +		bio->bi_iter.bi_size = len << 9;

And if you touch this anyway use SECTOR_SHIFT here.

Otherwise this looks good.

[PATCHv3 3/4] block: introduce io wait hang check helper

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This is the same in two places, and another will be added soon.
Create a helper for it.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/bio.c    | 12 +-----------
 block/blk-mq.c | 19 +++----------------
 block/blk.h    | 13 +++++++++++++
 3 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 00847ff1415c3..496867b51609f 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -16,7 +16,6 @@
 #include <linux/workqueue.h>
 #include <linux/cgroup.h>
 #include <linux/highmem.h>
-#include <linux/sched/sysctl.h>
 #include <linux/blk-crypto.h>
 #include <linux/xarray.h>
 
@@ -1371,21 +1370,12 @@ int submit_bio_wait(struct bio *bio)
 {
 	DECLARE_COMPLETION_ONSTACK_MAP(done,
 			bio->bi_bdev->bd_disk->lockdep_map);
-	unsigned long hang_check;
 
 	bio->bi_private = &done;
 	bio->bi_end_io = submit_bio_wait_endio;
 	bio->bi_opf |= REQ_SYNC;
 	submit_bio(bio);
-
-	/* Prevent hang_check timer from firing at us during very long I/O */
-	hang_check = sysctl_hung_task_timeout_secs;
-	if (hang_check)
-		while (!wait_for_completion_io_timeout(&done,
-					hang_check * (HZ/2)))
-			;
-	else
-		wait_for_completion_io(&done);
+	blk_wait_io(&done);
 
 	return blk_status_to_errno(bio->bi_status);
 }
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 6abb4ce46baa1..45f994c100446 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -21,7 +21,6 @@
 #include <linux/llist.h>
 #include <linux/cpu.h>
 #include <linux/cache.h>
-#include <linux/sched/sysctl.h>
 #include <linux/sched/topology.h>
 #include <linux/sched/signal.h>
 #include <linux/delay.h>
@@ -1409,22 +1408,10 @@ blk_status_t blk_execute_rq(struct request *rq, bool at_head)
 	blk_mq_insert_request(rq, at_head ? BLK_MQ_INSERT_AT_HEAD : 0);
 	blk_mq_run_hw_queue(hctx, false);
 
-	if (blk_rq_is_poll(rq)) {
+	if (blk_rq_is_poll(rq))
 		blk_rq_poll_completion(rq, &wait.done);
-	} else {
-		/*
-		 * Prevent hang_check timer from firing at us during very long
-		 * I/O
-		 */
-		unsigned long hang_check = sysctl_hung_task_timeout_secs;
-
-		if (hang_check)
-			while (!wait_for_completion_io_timeout(&wait.done,
-					hang_check * (HZ/2)))
-				;
-		else
-			wait_for_completion_io(&wait.done);
-	}
+	else
+		blk_wait_io(&wait.done);
 
 	return wait.ret;
 }
diff --git a/block/blk.h b/block/blk.h
index 7c30e2ac8ebcd..6c2749d122ab5 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -4,6 +4,7 @@
 
 #include <linux/blk-crypto.h>
 #include <linux/memblock.h>	/* for max_pfn/max_low_pfn */
+#include <linux/sched/sysctl.h>
 #include <linux/timekeeping.h>
 #include <xen/xen.h>
 #include "blk-crypto-internal.h"
@@ -71,6 +72,18 @@ static inline int bio_queue_enter(struct bio *bio)
 	return __bio_queue_enter(q, bio);
 }
 
+static inline void blk_wait_io(struct completion *done)
+{
+	/* Prevent hang_check timer from firing at us during very long I/O */
+	unsigned long timeout = sysctl_hung_task_timeout_secs * HZ / 2;
+
+	if (timeout)
+		while (!wait_for_completion_io_timeout(done, timeout))
+			;
+	else
+		wait_for_completion_io(done);
+}
+
 #define BIO_INLINE_VECS 4
 struct bio_vec *bvec_alloc(mempool_t *pool, unsigned short *nr_vecs,
 		gfp_t gfp_mask);
-- 
2.34.1

Re: [PATCHv3 3/4] block: introduce io wait hang check helper

[Ming Lei]

On Fri, Feb 23, 2024 at 3:20 AM Keith Busch <kbusch@meta.com> wrote:
>
> From: Keith Busch <kbusch@kernel.org>
>
> This is the same in two places, and another will be added soon.
> Create a helper for it.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>

On Fri, Feb 23, 2024 at 3:21 AM Keith Busch <kbusch@meta.com> wrote:
>
> From: Keith Busch <kbusch@kernel.org>
>
> Use consistent coding style in this file. All the other loops for the
> same purpose use "while (nr_sects)", so they win.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Re: [PATCHv3 3/4] block: introduce io wait hang check helper

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

(ven better would be a task state to not trigger the hang check, but..)

[PATCHv3 4/4] blk-lib: check for kill signal

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Some of these block operations can access a significant capacity and
take longer than the user expected. A user may change their mind about
wanting to run that command and attempt to kill the process and do
something else with their device. But since the task is uninterruptable,
they have to wait for it to finish, which could be many hours.

Check for a fatal signal at each iteration so the user doesn't have to
wait for their regretted operation to complete naturally.

Reported-by: Conrad Meyer <conradmeyer@meta.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-lib.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index d4c476cf3784a..9e594f641ce72 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -35,6 +35,23 @@ static sector_t bio_discard_limit(struct block_device *bdev, sector_t sector)
 	return round_down(UINT_MAX, discard_granularity) >> SECTOR_SHIFT;
 }
 
+static void abort_bio_endio(struct bio *bio)
+{
+	complete(bio->bi_private);
+	bio_put(bio);
+}
+
+static void abort_bio(struct bio *bio)
+{
+	DECLARE_COMPLETION_ONSTACK_MAP(done,
+			bio->bi_bdev->bd_disk->lockdep_map);
+
+	bio->bi_private = &done;
+	bio->bi_end_io = abort_bio_endio;
+	bio_endio(bio);
+	blk_wait_io(&done);
+}
+
 int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask, struct bio **biop)
 {
@@ -77,6 +94,10 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		 * is disabled.
 		 */
 		cond_resched();
+		if (fatal_signal_pending(current)) {
+			abort_bio(bio);
+			return -EINTR;
+		}
 	}
 
 	*biop = bio;
@@ -143,6 +164,10 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev,
 		nr_sects -= len;
 		sector += len;
 		cond_resched();
+		if (fatal_signal_pending(current)) {
+			abort_bio(bio);
+			return -EINTR;
+		}
 	}
 
 	*biop = bio;
@@ -187,6 +212,10 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev,
 				break;
 		}
 		cond_resched();
+		if (fatal_signal_pending(current)) {
+			abort_bio(bio);
+			return -EINTR;
+		}
 	}
 
 	*biop = bio;
@@ -329,6 +358,12 @@ int blkdev_issue_secure_erase(struct block_device *bdev, sector_t sector,
 		sector += len;
 		nr_sects -= len;
 		cond_resched();
+		if (fatal_signal_pending(current)) {
+			abort_bio(bio);
+			ret = -EINTR;
+			bio = NULL;
+			break;
+		}
 	}
 	if (bio) {
 		ret = submit_bio_wait(bio);
-- 
2.34.1

Re: [PATCHv3 4/4] blk-lib: check for kill signal

[Ming Lei]

On Fri, Feb 23, 2024 at 3:20 AM Keith Busch <kbusch@meta.com> wrote:
>
> From: Keith Busch <kbusch@kernel.org>
>
> Some of these block operations can access a significant capacity and
> take longer than the user expected. A user may change their mind about
> wanting to run that command and attempt to kill the process and do
> something else with their device. But since the task is uninterruptable,
> they have to wait for it to finish, which could be many hours.
>
> Check for a fatal signal at each iteration so the user doesn't have to
> wait for their regretted operation to complete naturally.
>
> Reported-by: Conrad Meyer <conradmeyer@meta.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Looks fine,

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Re: [PATCHv3 4/4] blk-lib: check for kill signal

[Christoph Hellwig]

> +static void abort_bio(struct bio *bio)
> +{
> +	DECLARE_COMPLETION_ONSTACK_MAP(done,
> +			bio->bi_bdev->bd_disk->lockdep_map);
> +
> +	bio->bi_private = &done;
> +	bio->bi_end_io = abort_bio_endio;
> +	bio_endio(bio);
> +	blk_wait_io(&done);
> +}

Without seeing the context below this looks pretty weird, but once
seeing that we're waiting for the previously submitted bios it all
makes sense.  Please write a a good comment explaining this, and
maybe also use a name that is a bit more descriptive.

Re: [PATCHv3 4/4] blk-lib: check for kill signal

[Nilay Shroff]

On 2/23/24 00:49, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Some of these block operations can access a significant capacity and
> take longer than the user expected. A user may change their mind about
> wanting to run that command and attempt to kill the process and do
> something else with their device. But since the task is uninterruptable,
> they have to wait for it to finish, which could be many hours.
> 
> Check for a fatal signal at each iteration so the user doesn't have to
> wait for their regretted operation to complete naturally.
> 
> Reported-by: Conrad Meyer <conradmeyer@meta.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  block/blk-lib.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index d4c476cf3784a..9e594f641ce72 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -35,6 +35,23 @@ static sector_t bio_discard_limit(struct block_device *bdev, sector_t sector)
>  	return round_down(UINT_MAX, discard_granularity) >> SECTOR_SHIFT;
>  }
>  
> +static void abort_bio_endio(struct bio *bio)
> +{
> +	complete(bio->bi_private);
> +	bio_put(bio);
> +}
> +
> +static void abort_bio(struct bio *bio)
> +{
> +	DECLARE_COMPLETION_ONSTACK_MAP(done,
> +			bio->bi_bdev->bd_disk->lockdep_map);
> +
> +	bio->bi_private = &done;
> +	bio->bi_end_io = abort_bio_endio;
> +	bio_endio(bio);
> +	blk_wait_io(&done);
> +}
> +

> @@ -143,6 +164,10 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev,
>  		nr_sects -= len;
>  		sector += len;
>  		cond_resched();
> +		if (fatal_signal_pending(current)) {
> +			abort_bio(bio);
> +			return -EINTR;
> +		}
>  	}
>  
>  	*biop = bio;
> @@ -187,6 +212,10 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev,
>  				break;
>  		}
>  		cond_resched();
> +		if (fatal_signal_pending(current)) {
> +			abort_bio(bio);
> +			return -EINTR;
> +		}
>  	}
>  

If a device with large capacity supports write zero offload and user kills that
long outstanding write zero operation then it appears we run through the fatal_signal_pending()
and abort_bio() twice: once under __blkdev_issue_write_zeroes() and then latter under 
__blkdev_issue_zero_pages(). The entry to __blkdev_issue_zero_pages() happens if 
__blkdev_issue_write_zeroes() returns the error code and BLKDEV_ZERO_NOFALLBACK is NOT 
specified in flags.

I think if fatal signal is intercepted while running __blkdev_issue_write_zeroes() then we 
shouldn't need to re-enter the __blkdev_issue_zero_pages(). We may want to add following code:

@@ -280,7 +306,7 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
                bio_put(bio);
        }
        blk_finish_plug(&plug);
-       if (ret && try_write_zeroes) {
+       if (ret && ret != -EINTR && try_write_zeroes) {
                if (!(flags & BLKDEV_ZERO_NOFALLBACK)) {
                        try_write_zeroes = false;
                        goto retry;

Thanks,
--Nilay

Re: [PATCHv3 4/4] blk-lib: check for kill signal

[Keith Busch]

On Fri, Feb 23, 2024 at 04:30:14PM +0530, Nilay Shroff wrote:
> I think if fatal signal is intercepted while running __blkdev_issue_write_zeroes() then we 
> shouldn't need to re-enter the __blkdev_issue_zero_pages(). We may want to add following code:
> 
> @@ -280,7 +306,7 @@ int blkdev_issue_zeroout(struct block_device *bdev, sector_t sector,
>                 bio_put(bio);
>         }
>         blk_finish_plug(&plug);
> -       if (ret && try_write_zeroes) {
> +       if (ret && ret != -EINTR && try_write_zeroes) {
>                 if (!(flags & BLKDEV_ZERO_NOFALLBACK)) {
>                         try_write_zeroes = false;
>                         goto retry;

Good point, I'll fold this in.

Re: [PATCHv3 0/4] block: make long runnint operations killable

[Nilay Shroff]

On 2/23/24 00:49, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Changes from v2:
> 
>   Wait for chained bio's to complete before returning from kill signal.
> 
>   Miscellaneous cleanup patches for style and consistency.
> 
> Keith Busch (4):
>   block: blkdev_issue_secure_erase loop style
>   block: cleanup __blkdev_issue_write_zeroes
>   block: io wait hang check helper
>   blk-lib: check for kill signal
> 
>  block/bio.c     | 12 +----------
>  block/blk-lib.c | 57 ++++++++++++++++++++++++++++++++++++++-----------
>  block/blk-mq.c  | 19 +++--------------
>  block/blk.h     | 13 +++++++++++
>  4 files changed, 61 insertions(+), 40 deletions(-)
> 

I tried applying patchset using "git am" but it failed to apply. Was the patchset 
created against the latest v6.8-rc5? Latter I applied the patchet manually. 

I have tested the changes on my NVMe with ~1.5 TB capacity and verified that
the patch fixed the reported issue.

NVMe details:

# nvme id-ns /dev/nvme0n1 -H 
NVME Identify Namespace 1:
nsze    : 0x1749a956
ncap    : 0x1749a956
nuse    : 0x1749a956
<snip>
nvmcap  : 1600321314816
<snip>
LBA Format  0 : Metadata Size: 0   bytes - Data Size: 4096 bytes - Relative Performance: 0 Best (in use)
<snip>

# cat /sys/block/nvme0n1/queue/write_zeroes_max_bytes 
8388608
# cat /sys/block/nvme0n1/queue/discard_granularity 
4096
# cat /sys/block/nvme0n1/queue/discard_max_bytes 
2199023255040
 
I tested following cases:
1) Zero-fill all sectors of NVMe using blkdiscard; While test is running 
   kill the 'blkdiscard' from other terminal and check the return status

   # blkdiscars -z /dev/nvme0n1
   Killed
   # echo $?
   137

2) Discard all sectors of NVMe using blkdiscard, While test is running kill 
   the 'blkdiscard' from other terminal

   # blkdiscard /dev/nvme0n1
   Killed
   #echo $?
   137

The bash return the status code 137 which signifies that 'blkdiscard' is killed.

My NVMe doesn't support secure erase operation so I couldn't test it. 
   # blkdiscard -s /dev/nvme0n1 
   blkdiscard: /dev/nvme0n1: BLKSECDISCARD ioctl failed: Operation not supported

Feel free to add:

Tested-by: Nilay Shroff<nilay@linux.ibm.com>

Thanks,
--Nilay

Re: [PATCHv3 0/4] block: make long runnint operations killable

[Keith Busch]

On Fri, Feb 23, 2024 at 04:55:51PM +0530, Nilay Shroff wrote:
> I tried applying patchset using "git am" but it failed to apply. Was the patchset 
> created against the latest v6.8-rc5? Latter I applied the patchet manually. 

Patchset was created from linux-block tree's for-6.9/block branch:

  https://git.kernel.dk/cgit/linux-block/log/?h=for-6.9/block