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

[Nilay Shroff <nilay@linux.ibm.com>]

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