diff mbox

[v3,2/3] xfs_io: Add RWF_NOWAIT to pwritev2()

Message ID 20171010105802.31353-2-rgoldwyn@suse.de (mailing list archive)
State Superseded
Headers show

Commit Message

Goldwyn Rodrigues Oct. 10, 2017, 10:58 a.m. UTC 
From: Goldwyn Rodrigues <rgoldwyn@suse.com>

This allows to make pwritev2() calls with RWF_NOWAIT,
which would fail in case the call blocks.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Changes since v2:
	- ifdef around -N which set RWF_NOWAIT
---
 io/pwrite.c       | 10 +++++++++-
 man/man8/xfs_io.8 |  6 ++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

Comments

Brian Foster Oct. 10, 2017, 1:41 p.m. UTC | #1 
On Tue, Oct 10, 2017 at 05:58:01AM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> This allows to make pwritev2() calls with RWF_NOWAIT,
> which would fail in case the call blocks.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Changes since v2:
> 	- ifdef around -N which set RWF_NOWAIT
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  io/pwrite.c       | 10 +++++++++-
>  man/man8/xfs_io.8 |  6 ++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/io/pwrite.c b/io/pwrite.c
> index 5ceb26c7..e06dfb46 100644
> --- a/io/pwrite.c
> +++ b/io/pwrite.c
> @@ -53,6 +53,9 @@ pwrite_help(void)
>  #ifdef HAVE_PWRITEV
>  " -V N -- use vectored IO with N iovecs of blocksize each (pwritev)\n"
>  #endif
> +#ifdef HAVE_PWRITEV2
> +" -N   -- Perform the pwritev2() with RWF_NOWAIT\n"
> +#endif
>  "\n"));
>  }
>  
> @@ -279,7 +282,7 @@ pwrite_f(
>  	init_cvtnum(&fsblocksize, &fssectsize);
>  	bsize = fsblocksize;
>  
> -	while ((c = getopt(argc, argv, "b:BCdf:Fi:qRs:S:uV:wWZ:")) != EOF) {
> +	while ((c = getopt(argc, argv, "b:BCdf:Fi:NqRs:S:uV:wWZ:")) != EOF) {
>  		switch (c) {
>  		case 'b':
>  			tmp = cvtnum(fsblocksize, fssectsize, optarg);
> @@ -308,6 +311,11 @@ pwrite_f(
>  		case 'i':
>  			infile = optarg;
>  			break;
> +#ifdef HAVE_PWRITEV2
> +		case 'N':
> +			pwritev2_flags |= RWF_NOWAIT;
> +			break;
> +#endif
>  		case 's':
>  			skip = cvtnum(fsblocksize, fssectsize, optarg);
>  			if (skip < 0) {
> diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
> index 0fd9b951..9c58914f 100644
> --- a/man/man8/xfs_io.8
> +++ b/man/man8/xfs_io.8
> @@ -282,6 +282,12 @@ Use the vectored IO write syscall
>  with a number of blocksize length iovecs. The number of iovecs is set by the
>  .I vectors
>  parameter.
> +.TP
> +.B \-N
> +Perform the
> +.BR pwritev2 (2)
> +call with
> +.I RWF_NOWAIT.
>  .RE
>  .PD
>  .TP
> -- 
> 2.14.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Sandeen Nov. 9, 2017, 4:27 a.m. UTC | #2 
On 10/10/17 5:58 AM, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> This allows to make pwritev2() calls with RWF_NOWAIT,
> which would fail in case the call blocks.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Changes since v2:
> 	- ifdef around -N which set RWF_NOWAIT
> ---
>  io/pwrite.c       | 10 +++++++++-
>  man/man8/xfs_io.8 |  6 ++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/io/pwrite.c b/io/pwrite.c
> index 5ceb26c7..e06dfb46 100644
> --- a/io/pwrite.c
> +++ b/io/pwrite.c
> @@ -53,6 +53,9 @@ pwrite_help(void)
>  #ifdef HAVE_PWRITEV
>  " -V N -- use vectored IO with N iovecs of blocksize each (pwritev)\n"
>  #endif
> +#ifdef HAVE_PWRITEV2
> +" -N   -- Perform the pwritev2() with RWF_NOWAIT\n"
> +#endif
>  "\n"));
>  }

This "-N" option didn't get added to the short help:

void
pwrite_init(void)
{
        pwrite_cmd.name = "pwrite";
        pwrite_cmd.altname = "w";
        pwrite_cmd.cfunc = pwrite_f;
        pwrite_cmd.argmin = 2;
        pwrite_cmd.argmax = -1;
        pwrite_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
        pwrite_cmd.args =
_("[-i infile [-d] [-s skip]] [-b bs] [-S seed] [-wW] [-FBR [-Z N]] [-V N] off len");

Is there any clean way to do that conditionally on the #ifdef as is done for long
help?  Otherwise just more #ifdefs I guess.
  
> @@ -279,7 +282,7 @@ pwrite_f(
>  	init_cvtnum(&fsblocksize, &fssectsize);
>  	bsize = fsblocksize;
>  
> -	while ((c = getopt(argc, argv, "b:BCdf:Fi:qRs:S:uV:wWZ:")) != EOF) {
> +	while ((c = getopt(argc, argv, "b:BCdf:Fi:NqRs:S:uV:wWZ:")) != EOF) {
>  		switch (c) {
>  		case 'b':
>  			tmp = cvtnum(fsblocksize, fssectsize, optarg);
> @@ -308,6 +311,11 @@ pwrite_f(
>  		case 'i':
>  			infile = optarg;
>  			break;
> +#ifdef HAVE_PWRITEV2
> +		case 'N':
> +			pwritev2_flags |= RWF_NOWAIT;
> +			break;
> +#endif

If pwritev2 isn't present at build time, specifying -N gives somewhat unexpected behavior:

xfs_io> pwrite -N 0 1k
pwrite [-i infile [-d] [-s skip]] [-b bs] [-S seed] [-wW] [-FBR [-Z N]] [-V N] off len -- writes a number of bytes at a specified offset
xfs_io>

vs a wholly unknown option:

xfs_io> pwrite -K 0 1k
pwrite: invalid option -- 'K'
xfs_io>

because you have 'N' in the getopt string.  I wonder if there's a better
way to handle it besides moar ifdefs ... I guess this wouldn't be
terrible:

+		case 'N':
+#ifdef HAVE_PWRITEV2
+			pwritev2_flags |= RWF_NOWAIT;
+#else
+			printf(_("Not built with pwritev2 functionality\n"));
+#endif
+			break;

>  		case 's':
>  			skip = cvtnum(fsblocksize, fssectsize, optarg);
>  			if (skip < 0) {
> diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
> index 0fd9b951..9c58914f 100644
> --- a/man/man8/xfs_io.8
> +++ b/man/man8/xfs_io.8
> @@ -282,6 +282,12 @@ Use the vectored IO write syscall
>  with a number of blocksize length iovecs. The number of iovecs is set by the
>  .I vectors
>  parameter.
> +.TP
> +.B \-N
> +Perform the
> +.BR pwritev2 (2)
> +call with
> +.I RWF_NOWAIT.

I guess maybe this should say something about "if it's built w/ pwritev2 functionality?"
I'm less worried about this, tbh, especially if -N gives the explanation above
if xfs_io doesn't have the support.

-Eric

>  .RE
>  .PD
>  .TP
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Goldwyn Rodrigues Nov. 9, 2017, 12:25 p.m. UTC | #3 
On 11/08/2017 10:27 PM, Eric Sandeen wrote:
> On 10/10/17 5:58 AM, Goldwyn Rodrigues wrote:
>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> This allows to make pwritev2() calls with RWF_NOWAIT,
>> which would fail in case the call blocks.
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> Changes since v2:
>> 	- ifdef around -N which set RWF_NOWAIT
>> ---
>>  io/pwrite.c       | 10 +++++++++-
>>  man/man8/xfs_io.8 |  6 ++++++
>>  2 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/io/pwrite.c b/io/pwrite.c
>> index 5ceb26c7..e06dfb46 100644
>> --- a/io/pwrite.c
>> +++ b/io/pwrite.c
>> @@ -53,6 +53,9 @@ pwrite_help(void)
>>  #ifdef HAVE_PWRITEV
>>  " -V N -- use vectored IO with N iovecs of blocksize each (pwritev)\n"
>>  #endif
>> +#ifdef HAVE_PWRITEV2
>> +" -N   -- Perform the pwritev2() with RWF_NOWAIT\n"
>> +#endif
>>  "\n"));
>>  }
> 
> This "-N" option didn't get added to the short help:

Ok, I will do that.

> 
> void
> pwrite_init(void)
> {
>         pwrite_cmd.name = "pwrite";
>         pwrite_cmd.altname = "w";
>         pwrite_cmd.cfunc = pwrite_f;
>         pwrite_cmd.argmin = 2;
>         pwrite_cmd.argmax = -1;
>         pwrite_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
>         pwrite_cmd.args =
> _("[-i infile [-d] [-s skip]] [-b bs] [-S seed] [-wW] [-FBR [-Z N]] [-V N] off len");
> 
> Is there any clean way to do that conditionally on the #ifdef as is done for long
> help?  Otherwise just more #ifdefs I guess.
>   
>> @@ -279,7 +282,7 @@ pwrite_f(
>>  	init_cvtnum(&fsblocksize, &fssectsize);
>>  	bsize = fsblocksize;
>>  
>> -	while ((c = getopt(argc, argv, "b:BCdf:Fi:qRs:S:uV:wWZ:")) != EOF) {
>> +	while ((c = getopt(argc, argv, "b:BCdf:Fi:NqRs:S:uV:wWZ:")) != EOF) {
>>  		switch (c) {
>>  		case 'b':
>>  			tmp = cvtnum(fsblocksize, fssectsize, optarg);
>> @@ -308,6 +311,11 @@ pwrite_f(
>>  		case 'i':
>>  			infile = optarg;
>>  			break;
>> +#ifdef HAVE_PWRITEV2
>> +		case 'N':
>> +			pwritev2_flags |= RWF_NOWAIT;
>> +			break;
>> +#endif
> 
> If pwritev2 isn't present at build time, specifying -N gives somewhat unexpected behavior:
> 
> xfs_io> pwrite -N 0 1k
> pwrite [-i infile [-d] [-s skip]] [-b bs] [-S seed] [-wW] [-FBR [-Z N]] [-V N] off len -- writes a number of bytes at a specified offset
> xfs_io>
> 
> vs a wholly unknown option:
> 
> xfs_io> pwrite -K 0 1k
> pwrite: invalid option -- 'K'
> xfs_io>
> 
> because you have 'N' in the getopt string.  I wonder if there's a better
> way to handle it besides moar ifdefs ... I guess this wouldn't be
> terrible:
> 
> +		case 'N':
> +#ifdef HAVE_PWRITEV2
> +			pwritev2_flags |= RWF_NOWAIT;
> +#else
> +			printf(_("Not built with pwritev2 functionality\n"));
> +#endif
> +			break;
> 

I had proposed something similar (with another message) in v2, but Dave
did not like it. I am fine to make it work either ways. Let me know.

Note, We would have to put similar checks for -V option which would add
some more ifdefs, which will make it a mess.



>>  		case 's':
>>  			skip = cvtnum(fsblocksize, fssectsize, optarg);
>>  			if (skip < 0) {
>> diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
>> index 0fd9b951..9c58914f 100644
>> --- a/man/man8/xfs_io.8
>> +++ b/man/man8/xfs_io.8
>> @@ -282,6 +282,12 @@ Use the vectored IO write syscall
>>  with a number of blocksize length iovecs. The number of iovecs is set by the
>>  .I vectors
>>  parameter.
>> +.TP
>> +.B \-N
>> +Perform the
>> +.BR pwritev2 (2)
>> +call with
>> +.I RWF_NOWAIT.
> 
> I guess maybe this should say something about "if it's built w/ pwritev2 functionality?"
> I'm less worried about this, tbh, especially if -N gives the explanation above
> if xfs_io doesn't have the support.
> 

Yes, I will add that.


> -Eric
> 
>>  .RE
>>  .PD
>>  .TP
>>
Eric Sandeen Nov. 9, 2017, 1:40 p.m. UTC | #4 
On 11/9/17 6:25 AM, Goldwyn Rodrigues wrote:
> 
> 
> On 11/08/2017 10:27 PM, Eric Sandeen wrote:
>> On 10/10/17 5:58 AM, Goldwyn Rodrigues wrote:

...

>> +		case 'N':
>> +#ifdef HAVE_PWRITEV2
>> +			pwritev2_flags |= RWF_NOWAIT;
>> +#else
>> +			printf(_("Not built with pwritev2 functionality\n"));
>> +#endif
>> +			break;
>>
> 
> I had proposed something similar (with another message) in v2, but Dave
> did not like it. I am fine to make it work either ways. Let me know.


> Note, We would have to put similar checks for -V option which would add
> some more ifdefs, which will make it a mess.

Oh, right sorry.  Dave had suggested that the default case should say:

"command -%c not supported"

to support all the configurable options, and I agree that's a better solution.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/io/pwrite.c b/io/pwrite.c
index 5ceb26c7..e06dfb46 100644
--- a/io/pwrite.c
+++ b/io/pwrite.c
@@ -53,6 +53,9 @@  pwrite_help(void)
 #ifdef HAVE_PWRITEV
 " -V N -- use vectored IO with N iovecs of blocksize each (pwritev)\n"
 #endif
+#ifdef HAVE_PWRITEV2
+" -N   -- Perform the pwritev2() with RWF_NOWAIT\n"
+#endif
 "\n"));
 }
 
@@ -279,7 +282,7 @@  pwrite_f(
 	init_cvtnum(&fsblocksize, &fssectsize);
 	bsize = fsblocksize;
 
-	while ((c = getopt(argc, argv, "b:BCdf:Fi:qRs:S:uV:wWZ:")) != EOF) {
+	while ((c = getopt(argc, argv, "b:BCdf:Fi:NqRs:S:uV:wWZ:")) != EOF) {
 		switch (c) {
 		case 'b':
 			tmp = cvtnum(fsblocksize, fssectsize, optarg);
@@ -308,6 +311,11 @@  pwrite_f(
 		case 'i':
 			infile = optarg;
 			break;
+#ifdef HAVE_PWRITEV2
+		case 'N':
+			pwritev2_flags |= RWF_NOWAIT;
+			break;
+#endif
 		case 's':
 			skip = cvtnum(fsblocksize, fssectsize, optarg);
 			if (skip < 0) {
diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
index 0fd9b951..9c58914f 100644
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -282,6 +282,12 @@  Use the vectored IO write syscall
 with a number of blocksize length iovecs. The number of iovecs is set by the
 .I vectors
 parameter.
+.TP
+.B \-N
+Perform the
+.BR pwritev2 (2)
+call with
+.I RWF_NOWAIT.
 .RE
 .PD
 .TP