Re: [PATCH RFC] ext4: never fall back to buffered I/O for atomic direct writes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch fails to return an error for short atomic writes, silently returning a torn write instead.

Pre-existing issues:
- [High] Silent fallback to non-atomic buffered I/O early in the function when `ext4_should_use_dio()` returns false.
--

commit 4f1077482a95547df7986c873d9a1c2cff493fea
Author: Tal Zussman <tz2294@columbia.edu>
ext4: never fall back to buffered I/O for atomic direct writes

This commit modifies the direct I/O path in ext4 to explicitly prevent falling
back to buffered I/O for atomic writes. It intends to return an error for short
atomic writes instead of completing them through the page cache and issuing
a warning.

Link: https://lore.kernel.org/linux-block/20260828-blkdev-fixes-v2-0-32f3f40cebed@columbia.edu/ [1]
Link: https://lore.kernel.org/linux-block/20260907071217.GD934@lst.de/ [2]
Link: https://lore.kernel.org/linux-block/a7752d1e-8ec1-44d1-a266-c287c315e8ab@oracle.com/ [3]

> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 374b4bc25bd5d..fb561c701d291 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -666,17 +666,16 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  	else
>  		inode_unlock(inode);
>  
> -	if (ret >= 0 && iov_iter_count(from)) {
> +	/*
> +	 * Buffered I/O provides no torn-write protection, so never finish an
> +	 * atomic write through the page cache. iomap should not return a short
> +	 * atomic write anyway.
> +	 */
> +	if (ret >= 0 && iov_iter_count(from) &&
> +	    !(iocb->ki_flags & IOCB_ATOMIC)) {

[Severity: High]
Does this unintentionally return a short torn write instead of an error?

The commit message states the patch "yields an error rather than a torn
write with a WARN()". However, if iomap_dio_rw() completes a short atomic
write, where ret is greater than 0 but less than the requested count, we
skip this fallback block entirely because !(iocb->ki_flags & IOCB_ATOMIC)
evaluates to false.

Since we skip the block, the function proceeds to the end and returns the
positive short byte count in ret to the VFS. This exposes a torn write to
userspace without returning an error. Should we explicitly return an error
like -EIO or -EFAULT when a short atomic write occurs?

>  		ssize_t err;
>  		loff_t endbyte;
>  
> -		/*
> -		 * There is no support for atomic writes on buffered-io yet,
> -		 * we should never fallback to buffered-io for DIO atomic
> -		 * writes.
> -		 */
> -		WARN_ON_ONCE(iocb->ki_flags & IOCB_ATOMIC);
> -
>  		offset = iocb->ki_pos;
>  		err = ext4_buffered_write_iter(iocb, from);
>  		if (err < 0)

[Severity: High]
This is a pre-existing issue, but it looks like we might silently downgrade
atomic writes to non-atomic buffered writes if the file doesn't support DIO.

Earlier in ext4_dio_write_iter(), there is an initial fallback:

    /* Fallback to buffered I/O if the inode does not support direct I/O. */
    if (!ext4_should_use_dio(iocb, from)) {
        if (ilock_shared)
            inode_unlock_shared(inode);
        else
            inode_unlock(inode);
        return ext4_buffered_write_iter(iocb, from);
    }

If a user requests an atomic write on an ext4 file that does not support
DIO (such as a file with data journaling enabled), ext4_should_use_dio()
will return false. This causes an early fallback to
ext4_buffered_write_iter().

Because ext4_buffered_write_iter() does not check for IOCB_ATOMIC, it
proceeds with a standard buffered write via generic_perform_write().
Does this silently break the atomicity guarantees expected by userspace
for such files?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-ext4-atomic-no-fallback-v1-1-5a6b9539c002@columbia.edu?part=1