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

On 9/7/26 10:49 AM, John Garry wrote:

On 28/08/2026 14:49, Tal Zussman wrote:

quoted

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.

quoted

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.

quoted

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@kernel.org>
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<redacted>
---
   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.

quoted

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

quoted

+		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().

quoted

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

quoted

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

-- 
2.39.5