[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e3c72790-ad06-4a59-883d-72ad68cca11b@oracle.com>
Date: Tue, 15 Oct 2024 13:24:25 +0100
From: John Garry <john.g.garry@...cle.com>
To: Christoph Hellwig <hch@....de>
Cc: axboe@...nel.dk, brauner@...nel.org, djwong@...nel.org,
viro@...iv.linux.org.uk, jack@...e.cz, dchinner@...hat.com,
cem@...nel.org, linux-block@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-xfs@...r.kernel.org,
linux-fsdevel@...r.kernel.org, hare@...e.de,
martin.petersen@...cle.com, catherine.hoang@...cle.com,
mcgrof@...nel.org, ritesh.list@...il.com, ojaswin@...ux.ibm.com
Subject: Re: [PATCH v8 4/7] fs: iomap: Atomic write support
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@...cle.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@....de>
cheers
Powered by blists - more mailing lists