iomap: Use FUA for pure data O_DSYNC DIO writes

From: Dave Chinner <[email protected]>

If we are doing direct IO writes with datasync semantics, we often have to flush metadata changes along with the data write. However, if we are overwriting existing data, there are no metadata changes that we need to flush. In this case, optimising the IO by using FUA write makes sense.

We know from teh IOMAP_F_DIRTY flag as to whether a specific inode requires a metadata flush - this is currently used by DAX to ensure extent modi$fication as stable in page fault operations. For direct

IO writes, we can use it to determine if we need to flush metadata or not once the data is on disk.

Hence if we have been returned a mapped extent that is not new and the IO mapping is not dirty, then we can use a FUA write to provide datasync semantics. This allows us to short-cut the generic_write_sync() call in IO completion and hence avoid unnecessary operations. This makes pure direct IO data write behaviour identical to the way block devices use REQ_FUA to provide datasync semantics.

Now that iomap_dio_rw() is determining if REQ_FUA can be used, we have to stop issuing generic_write_sync() calls from the XFS code when REQ_FUA is issued, otherwise it will still throw a cache flush to the device via xfs_file_fsync(). To do this, we need to make iomap_dio_rw() always responsible for issuing generic_write_sync() when necessary, not just for AIO calls. This means the filesystem doesn't have to guess when cache flushes are necessary now.

On a FUA enabled device, a synchronous direct IO write workload (sequential 4k overwrites in 32MB file) had the following results:

# xfs_io -fd -c "pwrite -V 1 -D 0 32m" /mnt/scratch/boo

kernel time write()s write iops Write b/w
------ ---- -------- ---------- ---------
(no dsync) 4s 2173/s 2173 8.5MB/s
vanilla 22s 370/s 750 1.4MB/s
patched 19s 420/s 420 1.6MB/s

The patched code clearly doesn't send cache flushes anymore, but instead uses FUA (confirmed via blktrace), and performance improves a bit as a result. However, the benefits will be higher on workloads that mix O_DSYNC overwrites with other write IO as we won't be flushing the entire device cache on every DSYNC overwrite IO anymore.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/iomap.c        | 38 +++++++++++++++++++++++++++++++++-----
 fs/xfs/xfs_file.c |  5 +++++
 2 files changed, 38 insertions(+), 5 deletions(-)

fs/iomap.c +1 -0

index afd163586aa0..bcc90e3a2e3f 100644
@@ -685,6 +685,7 @@ EXPORT_SYMBOL_GPL(iomap_seek_data);
  * Private flags for iomap_dio, must not overlap with the public ones in
  * iomap.h:
  */
+#define IOMAP_DIO_WRITE_FUA	(1 << 29)
 #define IOMAP_DIO_WRITE		(1 << 30)
 #define IOMAP_DIO_DIRTY		(1 << 31)
 

fs/iomap.c +29 -4

+	 * the write.
+	 */
+	if (ret > 0 &&
+	    (dio->flags & (IOMAP_DIO_WRITE|IOMAP_DIO_WRITE_FUA)) ==
+							IOMAP_DIO_WRITE) {
+		ret = generic_write_sync(iocb, ret);
+	}
+
+	kfree(dio);
 	return ret;
 }
 

@@ -769,12 +781,9 @@ static void iomap_dio_complete_work(struct work_struct *work)
 {
 	struct iomap_dio *dio = container_of(work, struct iomap_dio, aio.work);
 	struct kiocb *iocb = dio->iocb;
-	bool is_write = (dio->flags & IOMAP_DIO_WRITE);
 	ssize_t ret;
 
 	ret = iomap_dio_complete(dio);
-	if (is_write && ret > 0)
-		ret = generic_write_sync(iocb, ret);
 	iocb->ki_complete(iocb, ret, 0);
 }
 

@@ -883,6 +892,15 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length,
 			dio->flags |= IOMAP_DIO_COW;
 		if (iomap->flags & IOMAP_F_NEW)
 			need_zeroout = true;
+		/*
+		 * Use a FUA write if we need datasync semantics and this is a
+		 * pure data IO that doesn't require any metadata updates. This
+		 * allows us to avoid cache flushes on IO completion.
+		 */
+		else if (!(iomap->flags & (IOMAP_F_SHARED|IOMAP_F_DIRTY)) &&
+			 (dio->flags & IOMAP_DIO_WRITE) &&
+			 (dio->iocb->ki_flags & IOCB_DSYNC))
+			dio->flags |= IOMAP_DIO_WRITE_FUA;
 		break;
 	default:
 		WARN_ON_ONCE(1);

@@ -930,7 +948,11 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length,
 
 		n = bio->bi_iter.bi_size;
 		if (dio->flags & IOMAP_DIO_WRITE) {
-			bio_set_op_attrs(bio, REQ_OP_WRITE, REQ_SYNC | REQ_IDLE);
+			int op_flags = REQ_SYNC | REQ_IDLE;
+
+			if (dio->flags & IOMAP_DIO_WRITE_FUA)
+				op_flags |= REQ_FUA;
+			bio_set_op_attrs(bio, REQ_OP_WRITE, op_flags);
 			task_io_account_write(n);
 		} else {
 			bio_set_op_attrs(bio, REQ_OP_READ, 0);

@@ -961,6 +983,12 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length,
 	return copied;
 }
 
+/*
+ * iomap_dio_rw() always completes O_[D]SYNC writes regardless of whether the IO
+ * is being issued as AIO or not. This allows us to optimise pure data writes to
+ * use REQ_FUA rather than requiring generic_write_sync() to issue a REQ_FLUSH
+ * post write.
+ */
 ssize_t
 iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		const struct iomap_ops *ops, iomap_dio_end_io_t end_io)

fs/xfs/xfs_file.c +5 -0

index 260ff5e5c264..81aa3b73471e 100644
@@ -732,6 +732,11 @@ xfs_file_write_iter(
 		ret = xfs_file_dio_aio_write(iocb, from);
 		if (ret == -EREMCHG)
 			goto buffered;
+		/*
+		 * Direct IO handles sync type writes internally on I/O
+		 * completion.
+		 */
+		return ret;
 	} else {
 buffered:
 		ret = xfs_file_buffered_aio_write(iocb, from);

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

fs/iomap.c +2 -1

@@ -760,8 +761,19 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
 	}
 
 	inode_dio_end(file_inode(iocb->ki_filp));
-	kfree(dio);
 
+	/*
+	 * If a FUA write was done, then that is all we required for datasync

fs/xfs/xfs_file.c +0 -0

index 260ff5e5c264..81aa3b73471e 100644
@@ -732,6 +732,11 @@ xfs_file_write_iter(
 		ret = xfs_file_dio_aio_write(iocb, from);

Quoted 4 lines

fs/xfs/xfs_file.c +0 -0

@@ -732,6 +732,11 @@ xfs_file_write_iter(
 		ret = xfs_file_dio_aio_write(iocb, from);

fs/xfs/xfs_file.c xfs_file_write_iter( · line 742

Quoted 10 lines

fs/xfs/xfs_file.c +5 -0

 		if (ret == -EREMCHG)
 			goto buffered;
+		/*
+		 * Direct IO handles sync type writes internally on I/O
+		 * completion.
+		 */
+		return ret;
 	} else {
 buffered:
 		ret = xfs_file_buffered_aio_write(iocb, from);

The else is not needed and you can now have a much more sensible code flow here:

		ret = xfs_file_dio_aio_write(iocb, from);
		if (ret != -EREMCHG))
			return ret;
	}

	ret = xfs_file_buffered_aio_write(iocb, from);

While we're at it we should probably
also skil the generic_write_sync call for DAX pure overwrites while we're at it.

Quoted 3 lines · Christoph Hellwig

While we're at it we should probably
also skil the generic_write_sync call for DAX pure overwrites while we're at it.

Now that I've looked at it, DAX is a friggin' mess.

We can't push the generic_write_sync() call into dax_iomap_rw() because dax_iomap_rw is called with the inode_lock() held, and both ext2's and ext4's ->fsync implementation can call __generic_file_fsync() which takes the inode_lock(). Deadlock central right there - someone is going to have to screw with ext4's indoe sync code to before we can do this.

Further, XFS has post-write metadata updates to do in the case of extending writes, and hence we'd still need the call to generic_write_sync() after we have returned from dax_iomap_rw(). I'm thinking we'd really need a dax write IO completion callback (say in the struct iomap_ops) to run the filesystem specific IO completions (like the DIO path) before we can start to think about optimisations like this for DAX for XFS....

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 6 lines

fs/iomap.c +0 -1

@@ -760,8 +761,19 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
 	}
 
 	inode_dio_end(file_inode(iocb->ki_filp));
-	kfree(dio);
 

fs/iomap.c static ssize_t iomap_dio_complete(struct iomap_dio *dio) · line 776

Quoted 12 lines

fs/iomap.c +12 -0

+	/*
+	 * If a FUA write was done, then that is all we required for datasync
+	 * semantics -. we don't need to call generic_write_sync() to complete
+	 * the write.
+	 */
+	if (ret > 0 &&
+	    (dio->flags & (IOMAP_DIO_WRITE|IOMAP_DIO_WRITE_FUA)) ==
+							IOMAP_DIO_WRITE) {
+		ret = generic_write_sync(iocb, ret);
+	}
+
+	kfree(dio);

Can please split the move of the generic_write_sync call into generic_write_sync a separate prep patch? It's enough of a logic change on its own that it warrants a separate commit with a separate explanation.

Also I'd be tempted to invert the IOMAP_DIO_WRITE_FUA flag and replace it with an IOMAP_DIO_WRITE_SYNC flag to indicate we need the generic_write_sync call, as that should make the logic much more clear.

Quoted 6 lines

fs/xfs/xfs_file.c +0 -0

index 260ff5e5c264..81aa3b73471e 100644
@@ -732,6 +732,11 @@ xfs_file_write_iter(
 		ret = xfs_file_dio_aio_write(iocb, from);

fs/xfs/xfs_file.c xfs_file_write_iter( · line 742

Quoted 10 lines

fs/xfs/xfs_file.c +5 -0

 		if (ret == -EREMCHG)
 			goto buffered;
+		/*
+		 * Direct IO handles sync type writes internally on I/O
+		 * completion.
+		 */
+		return ret;
 	} else {
 buffered:
 		ret = xfs_file_buffered_aio_write(iocb, from);

The else is not needed and you can now have a much more sensible code flow here:

		ret = xfs_file_dio_aio_write(iocb, from);
		if (ret != -EREMCHG))
			return ret;
	}

	ret = xfs_file_buffered_aio_write(iocb, from);

I thought about that, but as you noticed the DAX case gets in the way so I didn't bother for a RFC patch.

I'll have a look at making the DAX iomap code do something similar with generic_write_sync() so we can simplify this high level code.

Quoted 6 lines · Christoph Hellwig

Quoted 6 lines

fs/iomap.c +0 -1

@@ -760,8 +761,19 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
 	}
 
 	inode_dio_end(file_inode(iocb->ki_filp));
-	kfree(dio);
 

Quoted 16 lines

fs/iomap.c static ssize_t iomap_dio_complete(struct iomap_dio *dio) · line 776

Quoted 12 lines

fs/iomap.c +12 -0

+	/*
+	 * If a FUA write was done, then that is all we required for datasync
+	 * semantics -. we don't need to call generic_write_sync() to complete
+	 * the write.
+	 */
+	if (ret > 0 &&
+	    (dio->flags & (IOMAP_DIO_WRITE|IOMAP_DIO_WRITE_FUA)) ==
+							IOMAP_DIO_WRITE) {
+		ret = generic_write_sync(iocb, ret);
+	}
+
+	kfree(dio);

Can please split the move of the generic_write_sync call into generic_write_sync a separate prep patch? It's enough of a logic change on its own that it warrants a separate commit with a separate explanation.

Ok, that makes sense.

Quoted 3 lines

Also I'd be tempted to invert the IOMAP_DIO_WRITE_FUA flag and replace it with an IOMAP_DIO_WRITE_SYNC flag to indicate we need the generic_write_sync call, as that should make the logic much more clear.

Ah, much cleaner.

Quoted 20 lines

Quoted 6 lines

fs/xfs/xfs_file.c +0 -0

index 260ff5e5c264..81aa3b73471e 100644
@@ -732,6 +732,11 @@ xfs_file_write_iter(
 		ret = xfs_file_dio_aio_write(iocb, from);

fs/xfs/xfs_file.c xfs_file_write_iter( · line 742

Quoted 10 lines

fs/xfs/xfs_file.c +5 -0

 		if (ret == -EREMCHG)
 			goto buffered;
+		/*
+		 * Direct IO handles sync type writes internally on I/O
+		 * completion.
+		 */
+		return ret;
 	} else {
 buffered:
 		ret = xfs_file_buffered_aio_write(iocb, from);

The else is not needed and you can now have a much more sensible code flow here:

Quoted 6 lines

		ret = xfs_file_dio_aio_write(iocb, from);
		if (ret != -EREMCHG))
			return ret;
	}

	ret = xfs_file_buffered_aio_write(iocb, from);

I thought about that, but as you noticed the DAX case gets in the way so I didn't bother for a RFC patch.

I'll have a look at making the DAX iomap code do something similar with generic_write_sync() so we can simplify this high level code.

Thanks Christoph!

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Oh, and another thing: I think you want to make this new code dependent on the block devie actually supporting REQ_FUA natively. Otherwise you'll cause a flush for every emulated FUA write, which is only going make things worse, especially for ATA where FLUSH is not queued. And last time I check libata still disabled FUA by default.

Quoted 5 lines · Christoph Hellwig

Oh, and another thing: I think you want to make this new code dependent on the block devie actually supporting REQ_FUA natively. Otherwise you'll cause a flush for every emulated FUA write, which is only going make things worse, especially for ATA where FLUSH is not queued. And last time I check libata still disabled FUA by default.

Yup, but the issue we have right now is that for pure RWF_DSYNC data overwrites we are already doing a post-flush on every IO. It's being issued as a separate zero-length IO, which is why REQ_FUA is faster and results in lower overall IOPS. The flush comes from this path:

From: Dave Chinner <[email protected]>

If we are doing direct IO writes with datasync semantics, we often have to flush metadata changes along with the data write. However, if we are overwriting existing data, there are no metadata changes that we need to flush. In this case, optimising the IO by using FUA write makes sense.

We know from teh IOMAP_F_DIRTY flag as to whether a specific inode requires a metadata flush - this is currently used by DAX to ensure extent modi$fication as stable in page fault operations. For direct IO writes, we can use it to determine if we need to flush metadata or not once the data is on disk.

Hence if we have been returned a mapped extent that is not new and the IO mapping is not dirty, then we can use a FUA write to provide datasync semantics. This allows us to short-cut the generic_write_sync() call in IO completion and hence avoid unnecessary operations. This makes pure direct IO data write behaviour identical to the way block devices use REQ_FUA to provide datasync semantics.

Now that iomap_dio_rw() is determining if REQ_FUA can be used, we have to stop issuing generic_write_sync() calls from the XFS code when REQ_FUA is issued, otherwise it will still throw a cache flush to the device via xfs_file_fsync(). To do this, we need to make iomap_dio_rw() always responsible for issuing generic_write_sync() when necessary, not just for AIO calls. This means the filesystem doesn't have to guess when cache flushes are necessary now.

On a FUA enabled device, a synchronous direct IO write workload (sequential 4k overwrites in 32MB file) had the following results:

# xfs_io -fd -c "pwrite -V 1 -D 0 32m" /mnt/scratch/boo

kernel          time    write()s        write iops      Write b/w
------          ----    --------        ----------      ---------
(no dsync)       4s     2173/s          2173            8.5MB/s
vanilla         22s      370/s           750            1.4MB/s
patched         19s      420/s           420            1.6MB/s

The patched code clearly doesn't send cache flushes anymore, but instead uses FUA (confirmed via blktrace), and performance improves a bit as a result. However, the benefits will be higher on workloads that mix O_DSYNC overwrites with other write IO as we won't be flushing the entire device cache on every DSYNC overwrite IO anymore.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/iomap.c        | 38 +++++++++++++++++++++++++++++++++-----
 fs/xfs/xfs_file.c |  5 +++++
 2 files changed, 38 insertions(+), 5 deletions(-)

fs/iomap.c +33 -5

index afd163586aa0..bcc90e3a2e3f 100644
@@ -685,6 +685,7 @@ EXPORT_SYMBOL_GPL(iomap_seek_data);
  * Private flags for iomap_dio, must not overlap with the public ones in
  * iomap.h:
  */
+#define IOMAP_DIO_WRITE_FUA    (1 << 29)
 #define IOMAP_DIO_WRITE                (1 << 30)
 #define IOMAP_DIO_DIRTY                (1 << 31)
 

@@ -760,8 +761,19 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
        }
 
        inode_dio_end(file_inode(iocb->ki_filp));
-       kfree(dio);
 
+       /*
+        * If a FUA write was done, then that is all we required for datasync
+        * semantics -. we don't need to call generic_write_sync() to complete
+        * the write.
+        */
+       if (ret > 0 &&
+           (dio->flags & (IOMAP_DIO_WRITE|IOMAP_DIO_WRITE_FUA)) ==
+                                                       IOMAP_DIO_WRITE) {
+               ret = generic_write_sync(iocb, ret);
+       }
+
+       kfree(dio);
        return ret;
 }
 

@@ -769,12 +781,9 @@ static void iomap_dio_complete_work(struct work_struct *work)
 {
        struct iomap_dio *dio = container_of(work, struct iomap_dio, aio.work);
        struct kiocb *iocb = dio->iocb;
-       bool is_write = (dio->flags & IOMAP_DIO_WRITE);
        ssize_t ret;
 
        ret = iomap_dio_complete(dio);
-       if (is_write && ret > 0)
-               ret = generic_write_sync(iocb, ret);
        iocb->ki_complete(iocb, ret, 0);
 }
 

@@ -883,6 +892,15 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length,
                        dio->flags |= IOMAP_DIO_COW;
                if (iomap->flags & IOMAP_F_NEW)
                        need_zeroout = true;
+               /*
+                * Use a FUA write if we need datasync semantics and this is a
+                * pure data IO that doesn't require any metadata updates. This
+                * allows us to avoid cache flushes on IO completion.
+                */
+               else if (!(iomap->flags & (IOMAP_F_SHARED|IOMAP_F_DIRTY)) &&
+                        (dio->flags & IOMAP_DIO_WRITE) &&
+                        (dio->iocb->ki_flags & IOCB_DSYNC))
+                       dio->flags |= IOMAP_DIO_WRITE_FUA;
                break;
        default:
                WARN_ON_ONCE(1);

@@ -930,7 +948,11 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length,
 
                n = bio->bi_iter.bi_size;
                if (dio->flags & IOMAP_DIO_WRITE) {
-                       bio_set_op_attrs(bio, REQ_OP_WRITE, REQ_SYNC | REQ_IDLE);
+                       int op_flags = REQ_SYNC | REQ_IDLE;
+
+                       if (dio->flags & IOMAP_DIO_WRITE_FUA)
+                               op_flags |= REQ_FUA;
+                       bio_set_op_attrs(bio, REQ_OP_WRITE, op_flags);
                        task_io_account_write(n);
                } else {
                        bio_set_op_attrs(bio, REQ_OP_READ, 0);

@@ -961,6 +983,12 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length,
        return copied;
 }
 
+/*
+ * iomap_dio_rw() always completes O_[D]SYNC writes regardless of whether the IO
+ * is being issued as AIO or not. This allows us to optimise pure data writes to
+ * use REQ_FUA rather than requiring generic_write_sync() to issue a REQ_FLUSH
+ * post write.
+ */
 ssize_t
 iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
                const struct iomap_ops *ops, iomap_dio_end_io_t end_io)

fs/xfs/xfs_file.c +5 -1

index 260ff5e5c264..81aa3b73471e 100644
@@ -732,6 +732,11 @@ xfs_file_write_iter(
                ret = xfs_file_dio_aio_write(iocb, from);
                if (ret == -EREMCHG)
                        goto buffered;
+               /*
+                * Direct IO handles sync type writes internally on I/O
+                * completion.
+                */
+               return ret;
        } else {
 buffered:
                ret = xfs_file_buffered_aio_write(iocb, from);
--

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Dave Chinner <[email protected]>

If we are doing direct IO writes with datasync semantics, we often have to flush metadata changes along with the data write. However, if we are overwriting existing data, there are no metadata changes that we need to flush. In this case, optimising the IO by using FUA write makes sense.

We know from teh IOMAP_F_DIRTY flag as to whether a specific inode requires a metadata flush - this is currently used by DAX to ensure extent modi$fication as stable in page fault operations. For direct IO writes, we can use it to determine if we need to flush metadata or not once the data is on disk.

Hence if we have been returned a mapped extent that is not new and the IO mapping is not dirty, then we can use a FUA write to provide datasync semantics. This allows us to short-cut the generic_write_sync() call in IO completion and hence avoid unnecessary operations. This makes pure direct IO data write behaviour identical to the way block devices use REQ_FUA to provide datasync semantics.

Now that iomap_dio_rw() is determining if REQ_FUA can be used, we have to stop issuing generic_write_sync() calls from the XFS code when REQ_FUA is issued, otherwise it will still throw a cache flush to the device via xfs_file_fsync(). To do this, we need to make iomap_dio_rw() always responsible for issuing generic_write_sync() when necessary, not just for AIO calls. This means the filesystem doesn't have to guess when cache flushes are necessary now.

On a FUA enabled device, a synchronous direct IO write workload (sequential 4k overwrites in 32MB file) had the following results:

# xfs_io -fd -c "pwrite -V 1 -D 0 32m" /mnt/scratch/boo

kernel          time    write()s        write iops      Write b/w
------          ----    --------        ----------      ---------
(no dsync)       4s     2173/s          2173            8.5MB/s
vanilla         22s      370/s           750            1.4MB/s
patched         19s      420/s           420            1.6MB/s

The patched code clearly doesn't send cache flushes anymore, but instead uses FUA (confirmed via blktrace), and performance improves a bit as a result. However, the benefits will be higher on workloads that mix O_DSYNC overwrites with other write IO as we won't be flushing the entire device cache on every DSYNC overwrite IO anymore.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/iomap.c        | 38 +++++++++++++++++++++++++++++++++-----
 fs/xfs/xfs_file.c |  5 +++++
 2 files changed, 38 insertions(+), 5 deletions(-)

fs/iomap.c +27 -5

index afd163586aa0..bcc90e3a2e3f 100644
@@ -685,6 +685,7 @@ EXPORT_SYMBOL_GPL(iomap_seek_data);
  * Private flags for iomap_dio, must not overlap with the public ones in
  * iomap.h:
  */
+#define IOMAP_DIO_WRITE_FUA    (1 << 29)
 #define IOMAP_DIO_WRITE                (1 << 30)
 #define IOMAP_DIO_DIRTY                (1 << 31)
 

@@ -760,8 +761,19 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
        }
 
        inode_dio_end(file_inode(iocb->ki_filp));
-       kfree(dio);
 
+       /*
+        * If a FUA write was done, then that is all we required for datasync
+        * semantics -. we don't need to call generic_write_sync() to complete
+        * the write.
+        */
+       if (ret > 0 &&
+           (dio->flags & (IOMAP_DIO_WRITE|IOMAP_DIO_WRITE_FUA)) ==
+                                                       IOMAP_DIO_WRITE) {
+               ret = generic_write_sync(iocb, ret);
+       }
+
+       kfree(dio);
        return ret;
 }
 

@@ -769,12 +781,9 @@ static void iomap_dio_complete_work(struct 
 work_struct *work)  {
        struct iomap_dio *dio = container_of(work, struct iomap_dio, aio.work);
        struct kiocb *iocb = dio->iocb;
-       bool is_write = (dio->flags & IOMAP_DIO_WRITE);
        ssize_t ret;
 
        ret = iomap_dio_complete(dio);
-       if (is_write && ret > 0)
-               ret = generic_write_sync(iocb, ret);
        iocb->ki_complete(iocb, ret, 0);  }
 

@@ -883,6 +892,15 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length,
                        dio->flags |= IOMAP_DIO_COW;
                if (iomap->flags & IOMAP_F_NEW)
                        need_zeroout = true;
+               /*
+                * Use a FUA write if we need datasync semantics and this is a
+                * pure data IO that doesn't require any metadata updates. This
+                * allows us to avoid cache flushes on IO completion.
+                */
+               else if (!(iomap->flags & (IOMAP_F_SHARED|IOMAP_F_DIRTY)) &&
+                        (dio->flags & IOMAP_DIO_WRITE) &&
+                        (dio->iocb->ki_flags & IOCB_DSYNC))
+                       dio->flags |= IOMAP_DIO_WRITE_FUA;
                break;
        default:
                WARN_ON_ONCE(1);

@@ -930,7 +948,11 @@ iomap_dio_actor(struct inode *inode, loff_t pos, 
 loff_t length,
 
                n = bio->bi_iter.bi_size;
                if (dio->flags & IOMAP_DIO_WRITE) {
-                       bio_set_op_attrs(bio, REQ_OP_WRITE, REQ_SYNC | REQ_IDLE);
+                       int op_flags = REQ_SYNC | REQ_IDLE;
+
+                       if (dio->flags & IOMAP_DIO_WRITE_FUA)
+                               op_flags |= REQ_FUA;
+                       bio_set_op_attrs(bio, REQ_OP_WRITE, op_flags);
                        task_io_account_write(n);
                } else {

                        bio_set_op_attrs(bio, REQ_OP_READ, 0); @@ 
-961,6 +983,12 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length,
        return copied;

}

fs/iomap.c +9 -0

+/*
+ * iomap_dio_rw() always completes O_[D]SYNC writes regardless of 
+whether the IO
+ * is being issued as AIO or not. This allows us to optimise pure 
+data writes to
+ * use REQ_FUA rather than requiring generic_write_sync() to issue a 
+REQ_FLUSH
+ * post write.
+ */
 ssize_t
 iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
                const struct iomap_ops *ops, iomap_dio_end_io_t 

end_io) diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 260ff5e5c264..81aa3b73471e 100644

fs/xfs/xfs_file.c +5 -1

@@ -732,6 +732,11 @@ xfs_file_write_iter(
                ret = xfs_file_dio_aio_write(iocb, from);
                if (ret == -EREMCHG)
                        goto buffered;
+               /*
+                * Direct IO handles sync type writes internally on I/O
+                * completion.
+                */
+               return ret;
        } else {
 buffered:
                ret = xfs_file_buffered_aio_write(iocb, from);
--

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at
https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.k
ernel.org%2Fmajordomo-info.html&data=04%7C01%7Crdorr%40microsoft.com%7 C70271d8bc9c84c12e87808d5887485e5%7C72f988bf86f141af91ab2d7cd011db47%7 C1%7C0%7C636564956441249840%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=KqPTOI97YQxhrm%2 BFu3MpoFMIWieH2nkool4tzs5IJ0M%3D&reserved=0

Hello all. I have a couple of follow-up questions around this effort, thanks you all for all your kind inputs, patience and knowledge transfer.

1. How does xfs or ext4 make sure a pattern of WS followed by
FWS does not allow the write (WS) completion to be visible before the flush completes?

I suspected the write was held in iomap_dio_complete_work but with the generic_write_sync change in the patch would a O_DSYNC write request to a DpoFua=0 block queue allow T2 to see the completion via io_getevents before T1 completed the actual flush?

2. How will my application be able to dynamically determine if
xfs and ext4 have the performance enhancement for FUA or I need engage alternate methods to use fsync/fdatasync at strategic locations?

3. Are there any plans yet to optimize ext4 as well?

4. Before the patched code the xfs_file_write_iter would call
generic_write_sync and that calls submit_io_wait. Does this hold the thread issuing the io_submit so it is unable to drive more async I/O?

Hello all. I have a couple of follow-up questions around this effort, thanks you all for all your kind inputs, patience and knowledge transfer.

1. How does xfs or ext4 make sure a pattern of WS followed by
FWS does not allow the write (WS) completion to be visible before the flush completes?

I suspected the write was held in
iomap_dio_complete_work but with the generic_write_sync change in the patch would a O_DSYNC write request to a DpoFua=0 block queue allow T2 to see the completion via io_getevents before T1 completed the actual flush?

2. How will my application be able to dynamically determine if
xfs and ext4 have the performance enhancement for FUA or I need engage alternate methods to use fsync/fdatasync at strategic locations?

3. Are there any plans yet to optimize ext4 as well?

4. Before the patched code the xfs_file_write_iter would call
generic_write_sync and that calls submit_io_wait. Does this hold the thread issuing the io_submit so it is unable to drive more async I/O?

I think the answer is the iomap* logic makes sure to issue generic_write_sync for O_DSYNC W1 after W1 is completed by hardware and then waits for completion of the flush request (REQ_PREFLUSH) before W1 is returned to the AIO completion ring, preventing io_getevents from processing W1 before the flush occurs and completes. I just need proper confirmation from the experts on this code that this is the expected behavior.

For SQL Server using O_DIRECT | O_DSYNC on current kernels is very performance impacting. Instead we enable a mode for SQL that opens O_DIRECT only and issues fsync/fdatasync when we are hardening log files or checkpointing data files. This reduces the write, flush, write, flush pattern allowing for write, write, write, ... then flush as we only issue flush requests when required to maintain the data integrity boundaries of SQL Server. The performance is significantly better then the device flush for each write as you can imagine.

Testing shows the FUA enhancement is better then the write, flush pattern. For SQL Server we want to dynamically open with O_DIRECT | O_DSYNC when REQ_FUA can be properly used and open with O_DIRECT and leverage SQL Server's alternate flush scheme when running on older kernel or a system that does not support FUA (SATA, IDE, ...)

I think the answer is the iomap* logic makes sure to issue generic_write_sync for O_DSYNC W1 after W1 is completed by hardware and then waits for completion of the flush request (REQ_PREFLUSH) before W1 is returned to the AIO completion ring, preventing io_getevents from processing W1 before the flush occurs and completes. I just need proper confirmation from the experts on this code that this is the expected behavior.

For SQL Server using O_DIRECT | O_DSYNC on current kernels is very performance impacting. Instead we enable a mode for SQL that opens O_DIRECT only and issues fsync/fdatasync when we are hardening log files or checkpointing data files. This reduces the write, flush, write, flush pattern allowing for write, write, write, ... then flush as we only issue flush requests when required to maintain the data integrity boundaries of SQL Server. The performance is significantly better then the device flush for each write as you can imagine.

Testing shows the FUA enhancement is better then the write, flush pattern. For SQL Server we want to dynamically open with O_DIRECT | O_DSYNC when REQ_FUA can be properly used and open with O_DIRECT and leverage SQL Server's alternate flush scheme when running on older kernel or a system that does not support FUA (SATA, IDE, ...)

I tried the RWF_ODSYNC and xfs seems to honor properly but it causes ext4 to go spin out a single CPU and hang the system. 😊

-----Original Message-----
From: Christoph Hellwig <[email protected]>
Sent: Tuesday, March 13, 2018 11:13 AM
To: Robert Dorr <[email protected]>
Cc: Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; Christoph Hellwig <[email protected]>; linux-fsdevel <[email protected]>; Jan Kara <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 1 line

I think the answer is the iomap* logic makes sure to issue generic_write_sync for O_DSYNC W1 after W1 is completed by hardware and then waits for completion of the flush request (REQ_PREFLUSH) before W1 is returned to the AIO completion ring, preventing io_getevents from processing W1 before the flush occurs and completes. I just need proper confirmation from the experts on this code that this is the expected behavior.

Yes. that is the case.

Quoted 3 lines

For SQL Server using O_DIRECT | O_DSYNC on current kernels is very performance impacting. Instead we enable a mode for SQL that opens O_DIRECT only and issues fsync/fdatasync when we are hardening log files or checkpointing data files. This reduces the write, flush, write, flush pattern allowing for write, write, write, ... then flush as we only issue flush requests when required to maintain the data integrity boundaries of SQL Server. The performance is significantly better then the device flush for each write as you can imagine.

Testing shows the FUA enhancement is better then the write, flush pattern. For SQL Server we want to dynamically open with O_DIRECT | O_DSYNC when REQ_FUA can be properly used and open with O_DIRECT and leverage SQL Server's alternate flush scheme when running on older kernel or a system that does not support FUA (SATA, IDE, ...)

There is no really good way to figure this out except for benchmarking, given that the FUA use an implementation detail.

Btw, another feature that might be interesting to you is the RWF_DSYNC flag to the pwritev2 syscall, which allows to apply O_DSYNC semantics on a per-I/O basis instead of using it at open time.

I tried the RWF_ODSYNC and xfs seems to honor properly but it causes ext4 to go spin out a single CPU and hang the system. 😊

-----Original Message-----
From: Christoph Hellwig <[email protected]>
Sent: Tuesday, March 13, 2018 11:13 AM
To: Robert Dorr <[email protected]>
Cc: Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; Christoph Hellwig <[email protected]>; linux-fsdevel <[email protected]>; Jan Kara <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 1 line

I think the answer is the iomap* logic makes sure to issue generic_write_sync for O_DSYNC W1 after W1 is completed by hardware and then waits for completion of the flush request (REQ_PREFLUSH) before W1 is returned to the AIO completion ring, preventing io_getevents from processing W1 before the flush occurs and completes. I just need proper confirmation from the experts on this code that this is the expected behavior.

Yes. that is the case.

Quoted 3 lines

For SQL Server using O_DIRECT | O_DSYNC on current kernels is very performance impacting. Instead we enable a mode for SQL that opens O_DIRECT only and issues fsync/fdatasync when we are hardening log files or checkpointing data files. This reduces the write, flush, write, flush pattern allowing for write, write, write, ... then flush as we only issue flush requests when required to maintain the data integrity boundaries of SQL Server. The performance is significantly better then the device flush for each write as you can imagine.

Testing shows the FUA enhancement is better then the write, flush pattern. For SQL Server we want to dynamically open with O_DIRECT | O_DSYNC when REQ_FUA can be properly used and open with O_DIRECT and leverage SQL Server's alternate flush scheme when running on older kernel or a system that does not support FUA (SATA, IDE, ...)

There is no really good way to figure this out except for benchmarking, given that the FUA use an implementation detail.

Btw, another feature that might be interesting to you is the RWF_DSYNC flag to the pwritev2 syscall, which allows to apply O_DSYNC semantics on a per-I/O basis instead of using it at open time.

I tried the RWF_ODSYNC and xfs seems to honor properly but it causes ext4 to go spin out a single CPU and hang the system. 😊

-----Original Message-----
From: Christoph Hellwig <[email protected]>
Sent: Tuesday, March 13, 2018 11:13 AM
To: Robert Dorr <[email protected]>
Cc: Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; Christoph Hellwig <[email protected]>; linux-fsdevel <[email protected]>; Jan Kara <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 1 line

I think the answer is the iomap* logic makes sure to issue generic_write_sync for O_DSYNC W1 after W1 is completed by hardware and then waits for completion of the flush request (REQ_PREFLUSH) before W1 is returned to the AIO completion ring, preventing io_getevents from processing W1 before the flush occurs and completes. I just need proper confirmation from the experts on this code that this is the expected behavior.

Yes. that is the case.

Quoted 3 lines

For SQL Server using O_DIRECT | O_DSYNC on current kernels is very performance impacting. Instead we enable a mode for SQL that opens O_DIRECT only and issues fsync/fdatasync when we are hardening log files or checkpointing data files. This reduces the write, flush, write, flush pattern allowing for write, write, write, ... then flush as we only issue flush requests when required to maintain the data integrity boundaries of SQL Server. The performance is significantly better then the device flush for each write as you can imagine.

Testing shows the FUA enhancement is better then the write, flush pattern. For SQL Server we want to dynamically open with O_DIRECT | O_DSYNC when REQ_FUA can be properly used and open with O_DIRECT and leverage SQL Server's alternate flush scheme when running on older kernel or a system that does not support FUA (SATA, IDE, ...)

There is no really good way to figure this out except for benchmarking, given that the FUA use an implementation detail.

Btw, another feature that might be interesting to you is the RWF_DSYNC flag to the pwritev2 syscall, which allows to apply O_DSYNC semantics on a per-I/O basis instead of using it at open time.

Awesome news on the spin.

Are you going to be able to make changes to EXT4 to accommodate REQ_FUA without generic_write_flush to improve performance like was done for xfs?

-----Original Message-----
From: Jan Kara <[email protected]>
Sent: Monday, March 19, 2018 11:07 AM
To: Robert Dorr <[email protected]>
Cc: Christoph Hellwig <[email protected]>; Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; linux-fsdevel <[email protected]>; Jan Kara <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 2 lines · Robert Dorr

I tried the RWF_ODSYNC and xfs seems to honor properly but it causes ext4 to go spin out a single CPU and hang the system. 😊

Yeah, that's a bug in generic direct IO code that I've just recently fixed (d9c10e5b8863 "direct-io: Fix sleep in atomic due to sync AIO" in 4.16-rc4).

Honza

Quoted 20 lines

-----Original Message-----
From: Christoph Hellwig <[email protected]>
Sent: Tuesday, March 13, 2018 11:13 AM
To: Robert Dorr <[email protected]>
Cc: Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; Christoph Hellwig <[email protected]>; linux-fsdevel <[email protected]>; Jan Kara <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 1 line

I think the answer is the iomap* logic makes sure to issue generic_write_sync for O_DSYNC W1 after W1 is completed by hardware and then waits for completion of the flush request (REQ_PREFLUSH) before W1 is returned to the AIO completion ring, preventing io_getevents from processing W1 before the flush occurs and completes. I just need proper confirmation from the experts on this code that this is the expected behavior.

Yes. that is the case.

Quoted 7 lines

Quoted 3 lines

For SQL Server using O_DIRECT | O_DSYNC on current kernels is very performance impacting. Instead we enable a mode for SQL that opens O_DIRECT only and issues fsync/fdatasync when we are hardening log files or checkpointing data files. This reduces the write, flush, write, flush pattern allowing for write, write, write, ... then flush as we only issue flush requests when required to maintain the data integrity boundaries of SQL Server. The performance is significantly better then the device flush for each write as you can imagine.

Testing shows the FUA enhancement is better then the write, flush pattern. For SQL Server we want to dynamically open with O_DIRECT | O_DSYNC when REQ_FUA can be properly used and open with O_DIRECT and leverage SQL Server's alternate flush scheme when running on older kernel or a system that does not support FUA (SATA, IDE, ...)

There is no really good way to figure this out except for benchmarking, given that the FUA use an implementation detail.

Btw, another feature that might be interesting to you is the RWF_DSYNC flag to the pwritev2 syscall, which allows to apply O_DSYNC semantics on a per-I/O basis instead of using it at open time.

--
Jan Kara <[email protected]>
SUSE Labs, CR

Awesome news on the spin.

Are you going to be able to make changes to EXT4 to accommodate REQ_FUA without generic_write_flush to improve performance like was done for xfs?

-----Original Message-----
From: Jan Kara <[email protected]>
Sent: Monday, March 19, 2018 11:07 AM
To: Robert Dorr <[email protected]>
Cc: Christoph Hellwig <[email protected]>; Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; linux-fsdevel <[email protected]>; Jan Kara <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 2 lines · Robert Dorr

I tried the RWF_ODSYNC and xfs seems to honor properly but it causes ext4 to go spin out a single CPU and hang the system. 😊

Yeah, that's a bug in generic direct IO code that I've just recently fixed
(d9c10e5b8863 "direct-io: Fix sleep in atomic due to sync AIO" in 4.16-rc4).

Honza

Quoted 22 lines

-----Original Message-----
From: Christoph Hellwig <[email protected]>
Sent: Tuesday, March 13, 2018 11:13 AM
To: Robert Dorr <[email protected]>
Cc: Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; Christoph Hellwig <[email protected]>; linux-fsdevel <[email protected]>; Jan Kara <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 1 line

I think the answer is the iomap* logic makes sure to issue generic_write_sync for O_DSYNC W1 after W1 is completed by hardware and then waits for completion of the flush request (REQ_PREFLUSH) before W1 is returned to the AIO completion ring, preventing io_getevents from processing W1 before the flush occurs and completes. I just need proper confirmation from the experts on this code that this is the expected behavior.

Yes. that is the case.

Quoted 2 lines

For SQL Server using O_DIRECT | O_DSYNC on current kernels is very performance impacting. Instead we enable a mode for SQL that opens O_DIRECT only and issues fsync/fdatasync when we are hardening log files or checkpointing data files. This reduces the write, flush, write, flush pattern allowing for write, write, write, ... then flush as we only issue flush requests when required to maintain the data integrity boundaries of SQL Server. The performance is significantly better then the device flush for each write as you can imagine.

Quoted 5 lines

Quoted 1 line

Testing shows the FUA enhancement is better then the write, flush pattern. For SQL Server we want to dynamically open with O_DIRECT | O_DSYNC when REQ_FUA can be properly used and open with O_DIRECT and leverage SQL Server's alternate flush scheme when running on older kernel or a system that does not support FUA (SATA, IDE, ...)

There is no really good way to figure this out except for benchmarking, given that the FUA use an implementation detail.

Btw, another feature that might be interesting to you is the RWF_DSYNC flag to the pwritev2 syscall, which allows to apply O_DSYNC semantics on a per-I/O basis instead of using it at open time.

--
Jan Kara <[email protected]>
SUSE Labs, CR

Awesome news on the spin.

Are you going to be able to make changes to EXT4 to accommodate REQ_FUA without generic_write_flush to improve performance like was done for xfs?

-----Original Message-----
From: Jan Kara <[email protected]>
Sent: Monday, March 19, 2018 11:07 AM
To: Robert Dorr <[email protected]>
Cc: Christoph Hellwig <[email protected]>; Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; linux-fsdevel <[email protected]>; Jan Kara <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 2 lines · Robert Dorr

I tried the RWF_ODSYNC and xfs seems to honor properly but it causes ext4 to go spin out a single CPU and hang the system. 😊

Yeah, that's a bug in generic direct IO code that I've just recently fixed
(d9c10e5b8863 "direct-io: Fix sleep in atomic due to sync AIO" in 4.16-rc4).

Honza

Quoted 22 lines

-----Original Message-----
From: Christoph Hellwig <[email protected]>
Sent: Tuesday, March 13, 2018 11:13 AM
To: Robert Dorr <[email protected]>
Cc: Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; Christoph Hellwig <[email protected]>; linux-fsdevel <[email protected]>; Jan Kara <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 1 line

I think the answer is the iomap* logic makes sure to issue generic_write_sync for O_DSYNC W1 after W1 is completed by hardware and then waits for completion of the flush request (REQ_PREFLUSH) before W1 is returned to the AIO completion ring, preventing io_getevents from processing W1 before the flush occurs and completes. I just need proper confirmation from the experts on this code that this is the expected behavior.

Yes. that is the case.

Quoted 2 lines

For SQL Server using O_DIRECT | O_DSYNC on current kernels is very performance impacting. Instead we enable a mode for SQL that opens O_DIRECT only and issues fsync/fdatasync when we are hardening log files or checkpointing data files. This reduces the write, flush, write, flush pattern allowing for write, write, write, ... then flush as we only issue flush requests when required to maintain the data integrity boundaries of SQL Server. The performance is significantly better then the device flush for each write as you can imagine.

Quoted 5 lines

Quoted 1 line

Testing shows the FUA enhancement is better then the write, flush pattern. For SQL Server we want to dynamically open with O_DIRECT | O_DSYNC when REQ_FUA can be properly used and open with O_DIRECT and leverage SQL Server's alternate flush scheme when running on older kernel or a system that does not support FUA (SATA, IDE, ...)

There is no really good way to figure this out except for benchmarking, given that the FUA use an implementation detail.

Btw, another feature that might be interesting to you is the RWF_DSYNC flag to the pwritev2 syscall, which allows to apply O_DSYNC semantics on a per-I/O basis instead of using it at open time.

--
Jan Kara <[email protected]>
SUSE Labs, CR

Hope you are feeling better.

1. How can we effect similar changes in ext4?
2. How do we get these changes pushed into all common release builds for both xfs or ext4 optimizations?

We are seeing dramatic performance increases with the xfs fixes. https://bugzilla.redhat.com/show_bug.cgi?id=1548524

4 socket, TPCC
9,108 – Old kernel
36,618 – Patched kernel

2 socket, TPCC
9,606 - Old kernel
18,840 - Patched kernel

--------------------------------------------------------------------------------------------------
-- Times in milli-seconds
-- wt=SQL writethrough setting
-- awt=SQL alternatewritethrough setting
-- -T3979=SQL open with Fua behavior
-- -T3982=SQL opens without Fua and use fsync (forced flush) behavior
--------------------------------------------------------------------------------------------------
--
-- Linux patched kernel for XFS FUA opts

--  SQL Server 2017 CU6
--  blktrace active
--  Centos7 - 4proc, 8GB ram running on Hyper-V
--  Win10, 8proc, 32GB ram host
--  Samsung SSD drive
--  Clean SQL Server restarts
--------------------------------------------------------------------------------------------------
--  PATCHED KERNEL 4.16.x
--	     								Create DB Inserts Checkpoint Options
--  ----------------------------------- --------- ------- ---------- -----------------------------------
--  Default (-T3982 = forced flush)		3787      29814   47		 O_DIRECT and fsync from SQL
--  -T3979 and wt=1 awt=1			    4224	  25880   30         O_DIRECT and specialized fdatasync  (Best without FUA is ~10x slower than FUA)
--
--  -T3979 and wt=0						2860	  2570	  13		 O_DIRECT 
--  -T3979 and wt=1 awt=0				4204	  2573	  17		 O_DIRECT | O_DSYNC - Inserts and checkpoint on allocated space nears O_DIRECT only  <--- !!! DESIRED BEHAVIOR !!!
--  -T3979 and wt=1 awt=0 NO STAMP		4127	  2823	  17		 O_DIRECT | O_DSYNC - Issues internal flushs for 1st writes, create faster but inserts slower
--
--  UNPATCHED KERNEL 3.x
--	     								Create DB Inserts Checkpoint Options
--  ----------------------------------- --------- ------- ---------- -----------------------------------
--  Default (-T3982 = forced flush)		3097      29700   26		 O_DIRECT and fsync from SQL
--  -T3979 and wt=1 awt=1			    4210	  25800   27         O_DIRECT and specialized fdatasync
--
--  -T3979 and wt=0						2880	  2134	  6			 O_DIRECT 
--  -T3979 and wt=1 awt=0 		        4443	  25290	  42		 O_DIRECT | O_DSYNC
--  -T3979 and wt=1 awt=0 NO STAMP		4120	  25680	  40		 O_DIRECT | O_DSYNC

--

-----Original Message-----
From: Robert Dorr
Sent: Thursday, March 22, 2018 7:38 AM
To: Jan Kara <[email protected]>
Cc: Christoph Hellwig <[email protected]>; Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; linux-fsdevel <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: RE: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Sorry to hear that. Get well soon.

Thanks for the response.

-----Original Message-----
From: Jan Kara <[email protected]>
Sent: Thursday, March 22, 2018 9:36 AM
To: Robert Dorr <[email protected]>
Cc: Jan Kara <[email protected]>; Christoph Hellwig <[email protected]>; Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; linux-fsdevel <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 4 lines · Robert Dorr

Awesome news on the spin.

Are you going to be able to make changes to EXT4 to accommodate REQ_FUA without generic_write_flush to improve performance like was done for xfs?

I'm currently on a sick leave so I'm slow on replies. Sorry. I don't see a reason why ext4 could not have the same optimization as XFS for that case of direct IO. However I'm not sure when I get to implementing that.

Honza

Quoted 51 lines

-----Original Message-----
From: Jan Kara <[email protected]>
Sent: Monday, March 19, 2018 11:07 AM
To: Robert Dorr <[email protected]>
Cc: Christoph Hellwig <[email protected]>; Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; linux-fsdevel <[email protected]>; Jan Kara <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 2 lines · Robert Dorr

I tried the RWF_ODSYNC and xfs seems to honor properly but it causes ext4 to go spin out a single CPU and hang the system. 😊

Yeah, that's a bug in generic direct IO code that I've just recently fixed
(d9c10e5b8863 "direct-io: Fix sleep in atomic due to sync AIO" in 4.16-rc4).

Honza

Quoted 26 lines

-----Original Message-----
From: Christoph Hellwig <[email protected]>
Sent: Tuesday, March 13, 2018 11:13 AM
To: Robert Dorr <[email protected]>
Cc: Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; Christoph Hellwig <[email protected]>; linux-fsdevel <[email protected]>; Jan Kara <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 1 line

I think the answer is the iomap* logic makes sure to issue generic_write_sync for O_DSYNC W1 after W1 is completed by hardware and then waits for completion of the flush request (REQ_PREFLUSH) before W1 is returned to the AIO completion ring, preventing io_getevents from processing W1 before the flush occurs and completes. I just need proper confirmation from the experts on this code that this is the expected behavior.

Yes. that is the case.

Quoted 3 lines

For SQL Server using O_DIRECT | O_DSYNC on current kernels is very performance impacting. Instead we enable a mode for SQL that opens O_DIRECT only and issues fsync/fdatasync when we are hardening log files or checkpointing data files. This reduces the write, flush, write, flush pattern allowing for write, write, write, ... then flush as we only issue flush requests when required to maintain the data integrity boundaries of SQL Server. The performance is significantly better then the device flush for each write as you can imagine.

Testing shows the FUA enhancement is better then the write, flush pattern. For SQL Server we want to dynamically open with O_DIRECT | O_DSYNC when REQ_FUA can be properly used and open with O_DIRECT and leverage SQL Server's alternate flush scheme when running on older kernel or a system that does not support FUA (SATA, IDE, ...)

There is no really good way to figure this out except for benchmarking, given that the FUA use an implementation detail.

Quoted 4 lines

Quoted 1 line

Btw, another feature that might be interesting to you is the RWF_DSYNC flag to the pwritev2 syscall, which allows to apply O_DSYNC semantics on a per-I/O basis instead of using it at open time.

--
Jan Kara <[email protected]>
SUSE Labs, CR

--
Jan Kara <[email protected]>
SUSE Labs, CR N�����r��y���b�X��ǧv�^�)޺{.n�+����{���"��^n�r���z���h����&��

Hope you are feeling better.

1. How can we effect similar changes in ext4?

2. How do we get these changes pushed into all common release builds for both xfs or ext4 optimizations?

-----Original Message-----
From: Robert Dorr
Sent: Thursday, March 22, 2018 7:38 AM
To: Jan Kara <[email protected]>
Cc: Christoph Hellwig <[email protected]>; Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; linux-fsdevel <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: RE: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Sorry to hear that. Get well soon.

Thanks for the response.

-----Original Message-----
From: Jan Kara <[email protected]>
Sent: Thursday, March 22, 2018 9:36 AM
To: Robert Dorr <[email protected]>
Cc: Jan Kara <[email protected]>; Christoph Hellwig <[email protected]>; Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; linux-fsdevel <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 4 lines · Robert Dorr

Awesome news on the spin.

Are you going to be able to make changes to EXT4 to accommodate REQ_FUA without generic_write_flush to improve performance like was done for xfs?

I'm currently on a sick leave so I'm slow on replies. Sorry. I don't see a reason why ext4 could not have the same optimization as XFS for that case of direct IO. However I'm not sure when I get to implementing that.

Honza

Quoted 47 lines

-----Original Message-----
From: Jan Kara <[email protected]>
Sent: Monday, March 19, 2018 11:07 AM
To: Robert Dorr <[email protected]>
Cc: Christoph Hellwig <[email protected]>; Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; linux-fsdevel <[email protected]>; Jan Kara <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 2 lines · Robert Dorr

I tried the RWF_ODSYNC and xfs seems to honor properly but it causes ext4 to go spin out a single CPU and hang the system. 😊

Yeah, that's a bug in generic direct IO code that I've just recently fixed
(d9c10e5b8863 "direct-io: Fix sleep in atomic due to sync AIO" in 4.16-rc4).

Honza

Quoted 22 lines

-----Original Message-----
From: Christoph Hellwig <[email protected]>
Sent: Tuesday, March 13, 2018 11:13 AM
To: Robert Dorr <[email protected]>
Cc: Dave Chinner <[email protected]>; Dan Williams <[email protected]>; [email protected]; Christoph Hellwig <[email protected]>; linux-fsdevel <[email protected]>; Jan Kara <[email protected]>; Theodore Ts'o <[email protected]>; Matthew Wilcox <[email protected]>; Scott Konersmann <[email protected]>; Slava Oks <[email protected]>; Jasraj Dange <[email protected]>; Michael Nelson <[email protected]>
Subject: Re: [PATCH] [RFC] iomap: Use FUA for pure data O_DSYNC DIO writes

Quoted 1 line

I think the answer is the iomap* logic makes sure to issue generic_write_sync for O_DSYNC W1 after W1 is completed by hardware and then waits for completion of the flush request (REQ_PREFLUSH) before W1 is returned to the AIO completion ring, preventing io_getevents from processing W1 before the flush occurs and completes. I just need proper confirmation from the experts on this code that this is the expected behavior.

Yes. that is the case.

Quoted 2 lines

For SQL Server using O_DIRECT | O_DSYNC on current kernels is very performance impacting. Instead we enable a mode for SQL that opens O_DIRECT only and issues fsync/fdatasync when we are hardening log files or checkpointing data files. This reduces the write, flush, write, flush pattern allowing for write, write, write, ... then flush as we only issue flush requests when required to maintain the data integrity boundaries of SQL Server. The performance is significantly better then the device flush for each write as you can imagine.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

Testing shows the FUA enhancement is better then the write, flush pattern. For SQL Server we want to dynamically open with O_DIRECT | O_DSYNC when REQ_FUA can be properly used and open with O_DIRECT and leverage SQL Server's alternate flush scheme when running on older kernel or a system that does not support FUA (SATA, IDE, ...)

There is no really good way to figure this out except for benchmarking, given that the FUA use an implementation detail.

Btw, another feature that might be interesting to you is the RWF_DSYNC flag to the pwritev2 syscall, which allows to apply O_DSYNC semantics on a per-I/O basis instead of using it at open time.

--
Jan Kara <[email protected]>
SUSE Labs, CR

--
Jan Kara <[email protected]>
SUSE Labs, CR

From: Dave Chinner <[email protected]>

If we are doing direct IO writes with datasync semantics, we often have to flush metadata changes along with the data write. However, if we are overwriting existing data, there are no metadata changes that we need to flush. In this case, optimising the IO by using FUA write makes sense.

We know from teh IOMAP_F_DIRTY flag as to whether a specific inode requires a metadata flush - this is currently used by DAX to ensure extent modi$fication as stable in page fault operations. For direct IO writes, we can use it to determine if we need to flush metadata or not once the data is on disk.

Today, ext4 does use iomap for DIO, but an experiment seems to indicate that something hasn't been wired up to enable FUA for O_DSYNC writes. I've looked at fs/iomap/direct-io.c and it wasn't immediately obvious what I need to add to enable this feature.

I was wondering if you could me some quick hints about what and where I should be looking?

Quoted 8 lines · Dave Chinner

From: Dave Chinner <[email protected]>

If we are doing direct IO writes with datasync semantics, we often have to flush metadata changes along with the data write. However, if we are overwriting existing data, there are no metadata changes that we need to flush. In this case, optimising the IO by using FUA write makes sense.

Quoted 5 lines

We know from teh IOMAP_F_DIRTY flag as to whether a specific inode requires a metadata flush - this is currently used by DAX to ensure extent modi$fication as stable in page fault operations. For direct IO writes, we can use it to determine if we need to flush metadata or not once the data is on disk.

Hi,

I've gotten an inquiry from some engineers at Microsoft who would really like it if ext4 could use FUA writes when doing O_DSYNC writes, since this is soemthing that SQL Server uses. In the discussion for this patch series back in 2018[1], ext4 hadn't yet converted over to iomap for Direct I/O, and so adding this feature for ext4 wasn't really practical.

[1] https://lore.kernel.org/all/[email protected]/

Today, ext4 does use iomap for DIO, but an experiment seems to indicate that something hasn't been wired up to enable FUA for O_DSYNC writes. I've looked at fs/iomap/direct-io.c and it wasn't immediately obvious what I need to add to enable this feature.