[PATCH] fiemap: Allow to specify range to fiemap

[PATCH] fiemap: Allow to specify range to fiemap

[Nikolay Borisov]

Add 2 additional, optional, arguments to the embedded fiemap command,
that way one can specify exact ranges to be fiemapped. This will be used
for a btrfs test. Since the arguments are optional, omitting them just
retains the old behavior. 

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 io/fiemap.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/io/fiemap.c b/io/fiemap.c
index 75e8820..a3c0c89 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -216,7 +216,11 @@ fiemap_f(
 	int		flg_w = 5;
 	__u64		blocksize = 512;
 	__u64		last_logical = 0;
+	__u64		len = -1LL;
 	struct stat	st;
+	size_t 		fsblocksize, fssectsize;
+
+	init_cvtnum(&fsblocksize, &fssectsize);
 
 	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
 		switch (c) {
@@ -237,6 +241,32 @@ fiemap_f(
 		}
 	}
 
+	if (argc == optind + 2) {
+		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;
+		last_logical = strtoull(argv[optind], NULL, 10);
+
+		optind++;
+
+		off64_t length = cvtnum(fsblocksize, fssectsize, argv[optind]);
+		if (length < 0) {
+			printf("non-numeric len argument -- %s\n", argv[optind]);
+			return 0;
+		}
+		len = length;
+	} else if (argc == optind + 1) {
+		off64_t start_offset = cvtnum(fsblocksize, fssectsize, argv[optind]);
+		if (last_logical < 0) {
+			printf("non-numeric offset argument -- %s\n", argv[optind]);
+			return 0;
+		}
+		last_logical = start_offset;
+	}
+
 	if (max_extents)
 		num_extents = min(num_extents, max_extents);
 	map_size = sizeof(struct fiemap) +
@@ -259,7 +289,7 @@ fiemap_f(
 		memset(fiemap, 0, map_size);
 		fiemap->fm_flags = fiemap_flags;
 		fiemap->fm_start = last_logical;
-		fiemap->fm_length = -1LL;
+		fiemap->fm_length = len;
 		fiemap->fm_extent_count = num_extents;
 
 		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
@@ -350,7 +380,7 @@ fiemap_init(void)
 	fiemap_cmd.argmin = 0;
 	fiemap_cmd.argmax = -1;
 	fiemap_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
-	fiemap_cmd.args = _("[-alv] [-n nx]");
+	fiemap_cmd.args = _("[-alv] [-n nx] [start offset] [len]");
 	fiemap_cmd.oneline = _("print block mapping for a file");
 	fiemap_cmd.help = fiemap_help;
 
-- 
2.7.4

Re: [PATCH] fiemap: Allow to specify range to fiemap

[Dave Chinner]

On Thu, Aug 10, 2017 at 02:07:44PM +0300, Nikolay Borisov wrote:
> Add 2 additional, optional, arguments to the embedded fiemap command,
> that way one can specify exact ranges to be fiemapped. This will be used
> for a btrfs test. Since the arguments are optional, omitting them just
> retains the old behavior. 
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  io/fiemap.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/io/fiemap.c b/io/fiemap.c
> index 75e8820..a3c0c89 100644
> --- a/io/fiemap.c
> +++ b/io/fiemap.c

Need to update the fiemap_help() text.

And a man page update.

> @@ -216,7 +216,11 @@ fiemap_f(
>  	int		flg_w = 5;
>  	__u64		blocksize = 512;
>  	__u64		last_logical = 0;
> +	__u64		len = -1LL;
>  	struct stat	st;
> +	size_t 		fsblocksize, fssectsize;
              ^

Whitespace damage.
> +
> +	init_cvtnum(&fsblocksize, &fssectsize);
>  
>  	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
>  		switch (c) {
> @@ -237,6 +241,32 @@ fiemap_f(
>  		}
>  	}
>  
> +	if (argc == optind + 2) {
> +		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;
> +		last_logical = strtoull(argv[optind], NULL, 10);

Ah, what?

> +
> +		optind++;
> +
> +		off64_t length = cvtnum(fsblocksize, fssectsize, argv[optind]);
> +		if (length < 0) {
> +			printf("non-numeric len argument -- %s\n", argv[optind]);
> +			return 0;
> +		}
> +		len = length;
> +	} else if (argc == optind + 1) {
> +		off64_t start_offset = cvtnum(fsblocksize, fssectsize, argv[optind]);
> +		if (last_logical < 0) {
> +			printf("non-numeric offset argument -- %s\n", argv[optind]);
> +			return 0;
> +		}
> +		last_logical = start_offset;
> +	}

Duplicate code to get start offset.

	if (optind < argc) {
		/* get start offset */
		...
		optind++
	}
	if (optind < argc) {
		/* get length */
		.....
	}

Cheers,

Dave.
> +
>  	if (max_extents)
>  		num_extents = min(num_extents, max_extents);
>  	map_size = sizeof(struct fiemap) +
> @@ -259,7 +289,7 @@ fiemap_f(
>  		memset(fiemap, 0, map_size);
>  		fiemap->fm_flags = fiemap_flags;
>  		fiemap->fm_start = last_logical;
> -		fiemap->fm_length = -1LL;
> +		fiemap->fm_length = len;
>  		fiemap->fm_extent_count = num_extents;
>  
>  		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
> @@ -350,7 +380,7 @@ fiemap_init(void)
>  	fiemap_cmd.argmin = 0;
>  	fiemap_cmd.argmax = -1;
>  	fiemap_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> -	fiemap_cmd.args = _("[-alv] [-n nx]");
> +	fiemap_cmd.args = _("[-alv] [-n nx] [start offset] [len]");

Given that it's {offset, len} or just {offset}, you can't have
length by itself as a parameter. i.e.

	fiemap_cmd.args = _("[-alv] [-n nx] [start offset [len]]");

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH v2] fiemap: Allow to specify range to fiemap

[Nikolay Borisov]

Add 2 additional, optional, arguments to the embedded fiemap command,
that way one can specify exact ranges to be fiemapped. This will be used
for a btrfs test. Since the arguments are optional, omitting them just
retains the old behavior.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

Changes since v1:
 * Added help in man describing the new options
 * Removed whitespace damage
 * Simplified/dedup the code used to get the parameters in
 * Updated one line help to indicate that len param can't exist on its own

 io/fiemap.c       | 28 ++++++++++++++++++++++++++--
 man/man8/xfs_io.8 |  5 +++--
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/io/fiemap.c b/io/fiemap.c
index 75e882057362..2a0698e9d1e9 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -34,6 +34,7 @@ fiemap_help(void)
 "\n"
 " Example:\n"
 " 'fiemap -v' - tabular format verbose map\n"
+" 'fiemap 0 4k' - print fiemap extents for 0-4k range\n"
 "\n"
 " fiemap prints the map of disk blocks used by the current file.\n"
 " The map lists each extent used by the file, as well as regions in the\n"
@@ -216,7 +217,11 @@ fiemap_f(
 	int		flg_w = 5;
 	__u64		blocksize = 512;
 	__u64		last_logical = 0;
+	__u64		len = -1LL;
 	struct stat	st;
+	size_t		fsblocksize, fssectsize;
+
+	init_cvtnum(&fsblocksize, &fssectsize);
 
 	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
 		switch (c) {
@@ -237,6 +242,25 @@ fiemap_f(
 		}
 	}
 
+	if (optind < argc) {
+		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++;
+	}
+
+	if (optind < argc) {
+		off64_t length = cvtnum(fsblocksize, fssectsize, argv[optind]);
+		if (length < 0) {
+			printf("non-numeric len argument -- %s\n", argv[optind]);
+			return 0;
+		}
+		len = length;
+	}
+
 	if (max_extents)
 		num_extents = min(num_extents, max_extents);
 	map_size = sizeof(struct fiemap) +
@@ -259,7 +283,7 @@ fiemap_f(
 		memset(fiemap, 0, map_size);
 		fiemap->fm_flags = fiemap_flags;
 		fiemap->fm_start = last_logical;
-		fiemap->fm_length = -1LL;
+		fiemap->fm_length = len;
 		fiemap->fm_extent_count = num_extents;
 
 		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
@@ -350,7 +374,7 @@ fiemap_init(void)
 	fiemap_cmd.argmin = 0;
 	fiemap_cmd.argmax = -1;
 	fiemap_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
-	fiemap_cmd.args = _("[-alv] [-n nx]");
+	fiemap_cmd.args = _("[-alv] [-n nx] [start offset [len]]");
 	fiemap_cmd.oneline = _("print block mapping for a file");
 	fiemap_cmd.help = fiemap_help;
 
diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
index 273b9c54c52d..125db9181851 100644
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -295,11 +295,12 @@ Prints the block mapping for the current open file. Refer to the
 .BR xfs_bmap (8)
 manual page for complete documentation.
 .TP
-.BI "fiemap [ \-alv ] [ \-n " nx " ]"
+.BI "fiemap [ \-alv ] [ \-n " nx " ] [ " offset " [ " len " ]]"
 Prints the block mapping for the current open file using the fiemap
 ioctl.  Options behave as described in the
 .BR xfs_bmap (8)
-manual page.
+manual page. Optionally, this command also supports passing the start offset 
+from where to begin the fiemap and the length of that region.
 .TP
 .BI "fsmap [ \-d | \-l | \-r ] [ \-m | \-v ] [ \-n " nx " ] [ " start " ] [ " end " ]
 Prints the mapping of disk blocks used by the filesystem hosting the current
-- 
2.7.4

Re: [PATCH v2] fiemap: Allow to specify range to fiemap

[Darrick J. Wong]

On Thu, Aug 10, 2017 at 03:30:41PM +0300, Nikolay Borisov wrote:
> Add 2 additional, optional, arguments to the embedded fiemap command,
> that way one can specify exact ranges to be fiemapped. This will be used
> for a btrfs test. Since the arguments are optional, omitting them just
> retains the old behavior.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> Changes since v1:
>  * Added help in man describing the new options
>  * Removed whitespace damage
>  * Simplified/dedup the code used to get the parameters in
>  * Updated one line help to indicate that len param can't exist on its own
> 
>  io/fiemap.c       | 28 ++++++++++++++++++++++++++--
>  man/man8/xfs_io.8 |  5 +++--
>  2 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/io/fiemap.c b/io/fiemap.c
> index 75e882057362..2a0698e9d1e9 100644
> --- a/io/fiemap.c
> +++ b/io/fiemap.c
> @@ -34,6 +34,7 @@ fiemap_help(void)
>  "\n"
>  " Example:\n"
>  " 'fiemap -v' - tabular format verbose map\n"
> +" 'fiemap 0 4k' - print fiemap extents for 0-4k range\n"
>  "\n"
>  " fiemap prints the map of disk blocks used by the current file.\n"
>  " The map lists each extent used by the file, as well as regions in the\n"
> @@ -216,7 +217,11 @@ fiemap_f(
>  	int		flg_w = 5;
>  	__u64		blocksize = 512;
>  	__u64		last_logical = 0;
> +	__u64		len = -1LL;
>  	struct stat	st;
> +	size_t		fsblocksize, fssectsize;
> +
> +	init_cvtnum(&fsblocksize, &fssectsize);
>  
>  	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
>  		switch (c) {
> @@ -237,6 +242,25 @@ fiemap_f(
>  		}
>  	}
>  
> +	if (optind < argc) {
> +		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++;
> +	}
> +
> +	if (optind < argc) {
> +		off64_t length = cvtnum(fsblocksize, fssectsize, argv[optind]);
> +		if (length < 0) {
> +			printf("non-numeric len argument -- %s\n", argv[optind]);
> +			return 0;
> +		}
> +		len = length;
> +	}
> +
>  	if (max_extents)
>  		num_extents = min(num_extents, max_extents);
>  	map_size = sizeof(struct fiemap) +
> @@ -259,7 +283,7 @@ fiemap_f(
>  		memset(fiemap, 0, map_size);
>  		fiemap->fm_flags = fiemap_flags;
>  		fiemap->fm_start = last_logical;
> -		fiemap->fm_length = -1LL;
> +		fiemap->fm_length = len;

We don't decrement len each time through the ioctl loop?  Doesn't that
cause us to request and retrieve rows for ranges we don't want?

--D

>  		fiemap->fm_extent_count = num_extents;
>  
>  		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
> @@ -350,7 +374,7 @@ fiemap_init(void)
>  	fiemap_cmd.argmin = 0;
>  	fiemap_cmd.argmax = -1;
>  	fiemap_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
> -	fiemap_cmd.args = _("[-alv] [-n nx]");
> +	fiemap_cmd.args = _("[-alv] [-n nx] [start offset [len]]");
>  	fiemap_cmd.oneline = _("print block mapping for a file");
>  	fiemap_cmd.help = fiemap_help;
>  
> diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
> index 273b9c54c52d..125db9181851 100644
> --- a/man/man8/xfs_io.8
> +++ b/man/man8/xfs_io.8
> @@ -295,11 +295,12 @@ Prints the block mapping for the current open file. Refer to the
>  .BR xfs_bmap (8)
>  manual page for complete documentation.
>  .TP
> -.BI "fiemap [ \-alv ] [ \-n " nx " ]"
> +.BI "fiemap [ \-alv ] [ \-n " nx " ] [ " offset " [ " len " ]]"
>  Prints the block mapping for the current open file using the fiemap
>  ioctl.  Options behave as described in the
>  .BR xfs_bmap (8)
> -manual page.
> +manual page. Optionally, this command also supports passing the start offset 
> +from where to begin the fiemap and the length of that region.
>  .TP
>  .BI "fsmap [ \-d | \-l | \-r ] [ \-m | \-v ] [ \-n " nx " ] [ " start " ] [ " end " ]
>  Prints the mapping of disk blocks used by the filesystem hosting the current
> -- 
> 2.7.4
> 
> --
> 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

Re: [PATCH v2] fiemap: Allow to specify range to fiemap

[Nikolay Borisov]

On 10.08.2017 19:13, Darrick J. Wong wrote:
> We don't decrement len each time through the ioctl loop?  Doesn't that
> cause us to request and retrieve rows for ranges we don't want?

Hm, you are right, I believe on every iteration we should do:

len -= last_logical - start_offset

Re: [PATCH v2] fiemap: Allow to specify range to fiemap

[Darrick J. Wong]

On Thu, Aug 10, 2017 at 07:46:35PM +0300, Nikolay Borisov wrote:
> 
> 
> On 10.08.2017 19:13, Darrick J. Wong wrote:
> > We don't decrement len each time through the ioctl loop?  Doesn't that
> > cause us to request and retrieve rows for ranges we don't want?
> 
> Hm, you are right, I believe on every iteration we should do:
> 
> len -= last_logical - start_offset

start_offset doesn't change, right?

I would think len -= last_logical - fiemap->fm_start.

Or just calculate end_offset = start_offset + length and set
fiemap->fm_length = end_offset - last_logical... 

...though now that I look at that file even closer I wonder what is
going on with that outer while loop counting extents?  Does -n exist to
let users cap the number of records to print, even if that means we
don't make it to the end of the range requested?

--D

> --
> 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

Re: [PATCH v2] fiemap: Allow to specify range to fiemap

[Eric Sandeen]

On 8/10/17 2:12 PM, Darrick J. Wong wrote:
> On Thu, Aug 10, 2017 at 07:46:35PM +0300, Nikolay Borisov wrote:
>>
>>
>> On 10.08.2017 19:13, Darrick J. Wong wrote:
>>> We don't decrement len each time through the ioctl loop?  Doesn't that
>>> cause us to request and retrieve rows for ranges we don't want?
>>
>> Hm, you are right, I believe on every iteration we should do:
>>
>> len -= last_logical - start_offset
> 
> start_offset doesn't change, right?
> 
> I would think len -= last_logical - fiemap->fm_start.
> 
> Or just calculate end_offset = start_offset + length and set
> fiemap->fm_length = end_offset - last_logical... 
> 
> ...though now that I look at that file even closer I wonder what is
> going on with that outer while loop counting extents?  Does -n exist to
> let users cap the number of records to print, even if that means we
> don't make it to the end of the range requested?

-n should simply be the size of each fiemap request, i.e. how many to
request and fill on each iteration.  It should not affect how many
get printed in the end.

At least that's how xfs_bmap is supposed to work, and therefore in theory
how xfs_io's fiemap should work.

-Eric

Re: [PATCH v2] fiemap: Allow to specify range to fiemap

[Eric Sandeen]

On 8/10/17 2:22 PM, Eric Sandeen wrote:
> On 8/10/17 2:12 PM, Darrick J. Wong wrote:
>> On Thu, Aug 10, 2017 at 07:46:35PM +0300, Nikolay Borisov wrote:
>>>
>>>
>>> On 10.08.2017 19:13, Darrick J. Wong wrote:
>>>> We don't decrement len each time through the ioctl loop?  Doesn't that
>>>> cause us to request and retrieve rows for ranges we don't want?
>>>
>>> Hm, you are right, I believe on every iteration we should do:
>>>
>>> len -= last_logical - start_offset
>>
>> start_offset doesn't change, right?
>>
>> I would think len -= last_logical - fiemap->fm_start.
>>
>> Or just calculate end_offset = start_offset + length and set
>> fiemap->fm_length = end_offset - last_logical... 
>>
>> ...though now that I look at that file even closer I wonder what is
>> going on with that outer while loop counting extents?  Does -n exist to
>> let users cap the number of records to print, even if that means we
>> don't make it to the end of the range requested?
> 
> -n should simply be the size of each fiemap request, i.e. how many to
> request and fill on each iteration.  It should not affect how many
> get printed in the end.
> 
> At least that's how xfs_bmap is supposed to work, and therefore in theory
> how xfs_io's fiemap should work.

... except apparently that is not actually how it works :(

-Eric

Re: [PATCH v2] fiemap: Allow to specify range to fiemap

[Nikolay Borisov]

On 11.08.2017 00:57, Eric Sandeen wrote:
> On 8/10/17 2:22 PM, Eric Sandeen wrote:
>> On 8/10/17 2:12 PM, Darrick J. Wong wrote:
>>> On Thu, Aug 10, 2017 at 07:46:35PM +0300, Nikolay Borisov wrote:
>>>>
>>>>
>>>> On 10.08.2017 19:13, Darrick J. Wong wrote:
>>>>> We don't decrement len each time through the ioctl loop?  Doesn't that
>>>>> cause us to request and retrieve rows for ranges we don't want?
>>>>
>>>> Hm, you are right, I believe on every iteration we should do:
>>>>
>>>> len -= last_logical - start_offset
>>>
>>> start_offset doesn't change, right?
>>>
>>> I would think len -= last_logical - fiemap->fm_start.
>>>
>>> Or just calculate end_offset = start_offset + length and set
>>> fiemap->fm_length = end_offset - last_logical... 
>>>
>>> ...though now that I look at that file even closer I wonder what is
>>> going on with that outer while loop counting extents?  Does -n exist to
>>> let users cap the number of records to print, even if that means we
>>> don't make it to the end of the range requested?
>>
>> -n should simply be the size of each fiemap request, i.e. how many to
>> request and fill on each iteration.  It should not affect how many
>> get printed in the end.
>>
>> At least that's how xfs_bmap is supposed to work, and therefore in theory
>> how xfs_io's fiemap should work.
> 
> ... except apparently that is not actually how it works :(

In my tests passing -n 1 doesn't print anything irrespective of whether
I use the newly added args, it was strange. It's supposed to define the
batch of extents to query at a time. However, that's definitely not what
it's doing... Also the man pages states that if -n is not passed fiemap
would query the number of extents required and use that, however the
code currently just uses the default of 32 extents ...
> 
> -Eric
>