[PATCHv4 0/4] block: make long running operations killable

[PATCHv4 0/4] block: make long running operations killable

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

Changes from v3:

 Added reviewed and tested by tags

 More formatting cleanups in patch 2 (Christoph)

 A more descriptive name for the bio chain wait helper (Christoph)

 Don't fallback to the zero page on fatal signal error (Nilay)

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 | 70 ++++++++++++++++++++++++++++++++++++-------------
 block/blk-mq.c  | 19 +++-----------
 block/blk.h     | 13 +++++++++
 4 files changed, 69 insertions(+), 45 deletions(-)

-- 
2.34.1

[PATCHv4 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.

Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
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

[PATCHv4 2/4] block: cleanup __blkdev_issue_write_zeroes

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

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

Reviewed-by: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-lib.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 91770da2239f2..a6954eafb8c8a 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -120,31 +120,28 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev,
 		struct bio **biop, unsigned flags)
 {
 	struct bio *bio = *biop;
-	unsigned int max_write_zeroes_sectors;
+	unsigned int max_sectors;
 
 	if (bdev_read_only(bdev))
 		return -EPERM;
 
-	/* Ensure that max_write_zeroes_sectors doesn't overflow bi_size */
-	max_write_zeroes_sectors = bdev_write_zeroes_sectors(bdev);
+	/* Ensure that max_sectors doesn't overflow bi_size */
+	max_sectors = bdev_write_zeroes_sectors(bdev);
 
-	if (max_write_zeroes_sectors == 0)
+	if (max_sectors == 0)
 		return -EOPNOTSUPP;
 
 	while (nr_sects) {
+		unsigned int len = min_t(sector_t, nr_sects, max_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 << SECTOR_SHIFT;
+		nr_sects -= len;
+		sector += len;
 		cond_resched();
 	}
 
-- 
2.34.1

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

[Christoph Hellwig]

Looks good:

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

[PATCHv4 3/4] block: 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.

Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
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

[PATCHv4 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>
Tested-by: Nilay Shroff<nilay@linux.ibm.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-lib.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index a6954eafb8c8a..dc8e35d0a51d6 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -35,6 +35,26 @@ static sector_t bio_discard_limit(struct block_device *bdev, sector_t sector)
 	return round_down(UINT_MAX, discard_granularity) >> SECTOR_SHIFT;
 }
 
+static void await_bio_endio(struct bio *bio)
+{
+	complete(bio->bi_private);
+	bio_put(bio);
+}
+
+/*
+ * await_bio_chain - ends @bio and waits for every chained bio to complete
+ */
+static void await_bio_chain(struct bio *bio)
+{
+	DECLARE_COMPLETION_ONSTACK_MAP(done,
+			bio->bi_bdev->bd_disk->lockdep_map);
+
+	bio->bi_private = &done;
+	bio->bi_end_io = await_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 +97,10 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		 * is disabled.
 		 */
 		cond_resched();
+		if (fatal_signal_pending(current)) {
+			await_bio_chain(bio);
+			return -EINTR;
+		}
 	}
 
 	*biop = bio;
@@ -143,6 +167,10 @@ static int __blkdev_issue_write_zeroes(struct block_device *bdev,
 		nr_sects -= len;
 		sector += len;
 		cond_resched();
+		if (fatal_signal_pending(current)) {
+			await_bio_chain(bio);
+			return -EINTR;
+		}
 	}
 
 	*biop = bio;
@@ -187,6 +215,10 @@ static int __blkdev_issue_zero_pages(struct block_device *bdev,
 				break;
 		}
 		cond_resched();
+		if (fatal_signal_pending(current)) {
+			await_bio_chain(bio);
+			return -EINTR;
+		}
 	}
 
 	*biop = bio;
@@ -277,7 +309,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;
@@ -329,6 +361,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)) {
+			await_bio_chain(bio);
+			ret = -EINTR;
+			bio = NULL;
+			break;
+		}
 	}
 	if (bio) {
 		ret = submit_bio_wait(bio);
-- 
2.34.1

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

[Christoph Hellwig]

Looks good:

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

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

[Nilay Shroff]

Looks good!

Reviewed-by : Nilay Shroff<nilay@linux.ibm.com)

Re: [PATCHv4 0/4] block: make long running operations killable

[Chaitanya Kulkarni]

On 2/23/24 07:59, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> Changes from v3:
>
>   Added reviewed and tested by tags
>
>   More formatting cleanups in patch 2 (Christoph)
>
>   A more descriptive name for the bio chain wait helper (Christoph)
>
>   Don't fallback to the zero page on fatal signal error (Nilay)
>

we need a blktests for this, for whole series :-

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCHv4 0/4] block: make long running operations killable

[Keith Busch]

On Fri, Feb 23, 2024 at 07:16:30PM +0000, Chaitanya Kulkarni wrote:
> On 2/23/24 07:59, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> >
> > Changes from v3:
> >
> >   Added reviewed and tested by tags
> >
> >   More formatting cleanups in patch 2 (Christoph)
> >
> >   A more descriptive name for the bio chain wait helper (Christoph)
> >
> >   Don't fallback to the zero page on fatal signal error (Nilay)
> >
> 
> we need a blktests for this, for whole series :-

How would you know know if the device completed the operation before the
fatal signal or not? This is a quick test script off the top of my head,
but whether or not it shows a difference with this patch series depends
on your device.

  # time sh -c "blkdiscard -z /dev/nvme0n1 & sleep 1 && kill %1 && wait"

Re: [PATCHv4 0/4] block: make long running operations killable

[Chaitanya Kulkarni]

On 2/23/24 11:31, Keith Busch wrote:
> On Fri, Feb 23, 2024 at 07:16:30PM +0000, Chaitanya Kulkarni wrote:
>> On 2/23/24 07:59, Keith Busch wrote:
>>> From: Keith Busch <kbusch@kernel.org>
>>>
>>> Changes from v3:
>>>
>>>    Added reviewed and tested by tags
>>>
>>>    More formatting cleanups in patch 2 (Christoph)
>>>
>>>    A more descriptive name for the bio chain wait helper (Christoph)
>>>
>>>    Don't fallback to the zero page on fatal signal error (Nilay)
>>>
>> we need a blktests for this, for whole series :-
> How would you know know if the device completed the operation before the
> fatal signal or not? This is a quick test script off the top of my head,
> but whether or not it shows a difference with this patch series depends
> on your device.
>
>    # time sh -c "blkdiscard -z /dev/nvme0n1 & sleep 1 && kill %1 && wait"

something like that :-

# record the start time
start_time=get_time_sec

# Use ginormous value for the device size without membacked for null_blk
# such that it will not finish before 20 min
modprobe -r null_blk
modprobe null_blk gb=2024000000 # this should not finish within 20 min

# kill the process
kill -9 `ps aux | grep blkdiscard | grep -v grep | tr -s ' ' ' ' | cut 
-f 2 -d ' '`

# record total time
total_time=get_time_sec - start_time

# make sure process doesn't exists
ps aux | grep -v grep | grep blkdiscard
if [ $? -eq 0]; then
     echo "Test fail"
fi

# make sure total_time < 60 sec
if [ $total_time -gt 60 ]; then
     echo "Fail"
fi

echo "success"

WDYT ?

-ck

Re: [PATCHv4 0/4] block: make long running operations killable

[Chaitanya Kulkarni]

On 2/24/24 23:40, Chaitanya Kulkarni wrote:
> On 2/23/24 11:31, Keith Busch wrote:
>> On Fri, Feb 23, 2024 at 07:16:30PM +0000, Chaitanya Kulkarni wrote:
>>> On 2/23/24 07:59, Keith Busch wrote:
>>>> From: Keith Busch <kbusch@kernel.org>
>>>>
>>>> Changes from v3:
>>>>
>>>>    Added reviewed and tested by tags
>>>>
>>>>    More formatting cleanups in patch 2 (Christoph)
>>>>
>>>>    A more descriptive name for the bio chain wait helper (Christoph)
>>>>
>>>>    Don't fallback to the zero page on fatal signal error (Nilay)
>>>>
>>> we need a blktests for this, for whole series :-
>> How would you know know if the device completed the operation before the
>> fatal signal or not? This is a quick test script off the top of my head,
>> but whether or not it shows a difference with this patch series depends
>> on your device.
>>
>>    # time sh -c "blkdiscard -z /dev/nvme0n1 & sleep 1 && kill %1 && 
>> wait"
>
> something like that :-
>
> # record the start time
> start_time=get_time_sec
>
> # Use ginormous value for the device size without membacked for null_blk
> # such that it will not finish before 20 min
> modprobe -r null_blk
> modprobe null_blk gb=2024000000 # this should not finish within 20 min
>

it got deleted from previous email somehow :-

  blkdiscard -z /dev/nullb0 &

> # kill the process
> kill -9 `ps aux | grep blkdiscard | grep -v grep | tr -s ' ' ' ' | cut 
> -f 2 -d ' '`
>
> # record total time
> total_time=get_time_sec - start_time
>
> # make sure process doesn't exists
> ps aux | grep -v grep | grep blkdiscard
> if [ $? -eq 0]; then
>     echo "Test fail"
> fi
>
> # make sure total_time < 60 sec
> if [ $total_time -gt 60 ]; then
>     echo "Fail"
> fi
>
> echo "success"
>
> WDYT ?
>
> -ck
>
>

Re: [PATCHv4 0/4] block: make long running operations killable

[Jens Axboe]

On Fri, 23 Feb 2024 07:59:06 -0800, Keith Busch wrote:
> Changes from v3:
> 
>  Added reviewed and tested by tags
> 
>  More formatting cleanups in patch 2 (Christoph)
> 
>  A more descriptive name for the bio chain wait helper (Christoph)
> 
> [...]

Applied, thanks!

[1/4] block: blkdev_issue_secure_erase loop style
      commit: 5affe497c346343ecc42e6095b60dafe15e1453e
[2/4] block: cleanup __blkdev_issue_write_zeroes
      commit: 76a27e1b53b94b5a23c221434146fda3e9d8d8e0
[3/4] block: io wait hang check helper
      commit: 0eb4db4706603db09644ec3bc9bb0d63ea5d326c
[4/4] blk-lib: check for kill signal
      commit: 8a08c5fd89b447a7de7eb293a7a274c46b932ba2

Best regards,
-- 
Jens Axboe