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

[Jan Kara]

On Wed 09-09-26 16:56:17, Tal Zussman wrote:
> ext4_dio_write_iter() finishes a short direct write through the page
> cache. For an atomic write that path only warns and then continues,
> which would tear the write.
> 
> This came out of a block device fix [1], where an IOCB_ATOMIC direct
> write could complete short when only part of the buffer could be
> pinned, with the rest going through the buffered fallback. Christoph
> Hellwig noted that ext4 has the same fallback and only warns in it [2],
> and John Garry agreed it should reject the write instead [3].
> 
> In contrast to the block device, nothing reaches the branch on ext4
> today: iomap requires the mapping to cover the whole atomic write, a
> buffer that can't be pinned in full fails in iomap before the bio is
> submitted, and a failed page cache invalidation returns -EAGAIN rather
> than reaching the buffered fallback. Exclude atomic writes from the
> fallback outright instead of WARN()-ing inside it, so any future bug
> yields an error rather than a torn write with a WARN().
> 
> Link: https://lore.kernel.org/linux-block/20260828-blkdev-fixes-v2-0-32f3f40cebed@xxxxxxxxxxxx/ [1]
> Link: https://lore.kernel.org/linux-block/20260907071217.GD934@xxxxxx/ [2]
> Link: https://lore.kernel.org/linux-block/a7752d1e-8ec1-44d1-a266-c287c315e8ab@xxxxxxxxxx/ [3]
> Signed-off-by: Tal Zussman <tz2294@xxxxxxxxxxxx>

I agree your patch makes things better but I have a question: are short
writes (even 0) allowed for atomic writes? I don't think so as far as my
understanding of atomic writes goes (and based on what I could find in the
code). But after your change we'd just return short write (including ret ==
0) now which doesn't look ideal and also doesn't match your "so any future
bug yields an error rather than a torn write with a WARN()" description. So
as a futureproofing I'd maybe suggest just:

	if (WARN_ON_ONCE(iocb->ki_flags & IOCB_ATOMIC && ret >= 0 &&
			 iov_iter_count(from)))
		ret = -EIO; /* or maybe -ENOPROTOOPT??? */

before the fallback to buffered IO as a more robust solution.

								Honza

> ---
>  fs/ext4/file.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 374b4bc25bd5..fb561c701d29 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)) {
>  		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)
> 
> ---
> base-commit: df2908090cda368b01ff43709f51890076c56157
> change-id: 20260909-ext4-atomic-no-fallback-b1550a7a3fcf
> 
> Best regards,
> --  
> Tal Zussman <tz2294@xxxxxxxxxxxx>
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR