Patch to add read and write fua bit options to the SG engine

Patch to add read and write fua bit options to the SG engine

[Kris Davis]

I like to submit the patch below to add read and write fua bit options to the SG engine.  It adds a readfua=bool, and writefua=bool options in the SG engine, which causes the "force unit access" bits to be set in the read and write operations respectively.  I patterned the code changes from what I found in the libaio engine option code.  So, I also added a "__FIO_OPT_G_SG" enum to optgroup.h, which seemed to follow the pattern - but I really don't understand where or how the enum settings values in the options are used, so I'm unsure.
Thanks
Kris Davis



diff --git a/HOWTO b/HOWTO
index acb9e97..4f9d26a 100644
--- a/HOWTO
+++ b/HOWTO
@@ -1747,6 +1747,7 @@
 			:manpage:`read(2)` and :manpage:`write(2)` for asynchronous
 			I/O. Requires :option:`filename` option to specify either block or
 			character devices.
+			The sg engine includes engine specific options.
 
 		**null**
 			Doesn't transfer any data, just pretends to.  This is mainly used to
@@ -2068,6 +2069,17 @@
 	multiple paths exist between the client and the server or in certain loopback
 	configurations.
 
+.. option:: readfua=bool : [sg]
+
+	With readfua option set to 1, read operations include the 
+	the force unit access (fua) flag.
+
+.. option:: writefua=bool : [sg]
+
+	With writefua option set to 1, write operations include the 
+	the force unit access (fua) flag.
+
+
 I/O depth
 ~~~~~~~~~
 
diff --git a/engines/sg.c b/engines/sg.c
index 4540b57..f240755 100644
--- a/engines/sg.c
+++ b/engines/sg.c
@@ -12,8 +12,42 @@
 #include <sys/poll.h>
 
 #include "../fio.h"
+#include "../optgroup.h"
 
 #ifdef FIO_HAVE_SGIO
+
+
+struct sg_options {
+	void *pad;
+	unsigned int readfua;
+	unsigned int writefua;
+};
+
+static struct fio_option options[] = {
+	{
+		.name	= "readfua",
+		.lname	= "sg engine read fua flag support",
+		.type	= FIO_OPT_BOOL,
+		.off1	= offsetof(struct sg_options, readfua),
+		.help	= "Set FUA flag (force unit access) for all Read operations",
+		.def	= "0",
+		.category = FIO_OPT_C_ENGINE,
+		.group	= FIO_OPT_G_SG,
+	},
+	{
+		.name	= "writefua",
+		.lname	= "sg engine write fua flag support",
+		.type	= FIO_OPT_BOOL,
+		.off1	= offsetof(struct sg_options, writefua),
+		.help	= "Set FUA flag (force unit access) for all Write operations",
+		.def	= "0",
+		.category = FIO_OPT_C_ENGINE,
+		.group	= FIO_OPT_G_SG,
+	},
+	{
+		.name	= NULL,
+	},
+};
 
 #define MAX_10B_LBA  0xFFFFFFFFULL
 #define SCSI_TIMEOUT_MS 30000   // 30 second timeout; currently no method to override
@@ -267,6 +301,7 @@
 static int fio_sgio_prep(struct thread_data *td, struct io_u *io_u)
 {
 	struct sg_io_hdr *hdr = &io_u->hdr;
+	struct sg_options *o = td->eo;
 	struct sgio_data *sd = td->io_ops_data;
 	long long nr_blocks, lba;
 
@@ -286,6 +321,10 @@
 			hdr->cmdp[0] = 0x28; // read(10)
 		else
 			hdr->cmdp[0] = 0x88; // read(16)
+
+		if (o->readfua)
+			hdr->cmdp[1] |= 0x08;
+
 	} else if (io_u->ddir == DDIR_WRITE) {
 		sgio_hdr_init(sd, hdr, io_u, 1);
 
@@ -294,6 +333,10 @@
 			hdr->cmdp[0] = 0x2a; // write(10)
 		else
 			hdr->cmdp[0] = 0x8a; // write(16)
+
+		if (o->writefua)
+			hdr->cmdp[1] |= 0x08;
+
 	} else {
 		sgio_hdr_init(sd, hdr, io_u, 0);
 		hdr->dxfer_direction = SG_DXFER_NONE;
@@ -822,6 +865,8 @@
 	.close_file	= generic_close_file,
 	.get_file_size	= fio_sgio_get_file_size,
 	.flags		= FIO_SYNCIO | FIO_RAWIO,
+	.options	= options,
+	.option_struct_size	= sizeof(struct sg_options)
 };
 
 #else /* FIO_HAVE_SGIO */
diff --git a/optgroup.h b/optgroup.h
index 815ac16..d5e968d 100644
--- a/optgroup.h
+++ b/optgroup.h
@@ -55,10 +55,11 @@
 	__FIO_OPT_G_LIBAIO,
 	__FIO_OPT_G_ACT,
 	__FIO_OPT_G_LATPROF,
-        __FIO_OPT_G_RBD,
-        __FIO_OPT_G_GFAPI,
-        __FIO_OPT_G_MTD,
+	__FIO_OPT_G_RBD,
+	__FIO_OPT_G_GFAPI,
+	__FIO_OPT_G_MTD,
 	__FIO_OPT_G_HDFS,
+	__FIO_OPT_G_SG,
 	__FIO_OPT_G_NR,
 
 	FIO_OPT_G_RATE		= (1ULL << __FIO_OPT_G_RATE),
@@ -93,6 +94,7 @@
 	FIO_OPT_G_GFAPI		= (1ULL << __FIO_OPT_G_GFAPI),
 	FIO_OPT_G_MTD		= (1ULL << __FIO_OPT_G_MTD),
 	FIO_OPT_G_HDFS		= (1ULL << __FIO_OPT_G_HDFS),
+	FIO_OPT_G_SG		= (1ULL << __FIO_OPT_G_SG),
 	FIO_OPT_G_INVALID	= (1ULL << __FIO_OPT_G_NR),
 };

Re: Patch to add read and write fua bit options to the SG engine

[Sitsofe Wheeler]

Hi,

On 15 March 2018 at 03:59, Kris Davis <Kris.Davis@wdc.com> wrote:
> I like to submit the patch below to add read and write fua bit options to the SG engine.  It adds a readfua=bool, and writefua=bool options in the SG engine, which causes the "force unit access" bits to be set in the read and write operations respectively.  I patterned the code changes from what I found in the libaio engine option code.  So, I also added a "__FIO_OPT_G_SG" enum to optgroup.h, which seemed to follow the pattern - but I really don't understand where or how the enum settings values in the options are used, so I'm unsure.

If you don't need different I/Os to have different FUA states (if they
were ever supported in the sg ioengine could trims via
unmaps/write_sames be FUA'd?) could you overload direct=1 for this
purpose?

-- 
Sitsofe | http://sucs.org/~sits/

On 15 March 2018 at 03:59, Kris Davis <Kris.Davis@wdc.com> wrote:
> I like to submit the patch below to add read and write fua bit options to the SG engine.  It adds a readfua=bool, and writefua=bool options in the SG engine, which causes the "force unit access" bits to be set in the read and write operations respectively.  I patterned the code changes from what I found in the libaio engine option code.  So, I also added a "__FIO_OPT_G_SG" enum to optgroup.h, which seemed to follow the pattern - but I really don't understand where or how the enum settings values in the options are used, so I'm unsure.
> Thanks
> Kris Davis
>
>
>
> diff --git a/HOWTO b/HOWTO
> index acb9e97..4f9d26a 100644
> --- a/HOWTO
> +++ b/HOWTO
> @@ -1747,6 +1747,7 @@
>                         :manpage:`read(2)` and :manpage:`write(2)` for asynchronous
>                         I/O. Requires :option:`filename` option to specify either block or
>                         character devices.
> +                       The sg engine includes engine specific options.
>
>                 **null**
>                         Doesn't transfer any data, just pretends to.  This is mainly used to
> @@ -2068,6 +2069,17 @@
>         multiple paths exist between the client and the server or in certain loopback
>         configurations.
>
> +.. option:: readfua=bool : [sg]
> +
> +       With readfua option set to 1, read operations include the
> +       the force unit access (fua) flag.
> +
> +.. option:: writefua=bool : [sg]
> +
> +       With writefua option set to 1, write operations include the
> +       the force unit access (fua) flag.
> +
> +
>  I/O depth
>  ~~~~~~~~~
>
> diff --git a/engines/sg.c b/engines/sg.c
> index 4540b57..f240755 100644
> --- a/engines/sg.c
> +++ b/engines/sg.c
> @@ -12,8 +12,42 @@
>  #include <sys/poll.h>
>
>  #include "../fio.h"
> +#include "../optgroup.h"
>
>  #ifdef FIO_HAVE_SGIO
> +
> +
> +struct sg_options {
> +       void *pad;
> +       unsigned int readfua;
> +       unsigned int writefua;
> +};
> +
> +static struct fio_option options[] = {
> +       {
> +               .name   = "readfua",
> +               .lname  = "sg engine read fua flag support",
> +               .type   = FIO_OPT_BOOL,
> +               .off1   = offsetof(struct sg_options, readfua),
> +               .help   = "Set FUA flag (force unit access) for all Read operations",
> +               .def    = "0",
> +               .category = FIO_OPT_C_ENGINE,
> +               .group  = FIO_OPT_G_SG,
> +       },
> +       {
> +               .name   = "writefua",
> +               .lname  = "sg engine write fua flag support",
> +               .type   = FIO_OPT_BOOL,
> +               .off1   = offsetof(struct sg_options, writefua),
> +               .help   = "Set FUA flag (force unit access) for all Write operations",
> +               .def    = "0",
> +               .category = FIO_OPT_C_ENGINE,
> +               .group  = FIO_OPT_G_SG,
> +       },
> +       {
> +               .name   = NULL,
> +       },
> +};
>
>  #define MAX_10B_LBA  0xFFFFFFFFULL
>  #define SCSI_TIMEOUT_MS 30000   // 30 second timeout; currently no method to override
> @@ -267,6 +301,7 @@
>  static int fio_sgio_prep(struct thread_data *td, struct io_u *io_u)
>  {
>         struct sg_io_hdr *hdr = &io_u->hdr;
> +       struct sg_options *o = td->eo;
>         struct sgio_data *sd = td->io_ops_data;
>         long long nr_blocks, lba;
>
> @@ -286,6 +321,10 @@
>                         hdr->cmdp[0] = 0x28; // read(10)
>                 else
>                         hdr->cmdp[0] = 0x88; // read(16)
> +
> +               if (o->readfua)
> +                       hdr->cmdp[1] |= 0x08;
> +
>         } else if (io_u->ddir == DDIR_WRITE) {
>                 sgio_hdr_init(sd, hdr, io_u, 1);
>
> @@ -294,6 +333,10 @@
>                         hdr->cmdp[0] = 0x2a; // write(10)
>                 else
>                         hdr->cmdp[0] = 0x8a; // write(16)
> +
> +               if (o->writefua)
> +                       hdr->cmdp[1] |= 0x08;
> +
>         } else {
>                 sgio_hdr_init(sd, hdr, io_u, 0);
>                 hdr->dxfer_direction = SG_DXFER_NONE;
> @@ -822,6 +865,8 @@
>         .close_file     = generic_close_file,
>         .get_file_size  = fio_sgio_get_file_size,
>         .flags          = FIO_SYNCIO | FIO_RAWIO,
> +       .options        = options,
> +       .option_struct_size     = sizeof(struct sg_options)
>  };
>
>  #else /* FIO_HAVE_SGIO */
> diff --git a/optgroup.h b/optgroup.h
> index 815ac16..d5e968d 100644
> --- a/optgroup.h
> +++ b/optgroup.h
> @@ -55,10 +55,11 @@
>         __FIO_OPT_G_LIBAIO,
>         __FIO_OPT_G_ACT,
>         __FIO_OPT_G_LATPROF,
> -        __FIO_OPT_G_RBD,
> -        __FIO_OPT_G_GFAPI,
> -        __FIO_OPT_G_MTD,
> +       __FIO_OPT_G_RBD,
> +       __FIO_OPT_G_GFAPI,
> +       __FIO_OPT_G_MTD,
>         __FIO_OPT_G_HDFS,
> +       __FIO_OPT_G_SG,
>         __FIO_OPT_G_NR,
>
>         FIO_OPT_G_RATE          = (1ULL << __FIO_OPT_G_RATE),
> @@ -93,6 +94,7 @@
>         FIO_OPT_G_GFAPI         = (1ULL << __FIO_OPT_G_GFAPI),
>         FIO_OPT_G_MTD           = (1ULL << __FIO_OPT_G_MTD),
>         FIO_OPT_G_HDFS          = (1ULL << __FIO_OPT_G_HDFS),
> +       FIO_OPT_G_SG            = (1ULL << __FIO_OPT_G_SG),
>         FIO_OPT_G_INVALID       = (1ULL << __FIO_OPT_G_NR),
>  };
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe fio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Sitsofe | http://sucs.org/~sits/

RE: Patch to add read and write fua bit options to the SG engine

[Kris Davis]

We definitely do need separate control between read and write fua, and only the read and write scsi commands actually support a fua bit.  Also, I don't think the sg engine can actually be considered general purpose, as it really only supports the read/write/sync operations currently.  

I also think overloading the meaning of the "direct" option would set a bad precedent - even though it's not used by the sg engine.  The "direct" bit changes behavior of the kernel or driver, but fua bit is a device operation.

Thanks

Kris Davis
Western Digital Coporation
Email: kris.davis@wdc.com
Office:: +1-507-322-2376

-----Original Message-----
From: Sitsofe Wheeler [mailto:sitsofe@gmail.com] 
Sent: Thursday, March 15, 2018 1:36 AM
To: Kris Davis <Kris.Davis@wdc.com>
Cc: fio@vger.kernel.org; Jens Axboe <axboe@kernel.dk>
Subject: Re: Patch to add read and write fua bit options to the SG engine

Hi,

On 15 March 2018 at 03:59, Kris Davis <Kris.Davis@wdc.com> wrote:
> I like to submit the patch below to add read and write fua bit options to the SG engine.  It adds a readfua=bool, and writefua=bool options in the SG engine, which causes the "force unit access" bits to be set in the read and write operations respectively.  I patterned the code changes from what I found in the libaio engine option code.  So, I also added a "__FIO_OPT_G_SG" enum to optgroup.h, which seemed to follow the pattern - but I really don't understand where or how the enum settings values in the options are used, so I'm unsure.

If you don't need different I/Os to have different FUA states (if they were ever supported in the sg ioengine could trims via unmaps/write_sames be FUA'd?) could you overload direct=1 for this purpose?

--
Sitsofe | http://sucs.org/~sits/

On 15 March 2018 at 03:59, Kris Davis <Kris.Davis@wdc.com> wrote:
> I like to submit the patch below to add read and write fua bit options to the SG engine.  It adds a readfua=bool, and writefua=bool options in the SG engine, which causes the "force unit access" bits to be set in the read and write operations respectively.  I patterned the code changes from what I found in the libaio engine option code.  So, I also added a "__FIO_OPT_G_SG" enum to optgroup.h, which seemed to follow the pattern - but I really don't understand where or how the enum settings values in the options are used, so I'm unsure.
> Thanks
> Kris Davis
>
>
>
> diff --git a/HOWTO b/HOWTO
> index acb9e97..4f9d26a 100644
> --- a/HOWTO
> +++ b/HOWTO
> @@ -1747,6 +1747,7 @@
>                         :manpage:`read(2)` and :manpage:`write(2)` for asynchronous
>                         I/O. Requires :option:`filename` option to specify either block or
>                         character devices.
> +                       The sg engine includes engine specific options.
>
>                 **null**
>                         Doesn't transfer any data, just pretends to.  
> This is mainly used to @@ -2068,6 +2069,17 @@
>         multiple paths exist between the client and the server or in certain loopback
>         configurations.
>
> +.. option:: readfua=bool : [sg]
> +
> +       With readfua option set to 1, read operations include the
> +       the force unit access (fua) flag.
> +
> +.. option:: writefua=bool : [sg]
> +
> +       With writefua option set to 1, write operations include the
> +       the force unit access (fua) flag.
> +
> +
>  I/O depth
>  ~~~~~~~~~
>
> diff --git a/engines/sg.c b/engines/sg.c index 4540b57..f240755 100644
> --- a/engines/sg.c
> +++ b/engines/sg.c
> @@ -12,8 +12,42 @@
>  #include <sys/poll.h>
>
>  #include "../fio.h"
> +#include "../optgroup.h"
>
>  #ifdef FIO_HAVE_SGIO
> +
> +
> +struct sg_options {
> +       void *pad;
> +       unsigned int readfua;
> +       unsigned int writefua;
> +};
> +
> +static struct fio_option options[] = {
> +       {
> +               .name   = "readfua",
> +               .lname  = "sg engine read fua flag support",
> +               .type   = FIO_OPT_BOOL,
> +               .off1   = offsetof(struct sg_options, readfua),
> +               .help   = "Set FUA flag (force unit access) for all Read operations",
> +               .def    = "0",
> +               .category = FIO_OPT_C_ENGINE,
> +               .group  = FIO_OPT_G_SG,
> +       },
> +       {
> +               .name   = "writefua",
> +               .lname  = "sg engine write fua flag support",
> +               .type   = FIO_OPT_BOOL,
> +               .off1   = offsetof(struct sg_options, writefua),
> +               .help   = "Set FUA flag (force unit access) for all Write operations",
> +               .def    = "0",
> +               .category = FIO_OPT_C_ENGINE,
> +               .group  = FIO_OPT_G_SG,
> +       },
> +       {
> +               .name   = NULL,
> +       },
> +};
>
>  #define MAX_10B_LBA  0xFFFFFFFFULL
>  #define SCSI_TIMEOUT_MS 30000   // 30 second timeout; currently no method to override
> @@ -267,6 +301,7 @@
>  static int fio_sgio_prep(struct thread_data *td, struct io_u *io_u)  
> {
>         struct sg_io_hdr *hdr = &io_u->hdr;
> +       struct sg_options *o = td->eo;
>         struct sgio_data *sd = td->io_ops_data;
>         long long nr_blocks, lba;
>
> @@ -286,6 +321,10 @@
>                         hdr->cmdp[0] = 0x28; // read(10)
>                 else
>                         hdr->cmdp[0] = 0x88; // read(16)
> +
> +               if (o->readfua)
> +                       hdr->cmdp[1] |= 0x08;
> +
>         } else if (io_u->ddir == DDIR_WRITE) {
>                 sgio_hdr_init(sd, hdr, io_u, 1);
>
> @@ -294,6 +333,10 @@
>                         hdr->cmdp[0] = 0x2a; // write(10)
>                 else
>                         hdr->cmdp[0] = 0x8a; // write(16)
> +
> +               if (o->writefua)
> +                       hdr->cmdp[1] |= 0x08;
> +
>         } else {
>                 sgio_hdr_init(sd, hdr, io_u, 0);
>                 hdr->dxfer_direction = SG_DXFER_NONE; @@ -822,6 +865,8 
> @@
>         .close_file     = generic_close_file,
>         .get_file_size  = fio_sgio_get_file_size,
>         .flags          = FIO_SYNCIO | FIO_RAWIO,
> +       .options        = options,
> +       .option_struct_size     = sizeof(struct sg_options)
>  };
>
>  #else /* FIO_HAVE_SGIO */
> diff --git a/optgroup.h b/optgroup.h
> index 815ac16..d5e968d 100644
> --- a/optgroup.h
> +++ b/optgroup.h
> @@ -55,10 +55,11 @@
>         __FIO_OPT_G_LIBAIO,
>         __FIO_OPT_G_ACT,
>         __FIO_OPT_G_LATPROF,
> -        __FIO_OPT_G_RBD,
> -        __FIO_OPT_G_GFAPI,
> -        __FIO_OPT_G_MTD,
> +       __FIO_OPT_G_RBD,
> +       __FIO_OPT_G_GFAPI,
> +       __FIO_OPT_G_MTD,
>         __FIO_OPT_G_HDFS,
> +       __FIO_OPT_G_SG,
>         __FIO_OPT_G_NR,
>
>         FIO_OPT_G_RATE          = (1ULL << __FIO_OPT_G_RATE),
> @@ -93,6 +94,7 @@
>         FIO_OPT_G_GFAPI         = (1ULL << __FIO_OPT_G_GFAPI),
>         FIO_OPT_G_MTD           = (1ULL << __FIO_OPT_G_MTD),
>         FIO_OPT_G_HDFS          = (1ULL << __FIO_OPT_G_HDFS),
> +       FIO_OPT_G_SG            = (1ULL << __FIO_OPT_G_SG),
>         FIO_OPT_G_INVALID       = (1ULL << __FIO_OPT_G_NR),
>  };
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe fio" in the 
> body of a message to majordomo@vger.kernel.org More majordomo info at  
> http://vger.kernel.org/majordomo-info.html



--
Sitsofe | http://sucs.org/~sits/

Re: Patch to add read and write fua bit options to the SG engine

[Sitsofe Wheeler]

On 15 March 2018 at 14:52, Kris Davis <Kris.Davis@wdc.com> wrote:
> We definitely do need separate control between read and write fua, and only the read and write scsi commands actually support a fua bit.  Also, I don't think the sg engine can actually be considered general purpose, as it really only supports the read/write/sync operations currently.
>
> I also think overloading the meaning of the "direct" option would set a bad precedent - even though it's not used by the sg engine.  The "direct" bit changes behavior of the kernel or driver, but fua bit is a device operation.

OK fair enough. Could you update the man page too and could you list
what the default is for those options in the HOWTO and man page?
Beyond that the rest LGTM.

-- 
Sitsofe | http://sucs.org/~sits/

RE: Patch to add read and write fua bit options to the SG engine

[Kris Davis]

> OK fair enough. Could you update the man page too and could you list what the default is for those options in the HOWTO and man page?
> Beyond that the rest LGTM.

Added defaults to the HOWTO (and man page).  New patch below.
Thanks again.
Kris Davis


diff --git a/HOWTO b/HOWTO
index acb9e97..9309811 100644
--- a/HOWTO
+++ b/HOWTO
@@ -1747,6 +1747,7 @@
 			:manpage:`read(2)` and :manpage:`write(2)` for asynchronous
 			I/O. Requires :option:`filename` option to specify either block or
 			character devices.
+			The sg engine includes engine specific options.
 
 		**null**
 			Doesn't transfer any data, just pretends to.  This is mainly used to
@@ -2068,6 +2069,17 @@
 	multiple paths exist between the client and the server or in certain loopback
 	configurations.
 
+.. option:: readfua=bool : [sg]
+
+	With readfua option set to 1, read operations include the 
+	the force unit access (fua) flag. Defaults is 0.
+
+.. option:: writefua=bool : [sg]
+
+	With writefua option set to 1, write operations include the 
+	the force unit access (fua) flag. Default is 0.
+
+
 I/O depth
 ~~~~~~~~~
 
diff --git a/engines/sg.c b/engines/sg.c
index 4540b57..f240755 100644
--- a/engines/sg.c
+++ b/engines/sg.c
@@ -12,8 +12,42 @@
 #include <sys/poll.h>
 
 #include "../fio.h"
+#include "../optgroup.h"
 
 #ifdef FIO_HAVE_SGIO
+
+
+struct sg_options {
+	void *pad;
+	unsigned int readfua;
+	unsigned int writefua;
+};
+
+static struct fio_option options[] = {
+	{
+		.name	= "readfua",
+		.lname	= "sg engine read fua flag support",
+		.type	= FIO_OPT_BOOL,
+		.off1	= offsetof(struct sg_options, readfua),
+		.help	= "Set FUA flag (force unit access) for all Read operations",
+		.def	= "0",
+		.category = FIO_OPT_C_ENGINE,
+		.group	= FIO_OPT_G_SG,
+	},
+	{
+		.name	= "writefua",
+		.lname	= "sg engine write fua flag support",
+		.type	= FIO_OPT_BOOL,
+		.off1	= offsetof(struct sg_options, writefua),
+		.help	= "Set FUA flag (force unit access) for all Write operations",
+		.def	= "0",
+		.category = FIO_OPT_C_ENGINE,
+		.group	= FIO_OPT_G_SG,
+	},
+	{
+		.name	= NULL,
+	},
+};
 
 #define MAX_10B_LBA  0xFFFFFFFFULL
 #define SCSI_TIMEOUT_MS 30000   // 30 second timeout; currently no method to override
@@ -267,6 +301,7 @@
 static int fio_sgio_prep(struct thread_data *td, struct io_u *io_u)
 {
 	struct sg_io_hdr *hdr = &io_u->hdr;
+	struct sg_options *o = td->eo;
 	struct sgio_data *sd = td->io_ops_data;
 	long long nr_blocks, lba;
 
@@ -286,6 +321,10 @@
 			hdr->cmdp[0] = 0x28; // read(10)
 		else
 			hdr->cmdp[0] = 0x88; // read(16)
+
+		if (o->readfua)
+			hdr->cmdp[1] |= 0x08;
+
 	} else if (io_u->ddir == DDIR_WRITE) {
 		sgio_hdr_init(sd, hdr, io_u, 1);
 
@@ -294,6 +333,10 @@
 			hdr->cmdp[0] = 0x2a; // write(10)
 		else
 			hdr->cmdp[0] = 0x8a; // write(16)
+
+		if (o->writefua)
+			hdr->cmdp[1] |= 0x08;
+
 	} else {
 		sgio_hdr_init(sd, hdr, io_u, 0);
 		hdr->dxfer_direction = SG_DXFER_NONE;
@@ -822,6 +865,8 @@
 	.close_file	= generic_close_file,
 	.get_file_size	= fio_sgio_get_file_size,
 	.flags		= FIO_SYNCIO | FIO_RAWIO,
+	.options	= options,
+	.option_struct_size	= sizeof(struct sg_options)
 };
 
 #else /* FIO_HAVE_SGIO */
diff --git a/optgroup.h b/optgroup.h
index 815ac16..d5e968d 100644
--- a/optgroup.h
+++ b/optgroup.h
@@ -55,10 +55,11 @@
 	__FIO_OPT_G_LIBAIO,
 	__FIO_OPT_G_ACT,
 	__FIO_OPT_G_LATPROF,
-        __FIO_OPT_G_RBD,
-        __FIO_OPT_G_GFAPI,
-        __FIO_OPT_G_MTD,
+	__FIO_OPT_G_RBD,
+	__FIO_OPT_G_GFAPI,
+	__FIO_OPT_G_MTD,
 	__FIO_OPT_G_HDFS,
+	__FIO_OPT_G_SG,
 	__FIO_OPT_G_NR,
 
 	FIO_OPT_G_RATE		= (1ULL << __FIO_OPT_G_RATE),
@@ -93,6 +94,7 @@
 	FIO_OPT_G_GFAPI		= (1ULL << __FIO_OPT_G_GFAPI),
 	FIO_OPT_G_MTD		= (1ULL << __FIO_OPT_G_MTD),
 	FIO_OPT_G_HDFS		= (1ULL << __FIO_OPT_G_HDFS),
+	FIO_OPT_G_SG		= (1ULL << __FIO_OPT_G_SG),
 	FIO_OPT_G_INVALID	= (1ULL << __FIO_OPT_G_NR),
 };

RE: Patch to add read and write fua bit options to the SG engine

[Kris Davis]

A question - how is the man page expected to be handled (manually updated)?  I saw that running "make -C doc man" would generate a man page, which included changes from the HOWTO.  However, even if I build that doc/output/man/fio.1 file from a master level image, it does to appear to equal what is in "fio.1" at the root directory.  I've never manually written a man page before, so I might guess that it depends on the tool images on whatever machine is used to build the man page.  

Anyway - what's the right way to handle the man pages
-- manually edit and include in patch?
-- build via make, copy to the root dir, and then include in patch?
-- something else?

Thanks

Kris Davis

-----Original Message-----
From: fio-owner@vger.kernel.org [mailto:fio-owner@vger.kernel.org] On Behalf Of Kris Davis
Sent: Thursday, March 15, 2018 12:26 PM
To: Sitsofe Wheeler <sitsofe@gmail.com>
Cc: fio@vger.kernel.org; Jens Axboe <axboe@kernel.dk>
Subject: RE: Patch to add read and write fua bit options to the SG engine

> OK fair enough. Could you update the man page too and could you list what the default is for those options in the HOWTO and man page?
> Beyond that the rest LGTM.

Added defaults to the HOWTO (and man page).  New patch below.
Thanks again.
Kris Davis


diff --git a/HOWTO b/HOWTO
index acb9e97..9309811 100644
--- a/HOWTO
+++ b/HOWTO
@@ -1747,6 +1747,7 @@
 			:manpage:`read(2)` and :manpage:`write(2)` for asynchronous
 			I/O. Requires :option:`filename` option to specify either block or
 			character devices.
+			The sg engine includes engine specific options.
 
 		**null**
 			Doesn't transfer any data, just pretends to.  This is mainly used to @@ -2068,6 +2069,17 @@
 	multiple paths exist between the client and the server or in certain loopback
 	configurations.
 
+.. option:: readfua=bool : [sg]
+
+	With readfua option set to 1, read operations include the 
+	the force unit access (fua) flag. Defaults is 0.
+
+.. option:: writefua=bool : [sg]
+
+	With writefua option set to 1, write operations include the 
+	the force unit access (fua) flag. Default is 0.
+
+
 I/O depth
 ~~~~~~~~~
 
diff --git a/engines/sg.c b/engines/sg.c index 4540b57..f240755 100644
--- a/engines/sg.c
+++ b/engines/sg.c
@@ -12,8 +12,42 @@
 #include <sys/poll.h>
 
 #include "../fio.h"
+#include "../optgroup.h"
 
 #ifdef FIO_HAVE_SGIO
+
+
+struct sg_options {
+	void *pad;
+	unsigned int readfua;
+	unsigned int writefua;
+};
+
+static struct fio_option options[] = {
+	{
+		.name	= "readfua",
+		.lname	= "sg engine read fua flag support",
+		.type	= FIO_OPT_BOOL,
+		.off1	= offsetof(struct sg_options, readfua),
+		.help	= "Set FUA flag (force unit access) for all Read operations",
+		.def	= "0",
+		.category = FIO_OPT_C_ENGINE,
+		.group	= FIO_OPT_G_SG,
+	},
+	{
+		.name	= "writefua",
+		.lname	= "sg engine write fua flag support",
+		.type	= FIO_OPT_BOOL,
+		.off1	= offsetof(struct sg_options, writefua),
+		.help	= "Set FUA flag (force unit access) for all Write operations",
+		.def	= "0",
+		.category = FIO_OPT_C_ENGINE,
+		.group	= FIO_OPT_G_SG,
+	},
+	{
+		.name	= NULL,
+	},
+};
 
 #define MAX_10B_LBA  0xFFFFFFFFULL
 #define SCSI_TIMEOUT_MS 30000   // 30 second timeout; currently no method to override
@@ -267,6 +301,7 @@
 static int fio_sgio_prep(struct thread_data *td, struct io_u *io_u)  {
 	struct sg_io_hdr *hdr = &io_u->hdr;
+	struct sg_options *o = td->eo;
 	struct sgio_data *sd = td->io_ops_data;
 	long long nr_blocks, lba;
 
@@ -286,6 +321,10 @@
 			hdr->cmdp[0] = 0x28; // read(10)
 		else
 			hdr->cmdp[0] = 0x88; // read(16)
+
+		if (o->readfua)
+			hdr->cmdp[1] |= 0x08;
+
 	} else if (io_u->ddir == DDIR_WRITE) {
 		sgio_hdr_init(sd, hdr, io_u, 1);
 
@@ -294,6 +333,10 @@
 			hdr->cmdp[0] = 0x2a; // write(10)
 		else
 			hdr->cmdp[0] = 0x8a; // write(16)
+
+		if (o->writefua)
+			hdr->cmdp[1] |= 0x08;
+
 	} else {
 		sgio_hdr_init(sd, hdr, io_u, 0);
 		hdr->dxfer_direction = SG_DXFER_NONE; @@ -822,6 +865,8 @@
 	.close_file	= generic_close_file,
 	.get_file_size	= fio_sgio_get_file_size,
 	.flags		= FIO_SYNCIO | FIO_RAWIO,
+	.options	= options,
+	.option_struct_size	= sizeof(struct sg_options)
 };
 
 #else /* FIO_HAVE_SGIO */
diff --git a/optgroup.h b/optgroup.h
index 815ac16..d5e968d 100644
--- a/optgroup.h
+++ b/optgroup.h
@@ -55,10 +55,11 @@
 	__FIO_OPT_G_LIBAIO,
 	__FIO_OPT_G_ACT,
 	__FIO_OPT_G_LATPROF,
-        __FIO_OPT_G_RBD,
-        __FIO_OPT_G_GFAPI,
-        __FIO_OPT_G_MTD,
+	__FIO_OPT_G_RBD,
+	__FIO_OPT_G_GFAPI,
+	__FIO_OPT_G_MTD,
 	__FIO_OPT_G_HDFS,
+	__FIO_OPT_G_SG,
 	__FIO_OPT_G_NR,
 
 	FIO_OPT_G_RATE		= (1ULL << __FIO_OPT_G_RATE),
@@ -93,6 +94,7 @@
 	FIO_OPT_G_GFAPI		= (1ULL << __FIO_OPT_G_GFAPI),
 	FIO_OPT_G_MTD		= (1ULL << __FIO_OPT_G_MTD),
 	FIO_OPT_G_HDFS		= (1ULL << __FIO_OPT_G_HDFS),
+	FIO_OPT_G_SG		= (1ULL << __FIO_OPT_G_SG),
 	FIO_OPT_G_INVALID	= (1ULL << __FIO_OPT_G_NR),
 };

\x04 {.n +       +%  lzwm  b 맲  r  y   {ay \x1dʇڙ ,j   f   h   z \x1e w       j:+v   w j m         zZ+     ݢj"  ! i

RE: Patch to add read and write fua bit options to the SG engine

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: fio-owner@vger.kernel.org [mailto:fio-owner@vger.kernel.org] On
> Behalf Of Kris Davis
> Sent: Thursday, March 15, 2018 3:38 PM
> Subject: RE: Patch to add read and write fua bit options to the SG engine
> 
> A question - how is the man page expected to be handled (manually
> updated)?  I saw that running "make -C doc man" would generate a man page,
> which included changes from the HOWTO.  However, even if I build that
> doc/output/man/fio.1 file from a master level image, it does to appear to
> equal what is in "fio.1" at the root directory.  I've never manually
> written a man page before, so I might guess that it depends on the tool
> images on whatever machine is used to build the man page.
> 
> Anyway - what's the right way to handle the man pages
> -- manually edit and include in patch?
> -- build via make, copy to the root dir, and then include in patch?
> -- something else?

fio.1 is the "source code."  Manually edit it and handcode the ancient 
troff formatting.

Patch f80dba8d2fc3052a added automatic generation of an fio.1 from
README and HOWTO, but I don't think anyone uses that output.  It
requires more packages to be present (something called sphinx and who
knows what else).  There have been over 70 direct patches to fio.1 
since then, ignoring that auto-generated version.

HOWTO and the manpage are always falling out of sync, so it'd be nice
if we only had one authoritative source file.


---
Robert Elliott, HPE Persistent Memory

RE: Patch to add read and write fua bit options to the SG engine

[Kris Davis]

Thanks for the explanation Robert.  I guess I need to work with other man pages for other changes as well.

Below is the (hopefully final) patch for the fua bit support, which also includes an update to the root fio.1 man page file
Thanks again everyone.
Kris Davis


diff --git a/HOWTO b/HOWTO
index acb9e97..9309811 100644
--- a/HOWTO
+++ b/HOWTO
@@ -1747,6 +1747,7 @@
 			:manpage:`read(2)` and :manpage:`write(2)` for asynchronous
 			I/O. Requires :option:`filename` option to specify either block or
 			character devices.
+			The sg engine includes engine specific options.
 
 		**null**
 			Doesn't transfer any data, just pretends to.  This is mainly used to
@@ -2068,6 +2069,17 @@
 	multiple paths exist between the client and the server or in certain loopback
 	configurations.
 
+.. option:: readfua=bool : [sg]
+
+	With readfua option set to 1, read operations include the 
+	the force unit access (fua) flag. Defaults is 0.
+
+.. option:: writefua=bool : [sg]
+
+	With writefua option set to 1, write operations include the 
+	the force unit access (fua) flag. Default is 0.
+
+
 I/O depth
 ~~~~~~~~~
 
diff --git a/engines/sg.c b/engines/sg.c
index 4540b57..f240755 100644
--- a/engines/sg.c
+++ b/engines/sg.c
@@ -12,8 +12,42 @@
 #include <sys/poll.h>
 
 #include "../fio.h"
+#include "../optgroup.h"
 
 #ifdef FIO_HAVE_SGIO
+
+
+struct sg_options {
+	void *pad;
+	unsigned int readfua;
+	unsigned int writefua;
+};
+
+static struct fio_option options[] = {
+	{
+		.name	= "readfua",
+		.lname	= "sg engine read fua flag support",
+		.type	= FIO_OPT_BOOL,
+		.off1	= offsetof(struct sg_options, readfua),
+		.help	= "Set FUA flag (force unit access) for all Read operations",
+		.def	= "0",
+		.category = FIO_OPT_C_ENGINE,
+		.group	= FIO_OPT_G_SG,
+	},
+	{
+		.name	= "writefua",
+		.lname	= "sg engine write fua flag support",
+		.type	= FIO_OPT_BOOL,
+		.off1	= offsetof(struct sg_options, writefua),
+		.help	= "Set FUA flag (force unit access) for all Write operations",
+		.def	= "0",
+		.category = FIO_OPT_C_ENGINE,
+		.group	= FIO_OPT_G_SG,
+	},
+	{
+		.name	= NULL,
+	},
+};
 
 #define MAX_10B_LBA  0xFFFFFFFFULL
 #define SCSI_TIMEOUT_MS 30000   // 30 second timeout; currently no method to override
@@ -267,6 +301,7 @@
 static int fio_sgio_prep(struct thread_data *td, struct io_u *io_u)
 {
 	struct sg_io_hdr *hdr = &io_u->hdr;
+	struct sg_options *o = td->eo;
 	struct sgio_data *sd = td->io_ops_data;
 	long long nr_blocks, lba;
 
@@ -286,6 +321,10 @@
 			hdr->cmdp[0] = 0x28; // read(10)
 		else
 			hdr->cmdp[0] = 0x88; // read(16)
+
+		if (o->readfua)
+			hdr->cmdp[1] |= 0x08;
+
 	} else if (io_u->ddir == DDIR_WRITE) {
 		sgio_hdr_init(sd, hdr, io_u, 1);
 
@@ -294,6 +333,10 @@
 			hdr->cmdp[0] = 0x2a; // write(10)
 		else
 			hdr->cmdp[0] = 0x8a; // write(16)
+
+		if (o->writefua)
+			hdr->cmdp[1] |= 0x08;
+
 	} else {
 		sgio_hdr_init(sd, hdr, io_u, 0);
 		hdr->dxfer_direction = SG_DXFER_NONE;
@@ -822,6 +865,8 @@
 	.close_file	= generic_close_file,
 	.get_file_size	= fio_sgio_get_file_size,
 	.flags		= FIO_SYNCIO | FIO_RAWIO,
+	.options	= options,
+	.option_struct_size	= sizeof(struct sg_options)
 };
 
 #else /* FIO_HAVE_SGIO */
diff --git a/fio.1 b/fio.1
index f955167..06cac67 100644
--- a/fio.1
+++ b/fio.1
@@ -1523,7 +1523,7 @@
 ioctl, or if the target is an sg character device we use
 \fBread\fR\|(2) and \fBwrite\fR\|(2) for asynchronous
 I/O. Requires \fBfilename\fR option to specify either block or
-character devices.
+character devices. The sg engine includes engine specific options.
 .TP
 .B null
 Doesn't transfer any data, just pretends to. This is mainly used to
@@ -1820,6 +1820,14 @@
 on the client site it will be used in the rdma_resolve_add()
 function. This can be useful when multiple paths exist between the
 client and the server or in certain loopback configurations.
+.TP
+.BI (sg)readfua \fR=\fPbool
+With readfua option set to 1, read operations include the the force 
+unit access (fua) flag. Default: 0.
+.TP
+.BI (sg)writefua \fR=\fPbool
+With writefua option set to 1, write operations include the the force 
+unit access (fua) flag. Default: 0.
 .SS "I/O depth"
 .TP
 .BI iodepth \fR=\fPint
diff --git a/optgroup.h b/optgroup.h
index 815ac16..d5e968d 100644
--- a/optgroup.h
+++ b/optgroup.h
@@ -55,10 +55,11 @@
 	__FIO_OPT_G_LIBAIO,
 	__FIO_OPT_G_ACT,
 	__FIO_OPT_G_LATPROF,
-        __FIO_OPT_G_RBD,
-        __FIO_OPT_G_GFAPI,
-        __FIO_OPT_G_MTD,
+	__FIO_OPT_G_RBD,
+	__FIO_OPT_G_GFAPI,
+	__FIO_OPT_G_MTD,
 	__FIO_OPT_G_HDFS,
+	__FIO_OPT_G_SG,
 	__FIO_OPT_G_NR,
 
 	FIO_OPT_G_RATE		= (1ULL << __FIO_OPT_G_RATE),
@@ -93,6 +94,7 @@
 	FIO_OPT_G_GFAPI		= (1ULL << __FIO_OPT_G_GFAPI),
 	FIO_OPT_G_MTD		= (1ULL << __FIO_OPT_G_MTD),
 	FIO_OPT_G_HDFS		= (1ULL << __FIO_OPT_G_HDFS),
+	FIO_OPT_G_SG		= (1ULL << __FIO_OPT_G_SG),
 	FIO_OPT_G_INVALID	= (1ULL << __FIO_OPT_G_NR),
 };

Re: Patch to add read and write fua bit options to the SG engine

[Jens Axboe]

On 3/15/18 4:55 PM, Kris Davis wrote:
> Thanks for the explanation Robert.  I guess I need to work with other
> man pages for other changes as well.
> 
> Below is the (hopefully final) patch for the fua bit support, which
> also includes an update to the root fio.1 man page file

Patch looks fine to me.

> +.. option:: readfua=bool : [sg]
> +
> +	With readfua option set to 1, read operations include the 
> +	the force unit access (fua) flag. Defaults is 0.

Either "defaults to" or "default is" would be better. I made that fixup.
You also have two "the" here for both the HOWTO and man page, and
trailing space. Fixed that up too.

-- 
Jens Axboe