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

[Jens Axboe <axboe@kernel.dk>]

On 1/16/23 10:11 AM, Michael Kelley (LINUX) wrote:
> From: Jens Axboe <axboe@kernel.dk> Sent: Monday, January 16, 2023 8:02 AM
>>
>> 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>
>>
>> ---
>>
>> diff --git a/block/fops.c b/block/fops.c
>> index 50d245e8c913..a03cb732c2a7 100644
>> --- a/block/fops.c
>> +++ b/block/fops.c
>> @@ -368,6 +368,14 @@ static ssize_t blkdev_direct_IO(struct kiocb *iocb, struct
>> iov_iter *iter)
>>  			return __blkdev_direct_IO_simple(iocb, iter, nr_pages);
>>  		return __blkdev_direct_IO_async(iocb, iter, nr_pages);
>>  	}
>> +	/*
>> +	 * We're doing more than a bio worth of IO (> 256 pages), and we
>> +	 * cannot guarantee that one of the sub bios will not fail getting
>> +	 * issued FOR NOWAIT as error results are coalesced across all of
>> +	 * them. Be safe and ask for a retry of this from blocking context.
>> +	 */
>> +	if (iocb->ki_flags & IOCB_NOWAIT)
>> +		return -EAGAIN;
>>  	return __blkdev_direct_IO(iocb, iter, bio_max_segs(nr_pages));
>>  }
> 
> A code observation:  __blkdev_direct_IO() has a test for IOCB_NOWAIT
> that now can't happen, as this is the only place it is called.  But maybe it's
> safer to leave the check in case of future code shuffling.

I think we should just keep it, or it will get missed later on. I am
pondering how we could make this better, but it's a bit more involved.

-- 
Jens Axboe