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 <rgoldwyn@xxxxxxxx>
>>>>
>>>> This allows to make pwritev2() calls with RWF_NOWAIT,
>>>> which would fail in case the call blocks.
>>>>
>>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@xxxxxxxx>
>>>> ---
>>>> 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.
--
Goldwyn
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
