[v4,2/2] fiemap: Implement ranged query

Commit Message

Eric Sandeen Nov. 17, 2017, 2:47 a.m. UTC

On 11/15/17 6:10 AM, Nikolay Borisov wrote:
> Currently the fiemap implementation of xfs_io doesn't support making ranged
> queries. This patch implements two optional arguments which take 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 <[e-mail]>

Ok, so first an apology for taking so long to really, really look at this.

When I started reviewing this, and dreading the complexity of the fiemap loop
control (why /is/ it so complicated today, anyway?) I started wondering: why
do we need a lot more logic?  Why can't we simply:

1) Set a non-zero mapping start
2) Set a non-maximal mapping length
3) Decrement that length as we map, and
4) When the kernel tells us mapped_extents for the range is 0, stop.

And this is what I ended up with, which seems a lot simpler.  Is there any
problem with this approach?

This /almost/ passes your test; what it doesn't do is show holes at the edges
of the mapping range, but I think that's ok.

The interface itself says nothing about holes, it only maps allocated ranges.

If we ask for a ranged query, I think it's appropriate to /not/ print holes
on either side of the requested range.


Thoughts?




--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Nikolay Borisov Nov. 17, 2017, 9:39 a.m. UTC | #1

On 17.11.2017 04:47, Eric Sandeen wrote:
> On 11/15/17 6:10 AM, Nikolay Borisov wrote:
>> Currently the fiemap implementation of xfs_io doesn't support making ranged
>> queries. This patch implements two optional arguments which take 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 <[e-mail]>
> 
> Ok, so first an apology for taking so long to really, really look at this.
> 
> When I started reviewing this, and dreading the complexity of the fiemap loop
> control (why /is/ it so complicated today, anyway?) I started wondering: why
> do we need a lot more logic?  Why can't we simply:
> 
> 1) Set a non-zero mapping start
> 2) Set a non-maximal mapping length
> 3) Decrement that length as we map, and
> 4) When the kernel tells us mapped_extents for the range is 0, stop.
> 
> And this is what I ended up with, which seems a lot simpler.  Is there any
> problem with this approach?
> 
> This /almost/ passes your test; what it doesn't do is show holes at the edges
> of the mapping range, but I think that's ok.
> 
> The interface itself says nothing about holes, it only maps allocated ranges.
> 
> If we ask for a ranged query, I think it's appropriate to /not/ print holes
> on either side of the requested range.
> 
> 
> Thoughts?

I definitely like the simplicity and am happy for this to replace my
patches. But with this do we require the fixup for existing hole tests
(I don't think so)?

> 
> 
> 
> diff --git a/io/fiemap.c b/io/fiemap.c
> index bdcfacd..bbf6d63 100644
> --- a/io/fiemap.c
> +++ b/io/fiemap.c
> @@ -49,6 +49,8 @@ fiemap_help(void)
>  " -l -- also displays the length of each extent in 512-byte blocks.\n"
>  " -n -- query n extents.\n"
>  " -v -- Verbose information\n"
> +" offset is the starting offset to map, and is optional.  If offset is\n"
> +" specified, mapping length may (optionally) be specified as well."
>  "\n"));
>  }
>  
> @@ -235,9 +237,15 @@ fiemap_f(
>  	int		boff_w = 16;
>  	int		tot_w = 5;	/* 5 since its just one number */
>  	int		flg_w = 5;
> -	__u64		last_logical = 0;
> +	__u64		last_logical = 0;	/* last extent offset handled */
> +	off64_t		start_offset = 0;	/* mapping start */
> +	off64_t		length = -1LL;		/* mapping length */
> +	off64_t		range_end = -1LL;	/* mapping end*/
> +	size_t		fsblocksize, fssectsize;
>  	struct stat	st;
>  
> +	init_cvtnum(&fsblocksize, &fssectsize);
> +
>  	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
>  		switch (c) {
>  		case 'a':
> @@ -257,6 +265,27 @@ fiemap_f(
>  		}
>  	}
>  
> +	/* Range start (optional) */
> +	if (optind < argc) {
> +		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++;
> +	}
> +
> +	/* Range length (optional if range start was specified) */
> +	if (optind < argc) {
> +		length = cvtnum(fsblocksize, fssectsize, argv[optind]);
> +		if (length < 0) {
> +			printf("non-numeric len argument -- %s\n", argv[optind]);
> +			return 0;
> +		}
> +		range_end = start_offset + length;
> +	}
> +
>  	map_size = sizeof(struct fiemap) +
>  		(EXTENT_BATCH * sizeof(struct fiemap_extent));
>  	fiemap = malloc(map_size);
> @@ -274,7 +303,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 = range_end - last_logical;
>  		fiemap->fm_extent_count = EXTENT_BATCH;
>  
>  		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
> @@ -336,7 +365,8 @@ fiemap_f(
>  		return 0;
>  	}
>  
> -	if (cur_extent && last_logical < st.st_size)
> +	/* Print last hole to EOF only if range end was not specified */
> +	if (cur_extent && (range_end == -1LL) && (last_logical < st.st_size))
>  		print_hole(foff_w, boff_w, tot_w, cur_extent, lflag, !vflag,
>  			   BTOBBT(last_logical), BTOBBT(st.st_size));
>  
> @@ -353,7 +383,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] [offset [len]]");
>  	fiemap_cmd.oneline = _("print block mapping for a file");
>  	fiemap_cmd.help = fiemap_help;
>  
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/io/fiemap.c b/io/fiemap.c
index bdcfacd..bbf6d63 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -49,6 +49,8 @@  fiemap_help(void)
 " -l -- also displays the length of each extent in 512-byte blocks.\n"
 " -n -- query n extents.\n"
 " -v -- Verbose information\n"
+" offset is the starting offset to map, and is optional.  If offset is\n"
+" specified, mapping length may (optionally) be specified as well."
 "\n"));
 }
 
@@ -235,9 +237,15 @@  fiemap_f(
 	int		boff_w = 16;
 	int		tot_w = 5;	/* 5 since its just one number */
 	int		flg_w = 5;
-	__u64		last_logical = 0;
+	__u64		last_logical = 0;	/* last extent offset handled */
+	off64_t		start_offset = 0;	/* mapping start */
+	off64_t		length = -1LL;		/* mapping length */
+	off64_t		range_end = -1LL;	/* mapping end*/
+	size_t		fsblocksize, fssectsize;
 	struct stat	st;
 
+	init_cvtnum(&fsblocksize, &fssectsize);
+
 	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
 		switch (c) {
 		case 'a':
@@ -257,6 +265,27 @@  fiemap_f(
 		}
 	}
 
+	/* Range start (optional) */
+	if (optind < argc) {
+		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++;
+	}
+
+	/* Range length (optional if range start was specified) */
+	if (optind < argc) {
+		length = cvtnum(fsblocksize, fssectsize, argv[optind]);
+		if (length < 0) {
+			printf("non-numeric len argument -- %s\n", argv[optind]);
+			return 0;
+		}
+		range_end = start_offset + length;
+	}
+
 	map_size = sizeof(struct fiemap) +
 		(EXTENT_BATCH * sizeof(struct fiemap_extent));
 	fiemap = malloc(map_size);
@@ -274,7 +303,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 = range_end - last_logical;
 		fiemap->fm_extent_count = EXTENT_BATCH;
 
 		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
@@ -336,7 +365,8 @@  fiemap_f(
 		return 0;
 	}
 
-	if (cur_extent && last_logical < st.st_size)
+	/* Print last hole to EOF only if range end was not specified */
+	if (cur_extent && (range_end == -1LL) && (last_logical < st.st_size))
 		print_hole(foff_w, boff_w, tot_w, cur_extent, lflag, !vflag,
 			   BTOBBT(last_logical), BTOBBT(st.st_size));
 
@@ -353,7 +383,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] [offset [len]]");
 	fiemap_cmd.oneline = _("print block mapping for a file");
 	fiemap_cmd.help = fiemap_help;