[PATCH 0/7] fio: atomic write support

[PATCH 0/7] fio: atomic write support

[John Garry]

This series re-introduces atomic write support.

Some time ago support was added for O_ATOMIC in fio, but O_ATOMIC support
never made it into the linux kernel, so fio support was dropped. However
some plumbing was left behind; specifically, we can still pass command
line arg --atomic=1, which sets thread option oatomic, but that is just
ignored.

Linux v6.11-rc added initial atomic write support through RWF_ATOMIC flag.
See latest man pages for details of RWF_ATOMIC:
https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/

This series uses the "atomic" command line option to set RWF_ATOMIC
on a per IO.

Support is added for libaio, pvsync2, and io_uring ioengines.

Since RWF_ATOMIC is only relevant to those ioengines mentioned, trying to
set --atomic=1 for other ioengines now errors.

The verify feature can be useful to prove atomic writes. However, atomic
writes only ensure a proper CRC, but not sequence number. So sequence
numbers are ignored in verify mode for atomic writes.

This series is marked as an RFC because:
- If ok to use pre-existing plumbing, I am not sure whether setting
  --atomic=1 for unsupported ioengines should error
- Whether verify mode should ignore sequence number always or we should
  add a new option for that.

John Garry (7):
  os-linux: Renumber RWF_UNCACHED
  os-linux: Define RWF_ATOMIC
  os: Reintroduce atomic write support
  verify: Ignore sequence number errors for atomic writes
  pvsync2: Support RWF_ATOMIC
  libaio: Support RWF_ATOMIC
  io_uring: Support RWF_ATOMIC

 engines/io_uring.c |  5 ++++-
 engines/libaio.c   |  7 ++++++-
 engines/sync.c     |  9 ++++++---
 init.c             | 20 +++++++++++++++++---
 ioengines.h        |  2 ++
 options.c          |  2 ++
 os/os-linux.h      |  7 ++++++-
 verify.c           |  2 +-
 8 files changed, 44 insertions(+), 10 deletions(-)

-- 
2.31.1

[PATCH 1/7] os-linux: Renumber RWF_UNCACHED

[John Garry]

This now conflicts with RWF_ATOMIC.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 os/os-linux.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/os/os-linux.h b/os/os-linux.h
index c5cd6515..bdd725f6 100644
--- a/os/os-linux.h
+++ b/os/os-linux.h
@@ -329,7 +329,7 @@ static inline int fio_set_sched_idle(void)
 #endif
 
 #ifndef RWF_UNCACHED
-#define RWF_UNCACHED	0x00000040
+#define RWF_UNCACHED	0x00000080
 #endif
 
 #ifndef RWF_WRITE_LIFE_SHIFT
-- 
2.31.1

Re: [PATCH 1/7] os-linux: Renumber RWF_UNCACHED

[Jens Axboe]

On 8/29/24 6:31 AM, John Garry wrote:
> This now conflicts with RWF_ATOMIC.

Still plan on resurrecting that at some point, but I think
for now we should just drop it. Let me commit something
for that...

-- 
Jens Axboe

[PATCH 2/7] os-linux: Define RWF_ATOMIC

[John Garry]

Add a definition of RWF_ATOMIC when not available from uapi headers.

RWF_ATOMIC is going to be part of Linux v6.11

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 os/os-linux.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/os/os-linux.h b/os/os-linux.h
index bdd725f6..5b2d1714 100644
--- a/os/os-linux.h
+++ b/os/os-linux.h
@@ -328,6 +328,10 @@ static inline int fio_set_sched_idle(void)
 #define RWF_NOWAIT	0x00000008
 #endif
 
+#ifndef RWF_ATOMIC
+#define RWF_ATOMIC	0x00000040
+#endif
+
 #ifndef RWF_UNCACHED
 #define RWF_UNCACHED	0x00000080
 #endif
-- 
2.31.1

[PATCH 3/7] os: Reintroduce atomic write support

[John Garry]

Previously O_ATOMIC support was added in commit d01612f3ae25 ("Add support
for O_ATOMIC"). But support was removed in commit a25ba6c64fe1 ("Get rid of
O_ATOMIC"), as support was never added in the Linux kernel.

Linux kernel 6.11 will add support for RWF_ATOMIC, which can be supported
for various ioengines. See latest man pages for details.

The plumbing was left in place for thread option oatomic, so that will be
reused.

Add a flag to say whether an engine supports atomic writes, and reject
when oatomic is set for an engine which does not support atomic writes.

This is a change in behaviour, as since commit a25ba6c64fe1 ("Get rid of
O_ATOMIC"), this oatomic has been ignored. However, it is better to tell
the user that their ioengine of choice does not support atomic writes.

Today RWF_ATOMIC is only supported for direct-IO. In future it may be
supported for buffered IO. As such, do not auto-set odirect=1 when
oatomic==1.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 init.c        | 6 ++++++
 ioengines.h   | 2 ++
 options.c     | 2 ++
 os/os-linux.h | 1 +
 4 files changed, 11 insertions(+)

diff --git a/init.c b/init.c
index ff3e9a90..84798b3a 100644
--- a/init.c
+++ b/init.c
@@ -855,6 +855,12 @@ static int fixup_options(struct thread_data *td)
 							o->max_bs[DDIR_WRITE]);
 	}
 
+	if (td->o.oatomic && !td_ioengine_flagged(td, FIO_ATOMICWRITES)) {
+		log_err("fio: engine does not support atomic writes\n");
+		td->o.oatomic = 0;
+		ret |= 1;
+	}
+
 	if (o->pre_read) {
 		if (o->invalidate_cache)
 			o->invalidate_cache = 0;
diff --git a/ioengines.h b/ioengines.h
index b9834fec..1531cd89 100644
--- a/ioengines.h
+++ b/ioengines.h
@@ -96,6 +96,7 @@ enum {
 	__FIO_RO_NEEDS_RW_OPEN,		/* open files in rw mode even if we have a read job; only
 					   affects ioengines using generic_open_file */
 	__FIO_MULTI_RANGE_TRIM,		/* ioengine supports trim with more than one range */
+	__FIO_ATOMICWRITES,		/* ioengine supports atomic writes */
 	__FIO_IOENGINE_F_LAST,		/* not a real bit; used to count number of bits */
 };
 
@@ -120,6 +121,7 @@ enum fio_ioengine_flags {
 	FIO_SKIPPABLE_IOMEM_ALLOC	= 1 << __FIO_SKIPPABLE_IOMEM_ALLOC,
 	FIO_RO_NEEDS_RW_OPEN		= 1 << __FIO_RO_NEEDS_RW_OPEN,
 	FIO_MULTI_RANGE_TRIM		= 1 << __FIO_MULTI_RANGE_TRIM,
+	FIO_ATOMICWRITES		= 1 << __FIO_ATOMICWRITES,
 };
 
 /*
diff --git a/options.c b/options.c
index ab650bb7..3f9e7894 100644
--- a/options.c
+++ b/options.c
@@ -2936,6 +2936,7 @@ struct fio_option fio_options[FIO_MAX_OPTS] = {
 		.category = FIO_OPT_C_IO,
 		.group	= FIO_OPT_G_IO_TYPE,
 	},
+#ifdef FIO_HAVE_RWF_ATOMIC
 	{
 		.name	= "atomic",
 		.lname	= "Atomic I/O",
@@ -2946,6 +2947,7 @@ struct fio_option fio_options[FIO_MAX_OPTS] = {
 		.category = FIO_OPT_C_IO,
 		.group	= FIO_OPT_G_IO_TYPE,
 	},
+#endif
 	{
 		.name	= "buffered",
 		.lname	= "Buffered I/O",
diff --git a/os/os-linux.h b/os/os-linux.h
index 5b2d1714..a8c7953d 100644
--- a/os/os-linux.h
+++ b/os/os-linux.h
@@ -62,6 +62,7 @@
 #define FIO_HAVE_BYTEORDER_FUNCS
 #define FIO_HAVE_PWRITEV2
 #define FIO_HAVE_SHM_ATTACH_REMOVED
+#define FIO_HAVE_RWF_ATOMIC
 
 #ifdef MAP_HUGETLB
 #define FIO_HAVE_MMAP_HUGE
-- 
2.31.1

[PATCH 4/7] verify: Ignore sequence number errors for atomic writes

[John Garry]

In verify mode, we check a header sequence number and CRC.

For a job with multiple writers, we may overwrite blocks and this can cause
verification errors.

The verification feature can be useful to prove that atomic writes won't
cause fractured writes from multiple writers. However, atomic writes can't
solve the problem of verification sequence number being incorrect for
multiple fio writers.

For verify mode and atomic writes, ignore sequnce number issues and tell
the user about this.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 init.c   | 14 +++++++++++---
 verify.c |  2 +-
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/init.c b/init.c
index 84798b3a..60f240f8 100644
--- a/init.c
+++ b/init.c
@@ -810,9 +810,17 @@ static int fixup_options(struct thread_data *td)
 		       strstr(o->filename_format, "$jobname") &&
 		       strstr(o->filename_format, "$jobnum") &&
 		       strstr(o->filename_format, "$filenum")))) {
-			log_info("fio: multiple writers may overwrite blocks "
-				"that belong to other jobs. This can cause "
-				"verification failures.\n");
+		       if (td->o.oatomic) {
+				log_info("fio: multiple writers may overwrite "
+				"blocks that belong to other jobs. Since in "
+				"atomic writes mode, this should mean no "
+				"verification failures, as errors in "
+				"sequence number will be ignored.\n");
+		       } else {
+				log_info("fio: multiple writers may overwrite "
+				"blocks that belong to other jobs. This can "
+				"cause verification failures.\n");
+			}
 			ret |= warnings_fatal;
 		}
 
diff --git a/verify.c b/verify.c
index b2fede24..7ebfd3dd 100644
--- a/verify.c
+++ b/verify.c
@@ -854,7 +854,7 @@ static int verify_header(struct io_u *io_u, struct thread_data *td,
 	if (td_write(td) && (td_min_bs(td) == td_max_bs(td)) &&
 	    !td->o.time_based)
 		if (!td->o.verify_only)
-			if (hdr->numberio != io_u->numberio) {
+			if (hdr->numberio != io_u->numberio && !td->o.oatomic) {
 				log_err("verify: bad header numberio %"PRIu16
 					", wanted %"PRIu16,
 					hdr->numberio, io_u->numberio);
-- 
2.31.1

[PATCH 5/7] pvsync2: Support RWF_ATOMIC

[John Garry]

Set RWF_ATOMIC for writes and oatomic==1.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 engines/sync.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/engines/sync.c b/engines/sync.c
index d1999122..6317adbd 100644
--- a/engines/sync.c
+++ b/engines/sync.c
@@ -182,9 +182,11 @@ static enum fio_q_status fio_pvsyncio2_queue(struct thread_data *td,
 
 	if (io_u->ddir == DDIR_READ)
 		ret = preadv2(f->fd, iov, 1, io_u->offset, flags);
-	else if (io_u->ddir == DDIR_WRITE)
+	else if (io_u->ddir == DDIR_WRITE) {
+		if (td->o.oatomic)
+			flags |= RWF_ATOMIC;
 		ret = pwritev2(f->fd, iov, 1, io_u->offset, flags);
-	else if (io_u->ddir == DDIR_TRIM) {
+	} else if (io_u->ddir == DDIR_TRIM) {
 		do_io_u_trim(td, io_u);
 		return FIO_Q_COMPLETED;
 	} else
@@ -483,7 +485,8 @@ static struct ioengine_ops ioengine_pvrw2 = {
 	.open_file	= generic_open_file,
 	.close_file	= generic_close_file,
 	.get_file_size	= generic_get_file_size,
-	.flags		= FIO_SYNCIO,
+	.flags		= FIO_SYNCIO |
+			  FIO_ATOMICWRITES,
 	.options	= options,
 	.option_struct_size	= sizeof(struct psyncv2_options),
 };
-- 
2.31.1

[PATCH 6/7] libaio: Support RWF_ATOMIC

[John Garry]

Set RWF_ATOMIC for writes and oatomic==1.

Guard setting RWF_ATOMIC by FIO_HAVE_RWF_ATOMIC, as only linux supports
RWF_ATOMIC.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 engines/libaio.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/engines/libaio.c b/engines/libaio.c
index aaccc7ce..c2d43793 100644
--- a/engines/libaio.c
+++ b/engines/libaio.c
@@ -110,6 +110,10 @@ static int fio_libaio_prep(struct thread_data *td, struct io_u *io_u)
 		io_prep_pwrite(iocb, f->fd, io_u->xfer_buf, io_u->xfer_buflen, io_u->offset);
 		if (o->nowait)
 			iocb->aio_rw_flags |= RWF_NOWAIT;
+#ifdef FIO_HAVE_RWF_ATOMIC
+		if (td->o.oatomic)
+			iocb->aio_rw_flags |= RWF_ATOMIC;
+#endif
 	} else if (ddir_sync(io_u->ddir))
 		io_prep_fsync(iocb, f->fd);
 
@@ -440,7 +444,8 @@ FIO_STATIC struct ioengine_ops ioengine = {
 	.name			= "libaio",
 	.version		= FIO_IOOPS_VERSION,
 	.flags			= FIO_ASYNCIO_SYNC_TRIM |
-					FIO_ASYNCIO_SETS_ISSUE_TIME,
+					FIO_ASYNCIO_SETS_ISSUE_TIME |
+					FIO_ATOMICWRITES,
 	.init			= fio_libaio_init,
 	.post_init		= fio_libaio_post_init,
 	.prep			= fio_libaio_prep,
-- 
2.31.1

[PATCH 7/7] io_uring: Support RWF_ATOMIC

[John Garry]

Set RWF_ATOMIC for writes and oatomic==1.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 engines/io_uring.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/engines/io_uring.c b/engines/io_uring.c
index bfbdc3b4..79217631 100644
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -399,6 +399,8 @@ static int fio_ioring_prep(struct thread_data *td, struct io_u *io_u)
 			sqe->rw_flags |= RWF_UNCACHED;
 		if (o->nowait)
 			sqe->rw_flags |= RWF_NOWAIT;
+		if (td->o.oatomic && io_u->ddir == DDIR_WRITE)
+			sqe->rw_flags |= RWF_ATOMIC;
 
 		/*
 		 * Since io_uring can have a submission context (sqthread_poll)
@@ -1591,7 +1593,8 @@ static struct ioengine_ops ioengine_uring = {
 	.name			= "io_uring",
 	.version		= FIO_IOOPS_VERSION,
 	.flags			= FIO_ASYNCIO_SYNC_TRIM | FIO_NO_OFFLOAD |
-					FIO_ASYNCIO_SETS_ISSUE_TIME,
+					FIO_ASYNCIO_SETS_ISSUE_TIME |
+					FIO_ATOMICWRITES,
 	.init			= fio_ioring_init,
 	.post_init		= fio_ioring_post_init,
 	.io_u_init		= fio_ioring_io_u_init,
-- 
2.31.1

Re: [PATCH 0/7] fio: atomic write support

[Luis Chamberlain]

On Thu, Aug 29, 2024 at 12:31:00PM +0000, John Garry wrote:
> The verify feature can be useful to prove atomic writes. However, atomic
> writes only ensure a proper CRC, but not sequence number. So sequence
> numbers are ignored in verify mode for atomic writes.

Why not? Note that NVMe TP4098 MAM support (not public yet I think so
I'm not going into details) is outside of scope as the current
RWF_ATOMIC requirements specifically require natural alignment and for
the writes to be between  stx_atomic_write_unit_min (LBA format) and
stx_atomic_write_unit_max (AWUPF) -- why are we not getting sequence
numbers right? Is that a SCSI issue?

> This series is marked as an RFC because:
> - Whether verify mode should ignore sequence number always or we should
>   add a new option for that.

  Luis

Re: [PATCH 0/7] fio: atomic write support

[John Garry]

On 05/09/2024 10:37, Luis Chamberlain wrote:


Sorry for the slow response. For some reason I did not receive this 
mail. I just downloaded from lore now.

> On Thu, Aug 29, 2024 at 12:31:00PM +0000, John Garry wrote:
>> The verify feature can be useful to prove atomic writes. However, atomic
>> writes only ensure a proper CRC, but not sequence number. So sequence
>> numbers are ignored in verify mode for atomic writes.

How fio verify mode works is that we write datablocks to the file with a 
CRC and sequence number per block. When verifying, we check the expected 
sequence number and embedded CRC per block - if not as expected, we 
error. This should not happen for a single job.

However, if we have multiple jobs, they may overwrite blocks from other 
jobs - fio warns us about that. As such, in verify mode, the read 
sequence number may not match was we were expecting with multiple jobs 
running. In normal non-atomic write verify mode, the CRC may also be 
incorrect, as we may see partially written blocks.

However, in atomic write verify mode, the CRC should be good with 
multiple jobs, as we won't see a partial write. The data may not 
actually be what we wrote - hence the possible incorrect sequence number 
- but the CRC itself should be ok.

> 
> Why not? Note that NVMe TP4098 MAM support (not public yet I think so
> I'm not going into details) 

This is not related to MAM, and really an fio feature.

> is outside of scope as the current
> RWF_ATOMIC requirements specifically require natural alignment and for
> the writes to be between  stx_atomic_write_unit_min (LBA format) and
> stx_atomic_write_unit_max (AWUPF) -- why are we not getting sequence
> numbers right? Is that a SCSI issue?

This is related to how fio verify mode works, and not related to SCSI or 
NVMe.

cheers,
John

Re: [PATCH 0/7] fio: atomic write support

[Luis Chamberlain]

On Fri, Sep 06, 2024 at 04:22:24PM +0100, John Garry wrote:
> However, if we have multiple jobs, they may overwrite blocks from other jobs
> - fio warns us about that. As such, in verify mode, the read sequence number
> may not match was we were expecting with multiple jobs running.

Is that a generic bug in fio?

  Luis

Re: [PATCH 0/7] fio: atomic write support

[John Garry]

On 07/09/2024 00:29, Luis Chamberlain wrote:
> On Fri, Sep 06, 2024 at 04:22:24PM +0100, John Garry wrote:
>> However, if we have multiple jobs, they may overwrite blocks from other jobs
>> - fio warns us about that. As such, in verify mode, the read sequence number
>> may not match was we were expecting with multiple jobs running.
> Is that a generic bug in fio?

I wouldn't say so. That is just how the verify mode works.

Fundamentally verify mode is to verify that we read back the contents of 
the file as expected after each iteration. If some other job comes along 
and overwrites that data, then that cannot be helped.

However maybe I should just create a new verify mode for atomic writes, 
so it is clear what the verify operation constraints are.

Thanks,
John

Re: [PATCH 0/7] fio: atomic write support

[Jens Axboe]

On 9/9/24 2:12 AM, John Garry wrote:
> On 07/09/2024 00:29, Luis Chamberlain wrote:
>> On Fri, Sep 06, 2024 at 04:22:24PM +0100, John Garry wrote:
>>> However, if we have multiple jobs, they may overwrite blocks from other jobs
>>> - fio warns us about that. As such, in verify mode, the read sequence number
>>> may not match was we were expecting with multiple jobs running.
>> Is that a generic bug in fio?
> 
> I wouldn't say so. That is just how the verify mode works.

It is, and it's quite possible to craft a job that won't verify (even
though fio warns you about it, as you said). If you have separate IO
threads stomping on each others writes, it won't verify. That's mostly a
"doctor it hurts when I..." situation.

FWIW, Luis, your email gets marked as spam. I think you need to check
settings. I only see John's replies.

-- 
Jens Axboe

Re: [PATCH 0/7] fio: atomic write support

[John Garry]

On 09/09/2024 20:48, Jens Axboe wrote:
> On 9/9/24 2:12 AM, John Garry wrote:
>> On 07/09/2024 00:29, Luis Chamberlain wrote:
>>> On Fri, Sep 06, 2024 at 04:22:24PM +0100, John Garry wrote:
>>>> However, if we have multiple jobs, they may overwrite blocks from other jobs
>>>> - fio warns us about that. As such, in verify mode, the read sequence number
>>>> may not match was we were expecting with multiple jobs running.
>>> Is that a generic bug in fio?
>>
>> I wouldn't say so. That is just how the verify mode works.
> 
> It is, and it's quite possible to craft a job that won't verify (even
> though fio warns you about it, as you said). If you have separate IO
> threads stomping on each others writes, it won't verify. That's mostly a
> "doctor it hurts when I..." situation.
> 
> FWIW, Luis, your email gets marked as spam. I think you need to check
> settings. I only see John's replies.
> 
Good, so it's not just me (not receiving them properly).

Re: [PATCH 0/7] fio: atomic write support

[Luis Chamberlain]

On Mon, Sep 09, 2024 at 01:48:23PM -0600, Jens Axboe wrote:
> 
> FWIW, Luis, your email gets marked as spam. I think you need to check
> settings. I only see John's replies.

kernel.org has bumped the requirements for spam, and so I need to use
kernel.org for SMTP now.

  Luis

Re: [PATCH 0/7] fio: atomic write support

[Jens Axboe]

John,

Maybe it wasn't clear, but I committed that change I mentioned in my
initial reply basically right after. If you could send a v2 on top of
the current tree, that'd be great.

-- 
Jens Axboe

Re: [PATCH 0/7] fio: atomic write support

[John Garry]

On 09/09/2024 20:53, Jens Axboe wrote:
> John,
> 
> Maybe it wasn't clear, but I committed that change I mentioned in my
> initial reply basically right after.

Thanks for doing that.

> If you could send a v2 on top of
> the current tree, that'd be great.

I was preparing a v2 the same as this, but do you think that I should 
just add a new verify option for atomic writes to ignore the header? Or 
is just saying "you have selected atomic=1, so I'll just ignore the 
header for you" ok?

Cheers,
John

Re: [PATCH 0/7] fio: atomic write support

[Jens Axboe]

On 9/9/24 2:28 PM, John Garry wrote:
> On 09/09/2024 20:53, Jens Axboe wrote:
>> John,
>>
>> Maybe it wasn't clear, but I committed that change I mentioned in my
>> initial reply basically right after.
> 
> Thanks for doing that.
> 
>> If you could send a v2 on top of
>> the current tree, that'd be great.
> 
> I was preparing a v2 the same as this, but do you think that I should
> just add a new verify option for atomic writes to ignore the header?
> Or is just saying "you have selected atomic=1, so I'll just ignore the
> header for you" ok?

I'm still not following why atomic writes make this any different. They
should not. If you have multiple writers writing the same blocks, yeah
you will get verification errors. This is true for any kind of write, be
it buffered, dio, or atomic. So unless I'm missing something here, I'd
just drop that patch.

-- 
Jens Axboe

Re: [PATCH 0/7] fio: atomic write support

[John Garry]

On 10/09/2024 02:01, Jens Axboe wrote:

+ Dave, who might be interested in this

>> I was preparing a v2 the same as this, but do you think that I should
>> just add a new verify option for atomic writes to ignore the header?
>> Or is just saying "you have selected atomic=1, so I'll just ignore the
>> header for you" ok?
> I'm still not following why atomic writes make this any different. They
> should not. If you have multiple writers writing the same blocks, yeah
> you will get verification errors. This is true for any kind of write, be
> it buffered, dio, or atomic. So unless I'm missing something here, I'd
> just drop that patch.

Some background is that main selling point of atomic writes is that we 
guarantee writes to storage will not be torn for a power failure or 
kernel crash.

Another aspect of atomic writes is that they handle racing writes and 
reads, such that a read racing with a write will see all the data from 
the write or none. Well, SCSI and NVMe guarantee this if using 
RWF_ATOMIC, but it is not formally stated as a feature of RWF_ATOMIC.

It can be argued that having racing reads and writes is an application 
bug. Furthermore, as I understand, even if posix guarantees that regular 
writes are "atomic", it is not the case generally.

So one part of the relevance of atomic writes to fio verify is that we 
can verify that atomic writes "safely" handle racing read and writes. 
For this, the CRC checks would be successful if we have many jobs; 
however header sequence numbers are not. Hence patch 4/7.

I had also been using the verify feature to test atomic writes for power 
failures. In this case, I run a single verify job with --rw=write, power 
fail, and use verify in read mode to prove no invalid data in the file, 
like:

fio --filename=mnt/file --direct=1 --rw=read --bs=8k --iodepth=100 --na
me=iops --numjobs=1 --loops=1 --verify=crc64 --ioengine=libaio
--verify_fatal=1 --group_reporting  --exitall_on_error

This power fail test is what I am mostly interested in.

So my point is that the patch to ignore invalid headers could be 
dropped, but let me know your thoughts.

Thanks,
John

Re: [PATCH 0/7] fio: atomic write support

[Jens Axboe]

On 9/10/24 4:03 AM, John Garry wrote:
> On 10/09/2024 02:01, Jens Axboe wrote:
> 
> + Dave, who might be interested in this
> 
>>> I was preparing a v2 the same as this, but do you think that I should
>>> just add a new verify option for atomic writes to ignore the header?
>>> Or is just saying "you have selected atomic=1, so I'll just ignore the
>>> header for you" ok?
>> I'm still not following why atomic writes make this any different. They
>> should not. If you have multiple writers writing the same blocks, yeah
>> you will get verification errors. This is true for any kind of write, be
>> it buffered, dio, or atomic. So unless I'm missing something here, I'd
>> just drop that patch.

(side note - please wrap your email lines, I always re-wrap when
replying)

> Some background is that main selling point of atomic writes is that we
> guarantee writes to storage will not be torn for a power failure or
> kernel crash.
> 
> Another aspect of atomic writes is that they handle racing writes and
> reads, such that a read racing with a write will see all the data from
> the write or none. Well, SCSI and NVMe guarantee this if using
> RWF_ATOMIC, but it is not formally stated as a feature of RWF_ATOMIC.
> 
> It can be argued that having racing reads and writes is an application
> bug. Furthermore, as I understand, even if posix guarantees that
> regular writes are "atomic", it is not the case generally.
> 
> So one part of the relevance of atomic writes to fio verify is that we
> can verify that atomic writes "safely" handle racing read and writes.
> For this, the CRC checks would be successful if we have many jobs;
> however header sequence numbers are not. Hence patch 4/7.
> 
> I had also been using the verify feature to test atomic writes for
> power failures. In this case, I run a single verify job with
> --rw=write, power fail, and use verify in read mode to prove no
> invalid data in the file, like:
> 
> fio --filename=mnt/file --direct=1 --rw=read --bs=8k --iodepth=100 --na
> me=iops --numjobs=1 --loops=1 --verify=crc64 --ioengine=libaio
> --verify_fatal=1 --group_reporting  --exitall_on_error
> 
> This power fail test is what I am mostly interested in.
> 
> So my point is that the patch to ignore invalid headers could be
> dropped, but let me know your thoughts.

Gotcha, that makes sense. For atomic writes, it's totally fine to have
overlapping writers if the write size is in the atomic units, but we can
of course expect sequences to be out of order depending on which one
makes it to stable storage. But you have no checks for whether or not
the write size is within the atomic range?

I think dropping patch 4 and adding a verify option to specifically
ignore the sequence would make sense, leaving the control in the hands
of the user. And then bonus points for adding an example job file (with
comments) in your series that shows how to use atomic writes (and uses
that option) would be useful.

-- 
Jens Axboe

Re: [PATCH 0/7] fio: atomic write support

[John Garry]

On 10/09/2024 16:56, Jens Axboe wrote:
>>> t drop that patch.
> (side note - please wrap your email lines, I always re-wrap when
> replying)

ok

> 
>> Some background is that main selling point of atomic writes is that we
>> guarantee writes to storage will not be torn for a power failure or
>> kernel crash.
>>
>> Another aspect of atomic writes is that they handle racing writes and
>> reads, such that a read racing with a write will see all the data from
>> the write or none. Well, SCSI and NVMe guarantee this if using
>> RWF_ATOMIC, but it is not formally stated as a feature of RWF_ATOMIC.
>>
>> It can be argued that having racing reads and writes is an application
>> bug. Furthermore, as I understand, even if posix guarantees that
>> regular writes are "atomic", it is not the case generally.
>>
>> So one part of the relevance of atomic writes to fio verify is that we
>> can verify that atomic writes "safely" handle racing read and writes.
>> For this, the CRC checks would be successful if we have many jobs;
>> however header sequence numbers are not. Hence patch 4/7.
>>
>> I had also been using the verify feature to test atomic writes for
>> power failures. In this case, I run a single verify job with
>> --rw=write, power fail, and use verify in read mode to prove no
>> invalid data in the file, like:
>>
>> fio --filename=mnt/file --direct=1 --rw=read --bs=8k --iodepth=100 --na
>> me=iops --numjobs=1 --loops=1 --verify=crc64 --ioengine=libaio
>> --verify_fatal=1 --group_reporting  --exitall_on_error
>>
>> This power fail test is what I am mostly interested in.
>>
>> So my point is that the patch to ignore invalid headers could be
>> dropped, but let me know your thoughts.
> Gotcha, that makes sense. For atomic writes, it's totally fine to have
> overlapping writers if the write size is in the atomic units, but we can
> of course expect sequences to be out of order depending on which one
> makes it to stable storage. But you have no checks for whether or not
> the write size is within the atomic range?

I don't currently. If an atomic write size is out-of-range, the kernel 
will reject it with -EINVAL. However -EINVAL can be returned for a 
multitude of issues, so not much help. So I could add a statx call to 
get the limits and then error the fio config (if bs is out-of-range).

> 
> I think dropping patch 4 and adding a verify option to specifically
> ignore the sequence would make sense, leaving the control in the hands
> of the user. 

I already saw verify modes "no header" and "header only", so I was 
reluctant to add a potentially conflicting new separate option to ignore 
the header sequence. However, I can look to add a new verify option to 
ignore the header sequence and ensure it respects those mentioned verify 
modes.

> And then bonus points for adding an example job file (with
> comments) in your series that shows how to use atomic writes (and uses
> that option) would be useful.

ok, I'm happy to do that.

Thanks,
John

Re: [PATCH 0/7] fio: atomic write support

[Jens Axboe]

On 9/10/24 10:38 AM, John Garry wrote:
> On 10/09/2024 16:56, Jens Axboe wrote:
>>>> t drop that patch.
>> (side note - please wrap your email lines, I always re-wrap when
>> replying)
> 
> ok

This one too :)

>>> Some background is that main selling point of atomic writes is that we
>>> guarantee writes to storage will not be torn for a power failure or
>>> kernel crash.
>>>
>>> Another aspect of atomic writes is that they handle racing writes and
>>> reads, such that a read racing with a write will see all the data from
>>> the write or none. Well, SCSI and NVMe guarantee this if using
>>> RWF_ATOMIC, but it is not formally stated as a feature of RWF_ATOMIC.
>>>
>>> It can be argued that having racing reads and writes is an application
>>> bug. Furthermore, as I understand, even if posix guarantees that
>>> regular writes are "atomic", it is not the case generally.
>>>
>>> So one part of the relevance of atomic writes to fio verify is that we
>>> can verify that atomic writes "safely" handle racing read and writes.
>>> For this, the CRC checks would be successful if we have many jobs;
>>> however header sequence numbers are not. Hence patch 4/7.
>>>
>>> I had also been using the verify feature to test atomic writes for
>>> power failures. In this case, I run a single verify job with
>>> --rw=write, power fail, and use verify in read mode to prove no
>>> invalid data in the file, like:
>>>
>>> fio --filename=mnt/file --direct=1 --rw=read --bs=8k --iodepth=100 --na
>>> me=iops --numjobs=1 --loops=1 --verify=crc64 --ioengine=libaio
>>> --verify_fatal=1 --group_reporting  --exitall_on_error
>>>
>>> This power fail test is what I am mostly interested in.
>>>
>>> So my point is that the patch to ignore invalid headers could be
>>> dropped, but let me know your thoughts.
>> Gotcha, that makes sense. For atomic writes, it's totally fine to have
>> overlapping writers if the write size is in the atomic units, but we can
>> of course expect sequences to be out of order depending on which one
>> makes it to stable storage. But you have no checks for whether or not
>> the write size is within the atomic range?
> 
> I don't currently. If an atomic write size is out-of-range, the kernel
> will reject it with -EINVAL. However -EINVAL can be returned for a
> multitude of issues, so not much help. So I could add a statx call to
> get the limits and then error the fio config (if bs is out-of-range).

Good point, I guess that's good enough then. If it's an invalid
configuration, then the writes will get errored anyway.

>> I think dropping patch 4 and adding a verify option to specifically
>> ignore the sequence would make sense, leaving the control in the hands
>> of the user. 
> 
> I already saw verify modes "no header" and "header only", so I was
> reluctant to add a potentially conflicting new separate option to
> ignore the header sequence. However, I can look to add a new verify
> option to ignore the header sequence and ensure it respects those
> mentioned verify modes.

I think you'd be fine adding a verify_write_sequence bool and just have
it default to true.

>> And then bonus points for adding an example job file (with
>> comments) in your series that shows how to use atomic writes (and uses
>> that option) would be useful.
> 
> ok, I'm happy to do that.

Great, thanks.

-- 
Jens Axboe

Re: [PATCH 0/7] fio: atomic write support

[John Garry]

On 10/09/2024 17:56, Jens Axboe wrote:
>> ok
> This one too 🙂

I won't write much..

but happy to make suggested changes.

Thanks,
John