Re: [PATCH 5/6] iomap: Lift blocksize restriction on atomic writes

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Hi John, 

John Garry <john.g.garry@oracle.com> writes:

> On 25/10/2024 04:45, Ritesh Harjani (IBM) wrote:
>> Filesystems like ext4 can submit writes in multiples of blocksizes.
>> But we still can't allow the writes to be split. Hence let's check if
>> the iomap_length() is same as iter->len or not.
>> 
>> This shouldn't affect XFS since it anyways checks for this in
>> xfs_file_write_iter() to not support atomic write size request of more
>> than FS blocksize.
>> 
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> ---
>>   fs/iomap/direct-io.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
>> index ed4764e3b8f0..1d33b4239b3e 100644
>> --- a/fs/iomap/direct-io.c
>> +++ b/fs/iomap/direct-io.c
>> @@ -306,7 +306,7 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>>   	size_t copied = 0;
>>   	size_t orig_count;
>>   
>> -	if (atomic && length != fs_block_size)
>> +	if (atomic && length != iter->len)
>>   		return -EINVAL;
>
> Here you expect just one iter for an atomic write always.

Here we are lifting the limitation of iomap to support entire iter->len
rather than just 1 fsblock. 

>
> In 6/6, you are saying that iomap does not allow an atomic write which 
> covers unwritten and written extents, right?

No, it's not that. If FS does not provide a full mapping to iomap in
->iomap_begin then the writes will get split. For atomic writes this
should not happen, hence the check in iomap above to return -EINVAL if
mapped length does not match iter->len.

>
> But for writing a single fs block atomically, we don't mandate it to be 
> in unwritten state. So there is a difference in behavior in writing a 
> single FS block vs multiple FS blocks atomically.

Same as mentioned above. We can't have atomic writes to get split.
This patch is just lifting the restriction of iomap to allow more than
blocksize but the mapped length should still meet iter->len, as
otherwise the writes can get split.

>
> So we have 3x choices, as I see:
> a. add a check now in iomap that the extent is in written state (for an 
> atomic write)
> b. add extent zeroing code, as I was trying for originally
> c. document this peculiarity
>
> Thanks,
> John