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

[Tal Zussman]

On 9/7/26 10:49 AM, John Garry wrote:
> On 28/08/2026 14:49, 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.
>> 
>> The second case can be triggered deterministically. A 16K
>> pwritev2(RWF_ATOMIC) whose last page is PROT_NONE, on a scsi_debug
> 
> If there is some scenario which does not allow the iovec to be written
> atomically for RWF_ATOMIC, then we should document it in the man pages
> description of RWF_ATOMIC.
> 
>> device with atomic_wr=1, completes short with only three of the four
>> pages written, violating RWF_ATOMIC semantics.
>> 
>> Fail the I/O instead. Return -EAGAIN when page cache invalidation fails
>> for IOCB_ATOMIC rather than retrying through the page cache, matching
>> __iomap_dio_rw(), which treats the failure as transient and lets the
>> caller retry. Release a short atomic pin and return -EFAULT before
>> submission, which is what a direct write already returns when none of
>> the buffer can be pinned. A sync atomic write can then never return
>> short with a remainder, so the buffered fallback is never reached.
>> 
>> ext4 has the same fallback and only warns in it. 
> 
> It should reject it, as IOCB_ATOMIC would be ignored in that path.
> 

I'll send an email to the ext4 maintainers as Christoph suggested.

>> 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/fops.c | 21 ++++++++++++++++++++-
>>    1 file changed, 20 insertions(+), 1 deletion(-)
>> 
>> diff --git a/block/fops.c b/block/fops.c
>> index a3a709697b40..8769bb13df1c 100644
>> --- a/block/fops.c
>> +++ b/block/fops.c
>> @@ -87,6 +87,12 @@ static ssize_t __blkdev_direct_IO_simple(struct kiocb *iocb,
>>    	ret = blkdev_iov_iter_get_pages(&bio, iter, bdev);
>>    	if (unlikely(ret))
>>    		goto out;
>> +	if ((iocb->ki_flags & IOCB_ATOMIC) && iov_iter_count(iter)) {
> 
> Could we even move this check into bio_iov_iter_get_pages()?
> bio_iov_iter_get_pages() is used in fs/iomap/direct-io.c in the same
> fashion, i.e. it's expected to be iter'ed only once for IOCB_ATOMIC.
> 
> If bio_iov_iter_get_pages() does not give all the pages for REQ_ATOMIC,
> then something is wrong and we should error. For this to work, we must
> ensure that bio_iov_iter_get_pages() is only called once for a
> REQ_ATOMIC bio - that would be the semantic.
> 

Yes, I'll move the check right after bio_iov_iter_align_down() in
bio_iov_iter_get_pages(). That ends up looking cleaner.

>> +		/* a short atomic write would be torn by definition */
>> +		bio_release_pages(&bio, false);
>> +		ret = -EFAULT;
> 
> Eh, generally we return -EINVAL for something which can't be written
> atomically - like in iomap_dio_bio_iter_one(). -EFAULT is not documented
> for RWF_ATOMIC (afair).
> 

Sure I'll change this to -EINVAL.

>> +		goto out;
>> +	}
>>    	ret = bio.bi_iter.bi_size;
>>    
>>    	if (iov_iter_rw(iter) == WRITE)
>> @@ -352,6 +358,12 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
>>    		ret = blkdev_iov_iter_get_pages(bio, iter, bdev);
>>    		if (unlikely(ret))
>>    			goto out_bio_put;
>> +		if ((iocb->ki_flags & IOCB_ATOMIC) && iov_iter_count(iter)) {
> 
> At least this check could be factored out of
> __blkdev_direct_IO_simple(), right?
> 

It's gone with the change to bio_iov_iter_get_pages().

>> +			/* a short atomic write would be torn by definition */
>> +			bio_release_pages(bio, false);
>> +			ret = -EFAULT;
>> +			goto out_bio_put;
>> +		}
>>    	}
>>    	dio->size = bio->bi_iter.bi_size;
>>    
>> @@ -691,8 +703,15 @@ blkdev_direct_write(struct kiocb *iocb, struct iov_iter *from)
>>
>>    	written = kiocb_invalidate_pages(iocb, count);
>>    	if (written) {
>> -		if (written == -EBUSY)
>> +		/*
>> +		 * The buffered write fallback cannot provide torn-write
>> +		 * protection, so atomic writes must fail instead.
>> +		 */
> 
> Would it be better to have this check in direct_write_fallback(), i.e.
> always -EAGAIN in direct_write_fallback() for IOCB_ATOMIC?
> 

This wouldn't work since direct_write_fallback() takes the result of the
buffered write as an argument, so the write will have already happened.
Instead, we can skip the fallback in blkdev_write_iter(), like we do for
IOCB_NOWAIT in patch 2. The new check in bio_iov_iter_get_pages() ensures
we get there with nothing written to disk for bad IOCB_ATOMIC so it should
be safe to skip.

Thanks for the review!

>> +		if (written == -EBUSY) {
>> +			if (iocb->ki_flags & IOCB_ATOMIC)
>> +				return -EAGAIN;
>>    			return 0;
>> +		}
>>    		return written;
>>    	}
>>    
>> 
>> -- 
>> 2.39.5
>