Re: [PATCH 4/6] ext4: Warn if we ever fallback to buffered-io for DIO atomic writes

[IBM]

"Darrick J. Wong" <djwong@xxxxxxxxxx> writes:

> On Fri, Oct 25, 2024 at 09:15:53AM +0530, Ritesh Harjani (IBM) wrote:
>> iomap will not return -ENOTBLK in case of dio atomic writes. But let's
>> also add a WARN_ON_ONCE and return -EIO as a safety net.
>> 
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx>
>> ---
>>  fs/ext4/file.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
>> index f9516121a036..af6ebd0ac0d6 100644
>> --- a/fs/ext4/file.c
>> +++ b/fs/ext4/file.c
>> @@ -576,8 +576,16 @@ 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)
>> +	if (ret == -ENOTBLK) {
>>  		ret = 0;
>> +		/*
>> +		 * iomap will never return -ENOTBLK if write fails for atomic
>> +		 * write. But let's just add a safety net.
>
> I think it can if the pagecache invalidation fails, so you really do
> need the safety net.

Ah, right! So in that case I should remove WARN_ON_ONCE and correct
the comment too.

> I suspect that the xfs version of this series
> needs it too, though it may have fallen out?
>

I think so yes. Looks like it got missed.


> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> --D
>

Thanks for pointing that out.

-ritesh

>> +		 */
>> +		if (WARN_ON_ONCE(iocb->ki_flags & IOCB_ATOMIC))
>> +			ret = -EIO;
>> +	}
>> +
>>  	if (extend) {
>>  		/*
>>  		 * We always perform extending DIO write synchronously so by
>> -- 
>> 2.46.0
>> 
>>