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

John Garry <john.g.garry@xxxxxxxxxx> · Tue, 15 Oct 2024 13:24:25 +0100

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@xxxxxxxxxx>
---
  .../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@xxxxxx>

cheers








