On 14.11.2017 00:22, Eric Sandeen wrote:
> On 11/13/17 4:05 PM, Nikolay Borisov wrote:
>>
>>
>> On 13.11.2017 23:44, Dave Chinner wrote:
>>> On Mon, Nov 13, 2017 at 05:47:53PM +0200, Nikolay Borisov wrote:
>>>> Currently the fiemap implementation of xfs_io doesn't support making ranged
>>>> queries. This patch implements the '-r' parameter, taking up to 2 arguments -
>>>> the starting offset and the length of the region to be queried. This also
>>>> requires changing of the final hole range is calculated. Namely, it's now being
>>>> done as [last_logical, logical addres of next extent] rather than being
>>>> statically determined by [last_logical, filesize].
>>>>
>>>> Signed-off-by: Nikolay Borisov <nborisov@xxxxxxxx>
>>>> ---
>>>> V3:
>>>> * Fixed a bug where incorrect extent index was shown if we didn't print a
>>>> hole. This was due to statically returning 2 at the end of print_(plain|verbose)
>>>> Now, the actual number of printed extents inside the 2 functions is used.
>>>> This bug is visible only with the -r parameter
>>>>
>>>> * Fixed a bug where 1 additional extent would be printed. This was a result of
>>>> the aforementioned bug fix, since now printing function can return 1 and still
>>>> have printed an extent and no hole. This can happen when you use -r parameter,
>>>> this is now fixed and a comment explaining it is put.
>>>>
>>>> * Reworked the handling of the new params by making them arguments to the
>>>> -r parameter.
>>>>
>>>> V2:
>>>> * Incorporated Daricks feedback - removed variables which weren't introduced
>>>> until the next patch as a result the build with this patch was broken. This is
>>>> fixed now
>>> .....
>>>
>>>> @@ -256,9 +269,12 @@ fiemap_f(
>>>> int tot_w = 5; /* 5 since its just one number */
>>>> int flg_w = 5;
>>>> __u64 last_logical = 0;
>>>> + size_t fsblocksize, fssectsize;
>>>> struct stat st;
>>>>
>>>> - while ((c = getopt(argc, argv, "aln:v")) != EOF) {
>>>> + init_cvtnum(&fsblocksize, &fssectsize);
>>>> +
>>>> + while ((c = getopt(argc, argv, "aln:vr")) != EOF) {
>>>
>>> Ok, you're not telling gotopt that "-r" has parameters, so....
>>>
>>>> switch (c) {
>>>> case 'a':
>>>> fiemap_flags |= FIEMAP_FLAG_XATTR;
>>>> @@ -272,6 +288,50 @@ fiemap_f(
>>>> case 'v':
>>>> vflag++;
>>>> break;
>>>> + case 'r':
>>>> + /* Parse the first option which is mandatory */
>>>> + if (optind < argc && argv[optind][0] != '-') {
>>>> + off64_t start_offset = cvtnum(fsblocksize,
>>>> + fssectsize,
>>>> + argv[optind]);
>>>> + if (start_offset < 0) {
>>>> + printf("non-numeric offset argument -- "
>>>> + "%s\n", argv[optind]);
>>>> + return 0;
>>>> + }
>>>> + last_logical = start_offset;
>>>> + optind++;
>>>> + } else {
>>>> + fprintf(stderr, _("Invalid offset argument for"
>>>> + " -r\n"));
>>>> + exitcode = 1;
>>>> + return 0;
>>>> + }
>>>> +
>>>> + if (optind < argc) {
>>>> + /* first check if what follows doesn't begin
>>>> + * with '-' which means it would be a param and
>>>> + * not an argument
>>>> + */
>>>> + if (argv[optind][0] == '-') {
>>>> + optind--;
>>>> + break;
>>>> +
>>>> + }
>>>> +
>>>> + off64_t length = cvtnum(fsblocksize,
>>>> + fssectsize,
>>>> + argv[optind]);
>>>> + if (length < 0) {
>>>> + printf("non-numeric len argument --"
>>>> + " %s\n", argv[optind]);
>>>> + return 0;
>>>> + }
>>>> + len = length;
>>>> + range_limit = true;
>>>> + }
>>>> + break;
>>>
>>> .... this is pretty nasty because you're having to check if the next
>>> option should be parsed by the main loop or not. This assumes that
>>> getopt is giving us the options in the order they were presented on
>>> the command line, which is not a good assumption to make as glibc
>>> can mutate the order as it parses the listi of know options and
>>> arguments.
>>>
>>> Given that "-r" is the only option that has parameters, then this
>>> can be massively simplified just by noting we've seen the rflag, and
>>> leaving the non-arg parameter parsing to after the end of the loop.
>>> i.e.:
>>
>>
>> You are right this is a bit ugly, but it seems more consistend to me,
>> rather than something like
>>
>> xfs_io -c "fiemap -r -n 10 -v 4k 10k" for example. And the reason why
>> I'm using this hackish way and not declaring r as r: in getopt is
>> because getopt doesn't recognise when a parameter takes more than 1
>> argument.
>
> Sorry for chiming in so late after all the go-rounds, but:
>
> Why not just drop -r entirely, and make fiemap go into ranged mode iff a
> range is specified at the end of the command, i.e.:
>
> fiemap [ -alv ] [ -n nx ] [ offset length ]
V2 was like that but it required some changing to the generic code which
detects the presence of this feature and Dave objected hence v3.
>
> and if no offset/length is specified, map the whole file. You can parse
> the optional last 2 arguments just like pread, write, sync_range, sendfile,
> or a host of other commands already do (though some are not optional.)
>
> There are /many/ other xfs_io commands that take offset & length at the
> end, so this usage should be very familiar to users.
>
> -Eric
>
>
--
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
