mbox series

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

Message ID 20240223155910.3622666-1-kbusch@meta.com (mailing list archive)
Headers show
Series block: make long running operations killable | expand

Message

Keith Busch Feb. 23, 2024, 3:59 p.m. UTC
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(-)

Comments

Chaitanya Kulkarni Feb. 23, 2024, 7:16 p.m. UTC | #1
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
Keith Busch Feb. 23, 2024, 7:31 p.m. UTC | #2
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"
Jens Axboe Feb. 24, 2024, 7:48 p.m. UTC | #3
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,
Chaitanya Kulkarni Feb. 25, 2024, 7:40 a.m. UTC | #4
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
Chaitanya Kulkarni Feb. 25, 2024, 7:53 a.m. UTC | #5
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
>
>