Re: [PATCH v2] block: don't allow multiple bios for IOCB_NOWAIT issue

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 1/17/23 06:06, Jens Axboe wrote:
> If we're doing a large IO request which needs to be split into multiple
> bios for issue, then we can run into the same situation as the below
> marked commit fixes - parts will complete just fine, one or more parts
> will fail to allocate a request. This will result in a partially
> completed read or write request, where the caller gets EAGAIN even though
> parts of the IO completed just fine.
> 
> Do the same for large bios as we do for splits - fail a NOWAIT request
> with EAGAIN. This isn't technically fixing an issue in the below marked
> patch, but for stable purposes, we should have either none of them or
> both.
> 
> This depends on: 613b14884b85 ("block: handle bio_split_to_limits() NULL return")
> 
> Cc: stable@vger.kernel.org # 5.15+
> Fixes: 9cea62b2cbab ("block: don't allow splitting of a REQ_NOWAIT bio")
> Link: https://github.com/axboe/liburing/issues/766
> Reported-and-tested-by: Michael Kelley <mikelley@microsoft.com>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> ---
> 
> Since v1: catch this at submit time instead, since we can have various
> valid cases where the number of single page segments will not take a
> bio segment (page merging, huge pages).
> 
> diff --git a/block/fops.c b/block/fops.c
> index 50d245e8c913..d2e6be4e3d1c 100644
> --- a/block/fops.c
> +++ b/block/fops.c
> @@ -221,6 +221,24 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
>  			bio_endio(bio);
>  			break;
>  		}
> +		if (iocb->ki_flags & IOCB_NOWAIT) {
> +			/*
> +			 * This is nonblocking IO, and we need to allocate
> +			 * another bio if we have data left to map. As we
> +			 * cannot guarantee that one of the sub bios will not
> +			 * fail getting issued FOR NOWAIT and as error results
> +			 * are coalesced across all of them, be safe and ask for
> +			 * a retry of this from blocking context.
> +			 */
> +			if (unlikely(iov_iter_count(iter))) {
> +				bio_release_pages(bio, false);
> +				bio_clear_flag(bio, BIO_REFFED);
> +				bio_put(bio);
> +				blk_finish_plug(&plug);
> +				return -EAGAIN;

Doesn't this mean that for a really very large IO request that has 100%
chance of being split, the user will always get -EAGAIN ? Not that I mind,
doing super large IOs with NOWAIT is not a smart thing to do in the first
place... But as a user interface, it seems that this will prevent any
forward progress for such really large NOWAIT IOs. Is that OK ?

> +			}
> +			bio->bi_opf |= REQ_NOWAIT;
> +		}
>  
>  		if (is_read) {
>  			if (dio->flags & DIO_SHOULD_DIRTY)
> @@ -228,9 +246,6 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
>  		} else {
>  			task_io_account_write(bio->bi_iter.bi_size);
>  		}
> -		if (iocb->ki_flags & IOCB_NOWAIT)
> -			bio->bi_opf |= REQ_NOWAIT;
> -
>  		dio->size += bio->bi_iter.bi_size;
>  		pos += bio->bi_iter.bi_size;
>  

-- 
Damien Le Moal
Western Digital Research