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

[Dave Chinner <david@fromorbit.com>]

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 <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>
> >> ---
> >>  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.
-- 
Dave Chinner
david@fromorbit.com