Re: [PATCH v3 5/7] block: fail atomic writes instead of falling back to buffered I/O

[Tal Zussman]

On 9/10/26 2:40 AM, John Garry wrote:
> On 9/9/26 23:05, Tal Zussman wrote:
>> An IOCB_ATOMIC direct write to a block device can silently lose its
>> torn-write guarantee in two ways:
>> 
>>    1. blkdev_direct_write() turns an -EBUSY from page cache invalidation
>>       into a 0 return, so the whole write is retried through
>>       blkdev_buffered_write(), with no atomicity guarantee.
>> 
>>    2. On a partial page pin, __blkdev_direct_IO_simple() and
>>       __blkdev_direct_IO_async() submit what was pinned with REQ_ATOMIC
>>       set and leave the rest to the buffered fallback.
> 
> This really should be 2x separate changes - 1x for fops.c and 1x for bio.c
> 

Will split for v4.

>> 
>> The second case can be triggered deterministically. A 16K
>> pwritev2(RWF_ATOMIC) whose last page is PROT_NONE, on a scsi_debug
>> device with atomic_wr=1, completes short with only three of the four
>> pages written, violating RWF_ATOMIC semantics.
>> 
>> Fail the I/O instead. Make bio_iov_iter_get_pages() release the pins
>> and return -EINVAL when a REQ_ATOMIC bio doesn't cover the whole
>> iterator, since an atomic write is submitted as a single bio and a
>> short one would be torn. That covers iomap as well, where a partially
>> unmapped buffer could trip the WARN_ON_ONCE() in
>> iomap_dio_bio_iter_one(). The async block device path currently sets
>> REQ_ATOMIC after pinning, so set it before.
>> 
>> Skip the buffered fallback in blkdev_write_iter() for IOCB_ATOMIC, as
>> it already does for IOCB_NOWAIT, so the -EBUSY case returns -EAGAIN and
>> the caller retries, matching __iomap_dio_rw().
>> 
>> ext4 has the same fallback and only warns in it. For block devices both
>> ways in can be detected before any I/O is submitted, so fail early instead.
>> 
>> Fixes: caf336f81b3a ("block: Add fops atomic write support")
>> Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
>> Link: https://sashiko.dev/#/patchset/20260802-blkdev-fixes-v1-0-a82fc549fd74%40columbia.edu?part=2
>> Assisted-by: Claude:claude-fable-5
>> Signed-off-by: Tal Zussman <tz2294@xxxxxxxxxxxx>
>> ---
>>   block/bio.c  | 29 ++++++++++++++++++++++-------
>>   block/fops.c | 10 +++++-----
>>   2 files changed, 27 insertions(+), 12 deletions(-)
>> 
>> diff --git a/block/bio.c b/block/bio.c
>> index 898b2f5ef8c8..63e266d861f1 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -1284,6 +1284,7 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter,
>>   			   unsigned mem_align_mask, unsigned len_align_mask)
>>   {
>>   	iov_iter_extraction_t flags = 0;
>> +	int ret;
>>   
>>   	if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
>>   		return -EIO;
>> @@ -1303,34 +1304,48 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter,
>>   		flags |= ITER_ALLOW_P2PDMA;
>>   
>>   	do {
>> -		ssize_t ret;
>> +		ssize_t len;
> 
> iov_iter_extract_bvecs() local variable is called "size", so maybe use
> the same here
> 

Will change.

>>   
>> -		ret = iov_iter_extract_bvecs(iter, bio->bi_io_vec,
>> +		len = iov_iter_extract_bvecs(iter, bio->bi_io_vec,
>>   				BIO_MAX_SIZE - bio->bi_iter.bi_size,
>>   				&bio->bi_vcnt, bio->bi_max_vecs,
>>   				mem_align_mask, flags);
>> -		if (ret <= 0) {
>> +		if (len <= 0) {
>>   			/*
>>   			 * A misaligned vector fails the whole I/O.  Release any
>>   			 * pages pinned by earlier iterations before returning
>>   			 * since this bio won't be submitted to release them.
>>   			 */
>> -			if (ret == -EINVAL) {
>> +			if (len == -EINVAL) {
>>   				bio_release_pages(bio, false);
>>   				bio_clear_flag(bio, BIO_PAGE_PINNED);
>>   				bio->bi_vcnt = 0;
>>   			}
>>   			if (!bio->bi_vcnt)
>> -				return ret;
>> +				return len;
>>   			break;
>>   		}
>> -		bio->bi_iter.bi_size += ret;
>> +		bio->bi_iter.bi_size += len;
>>   	} while (iov_iter_count(iter) && !bio_full(bio, 0));
>>   
>>   	if (is_pci_p2pdma_page(bio->bi_io_vec->bv_page))
>>   		bio->bi_opf |= REQ_NOMERGE;
>> -	return bio_iov_iter_align_down(bio, iter,
>> +	ret = bio_iov_iter_align_down(bio, iter,
>>   			&bio->bi_io_vec[bio->bi_vcnt - 1], len_align_mask);
>> +	if (ret)
>> +		return ret;
>   > +> +	/*
>> +	 * An atomic write is submitted as a single bio, so it has to cover
>> +	 * the whole iterator or it would be torn.
>> +	 */
>> +	if ((bio->bi_opf & REQ_ATOMIC) && iov_iter_count(iter)) {
>> +		bio_release_pages(bio, false);
>> +		bio_clear_flag(bio, BIO_PAGE_PINNED);
>> +		bio->bi_vcnt = 0;
>> +		return -EINVAL;
>> +	}
> 
> This all looks ok, but I'll check again ...
> 
>> +	return 0;
>>   }
> 
> iomap_dio_bio_iter_one() can be updated at some stage to remove its own
> check for improper length returned from bio_iov_iter_get_pages() for
> IOCB_ATOMIC
> 

Yes, although I think bio_iov_iter_bounce_write() may need to have a
similar check introduced before that can be done, but I haven't looked at
it closely enough yet...

>>   
>>   static struct folio *folio_alloc_greedy(gfp_t gfp, size_t *size,
>> diff --git a/block/fops.c b/block/fops.c
>> index a3a709697b40..0b614d76d128 100644
>> --- a/block/fops.c
>> +++ b/block/fops.c
>> @@ -341,6 +341,8 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
>>   	bio->bi_write_stream = iocb->ki_write_stream;
>>   	bio->bi_end_io = blkdev_bio_end_io_async;
>>   	bio->bi_ioprio = iocb->ki_ioprio;
>> +	if (iocb->ki_flags & IOCB_ATOMIC)
>> +		bio->bi_opf |= REQ_ATOMIC;
>>   
>>   	/*
>>   	 * Users don't rely on the iterator being in any particular
>> @@ -371,9 +373,6 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
>>   			goto out_bio_put;
>>   	}
>>   
>> -	if (iocb->ki_flags & IOCB_ATOMIC)
>> -		bio->bi_opf |= REQ_ATOMIC;
>> -
>>   	if (iocb->ki_flags & IOCB_NOWAIT)
>>   		bio->bi_opf |= REQ_NOWAIT;
> 
> I think that you relocate this as well to have similar functionality
> co-located
> 

Will do.

>>   
>> @@ -766,10 +765,11 @@ static ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
>>   	if (iocb->ki_flags & IOCB_DIRECT) {
>>   		ret = blkdev_direct_write(iocb, from);
>>   		if (ret >= 0 && iov_iter_count(from)) {
>> -			if (iocb->ki_flags & IOCB_NOWAIT) {
>> +			if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_ATOMIC)) {
> 
> An alternative could be to have iomap_file_buffered_write() reject
> IOCB_ATOMIC.

Yeah, I think that could make sense as an additional change. But the check
here is nice because it stops us from taking i_rwsem unnecessarily and we
already need to check for IOCB_NOWAIT, so I'll leave it like this for now.

Thanks for reviewing!

> 
>>   				/*
>>   				 * The buffered fallback blocks on i_rwsem and
>> -				 * on writeback of the data it copied: return
>> +				 * on writeback of the data it copied, and
>> +				 * can't provide torn-write protection: return
>>   				 * the short direct write instead and let the
>>   				 * caller retry.
>>   				 */
>