Re: [PATCH v8 4/7] fs: iomap: Atomic write support

[John Garry <john.g.garry@oracle.com>]

On 15/10/2024 13:12, Christoph Hellwig wrote:
> On Tue, Oct 15, 2024 at 09:01:39AM +0000, John Garry wrote:
>> Support direct I/O atomic writes by producing a single bio with REQ_ATOMIC
>> flag set.
>>
>> Initially FSes (XFS) should only support writing a single FS block
>> atomically.
>>
>> As with any atomic write, we should produce a single bio which covers the
>> complete write length.
>>
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>>   .../filesystems/iomap/operations.rst          | 11 ++++++
>>   fs/iomap/direct-io.c                          | 38 +++++++++++++++++--
>>   fs/iomap/trace.h                              |  3 +-
>>   include/linux/iomap.h                         |  1 +
>>   4 files changed, 48 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst
>> index 8e6c721d2330..fb95e99ca1a0 100644
>> --- a/Documentation/filesystems/iomap/operations.rst
>> +++ b/Documentation/filesystems/iomap/operations.rst
>> @@ -513,6 +513,17 @@ IOMAP_WRITE`` with any combination of the following enhancements:
>>      if the mapping is unwritten and the filesystem cannot handle zeroing
>>      the unaligned regions without exposing stale contents.
>>   
>> + * ``IOMAP_ATOMIC``: This write is being issued with torn-write
>> +   protection. Only a single bio can be created for the write, and the
>> +   write must not be split into multiple I/O requests, i.e. flag
>> +   REQ_ATOMIC must be set.
>> +   The file range to write must be aligned to satisfy the requirements
>> +   of both the filesystem and the underlying block device's atomic
>> +   commit capabilities.
>> +   If filesystem metadata updates are required (e.g. unwritten extent
>> +   conversion or copy on write), all updates for the entire file range
>> +   must be committed atomically as well.
>> +
>>   Callers commonly hold ``i_rwsem`` in shared or exclusive mode before
>>   calling this function.
>>   
>> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
>> index f637aa0706a3..c968a0e2a60b 100644
>> --- a/fs/iomap/direct-io.c
>> +++ b/fs/iomap/direct-io.c
>> @@ -271,7 +271,7 @@ static int iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio,
>>    * clearing the WRITE_THROUGH flag in the dio request.
>>    */
>>   static inline blk_opf_t iomap_dio_bio_opflags(struct iomap_dio *dio,
>> -		const struct iomap *iomap, bool use_fua)
>> +		const struct iomap *iomap, bool use_fua, bool atomic)
>>   {
>>   	blk_opf_t opflags = REQ_SYNC | REQ_IDLE;
>>   
>> @@ -283,6 +283,8 @@ static inline blk_opf_t iomap_dio_bio_opflags(struct iomap_dio *dio,
>>   		opflags |= REQ_FUA;
>>   	else
>>   		dio->flags &= ~IOMAP_DIO_WRITE_THROUGH;
>> +	if (atomic)
>> +		opflags |= REQ_ATOMIC;
>>   
>>   	return opflags;
>>   }
>> @@ -293,7 +295,8 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>>   	const struct iomap *iomap = &iter->iomap;
>>   	struct inode *inode = iter->inode;
>>   	unsigned int fs_block_size = i_blocksize(inode), pad;
>> -	loff_t length = iomap_length(iter);
>> +	const loff_t length = iomap_length(iter);
>> +	bool atomic = iter->flags & IOMAP_ATOMIC;
>>   	loff_t pos = iter->pos;
>>   	blk_opf_t bio_opf;
>>   	struct bio *bio;
>> @@ -303,6 +306,9 @@ 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))
> 
> Nit: no need for the inner braces here.

ok

> 
>> +		if (atomic && n != length) {
>> +			/*
>> +			 * This bio should have covered the complete length,
>> +			 * which it doesn't, so error. We may need to zero out
>> +			 * the tail (complete FS block), similar to when
>> +			 * bio_iov_iter_get_pages() returns an error, above.
>> +			 */
>> +			ret = -EINVAL;
> 
> Do we want a WARN_ON_ONCE here because this is a condition that should be
> impossible to hit?

I have no objection - as you said, it should not be hit. But it would be 
nice to see why we are getting the -EINVAL if it were hit (so I can add it)

> 
> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

cheers