Re: [PATCH v6 03/13] iomap: rework IOMAP atomic flags

John Garry <john.g.garry@xxxxxxxxxx> · Mon, 17 Mar 2025 14:25:30 +0000

On 17/03/2025 13:44, Ritesh Harjani (IBM) wrote:
  	if (flags & IOMAP_DAX)
  		iomap->dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
  	else
@@ -3467,7 +3470,7 @@ static inline bool ext4_want_directio_fallback(unsigned flags, ssize_t written)
  		return false;
  
  	/* atomic writes are all-or-nothing */
-	if (flags & IOMAP_ATOMIC_HW)
+	if (flags & IOMAP_ATOMIC)
  		return false;
  
The changes in ext4 is mostly straight forward. Essentially for
an IOMAP_ATOMIC write requests we are always setting IOMAP_F_ATOMIC_BIO in
the ->iomap_begin() routine. This is done to inform the iomap that this
write request needs to issue an atomic bio, so iomap then goes and sets
REQ_ATOMIC flag in the bio.

Right



  	/* can only try again if we wrote nothing */

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 9d72b99cb447..c28685fd3362 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -349,7 +349,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
  	if (dio->flags & IOMAP_DIO_WRITE) {
  		bio_opf |= REQ_OP_WRITE;
  
-		if (iter->flags & IOMAP_ATOMIC_HW) {
+		if (iomap->flags & IOMAP_F_ATOMIC_BIO) {
  			/*
  			* Ensure that the mapping covers the full write length,
  			* otherwise we will submit multiple BIOs, which is
@@ -677,10 +677,8 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
  			iomi.flags |= IOMAP_OVERWRITE_ONLY;
  		}
  
-		if (dio_flags & IOMAP_DIO_ATOMIC_SW)
-			iomi.flags |= IOMAP_ATOMIC_SW;
-		else if (iocb->ki_flags & IOCB_ATOMIC)
-			iomi.flags |= IOMAP_ATOMIC_HW;
+		if (iocb->ki_flags & IOCB_ATOMIC)
+			iomi.flags |= IOMAP_ATOMIC;
  
  		/* for data sync or sync, we need sync completion processing */
  		if (iocb_is_dsync(iocb)) {
diff --git a/fs/iomap/trace.h b/fs/iomap/trace.h
index 69af89044ebd..9eab2c8ac3c5 100644
--- a/fs/iomap/trace.h
+++ b/fs/iomap/trace.h
@@ -99,7 +99,7 @@ DEFINE_RANGE_EVENT(iomap_dio_rw_queued);
  	{ IOMAP_FAULT,		"FAULT" }, \
  	{ IOMAP_DIRECT,		"DIRECT" }, \
  	{ IOMAP_NOWAIT,		"NOWAIT" }, \
-	{ IOMAP_ATOMIC_HW,	"ATOMIC_HW" }
+	{ IOMAP_ATOMIC,		"ATOMIC" }
  
  #define IOMAP_F_FLAGS_STRINGS \
  	{ IOMAP_F_NEW,		"NEW" }, \
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 30e257f683bb..9a22ecd794eb 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -831,6 +831,9 @@ xfs_direct_write_iomap_begin(
  	if (offset + length > i_size_read(inode))
  		iomap_flags |= IOMAP_F_DIRTY;
  
+	if (flags & IOMAP_ATOMIC)
+		iomap_flags |= IOMAP_F_ATOMIC_BIO;
+
  	/*
  	 * COW writes may allocate delalloc space or convert unwritten COW
  	 * extents, so we need to make sure to take the lock exclusively here.
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 9cd93530013c..51f4c13bd17a 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -60,6 +60,9 @@ struct vm_fault;
   * IOMAP_F_ANON_WRITE indicates that (write) I/O does not have a target block
   * assigned to it yet and the file system will do that in the bio submission
   * handler, splitting the I/O as needed.
+ *
+ * IOMAP_F_ATOMIC_BIO indicates that (write) I/O needs to be issued as an
+ * atomic bio, i.e. set REQ_ATOMIC.
   */

Maybe we can be more explicit here?

IOMAP_F_ATOMIC_BIO flag indicates that write I/O must be issued as an
atomic bio by setting the REQ_ATOMIC flag. Filesystems need to set this
flag to inform iomap that the write I/O operation should be submitted as
an atomic bio.

The comment for all these flags is that they should be set by the FS:

"Flags reported by the file system from iomap_begin"

So the second sentence seems to just repeat what is already said.



This definition (or whatever you feel is the better version), should also
go in Documentation/filesystems/iomap/design.rst

Yes, I need to update that again


  #define IOMAP_F_NEW		(1U << 0)
  #define IOMAP_F_DIRTY		(1U << 1)
@@ -73,6 +76,7 @@ struct vm_fault;
  #define IOMAP_F_XATTR		(1U << 5)
  #define IOMAP_F_BOUNDARY	(1U << 6)
  #define IOMAP_F_ANON_WRITE	(1U << 7)
+#define IOMAP_F_ATOMIC_BIO	(1U << 8)
  
  /*
   * Flags set by the core iomap code during operations:
@@ -189,9 +193,8 @@ struct iomap_folio_ops {
  #else
  #define IOMAP_DAX		0
  #endif /* CONFIG_FS_DAX */
-#define IOMAP_ATOMIC_HW		(1 << 9) /* HW-based torn-write protection */
+#define IOMAP_ATOMIC		(1 << 9) /* torn-write protection */
  #define IOMAP_DONTCACHE		(1 << 10)
-#define IOMAP_ATOMIC_SW		(1 << 11)/* SW-based torn-write protection */
Now that we are killing separate IOMAP_ATOMIC_** names, we may would
like to update the iomap design document as well. Otherwise it will
carry use of IOMAP_ATOMIC_HW & IOMAP_ATOMIC_SW definitions. Instead we
should only keep IOMAP_ATOMIC and update the design info there.

Yes, I will update it.

Thanks for the reminder.

John








