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

Commit Message

Goldwyn Rodrigues Sept. 29, 2017, 1 p.m. UTC

From: Goldwyn Rodrigues <[e-mail]>

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

Signed-off-by: Goldwyn Rodrigues <[e-mail]>
---
 io/pwrite.c       | 8 +++++++-
 man/man8/xfs_io.8 | 6 ++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

Comments

Brian Foster Oct. 9, 2017, 3:02 p.m. UTC | #1

On Fri, Sep 29, 2017 at 08:00:34AM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <[e-mail]>
> 
> This allows to make pwritev2() calls with RWF_NOWAIT,
> which would fail in case the call blocks.
> 
> Signed-off-by: Goldwyn Rodrigues <[e-mail]>
> ---
>  io/pwrite.c       | 8 +++++++-
>  man/man8/xfs_io.8 | 6 ++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/io/pwrite.c b/io/pwrite.c
> index e7d411bb..2b85a528 100644
> --- a/io/pwrite.c
> +++ b/io/pwrite.c
> @@ -52,6 +52,9 @@ pwrite_help(void)
>  "         (heh, zorry, the -s/-S arguments were already in use in pwrite)\n"
>  #ifdef HAVE_PWRITEV
>  " -V N -- use vectored IO with N iovecs of blocksize each (pwritev)\n"
> +#ifdef HAVE_PWRITEV2
> +" -N   -- Perform the pwritev2() with RWF_NOWAIT\n"
> +#endif
>  #endif
>  "\n"));
>  }
> @@ -276,7 +279,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);
> @@ -305,6 +308,9 @@ pwrite_f(
>  		case 'i':
>  			infile = optarg;
>  			break;
> +		case 'N':
> +			pwritev2_flags |= RWF_NOWAIT;

This causes a build failure on my local machine due to lack of
RWF_NOWAIT. #ifdef HAVE_PWRITEV2 around this case? Otherwise the code
looks Ok.

Brian

> +			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.
>  .RE
>  .PD
>  .TP
> -- 
> 2.14.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Darrick J. Wong Oct. 9, 2017, 5:19 p.m. UTC | #2

On Fri, Sep 29, 2017 at 08:00:34AM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <[e-mail]>
> 
> This allows to make pwritev2() calls with RWF_NOWAIT,
> which would fail in case the call blocks.
> 
> Signed-off-by: Goldwyn Rodrigues <[e-mail]>
> ---
>  io/pwrite.c       | 8 +++++++-
>  man/man8/xfs_io.8 | 6 ++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/io/pwrite.c b/io/pwrite.c
> index e7d411bb..2b85a528 100644
> --- a/io/pwrite.c
> +++ b/io/pwrite.c
> @@ -52,6 +52,9 @@ pwrite_help(void)
>  "         (heh, zorry, the -s/-S arguments were already in use in pwrite)\n"
>  #ifdef HAVE_PWRITEV
>  " -V N -- use vectored IO with N iovecs of blocksize each (pwritev)\n"
> +#ifdef HAVE_PWRITEV2
> +" -N   -- Perform the pwritev2() with RWF_NOWAIT\n"
> +#endif

Separate these two? i.e.

#ifdef HAVE_PWRITEV
" -V N..."
#endif
#ifdef HAVE_PWRITEV2
" -N ..."
#endif

They're tested separately in configure, so the ifdefs needn't be nested.

>  #endif
>  "\n"));
>  }
> @@ -276,7 +279,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);
> @@ -305,6 +308,9 @@ pwrite_f(
>  		case 'i':
>  			infile = optarg;
>  			break;
> +		case 'N':
> +			pwritev2_flags |= RWF_NOWAIT;
> +			break;

Needs #ifdef HAVE_PWRITEV2.

If you feel the need to be extra cautious, you could also put:

#ifndef HAVE_PWRITEV2
	assert(pwritev2_flags == 0);
#endif

...just after the getopt parsing to make sure that we never silently
drop RWF_* flags due to code bugs.

--D

>  		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.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Goldwyn Rodrigues Oct. 9, 2017, 9:11 p.m. UTC | #3

On 10/09/2017 12:19 PM, Darrick J. Wong wrote:
> On Fri, Sep 29, 2017 at 08:00:34AM -0500, Goldwyn Rodrigues wrote:
>> From: Goldwyn Rodrigues <[e-mail]>
>>
>> This allows to make pwritev2() calls with RWF_NOWAIT,
>> which would fail in case the call blocks.
>>
>> Signed-off-by: Goldwyn Rodrigues <[e-mail]>
>> ---
>>  io/pwrite.c       | 8 +++++++-
>>  man/man8/xfs_io.8 | 6 ++++++
>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/io/pwrite.c b/io/pwrite.c
>> index e7d411bb..2b85a528 100644
>> --- a/io/pwrite.c
>> +++ b/io/pwrite.c
>> @@ -52,6 +52,9 @@ pwrite_help(void)
>>  "         (heh, zorry, the -s/-S arguments were already in use in pwrite)\n"
>>  #ifdef HAVE_PWRITEV
>>  " -V N -- use vectored IO with N iovecs of blocksize each (pwritev)\n"
>> +#ifdef HAVE_PWRITEV2
>> +" -N   -- Perform the pwritev2() with RWF_NOWAIT\n"
>> +#endif
> 
> Separate these two? i.e.
> 
> #ifdef HAVE_PWRITEV
> " -V N..."
> #endif
> #ifdef HAVE_PWRITEV2
> " -N ..."
> #endif
> 
> They're tested separately in configure, so the ifdefs needn't be nested.
> 
>>  #endif
>>  "\n"));
>>  }
>> @@ -276,7 +279,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);
>> @@ -305,6 +308,9 @@ pwrite_f(
>>  		case 'i':
>>  			infile = optarg;
>>  			break;
>> +		case 'N':
>> +			pwritev2_flags |= RWF_NOWAIT;
>> +			break;
> 
> Needs #ifdef HAVE_PWRITEV2.
> 
> If you feel the need to be extra cautious, you could also put:
> 
> #ifndef HAVE_PWRITEV2
> 	assert(pwritev2_flags == 0);
> #endif
> 
> ...just after the getopt parsing to make sure that we never silently
> drop RWF_* flags due to code bugs.
> 

Instead I am planning to put an #else to #ifdef in the case 'N' to make
sure that people who do not have pwritev2() get the proper message
instead of the command failing without reason.

+               case 'N':
+#ifdef HAVE_PWRITEV2
+                       pwritev2_flags |= RWF_NOWAIT;
+                       break;
+#else
+                       printf(_("-N: Kernel does not support
pwritev2()\n"));
+                       return 0;
+#endif



> --D
> 
>>  		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.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner Oct. 9, 2017, 10:37 p.m. UTC | #4

On Mon, Oct 09, 2017 at 04:11:17PM -0500, Goldwyn Rodrigues wrote:
> 
> 
> On 10/09/2017 12:19 PM, Darrick J. Wong wrote:
> > On Fri, Sep 29, 2017 at 08:00:34AM -0500, Goldwyn Rodrigues wrote:
> >> From: Goldwyn Rodrigues <[e-mail]>
> >>
> >> This allows to make pwritev2() calls with RWF_NOWAIT,
> >> which would fail in case the call blocks.
> >>
> >> Signed-off-by: Goldwyn Rodrigues <[e-mail]>
> >> ---
> >>  io/pwrite.c       | 8 +++++++-
> >>  man/man8/xfs_io.8 | 6 ++++++
> >>  2 files changed, 13 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/io/pwrite.c b/io/pwrite.c
> >> index e7d411bb..2b85a528 100644
> >> --- a/io/pwrite.c
> >> +++ b/io/pwrite.c
> >> @@ -52,6 +52,9 @@ pwrite_help(void)
> >>  "         (heh, zorry, the -s/-S arguments were already in use in pwrite)\n"
> >>  #ifdef HAVE_PWRITEV
> >>  " -V N -- use vectored IO with N iovecs of blocksize each (pwritev)\n"
> >> +#ifdef HAVE_PWRITEV2
> >> +" -N   -- Perform the pwritev2() with RWF_NOWAIT\n"
> >> +#endif
> > 
> > Separate these two? i.e.
> > 
> > #ifdef HAVE_PWRITEV
> > " -V N..."
> > #endif
> > #ifdef HAVE_PWRITEV2
> > " -N ..."
> > #endif
> > 
> > They're tested separately in configure, so the ifdefs needn't be nested.
> > 
> >>  #endif
> >>  "\n"));
> >>  }
> >> @@ -276,7 +279,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);
> >> @@ -305,6 +308,9 @@ pwrite_f(
> >>  		case 'i':
> >>  			infile = optarg;
> >>  			break;
> >> +		case 'N':
> >> +			pwritev2_flags |= RWF_NOWAIT;
> >> +			break;
> > 
> > Needs #ifdef HAVE_PWRITEV2.
> > 
> > If you feel the need to be extra cautious, you could also put:
> > 
> > #ifndef HAVE_PWRITEV2
> > 	assert(pwritev2_flags == 0);
> > #endif
> > 
> > ...just after the getopt parsing to make sure that we never silently
> > drop RWF_* flags due to code bugs.
> > 
> 
> Instead I am planning to put an #else to #ifdef in the case 'N' to make
> sure that people who do not have pwritev2() get the proper message
> instead of the command failing without reason.
> 
> +               case 'N':
> +#ifdef HAVE_PWRITEV2
> +                       pwritev2_flags |= RWF_NOWAIT;
> +                       break;
> +#else
> +                       printf(_("-N: Kernel does not support
> pwritev2()\n"));
> +                       return 0;
> +#endif

Why add support for a command only to say "command not supported"?
The error message is also incorrect - the build environment didn't
support pwritev2, not the kernel the xfs_io binary is currently
running on.

As it is, I really don't like this sort of ifdef pattern because it
means over time we'll end up with an ifdef mess in the option
parsing as more flags are added. Just make the default case say
"command -%c not supported", and that removes the need for any of
these else cases.

Cheers,

Dave.

Goldwyn Rodrigues Oct. 9, 2017, 10:51 p.m. UTC | #5

On 10/09/2017 05:37 PM, Dave Chinner wrote:
> On Mon, Oct 09, 2017 at 04:11:17PM -0500, Goldwyn Rodrigues wrote:
>>
>>
>> On 10/09/2017 12:19 PM, Darrick J. Wong wrote:
>>> On Fri, Sep 29, 2017 at 08:00:34AM -0500, Goldwyn Rodrigues wrote:
>>>> From: Goldwyn Rodrigues <[e-mail]>
>>>>
>>>> This allows to make pwritev2() calls with RWF_NOWAIT,
>>>> which would fail in case the call blocks.
>>>>
>>>> Signed-off-by: Goldwyn Rodrigues <[e-mail]>
>>>> ---
>>>>  io/pwrite.c       | 8 +++++++-
>>>>  man/man8/xfs_io.8 | 6 ++++++
>>>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/io/pwrite.c b/io/pwrite.c
>>>> index e7d411bb..2b85a528 100644
>>>> --- a/io/pwrite.c
>>>> +++ b/io/pwrite.c
>>>> @@ -52,6 +52,9 @@ pwrite_help(void)
>>>>  "         (heh, zorry, the -s/-S arguments were already in use in pwrite)\n"
>>>>  #ifdef HAVE_PWRITEV
>>>>  " -V N -- use vectored IO with N iovecs of blocksize each (pwritev)\n"
>>>> +#ifdef HAVE_PWRITEV2
>>>> +" -N   -- Perform the pwritev2() with RWF_NOWAIT\n"
>>>> +#endif
>>>
>>> Separate these two? i.e.
>>>
>>> #ifdef HAVE_PWRITEV
>>> " -V N..."
>>> #endif
>>> #ifdef HAVE_PWRITEV2
>>> " -N ..."
>>> #endif
>>>
>>> They're tested separately in configure, so the ifdefs needn't be nested.
>>>
>>>>  #endif
>>>>  "\n"));
>>>>  }
>>>> @@ -276,7 +279,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);
>>>> @@ -305,6 +308,9 @@ pwrite_f(
>>>>  		case 'i':
>>>>  			infile = optarg;
>>>>  			break;
>>>> +		case 'N':
>>>> +			pwritev2_flags |= RWF_NOWAIT;
>>>> +			break;
>>>
>>> Needs #ifdef HAVE_PWRITEV2.
>>>
>>> If you feel the need to be extra cautious, you could also put:
>>>
>>> #ifndef HAVE_PWRITEV2
>>> 	assert(pwritev2_flags == 0);
>>> #endif
>>>
>>> ...just after the getopt parsing to make sure that we never silently
>>> drop RWF_* flags due to code bugs.
>>>
>>
>> Instead I am planning to put an #else to #ifdef in the case 'N' to make
>> sure that people who do not have pwritev2() get the proper message
>> instead of the command failing without reason.
>>
>> +               case 'N':
>> +#ifdef HAVE_PWRITEV2
>> +                       pwritev2_flags |= RWF_NOWAIT;
>> +                       break;
>> +#else
>> +                       printf(_("-N: Kernel does not support
>> pwritev2()\n"));
>> +                       return 0;
>> +#endif
> 
> Why add support for a command only to say "command not supported"?
> The error message is also incorrect - the build environment didn't
> support pwritev2, not the kernel the xfs_io binary is currently
> running on.
> 
> As it is, I really don't like this sort of ifdef pattern because it
> means over time we'll end up with an ifdef mess in the option
> parsing as more flags are added. Just make the default case say
> "command -%c not supported", and that removes the need for any of
> these else cases.
> 

In that case, it would go to "default" which will print the
command_usage. I am fine with that as long as the user understands why
-N is not working.

Patch

diff --git a/io/pwrite.c b/io/pwrite.c
index e7d411bb..2b85a528 100644
--- a/io/pwrite.c
+++ b/io/pwrite.c
@@ -52,6 +52,9 @@  pwrite_help(void)
 "         (heh, zorry, the -s/-S arguments were already in use in pwrite)\n"
 #ifdef HAVE_PWRITEV
 " -V N -- use vectored IO with N iovecs of blocksize each (pwritev)\n"
+#ifdef HAVE_PWRITEV2
+" -N   -- Perform the pwritev2() with RWF_NOWAIT\n"
+#endif
 #endif
 "\n"));
 }
@@ -276,7 +279,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);
@@ -305,6 +308,9 @@  pwrite_f(
 		case 'i':
 			infile = optarg;
 			break;
+		case 'N':
+			pwritev2_flags |= RWF_NOWAIT;
+			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.
 .RE
 .PD
 .TP