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

[John Garry]

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.

> 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://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260802-blkdev-
> fixes-v1-0-a82fc549fd74*40columbia.edu?part=2__;IyU!!ACWV5N9M2RV99hQ!
> Ot6CcSo02WbMqinlLCziLAkOQYQGgeUA0ZNlGPWcDEbGge8zNfHyjKzp4_TU7rV9CEVu9_fWOEwt6d_RQBbgVw$<https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260802- 
> blkdev-fixes-v1-0-a82fc549fd74*40columbia.edu?part=2__;IyU!! 
> ACWV5N9M2RV99hQ! 
> Ot6CcSo02WbMqinlLCziLAkOQYQGgeUA0ZNlGPWcDEbGge8zNfHyjKzp4_TU7rV9CEVu9_fWOEwt6d_RQBbgVw$>
> 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.

> +		/* 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).

> +		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?

> +			/* 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?

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