Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx> writes:
> Jan Kara <jack@xxxxxxx> writes:
>
>> On Tue 26-08-25 10:29:58, Ritesh Harjani wrote:
>>> Keith Busch <kbusch@xxxxxxxxxx> writes:
>>>
>>> > On Mon, Aug 25, 2025 at 02:07:15PM +0200, Jan Kara wrote:
>>> >> On Fri 22-08-25 18:57:08, Ritesh Harjani wrote:
>>> >> > Keith Busch <kbusch@xxxxxxxx> writes:
>>> >> > >
>>> >> > > - EXT4 falls back to buffered io for writes but not for reads.
>>> >> >
>>> >> > ++linux-ext4 to get any historical context behind why the difference of
>>> >> > behaviour in reads v/s writes for EXT4 DIO.
>>> >>
>>> >> Hum, how did you test? Because in the basic testing I did (with vanilla
>>> >> kernel) I get EINVAL when doing unaligned DIO write in ext4... We should be
>>> >> falling back to buffered IO only if the underlying file itself does not
>>> >> support any kind of direct IO.
>>> >
>>> > Simple test case (dio-offset-test.c) below.
>>> >
>>> > I also ran this on vanilla kernel and got these results:
>>> >
>>> > # mkfs.ext4 /dev/vda
>>> > # mount /dev/vda /mnt/ext4/
>>> > # make dio-offset-test
>>> > # ./dio-offset-test /mnt/ext4/foobar
>>> > write: Success
>>> > read: Invalid argument
>>> >
>>> > I tracked the "write: Success" down to ext4's handling for the "special"
>>> > -ENOTBLK error after ext4_want_directio_fallback() returns "true".
>>> >
>>>
>>> Right. Ext4 has fallback only for dio writes but not for DIO reads...
>>>
>>> buffered
>>> static inline bool ext4_want_directio_fallback(unsigned flags, ssize_t written)
>>> {
>>> /* must be a directio to fall back to buffered */
>>> if ((flags & (IOMAP_WRITE | IOMAP_DIRECT)) !=
>>> (IOMAP_WRITE | IOMAP_DIRECT))
>>> return false;
>>>
>>> ...
>>> }
>>>
>>> So basically the path is ext4_file_[read|write]_iter() -> iomap_dio_rw
>>> -> iomap_dio_bio_iter() -> return -EINVAL. i.e. from...
>>>
>>>
>>> if ((pos | length) & (bdev_logical_block_size(iomap->bdev) - 1) ||
>>> !bdev_iter_is_aligned(iomap->bdev, dio->submit.iter))
>>> return -EINVAL;
>>>
>>> EXT4 then fallsback to buffered-io only for writes, but not for reads.
>>
>> Right. And the fallback for writes was actually inadvertedly "added" by
>> commit bc264fea0f6f "iomap: support incremental iomap_iter advances". That
>> changed the error handling logic. Previously if iomap_dio_bio_iter()
>> returned EINVAL, it got propagated to userspace regardless of what
>> ->iomap_end() returned. After this commit if ->iomap_end() returns error
>> (which is ENOTBLK in ext4 case), it gets propagated to userspace instead of
>> the error returned by iomap_dio_bio_iter().
>>
>> Now both the old and new behavior make some sense so I won't argue that the
>> new iomap_iter() behavior is wrong. But I think we should change ext4 back
>> to the old behavior of failing unaligned dio writes instead of them falling
>> back to buffered IO. I think something like the attached patch should do
>> the trick - it makes unaligned dio writes fail again while writes to holes
>> of indirect-block mapped files still correctly fall back to buffered IO.
>> Once fstests run completes, I'll do a proper submission...
>>
>
> Aah, right. So it wasn't EXT4 which had this behaviour of falling back
> to buffered I/O for unaligned writes. Earlier EXT4 was assuming an error
> code will be detected by iomap and will be passed to it as "written" in
> ext4_iomap_end() for such unaligned writes. But I guess that logic
> silently got changed with that commit. Thanks for analyzing that.
> I missed looking underneath iomap behaviour change :).
>
>
>>
>> Honza
>> --
>> Jan Kara <jack@xxxxxxxx>
>> SUSE Labs, CR
>> From ce6da00a09647a03013c3f420c2e7ef7489c3de8 Mon Sep 17 00:00:00 2001
>> From: Jan Kara <jack@xxxxxxx>
>> Date: Wed, 27 Aug 2025 14:55:19 +0200
>> Subject: [PATCH] ext4: Fail unaligned direct IO write with EINVAL
>>
>> Commit bc264fea0f6f ("iomap: support incremental iomap_iter advances")
>> changed the error handling logic in iomap_iter(). Previously any error
>> from iomap_dio_bio_iter() got propagated to userspace, after this commit
>> if ->iomap_end returns error, it gets propagated to userspace instead of
>> an error from iomap_dio_bio_iter(). This results in unaligned writes to
>> ext4 to silently fallback to buffered IO instead of erroring out.
>>
>> Now returning ENOTBLK for DIO writes from ext4_iomap_end() seems
>> unnecessary these days. It is enough to return ENOTBLK from
>> ext4_iomap_begin() when we don't support DIO write for that particular
>> file offset (due to hole).
>
> Right. This mainly only happens if we have holes in non-extent (indirect
> blocks) case.
>
Thinking more on this case. Do we really want a fallback to buffered-io
for unaligned writes in this case (indirect block case)?
I don't think we care much here, right? And anyways the unaligned writes
should have the same behaviour for extents v/s non-extents case right?
I guess the problem is, iomap alignment check happens in
iomap_dio_bio_iter() where it has a valid bdev (populated by filesystem
during ->iomap_begin() call) to check the alignment against. But in this
indirect block case we return -ENOTBLK much earlier from ->iomap_begin()
call itself.
-ritesh
> Also, as I see ext4 always just fallsback to buffered-io for no or
> partial writes (unless iomap returned any error code). So, I was just
> wondering if that could ever happen for DIO atomic write case. It's good
> that we have a WARN_ON_ONCE() check in there to catch it. But I was
> wondering if this needs an explicit handling in ext4_dio_write_iter() to
> not fallback to buffered-writes for atomic DIO requests?
>
> -ritesh
>
>
>
>>
>> Fixes: bc264fea0f6f ("iomap: support incremental iomap_iter advances")
>> Signed-off-by: Jan Kara <jack@xxxxxxx>
>> ---
>> fs/ext4/file.c | 2 --
>> fs/ext4/inode.c | 35 -----------------------------------
>> 2 files changed, 37 deletions(-)
>>
>> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
>> index 93240e35ee36..cf39f57d21e9 100644
>> --- a/fs/ext4/file.c
>> +++ b/fs/ext4/file.c
>> @@ -579,8 +579,6 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
>> iomap_ops = &ext4_iomap_overwrite_ops;
>> ret = iomap_dio_rw(iocb, from, iomap_ops, &ext4_dio_write_ops,
>> dio_flags, NULL, 0);
>> - if (ret == -ENOTBLK)
>> - ret = 0;
>> if (extend) {
>> /*
>> * We always perform extending DIO write synchronously so by
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 5b7a15db4953..c3b23c90fd11 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -3872,47 +3872,12 @@ static int ext4_iomap_overwrite_begin(struct inode *inode, loff_t offset,
>> return ret;
>> }
>>
>> -static inline bool ext4_want_directio_fallback(unsigned flags, ssize_t written)
>> -{
>> - /* must be a directio to fall back to buffered */
>> - if ((flags & (IOMAP_WRITE | IOMAP_DIRECT)) !=
>> - (IOMAP_WRITE | IOMAP_DIRECT))
>> - return false;
>> -
>> - /* atomic writes are all-or-nothing */
>> - if (flags & IOMAP_ATOMIC)
>> - return false;
>> -
>> - /* can only try again if we wrote nothing */
>> - return written == 0;
>> -}
>> -
>> -static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,
>> - ssize_t written, unsigned flags, struct iomap *iomap)
>> -{
>> - /*
>> - * Check to see whether an error occurred while writing out the data to
>> - * the allocated blocks. If so, return the magic error code for
>> - * non-atomic write so that we fallback to buffered I/O and attempt to
>> - * complete the remainder of the I/O.
>> - * For non-atomic writes, any blocks that may have been
>> - * allocated in preparation for the direct I/O will be reused during
>> - * buffered I/O. For atomic write, we never fallback to buffered-io.
>> - */
>> - if (ext4_want_directio_fallback(flags, written))
>> - return -ENOTBLK;
>> -
>> - return 0;
>> -}
>> -
>> const struct iomap_ops ext4_iomap_ops = {
>> .iomap_begin = ext4_iomap_begin,
>> - .iomap_end = ext4_iomap_end,
>> };
>>
>> const struct iomap_ops ext4_iomap_overwrite_ops = {
>> .iomap_begin = ext4_iomap_overwrite_begin,
>> - .iomap_end = ext4_iomap_end,
>> };
>>
>> static int ext4_iomap_begin_report(struct inode *inode, loff_t offset,
>> --
>> 2.43.0
