Flush enhancements and fixes #1777

minwooim · 2024-06-22T16:17:46Z

This patchset introduces enhancements of fsync/fdatasync operations and
fixes. The first patch added support for NVMe FLUSH commands in
io_uring_cmd ioengine. The second one fixes issuing fsync/fdatasync
even if the previous command is READ. As documentation says, fsync
should sync the file after every N WRITE commands issued. The third one
fixes not issuing fsync/fdatasync in trimwrite worklload.
report.

Minwoo Im (4):
io_uring: Add support FLUSH command
io_u: ensure fsync only after write(s)
io_u: Support fsync for --rw=trimwrite

vincentkfu · 2024-06-26T20:50:41Z

engines/io_uring.c

@@ -39,6 +39,7 @@ enum uring_cmd_write_mode {
 	FIO_URING_CMD_WMODE_UNCOR,
 	FIO_URING_CMD_WMODE_ZEROES,
 	FIO_URING_CMD_WMODE_VERIFY,
+	FIO_URING_CMD_WMODE_FLUSH,


Why is this added?

Uhh... my bad. It's something remaining from the previous approach. Will remove it!

Still needs to be removed

Will remove it!

vincentkfu · 2024-06-26T21:29:52Z

For the second patch, it looks like what's happening is that fio issues a write, followed by a sync, then a read, then (since last_was_sync is false) another sync.

For the second patch, why don't you also remove last_was_sync when adding last_ddir_issued?

In should_fsync replace the check of last_was_sync by checking if last_ddir_issued is false.

Also please rename last_ddir to last_ddir_completed.

Can you create a test case for this?

minwooim · 2024-06-29T00:58:02Z

For the second patch, it looks like what's happening is that fio issues a write, followed by a sync, then a read, then (since last_was_sync is false) another sync.

If last_ddir_issued are going to like:

last_ddir_issued = DDIR_WRITE
last_ddir_issued = DDIR_SYNC
last_ddir_issued = DDIR_READ

then if (should_fsync(td) && td->last_ddir_issued != DDIR_READ) should be false and another sync will not happen.

For the second patch, why don't you also remove last_was_sync when adding last_ddir_issued?

In should_fsync replace the check of last_was_sync by checking if last_ddir_issued is false.

If you meant last_ddir_issued is not DDIR_SYNC, got your point.

Also please rename last_ddir to last_ddir_completed.

Okay.

Can you create a test case for this?

Alright, I'll work on the mixed workload and create test cases using the issued rwts section of the report.

minwooim · 2024-07-04T14:11:32Z

@vincentkfu ,

I've added a test case to test whether FLUSH commands are issued properly or not.
Please review.

vincentkfu · 2024-07-08T19:28:55Z

fio.h

@@ -629,7 +630,7 @@ static inline bool multi_range_trim(struct thread_data *td, struct io_u *io_u)

 static inline bool should_fsync(struct thread_data *td)
 {
-	if (td->last_was_sync)
+	if (td->last_ddir_issued == DDIR_SYNC)


With last_was_sync we had:

td->last_was_sync = ddir_sync(io_u->ddir);

So this should use ddir_sync() instead of checking for equality with DDIR_SYNC because we actually have three different sync DDIRs.

Agreed! will fix it. Thanks.

vincentkfu · 2024-07-08T19:45:31Z

For the io_u: ensure fsync only after write(s) patch can you split it into two patches? One with the rename last_ddir to last_ddir_completed, and one with the bug fix.

Finally, the test cases look good but can you just add them to nvmept.py? It doesn't make sense to have a new script for these four test cases.

minwooim · 2024-07-08T21:12:35Z

For the io_u: ensure fsync only after write(s) patch can you split it into two patches? One with the rename last_ddir to last_ddir_completed, and one with the bug fix.

Sure, I will.

Finally, the test cases look good but can you just add them to nvmept.py? It doesn't make sense to have a new script for these four test cases.

Okay.

vincentkfu · 2024-07-09T14:18:51Z

Don't forget to remove the superfluous item from uring_cmd_write_mode.

Add support for --fsync and --fdatasync in io_uring_cmd ioengine to enable FLUSH commands just like libaio or io_uring ioengines. If --fsync or --fdatasync is given N, FLUSH command will be issued as per N write commands. Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>

`last_ddir` represents the data direction of the latest completed command. To avoid confusions, this patch renamed `last_ddir` to `last_ddir_completed` to make it much more clear. Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>

minwooim · 2024-07-09T23:15:54Z

@vincentkfu ,

Applied review comments, Thanks for your time.

vincentkfu · 2024-07-10T18:04:27Z

ioengines.c

@@ -466,6 +462,7 @@ enum fio_q_status td_io_queue(struct thread_data *td, struct io_u *io_u)
 		}
 	}

+	td->last_ddir_issued = ddir;


I did not notice this in the previous version of your patch but if ret == FIO_Q_BUSY then the I/O was not actually issued. Assignment of last_ddir_issued should go in the two places where last_was_sync was set.

Ah, indeed. Let me fix this up!

vincentkfu · 2024-07-10T18:08:47Z

libfio.c

@@ -101,7 +101,6 @@ static void reset_io_counters(struct thread_data *td, int all)

 	td->zone_bytes = 0;

-	td->last_was_sync = false;


I think we never should have set last_was_sync to false here because when ramp time is over we want to continue to sync at the appropriate point, so there is no need to modify the value for last_ddir_issued. Agree?

If you meant we don't need to modify the value of last_ddir_issued in this place, yes, I agree.

`last_was_sync` has represented that the last command had DDIR_SYNC. This can be replaced with `ddir_sync(last_ddir_issued)` and it's much more flexible to represent the last issued command's data direction. Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>

When using `--rw=write --fsync=N`, the FLUSH command is correctly issued after N WRITE commands. However, if READ commands are mixed in with --rw, fsync occurs after READ commands as well. This patch ensures that fsync is only triggered after the specified number of WRITE commands, regardless of READ commands. Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>

Even if ddir is determined in get_rw_ddir(), ddir might be updated in set_rw_ddir(). if td represents trimwrite, it will be updated to either DDIR_TRIM or DDIR_WRITE even ddir already represents for DDIR_SYNC. To support DDIR_SYNC(fsync) for trimwrite, this patch checks ddir_sync() in case of trimwrite not to update the pre-determined ddir. Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>

This test script tests number of FLUSH commands triggered by --fsync=<N> options to make FLUSH commands are followed by the WRITE commands from the various --rw I/O workload. Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>

vincentkfu · 2024-07-11T14:00:21Z

Applied, thanks.

minwooim force-pushed the io-uring-cmd/nvme/add-flush branch from 0331123 to 8dfaf60 Compare June 22, 2024 16:40

vincentkfu reviewed Jun 26, 2024

View reviewed changes

minwooim force-pushed the io-uring-cmd/nvme/add-flush branch from 8dfaf60 to ac1fa5f Compare July 4, 2024 14:10

vincentkfu reviewed Jul 8, 2024

View reviewed changes

minwooim added 2 commits July 10, 2024 07:40

td: Rename last_ddir to last_ddir_completed

87b1002

`last_ddir` represents the data direction of the latest completed command. To avoid confusions, this patch renamed `last_ddir` to `last_ddir_completed` to make it much more clear. Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>

minwooim force-pushed the io-uring-cmd/nvme/add-flush branch from ac1fa5f to 9d3f7dd Compare July 9, 2024 23:14

vincentkfu reviewed Jul 10, 2024

View reviewed changes

minwooim added 4 commits July 11, 2024 16:58

t/nvmept.py: Add test cases for FLUSH

e84adcf

This test script tests number of FLUSH commands triggered by --fsync=<N> options to make FLUSH commands are followed by the WRITE commands from the various --rw I/O workload. Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>

minwooim force-pushed the io-uring-cmd/nvme/add-flush branch from 9d3f7dd to e84adcf Compare July 11, 2024 08:11

axboe merged commit 89fc401 into axboe:master Jul 11, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flush enhancements and fixes #1777

Flush enhancements and fixes #1777

minwooim commented Jun 22, 2024 •

edited

Loading

vincentkfu Jun 26, 2024

minwooim Jun 26, 2024

vincentkfu Jul 8, 2024

minwooim Jul 8, 2024

vincentkfu commented Jun 26, 2024

minwooim commented Jun 29, 2024

minwooim commented Jul 4, 2024

vincentkfu Jul 8, 2024

minwooim Jul 8, 2024

vincentkfu commented Jul 8, 2024

minwooim commented Jul 8, 2024

vincentkfu commented Jul 9, 2024 •

edited

Loading

minwooim commented Jul 9, 2024

vincentkfu Jul 10, 2024

minwooim Jul 11, 2024

vincentkfu Jul 10, 2024

minwooim Jul 11, 2024

vincentkfu commented Jul 11, 2024

		@@ -101,7 +101,6 @@ static void reset_io_counters(struct thread_data *td, int all)

		td->zone_bytes = 0;

		td->last_was_sync = false;

Flush enhancements and fixes #1777

Flush enhancements and fixes #1777

Conversation

minwooim commented Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentkfu commented Jun 26, 2024

minwooim commented Jun 29, 2024

minwooim commented Jul 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentkfu commented Jul 8, 2024

minwooim commented Jul 8, 2024

vincentkfu commented Jul 9, 2024 • edited Loading

minwooim commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentkfu commented Jul 11, 2024

minwooim commented Jun 22, 2024 •

edited

Loading

vincentkfu commented Jul 9, 2024 •

edited

Loading