[PATCH] fiemap: Refactor fiemap + implement range parameters

[PATCH] fiemap: Refactor fiemap + implement range parameters

[Nikolay Borisov]

fiemap's code was rather complicated for the simple task it did so this
patch aims to rectify the situation. The main changes are:

* Add a start_offset, len parameters, this allows to specify a range of the
file for which we want information, if those parameters are omitted the old
behavior is retained i.e. we get information for the whole file.

* We now always allocate as many struct fiemap_extent structs as necessary
to hold the information for the passed range (or the whole file)

* Made max_extents, blocksize global variables. They are never modified apart
from the initilization of the program.

* Eliminated the outer while loop, now that we allocate enough extent to
perform a single fiemap ioctl call there is no need for it.

* As a result the -n parameter works correctly for -n 1

* Also changed the way the last hole is being calculated, so far every time
the outter while loop finished the extenet from last_logical .. file size would
be output as a hole. This was not always true

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

Hello, 

Here is more or less a major cleanup of the fiemap code. I've tested it with a 
file with with the following layout HDHDHDHDHDHD (H - hole, D-data) and 
varying permutation of the arguments and so far everthing seems to work as 
expected. The biggest benefit I see is that the code is easier to follow now.


 io/fiemap.c       | 187 +++++++++++++++++++++++++++++-------------------------
 man/man8/xfs_io.8 |   5 +-
 2 files changed, 104 insertions(+), 88 deletions(-)

diff --git a/io/fiemap.c b/io/fiemap.c
index 75e882057362..5659f16d767d 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -24,6 +24,8 @@
 #include "io.h"
 
 static cmdinfo_t fiemap_cmd;
+static const __u64 blocksize = 512;
+static int max_extents = 0;
 
 static void
 fiemap_help(void)
@@ -34,6 +36,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"
@@ -52,12 +55,10 @@ fiemap_help(void)
 static void
 print_verbose(
 	struct fiemap_extent	*extent,
-	int			blocksize,
 	int			foff_w,
 	int			boff_w,
 	int			tot_w,
 	int			flg_w,
-	int			max_extents,
 	int			*cur_extent,
 	__u64			*last_logical)
 {
@@ -85,6 +86,7 @@ print_verbose(
 			flg_w, _("FLAGS"));
 	}
 
+
 	if (lstart != llast) {
 		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", llast,
 			 lstart - 1ULL);
@@ -94,7 +96,7 @@ print_verbose(
 		memset(lbuf, 0, sizeof(lbuf));
 	}
 
-	if ((*cur_extent + 1) == max_extents)
+	if (max_extents && *cur_extent == max_extents)
 		return;
 
 	snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", lstart,
@@ -112,8 +114,6 @@ static void
 print_plain(
 	struct fiemap_extent	*extent,
 	int			lflag,
-	int			blocksize,
-	int			max_extents,
 	int			*cur_extent,
 	__u64			*last_logical)
 {
@@ -137,7 +137,7 @@ print_plain(
 		(*cur_extent)++;
 	}
 
-	if ((*cur_extent + 1) == max_extents)
+	if (max_extents && *cur_extent == max_extents)
 		return;
 
 	printf("\t%d: [%llu..%llu]: %llu..%llu", *cur_extent,
@@ -159,13 +159,12 @@ print_plain(
 static void
 calc_print_format(
 	struct fiemap		*fiemap,
-	__u64			blocksize,
 	int			*foff_w,
 	int			*boff_w,
 	int			*tot_w,
 	int			*flg_w)
 {
-	int 			i;
+	int			i;
 	char			lbuf[32];
 	char			bbuf[32];
 	__u64			logical;
@@ -193,15 +192,28 @@ calc_print_format(
 	}
 }
 
+static ssize_t
+get_extent_num(int fd, __u64 startoffset, __u64 len)
+{
+	struct fiemap fiemap = { 0 } ;
+	fiemap.fm_start = startoffset;
+	fiemap.fm_length = len;
+	if (ioctl(fd, FS_IOC_FIEMAP, (unsigned long)&fiemap) < 0) {
+		fprintf(stderr, _("%s: Failed to query fiemap extent count.\n"),
+			progname);
+		return -1;
+	}
+
+	return fiemap.fm_mapped_extents;
+}
+
 int
 fiemap_f(
 	int		argc,
 	char		**argv)
 {
 	struct fiemap	*fiemap;
-	int		max_extents = 0;
-	int		num_extents = 32;
-	int		last = 0;
+	int		num_extents;
 	int		lflag = 0;
 	int		vflag = 0;
 	int		fiemap_flags = FIEMAP_FLAG_SYNC;
@@ -210,24 +222,29 @@ fiemap_f(
 	int		map_size;
 	int		ret;
 	int		cur_extent = 0;
-	int		foff_w = 16;	/* 16 just for a good minimum range */
+	int		foff_w = 16; /* 16 just for a good minimum range */
 	int		boff_w = 16;
 	int		tot_w = 5;	/* 5 since its just one number */
 	int		flg_w = 5;
-	__u64		blocksize = 512;
 	__u64		last_logical = 0;
-	struct stat	st;
+	__u64		len = -1LL;
+	size_t		fsblocksize, fssectsize;
+	off64_t		start_offset = 0;
+	__u64		end_offset;
+	__u64		llast;
+	bool		last_extent = false;
+
+	init_cvtnum(&fsblocksize, &fssectsize);
 
 	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
 		switch (c) {
 		case 'a':
 			fiemap_flags |= FIEMAP_FLAG_XATTR;
 			break;
-		case 'l':
-			lflag = 1;
-			break;
 		case 'n':
 			max_extents = atoi(optarg);
+		case 'l':
+			lflag = 1;
 			break;
 		case 'v':
 			vflag++;
@@ -237,8 +254,35 @@ fiemap_f(
 		}
 	}
 
-	if (max_extents)
-		num_extents = min(num_extents, max_extents);
+	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++;
+	}
+
+	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;
+	}
+
+
+	end_offset = (start_offset + len) / blocksize;
+
+	ret = get_extent_num(file->fd, last_logical, len);
+	if (ret < 0) {
+		exitcode = 1;
+		return 0;
+	}
+	num_extents = ret;
+
 	map_size = sizeof(struct fiemap) +
 		(num_extents * sizeof(struct fiemap_extent));
 	fiemap = malloc(map_size);
@@ -251,92 +295,63 @@ fiemap_f(
 
 	printf("%s:\n", file->name);
 
-	while (!last && ((cur_extent + 1) != max_extents)) {
-		if (max_extents)
-			num_extents = min(num_extents,
-					  max_extents - (cur_extent + 1));
-
-		memset(fiemap, 0, map_size);
-		fiemap->fm_flags = fiemap_flags;
-		fiemap->fm_start = last_logical;
-		fiemap->fm_length = -1LL;
-		fiemap->fm_extent_count = num_extents;
-
-		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
-		if (ret < 0) {
-			fprintf(stderr, "%s: ioctl(FS_IOC_FIEMAP) [\"%s\"]: "
-				"%s\n", progname, file->name, strerror(errno));
-			free(fiemap);
-			exitcode = 1;
-			return 0;
-		}
+	memset(fiemap, 0, map_size);
+	fiemap->fm_flags = fiemap_flags;
+	fiemap->fm_start = last_logical;
+	fiemap->fm_length = len;
+	fiemap->fm_extent_count = num_extents;
 
-		/* No more extents to map, exit */
-		if (!fiemap->fm_mapped_extents)
-			break;
+	ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
+	if (ret < 0) {
+		fprintf(stderr, "%s: ioctl(FS_IOC_FIEMAP) [\"%s\"]: "
+			"%s\n", progname, file->name, strerror(errno));
+		free(fiemap);
+		exitcode = 1;
+		return 0;
+	}
+
+	for (i = 0; i < fiemap->fm_mapped_extents; i++) {
+		struct fiemap_extent	*extent;
 
-		for (i = 0; i < fiemap->fm_mapped_extents; i++) {
-			struct fiemap_extent	*extent;
-
-			extent = &fiemap->fm_extents[i];
-			if (vflag) {
-				if (cur_extent == 0) {
-					calc_print_format(fiemap, blocksize,
-							  &foff_w, &boff_w,
-							  &tot_w, &flg_w);
-				}
-
-				print_verbose(extent, blocksize, foff_w,
-					      boff_w, tot_w, flg_w,
-					      max_extents, &cur_extent,
-					      &last_logical);
-			} else
-				print_plain(extent, lflag, blocksize,
-					    max_extents, &cur_extent,
-					    &last_logical);
-
-			if (extent->fe_flags & FIEMAP_EXTENT_LAST) {
-				last = 1;
-				break;
+		extent = &fiemap->fm_extents[i];
+		if (vflag) {
+			if (cur_extent == 0) {
+				calc_print_format(fiemap, &foff_w, &boff_w,
+						  &tot_w, &flg_w);
 			}
 
-			if ((cur_extent + 1) == max_extents)
-				break;
-		}
-	}
+			print_verbose(extent, foff_w, boff_w, tot_w, flg_w,
+				      &cur_extent, &last_logical);
+		} else
+			print_plain(extent, lflag, &cur_extent, &last_logical);
 
-	if ((cur_extent + 1) == max_extents)
-		goto out;
+		if (max_extents && cur_extent == max_extents)
+			goto out;
+
+		if (extent->fe_flags & FIEMAP_EXTENT_LAST)
+			last_extent = true;
 
-	memset(&st, 0, sizeof(st));
-	if (fstat(file->fd, &st)) {
-		fprintf(stderr, "%s: fstat failed: %s\n", progname,
-			strerror(errno));
-		free(fiemap);
-		exitcode = 1;
-		return 0;
 	}
 
-	if (cur_extent && last_logical < st.st_size) {
-		char	lbuf[32];
+	llast = last_logical / blocksize;
+	if (!last_extent && llast < end_offset) {
+		char lbuf[32];
+		__u64 difference = end_offset - llast;
+		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", llast, llast + difference);
 
-		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:",
-			 last_logical / blocksize, (st.st_size / blocksize) - 1);
 		if (vflag) {
 			printf("%4d: %-*s %-*s %*llu\n", cur_extent,
 			       foff_w, lbuf, boff_w, _("hole"), tot_w,
-			       (st.st_size - last_logical) / blocksize);
+			       difference);
 		} else {
 			printf("\t%d: %s %s", cur_extent, lbuf,
 			       _("hole"));
 			if (lflag)
-				printf(_(" %llu blocks\n"),
-				       (st.st_size - last_logical) / blocksize);
+				printf(_(" %llu blocks\n"), len / blocksize);
 			else
 				printf("\n");
 		}
 	}
-
 out:
 	free(fiemap);
 	return 0;
@@ -350,7 +365,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] fiemap: Refactor fiemap + implement range parameters

[Nikolay Borisov]

On 11.08.2017 14:55, Nikolay Borisov wrote:
> fiemap's code was rather complicated for the simple task it did so this
> patch aims to rectify the situation. The main changes are:
> 
> * Add a start_offset, len parameters, this allows to specify a range of the
> file for which we want information, if those parameters are omitted the old
> behavior is retained i.e. we get information for the whole file.
> 
> * We now always allocate as many struct fiemap_extent structs as necessary
> to hold the information for the passed range (or the whole file)
> 
> * Made max_extents, blocksize global variables. They are never modified apart
> from the initilization of the program.
> 
> * Eliminated the outer while loop, now that we allocate enough extent to
> perform a single fiemap ioctl call there is no need for it.
> 
> * As a result the -n parameter works correctly for -n 1
> 
> * Also changed the way the last hole is being calculated, so far every time
> the outter while loop finished the extenet from last_logical .. file size would
> be output as a hole. This was not always true
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> Hello, 
> 
> Here is more or less a major cleanup of the fiemap code. I've tested it with a 
> file with with the following layout HDHDHDHDHDHD (H - hole, D-data) and 
> varying permutation of the arguments and so far everthing seems to work as 
> expected. The biggest benefit I see is that the code is easier to follow now.

Ping


> 
> 
>  io/fiemap.c       | 187 +++++++++++++++++++++++++++++-------------------------
>  man/man8/xfs_io.8 |   5 +-
>  2 files changed, 104 insertions(+), 88 deletions(-)
> 
> diff --git a/io/fiemap.c b/io/fiemap.c
> index 75e882057362..5659f16d767d 100644
> --- a/io/fiemap.c
> +++ b/io/fiemap.c
> @@ -24,6 +24,8 @@
>  #include "io.h"
>  
>  static cmdinfo_t fiemap_cmd;
> +static const __u64 blocksize = 512;
> +static int max_extents = 0;
>  
>  static void
>  fiemap_help(void)
> @@ -34,6 +36,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"
> @@ -52,12 +55,10 @@ fiemap_help(void)
>  static void
>  print_verbose(
>  	struct fiemap_extent	*extent,
> -	int			blocksize,
>  	int			foff_w,
>  	int			boff_w,
>  	int			tot_w,
>  	int			flg_w,
> -	int			max_extents,
>  	int			*cur_extent,
>  	__u64			*last_logical)
>  {
> @@ -85,6 +86,7 @@ print_verbose(
>  			flg_w, _("FLAGS"));
>  	}
>  
> +
>  	if (lstart != llast) {
>  		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", llast,
>  			 lstart - 1ULL);
> @@ -94,7 +96,7 @@ print_verbose(
>  		memset(lbuf, 0, sizeof(lbuf));
>  	}
>  
> -	if ((*cur_extent + 1) == max_extents)
> +	if (max_extents && *cur_extent == max_extents)
>  		return;
>  
>  	snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", lstart,
> @@ -112,8 +114,6 @@ static void
>  print_plain(
>  	struct fiemap_extent	*extent,
>  	int			lflag,
> -	int			blocksize,
> -	int			max_extents,
>  	int			*cur_extent,
>  	__u64			*last_logical)
>  {
> @@ -137,7 +137,7 @@ print_plain(
>  		(*cur_extent)++;
>  	}
>  
> -	if ((*cur_extent + 1) == max_extents)
> +	if (max_extents && *cur_extent == max_extents)
>  		return;
>  
>  	printf("\t%d: [%llu..%llu]: %llu..%llu", *cur_extent,
> @@ -159,13 +159,12 @@ print_plain(
>  static void
>  calc_print_format(
>  	struct fiemap		*fiemap,
> -	__u64			blocksize,
>  	int			*foff_w,
>  	int			*boff_w,
>  	int			*tot_w,
>  	int			*flg_w)
>  {
> -	int 			i;
> +	int			i;
>  	char			lbuf[32];
>  	char			bbuf[32];
>  	__u64			logical;
> @@ -193,15 +192,28 @@ calc_print_format(
>  	}
>  }
>  
> +static ssize_t
> +get_extent_num(int fd, __u64 startoffset, __u64 len)
> +{
> +	struct fiemap fiemap = { 0 } ;
> +	fiemap.fm_start = startoffset;
> +	fiemap.fm_length = len;
> +	if (ioctl(fd, FS_IOC_FIEMAP, (unsigned long)&fiemap) < 0) {
> +		fprintf(stderr, _("%s: Failed to query fiemap extent count.\n"),
> +			progname);
> +		return -1;
> +	}
> +
> +	return fiemap.fm_mapped_extents;
> +}
> +
>  int
>  fiemap_f(
>  	int		argc,
>  	char		**argv)
>  {
>  	struct fiemap	*fiemap;
> -	int		max_extents = 0;
> -	int		num_extents = 32;
> -	int		last = 0;
> +	int		num_extents;
>  	int		lflag = 0;
>  	int		vflag = 0;
>  	int		fiemap_flags = FIEMAP_FLAG_SYNC;
> @@ -210,24 +222,29 @@ fiemap_f(
>  	int		map_size;
>  	int		ret;
>  	int		cur_extent = 0;
> -	int		foff_w = 16;	/* 16 just for a good minimum range */
> +	int		foff_w = 16; /* 16 just for a good minimum range */
>  	int		boff_w = 16;
>  	int		tot_w = 5;	/* 5 since its just one number */
>  	int		flg_w = 5;
> -	__u64		blocksize = 512;
>  	__u64		last_logical = 0;
> -	struct stat	st;
> +	__u64		len = -1LL;
> +	size_t		fsblocksize, fssectsize;
> +	off64_t		start_offset = 0;
> +	__u64		end_offset;
> +	__u64		llast;
> +	bool		last_extent = false;
> +
> +	init_cvtnum(&fsblocksize, &fssectsize);
>  
>  	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
>  		switch (c) {
>  		case 'a':
>  			fiemap_flags |= FIEMAP_FLAG_XATTR;
>  			break;
> -		case 'l':
> -			lflag = 1;
> -			break;
>  		case 'n':
>  			max_extents = atoi(optarg);
> +		case 'l':
> +			lflag = 1;
>  			break;
>  		case 'v':
>  			vflag++;
> @@ -237,8 +254,35 @@ fiemap_f(
>  		}
>  	}
>  
> -	if (max_extents)
> -		num_extents = min(num_extents, max_extents);
> +	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++;
> +	}
> +
> +	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;
> +	}
> +
> +
> +	end_offset = (start_offset + len) / blocksize;
> +
> +	ret = get_extent_num(file->fd, last_logical, len);
> +	if (ret < 0) {
> +		exitcode = 1;
> +		return 0;
> +	}
> +	num_extents = ret;
> +
>  	map_size = sizeof(struct fiemap) +
>  		(num_extents * sizeof(struct fiemap_extent));
>  	fiemap = malloc(map_size);
> @@ -251,92 +295,63 @@ fiemap_f(
>  
>  	printf("%s:\n", file->name);
>  
> -	while (!last && ((cur_extent + 1) != max_extents)) {
> -		if (max_extents)
> -			num_extents = min(num_extents,
> -					  max_extents - (cur_extent + 1));
> -
> -		memset(fiemap, 0, map_size);
> -		fiemap->fm_flags = fiemap_flags;
> -		fiemap->fm_start = last_logical;
> -		fiemap->fm_length = -1LL;
> -		fiemap->fm_extent_count = num_extents;
> -
> -		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
> -		if (ret < 0) {
> -			fprintf(stderr, "%s: ioctl(FS_IOC_FIEMAP) [\"%s\"]: "
> -				"%s\n", progname, file->name, strerror(errno));
> -			free(fiemap);
> -			exitcode = 1;
> -			return 0;
> -		}
> +	memset(fiemap, 0, map_size);
> +	fiemap->fm_flags = fiemap_flags;
> +	fiemap->fm_start = last_logical;
> +	fiemap->fm_length = len;
> +	fiemap->fm_extent_count = num_extents;
>  
> -		/* No more extents to map, exit */
> -		if (!fiemap->fm_mapped_extents)
> -			break;
> +	ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
> +	if (ret < 0) {
> +		fprintf(stderr, "%s: ioctl(FS_IOC_FIEMAP) [\"%s\"]: "
> +			"%s\n", progname, file->name, strerror(errno));
> +		free(fiemap);
> +		exitcode = 1;
> +		return 0;
> +	}
> +
> +	for (i = 0; i < fiemap->fm_mapped_extents; i++) {
> +		struct fiemap_extent	*extent;
>  
> -		for (i = 0; i < fiemap->fm_mapped_extents; i++) {
> -			struct fiemap_extent	*extent;
> -
> -			extent = &fiemap->fm_extents[i];
> -			if (vflag) {
> -				if (cur_extent == 0) {
> -					calc_print_format(fiemap, blocksize,
> -							  &foff_w, &boff_w,
> -							  &tot_w, &flg_w);
> -				}
> -
> -				print_verbose(extent, blocksize, foff_w,
> -					      boff_w, tot_w, flg_w,
> -					      max_extents, &cur_extent,
> -					      &last_logical);
> -			} else
> -				print_plain(extent, lflag, blocksize,
> -					    max_extents, &cur_extent,
> -					    &last_logical);
> -
> -			if (extent->fe_flags & FIEMAP_EXTENT_LAST) {
> -				last = 1;
> -				break;
> +		extent = &fiemap->fm_extents[i];
> +		if (vflag) {
> +			if (cur_extent == 0) {
> +				calc_print_format(fiemap, &foff_w, &boff_w,
> +						  &tot_w, &flg_w);
>  			}
>  
> -			if ((cur_extent + 1) == max_extents)
> -				break;
> -		}
> -	}
> +			print_verbose(extent, foff_w, boff_w, tot_w, flg_w,
> +				      &cur_extent, &last_logical);
> +		} else
> +			print_plain(extent, lflag, &cur_extent, &last_logical);
>  
> -	if ((cur_extent + 1) == max_extents)
> -		goto out;
> +		if (max_extents && cur_extent == max_extents)
> +			goto out;
> +
> +		if (extent->fe_flags & FIEMAP_EXTENT_LAST)
> +			last_extent = true;
>  
> -	memset(&st, 0, sizeof(st));
> -	if (fstat(file->fd, &st)) {
> -		fprintf(stderr, "%s: fstat failed: %s\n", progname,
> -			strerror(errno));
> -		free(fiemap);
> -		exitcode = 1;
> -		return 0;
>  	}
>  
> -	if (cur_extent && last_logical < st.st_size) {
> -		char	lbuf[32];
> +	llast = last_logical / blocksize;
> +	if (!last_extent && llast < end_offset) {
> +		char lbuf[32];
> +		__u64 difference = end_offset - llast;
> +		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", llast, llast + difference);
>  
> -		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:",
> -			 last_logical / blocksize, (st.st_size / blocksize) - 1);
>  		if (vflag) {
>  			printf("%4d: %-*s %-*s %*llu\n", cur_extent,
>  			       foff_w, lbuf, boff_w, _("hole"), tot_w,
> -			       (st.st_size - last_logical) / blocksize);
> +			       difference);
>  		} else {
>  			printf("\t%d: %s %s", cur_extent, lbuf,
>  			       _("hole"));
>  			if (lflag)
> -				printf(_(" %llu blocks\n"),
> -				       (st.st_size - last_logical) / blocksize);
> +				printf(_(" %llu blocks\n"), len / blocksize);
>  			else
>  				printf("\n");
>  		}
>  	}
> -
>  out:
>  	free(fiemap);
>  	return 0;
> @@ -350,7 +365,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
> 

Re: [PATCH] fiemap: Refactor fiemap + implement range parameters

[Eric Sandeen]

On 8/11/17 6:55 AM, Nikolay Borisov wrote:
> fiemap's code was rather complicated for the simple task it did so this
> patch aims to rectify the situation.

Thanks for looking into this.

Yes, it is complicated.  It's funny; e2fsprogs' filefrag is horribly
complex too.  What is it about extent mapping? ;)

> The main changes are:
> 
> * Add a start_offset, len parameters, this allows to specify a range of the
> file for which we want information, if those parameters are omitted the old
> behavior is retained i.e. we get information for the whole file.
> 
> * We now always allocate as many struct fiemap_extent structs as necessary
> to hold the information for the passed range (or the whole file)

Hm, what if a file has a huge number of extents?  It could, in theory
be hundreds of thousands, resulting in a multi-megabyte allocation.
Is that a good tradeoff?

Darrick reminds me that xfs_getbmap does:

        out = kmem_zalloc_large(bmv->bmv_count * sizeof(struct getbmapx), 0);
        if (!out)
                return -ENOMEM;

and until very recently, the FIEMAP ioctl went through xfs_getbmap, so I think
this introduces a new chance of failure on older kernels due to -ENOMEM.  For
that reason, I think it's probably necessary to do the queries in smaller,
chunks, I'm afraid.

> * Made max_extents, blocksize global variables. They are never modified apart
> from the initilization of the program.
> 
> * Eliminated the outer while loop, now that we allocate enough extent to
> perform a single fiemap ioctl call there is no need for it.
> 
> * As a result the -n parameter works correctly for -n 1
> 
> * Also changed the way the last hole is being calculated, so far every time
> the outter while loop finished the extenet from last_logical .. file size would
> be output as a hole. This was not always true

Ok, so that's a lot of changes all wrapped into one.  Could you please
resubmit this in more reviewable, bisectable chunks?

At a minimum, the change to add new arguments should be a patch
after any preparatory refactoring.  The change to preallocate the entire
extent array should stand on its own as well, but given my above concern,
I'm not sure we should make that change.  It seems like the
"last hole" change is a bugfix that should stand on its own too, yes?

Other comments below.

> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> Hello, 
> 
> Here is more or less a major cleanup of the fiemap code. I've tested it with a 
> file with with the following layout HDHDHDHDHDHD (H - hole, D-data) and 
> varying permutation of the arguments and so far everthing seems to work as 
> expected. The biggest benefit I see is that the code is easier to follow now.

Have you run it through the xfstests which test fiemap, specifically
generic/094 and generic/225?

(those two do seem to pass here, FWIW)

> 
> 
>  io/fiemap.c       | 187 +++++++++++++++++++++++++++++-------------------------
>  man/man8/xfs_io.8 |   5 +-
>  2 files changed, 104 insertions(+), 88 deletions(-)
> 
> diff --git a/io/fiemap.c b/io/fiemap.c
> index 75e882057362..5659f16d767d 100644
> --- a/io/fiemap.c
> +++ b/io/fiemap.c
> @@ -24,6 +24,8 @@
>  #include "io.h"
>  
>  static cmdinfo_t fiemap_cmd;
> +static const __u64 blocksize = 512;
> +static int max_extents = 0;
>  
>  static void
>  fiemap_help(void)
> @@ -34,6 +36,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"
> @@ -52,12 +55,10 @@ fiemap_help(void)
>  static void
>  print_verbose(
>  	struct fiemap_extent	*extent,
> -	int			blocksize,
>  	int			foff_w,
>  	int			boff_w,
>  	int			tot_w,
>  	int			flg_w,
> -	int			max_extents,
>  	int			*cur_extent,
>  	__u64			*last_logical)
>  {
> @@ -85,6 +86,7 @@ print_verbose(
>  			flg_w, _("FLAGS"));
>  	}
>  
> +

gratuitous newline ;)

>  	if (lstart != llast) {
>  		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", llast,
>  			 lstart - 1ULL);
> @@ -94,7 +96,7 @@ print_verbose(
>  		memset(lbuf, 0, sizeof(lbuf));
>  	}
>  
> -	if ((*cur_extent + 1) == max_extents)
> +	if (max_extents && *cur_extent == max_extents)

is this the "last hole" change?

>  		return;
>  
>  	snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", lstart,
> @@ -112,8 +114,6 @@ static void
>  print_plain(
>  	struct fiemap_extent	*extent,
>  	int			lflag,
> -	int			blocksize,
> -	int			max_extents,
>  	int			*cur_extent,
>  	__u64			*last_logical)
>  {
> @@ -137,7 +137,7 @@ print_plain(
>  		(*cur_extent)++;
>  	}
>  
> -	if ((*cur_extent + 1) == max_extents)
> +	if (max_extents && *cur_extent == max_extents)
>  		return;
>  
>  	printf("\t%d: [%llu..%llu]: %llu..%llu", *cur_extent,
> @@ -159,13 +159,12 @@ print_plain(
>  static void
>  calc_print_format(
>  	struct fiemap		*fiemap,
> -	__u64			blocksize,
>  	int			*foff_w,
>  	int			*boff_w,
>  	int			*tot_w,
>  	int			*flg_w)
>  {
> -	int 			i;
> +	int			i;
>  	char			lbuf[32];
>  	char			bbuf[32];
>  	__u64			logical;
> @@ -193,15 +192,28 @@ calc_print_format(
>  	}
>  }
>  
> +static ssize_t

isn't fm_mapped_extents a u32?

Is this ssize_t so that you can return the -1?  I suppose a comment about
it might be good.

> +get_extent_num(int fd, __u64 startoffset, __u64 len)

could you rename this get_extent_count()?  "num" sounds like
an ordinal (i.e. "this is extent number 12") vs a count
("This range contains 12 extents")

> +{
> +	struct fiemap fiemap = { 0 } ;
> +	fiemap.fm_start = startoffset;
> +	fiemap.fm_length = len;
> +	if (ioctl(fd, FS_IOC_FIEMAP, (unsigned long)&fiemap) < 0) {
> +		fprintf(stderr, _("%s: Failed to query fiemap extent count.\n"),
> +			progname);
> +		return -1;
> +	}
> +
> +	return fiemap.fm_mapped_extents;
> +}
> +
>  int
>  fiemap_f(
>  	int		argc,
>  	char		**argv)
>  {
>  	struct fiemap	*fiemap;
> -	int		max_extents = 0;
> -	int		num_extents = 32;
> -	int		last = 0;
> +	int		num_extents;
>  	int		lflag = 0;
>  	int		vflag = 0;
>  	int		fiemap_flags = FIEMAP_FLAG_SYNC;
> @@ -210,24 +222,29 @@ fiemap_f(
>  	int		map_size;
>  	int		ret;
>  	int		cur_extent = 0;
> -	int		foff_w = 16;	/* 16 just for a good minimum range */
> +	int		foff_w = 16; /* 16 just for a good minimum range */

Dumb nitpick, but I don't see a reason to remove the tab.  Lined
up comments look nice and the line was < 80chars... *shrug*

>  	int		boff_w = 16;
>  	int		tot_w = 5;	/* 5 since its just one number */
>  	int		flg_w = 5;
> -	__u64		blocksize = 512;
>  	__u64		last_logical = 0;
> -	struct stat	st;
> +	__u64		len = -1LL;
> +	size_t		fsblocksize, fssectsize;
> +	off64_t		start_offset = 0;
> +	__u64		end_offset;
> +	__u64		llast;
> +	bool		last_extent = false;
> +
> +	init_cvtnum(&fsblocksize, &fssectsize);

<this makes me want to make cvtnum blocksize & sector size globals, and 
run init_cvtnum only when we switch files in xfs_io, but that's
a patch for a different day> :)

>  	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
>  		switch (c) {
>  		case 'a':
>  			fiemap_flags |= FIEMAP_FLAG_XATTR;
>  			break;
> -		case 'l':
> -			lflag = 1;
> -			break;
>  		case 'n':
>  			max_extents = atoi(optarg);
> +		case 'l':
> +			lflag = 1;

Why reorder this?  Seems gratuitous.

>  			break;
>  		case 'v':
>  			vflag++;
> @@ -237,8 +254,35 @@ fiemap_f(
>  		}
>  	}
>  
> -	if (max_extents)
> -		num_extents = min(num_extents, max_extents);
> +	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;

Hm, why do we have both last_logical and start_offset at this point?
Is there a need to hold this value in 2 different variables?

(the print functions can modify last_logical, but I think we're done
with start_offset by then, right?)

> +		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;
> +	}
> +
> +
> +	end_offset = (start_offset + len) / blocksize;
> +
> +	ret = get_extent_num(file->fd, last_logical, len);
> +	if (ret < 0) {
> +		exitcode = 1;
> +		return 0;
> +	}
> +	num_extents = ret;
> +
>  	map_size = sizeof(struct fiemap) +
>  		(num_extents * sizeof(struct fiemap_extent));
>  	fiemap = malloc(map_size);
> @@ -251,92 +295,63 @@ fiemap_f(
>  
>  	printf("%s:\n", file->name);
>  
> -	while (!last && ((cur_extent + 1) != max_extents)) {
> -		if (max_extents)
> -			num_extents = min(num_extents,
> -					  max_extents - (cur_extent + 1));
> -
> -		memset(fiemap, 0, map_size);
> -		fiemap->fm_flags = fiemap_flags;
> -		fiemap->fm_start = last_logical;
> -		fiemap->fm_length = -1LL;
> -		fiemap->fm_extent_count = num_extents;
> -
> -		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
> -		if (ret < 0) {
> -			fprintf(stderr, "%s: ioctl(FS_IOC_FIEMAP) [\"%s\"]: "
> -				"%s\n", progname, file->name, strerror(errno));
> -			free(fiemap);
> -			exitcode = 1;
> -			return 0;
> -		}
> +	memset(fiemap, 0, map_size);
> +	fiemap->fm_flags = fiemap_flags;
> +	fiemap->fm_start = last_logical;
> +	fiemap->fm_length = len;
> +	fiemap->fm_extent_count = num_extents;
>  
> -		/* No more extents to map, exit */
> -		if (!fiemap->fm_mapped_extents)
> -			break;
> +	ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
> +	if (ret < 0) {
> +		fprintf(stderr, "%s: ioctl(FS_IOC_FIEMAP) [\"%s\"]: "
> +			"%s\n", progname, file->name, strerror(errno));
> +		free(fiemap);
> +		exitcode = 1;
> +		return 0;
> +	}
> +
> +	for (i = 0; i < fiemap->fm_mapped_extents; i++) {
> +		struct fiemap_extent	*extent;
>  
> -		for (i = 0; i < fiemap->fm_mapped_extents; i++) {
> -			struct fiemap_extent	*extent;
> -
> -			extent = &fiemap->fm_extents[i];
> -			if (vflag) {
> -				if (cur_extent == 0) {
> -					calc_print_format(fiemap, blocksize,
> -							  &foff_w, &boff_w,
> -							  &tot_w, &flg_w);
> -				}
> -
> -				print_verbose(extent, blocksize, foff_w,
> -					      boff_w, tot_w, flg_w,
> -					      max_extents, &cur_extent,
> -					      &last_logical);
> -			} else
> -				print_plain(extent, lflag, blocksize,
> -					    max_extents, &cur_extent,
> -					    &last_logical);
> -
> -			if (extent->fe_flags & FIEMAP_EXTENT_LAST) {
> -				last = 1;
> -				break;
> +		extent = &fiemap->fm_extents[i];
> +		if (vflag) {
> +			if (cur_extent == 0) {
> +				calc_print_format(fiemap, &foff_w, &boff_w,
> +						  &tot_w, &flg_w);
>  			}
>  
> -			if ((cur_extent + 1) == max_extents)
> -				break;
> -		}
> -	}
> +			print_verbose(extent, foff_w, boff_w, tot_w, flg_w,
> +				      &cur_extent, &last_logical);
> +		} else
> +			print_plain(extent, lflag, &cur_extent, &last_logical);
>  
> -	if ((cur_extent + 1) == max_extents)
> -		goto out;
> +		if (max_extents && cur_extent == max_extents)
> +			goto out;
> +
> +		if (extent->fe_flags & FIEMAP_EXTENT_LAST)
> +			last_extent = true;
>  
> -	memset(&st, 0, sizeof(st));
> -	if (fstat(file->fd, &st)) {
> -		fprintf(stderr, "%s: fstat failed: %s\n", progname,
> -			strerror(errno));
> -		free(fiemap);
> -		exitcode = 1;
> -		return 0;
>  	}
>  
> -	if (cur_extent && last_logical < st.st_size) {
> -		char	lbuf[32];
> +	llast = last_logical / blocksize;
> +	if (!last_extent && llast < end_offset) {
> +		char lbuf[32];
> +		__u64 difference = end_offset - llast;
> +		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", llast, llast + difference);
>  
> -		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:",
> -			 last_logical / blocksize, (st.st_size / blocksize) - 1);
>  		if (vflag) {
>  			printf("%4d: %-*s %-*s %*llu\n", cur_extent,
>  			       foff_w, lbuf, boff_w, _("hole"), tot_w,
> -			       (st.st_size - last_logical) / blocksize);
> +			       difference);
>  		} else {
>  			printf("\t%d: %s %s", cur_extent, lbuf,
>  			       _("hole"));
>  			if (lflag)
> -				printf(_(" %llu blocks\n"),
> -				       (st.st_size - last_logical) / blocksize);
> +				printf(_(" %llu blocks\n"), len / blocksize);
>  			else
>  				printf("\n");
>  		}
>  	}
> -
>  out:
>  	free(fiemap);
>  	return 0;
> @@ -350,7 +365,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]]");

should probably be "start_offset" so it doesn't look like 2 args.
Or maybe better, just "offset" to match the manpage.

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

Perhaps mention that offset & len can take the standard unit suffixes.

-Eric

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

Re: [PATCH] fiemap: Refactor fiemap + implement range parameters

[Nikolay Borisov]

On 22.08.2017 22:11, Eric Sandeen wrote:
> On 8/11/17 6:55 AM, Nikolay Borisov wrote:
>> fiemap's code was rather complicated for the simple task it did so this
>> patch aims to rectify the situation.
> 
> Thanks for looking into this.
> 
> Yes, it is complicated.  It's funny; e2fsprogs' filefrag is horribly
> complex too.  What is it about extent mapping? ;)
> 
>> The main changes are:
>>
>> * Add a start_offset, len parameters, this allows to specify a range of the
>> file for which we want information, if those parameters are omitted the old
>> behavior is retained i.e. we get information for the whole file.
>>
>> * We now always allocate as many struct fiemap_extent structs as necessary
>> to hold the information for the passed range (or the whole file)
> 
> Hm, what if a file has a huge number of extents?  It could, in theory
> be hundreds of thousands, resulting in a multi-megabyte allocation.
> Is that a good tradeoff?
> 
> Darrick reminds me that xfs_getbmap does:
> 
>         out = kmem_zalloc_large(bmv->bmv_count * sizeof(struct getbmapx), 0);
>         if (!out)
>                 return -ENOMEM;
> 
> and until very recently, the FIEMAP ioctl went through xfs_getbmap, so I think
> this introduces a new chance of failure on older kernels due to -ENOMEM.  For
> that reason, I think it's probably necessary to do the queries in smaller,
> chunks, I'm afraid.


Okay I will do it in batches then.

> 
>> * Made max_extents, blocksize global variables. They are never modified apart
>> from the initilization of the program.
>>
>> * Eliminated the outer while loop, now that we allocate enough extent to
>> perform a single fiemap ioctl call there is no need for it.
>>
>> * As a result the -n parameter works correctly for -n 1
>>
>> * Also changed the way the last hole is being calculated, so far every time
>> the outter while loop finished the extenet from last_logical .. file size would
>> be output as a hole. This was not always true
> 
> Ok, so that's a lot of changes all wrapped into one.  Could you please
> resubmit this in more reviewable, bisectable chunks?
> 
> At a minimum, the change to add new arguments should be a patch
> after any preparatory refactoring.  The change to preallocate the entire
> extent array should stand on its own as well, but given my above concern,
> I'm not sure we should make that change.  It seems like the
> "last hole" change is a bugfix that should stand on its own too, yes?

Sure. Actually now that I think more about the last_hole. It does make
sense in the case where we don't have the range arguments. Because what
happens is that we print all the extents of the file and anything which
goes beyond the last extents is supposed to be a hole. However, with the
introduction of the range parameter this no longer holds. I will pay
attention to this detail when splitting up the patches.

> 
> Other comments below.
> 
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>
>> Hello, 
>>
>> Here is more or less a major cleanup of the fiemap code. I've tested it with a 
>> file with with the following layout HDHDHDHDHDHD (H - hole, D-data) and 
>> varying permutation of the arguments and so far everthing seems to work as 
>> expected. The biggest benefit I see is that the code is easier to follow now.
> 
> Have you run it through the xfstests which test fiemap, specifically
> generic/094 and generic/225?
> 
> (those two do seem to pass here, FWIW)
> 
>>
>>
>>  io/fiemap.c       | 187 +++++++++++++++++++++++++++++-------------------------
>>  man/man8/xfs_io.8 |   5 +-
>>  2 files changed, 104 insertions(+), 88 deletions(-)
>>
>> diff --git a/io/fiemap.c b/io/fiemap.c
>> index 75e882057362..5659f16d767d 100644
>> --- a/io/fiemap.c
>> +++ b/io/fiemap.c
>> @@ -24,6 +24,8 @@
>>  #include "io.h"
>>  
>>  static cmdinfo_t fiemap_cmd;
>> +static const __u64 blocksize = 512;
>> +static int max_extents = 0;
>>  
>>  static void
>>  fiemap_help(void)
>> @@ -34,6 +36,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"
>> @@ -52,12 +55,10 @@ fiemap_help(void)
>>  static void
>>  print_verbose(
>>  	struct fiemap_extent	*extent,
>> -	int			blocksize,
>>  	int			foff_w,
>>  	int			boff_w,
>>  	int			tot_w,
>>  	int			flg_w,
>> -	int			max_extents,
>>  	int			*cur_extent,
>>  	__u64			*last_logical)
>>  {
>> @@ -85,6 +86,7 @@ print_verbose(
>>  			flg_w, _("FLAGS"));
>>  	}
>>  
>> +
> 
> gratuitous newline ;)
> 
>>  	if (lstart != llast) {
>>  		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", llast,
>>  			 lstart - 1ULL);
>> @@ -94,7 +96,7 @@ print_verbose(
>>  		memset(lbuf, 0, sizeof(lbuf));
>>  	}
>>  
>> -	if ((*cur_extent + 1) == max_extents)
>> +	if (max_extents && *cur_extent == max_extents)
> 
> is this the "last hole" change?

So this is not the last hole, but rather the  -n1 actually working. So
it's a fix for the limit argument.

> 
>>  		return;
>>  
>>  	snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", lstart,
>> @@ -112,8 +114,6 @@ static void
>>  print_plain(
>>  	struct fiemap_extent	*extent,
>>  	int			lflag,
>> -	int			blocksize,
>> -	int			max_extents,
>>  	int			*cur_extent,
>>  	__u64			*last_logical)
>>  {
>> @@ -137,7 +137,7 @@ print_plain(
>>  		(*cur_extent)++;
>>  	}
>>  
>> -	if ((*cur_extent + 1) == max_extents)
>> +	if (max_extents && *cur_extent == max_extents)
>>  		return;
>>  
>>  	printf("\t%d: [%llu..%llu]: %llu..%llu", *cur_extent,
>> @@ -159,13 +159,12 @@ print_plain(
>>  static void
>>  calc_print_format(
>>  	struct fiemap		*fiemap,
>> -	__u64			blocksize,
>>  	int			*foff_w,
>>  	int			*boff_w,
>>  	int			*tot_w,
>>  	int			*flg_w)
>>  {
>> -	int 			i;
>> +	int			i;
>>  	char			lbuf[32];
>>  	char			bbuf[32];
>>  	__u64			logical;
>> @@ -193,15 +192,28 @@ calc_print_format(
>>  	}
>>  }
>>  
>> +static ssize_t
> 
> isn't fm_mapped_extents a u32?
> 
> Is this ssize_t so that you can return the -1?  I suppose a comment about
> it might be good.

I will add a comment.

> 
>> +get_extent_num(int fd, __u64 startoffset, __u64 len)
> 
> could you rename this get_extent_count()?  "num" sounds like
> an ordinal (i.e. "this is extent number 12") vs a count
> ("This range contains 12 extents")
> 
>> +{
>> +	struct fiemap fiemap = { 0 } ;
>> +	fiemap.fm_start = startoffset;
>> +	fiemap.fm_length = len;
>> +	if (ioctl(fd, FS_IOC_FIEMAP, (unsigned long)&fiemap) < 0) {
>> +		fprintf(stderr, _("%s: Failed to query fiemap extent count.\n"),
>> +			progname);
>> +		return -1;
>> +	}
>> +
>> +	return fiemap.fm_mapped_extents;
>> +}
>> +
>>  int
>>  fiemap_f(
>>  	int		argc,
>>  	char		**argv)
>>  {
>>  	struct fiemap	*fiemap;
>> -	int		max_extents = 0;
>> -	int		num_extents = 32;
>> -	int		last = 0;
>> +	int		num_extents;
>>  	int		lflag = 0;
>>  	int		vflag = 0;
>>  	int		fiemap_flags = FIEMAP_FLAG_SYNC;
>> @@ -210,24 +222,29 @@ fiemap_f(
>>  	int		map_size;
>>  	int		ret;
>>  	int		cur_extent = 0;
>> -	int		foff_w = 16;	/* 16 just for a good minimum range */
>> +	int		foff_w = 16; /* 16 just for a good minimum range */
> 
> Dumb nitpick, but I don't see a reason to remove the tab.  Lined
> up comments look nice and the line was < 80chars... *shrug*

That's some sort of braino, will be gone in next repost.

> 
>>  	int		boff_w = 16;
>>  	int		tot_w = 5;	/* 5 since its just one number */
>>  	int		flg_w = 5;
>> -	__u64		blocksize = 512;
>>  	__u64		last_logical = 0;
>> -	struct stat	st;
>> +	__u64		len = -1LL;
>> +	size_t		fsblocksize, fssectsize;
>> +	off64_t		start_offset = 0;
>> +	__u64		end_offset;
>> +	__u64		llast;
>> +	bool		last_extent = false;
>> +
>> +	init_cvtnum(&fsblocksize, &fssectsize);
> 
> <this makes me want to make cvtnum blocksize & sector size globals, and 
> run init_cvtnum only when we switch files in xfs_io, but that's
> a patch for a different day> :)
> 
>>  	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
>>  		switch (c) {
>>  		case 'a':
>>  			fiemap_flags |= FIEMAP_FLAG_XATTR;
>>  			break;
>> -		case 'l':
>> -			lflag = 1;
>> -			break;
>>  		case 'n':
>>  			max_extents = atoi(optarg);
>> +		case 'l':
>> +			lflag = 1;
> 
> Why reorder this?  Seems gratuitous.

Mistake on my part.

> 
>>  			break;
>>  		case 'v':
>>  			vflag++;
>> @@ -237,8 +254,35 @@ fiemap_f(
>>  		}
>>  	}
>>  
>> -	if (max_extents)
>> -		num_extents = min(num_extents, max_extents);
>> +	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;
> 
> Hm, why do we have both last_logical and start_offset at this point?
> Is there a need to hold this value in 2 different variables?
> 
> (the print functions can modify last_logical, but I think we're done
> with start_offset by then, right?)

Seems you are right.

> 
>> +		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;
>> +	}
>> +
>> +
>> +	end_offset = (start_offset + len) / blocksize;
>> +
>> +	ret = get_extent_num(file->fd, last_logical, len);
>> +	if (ret < 0) {
>> +		exitcode = 1;
>> +		return 0;
>> +	}
>> +	num_extents = ret;
>> +
>>  	map_size = sizeof(struct fiemap) +
>>  		(num_extents * sizeof(struct fiemap_extent));
>>  	fiemap = malloc(map_size);
>> @@ -251,92 +295,63 @@ fiemap_f(
>>  
>>  	printf("%s:\n", file->name);
>>  
>> -	while (!last && ((cur_extent + 1) != max_extents)) {
>> -		if (max_extents)
>> -			num_extents = min(num_extents,
>> -					  max_extents - (cur_extent + 1));
>> -
>> -		memset(fiemap, 0, map_size);
>> -		fiemap->fm_flags = fiemap_flags;
>> -		fiemap->fm_start = last_logical;
>> -		fiemap->fm_length = -1LL;
>> -		fiemap->fm_extent_count = num_extents;
>> -
>> -		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
>> -		if (ret < 0) {
>> -			fprintf(stderr, "%s: ioctl(FS_IOC_FIEMAP) [\"%s\"]: "
>> -				"%s\n", progname, file->name, strerror(errno));
>> -			free(fiemap);
>> -			exitcode = 1;
>> -			return 0;
>> -		}
>> +	memset(fiemap, 0, map_size);
>> +	fiemap->fm_flags = fiemap_flags;
>> +	fiemap->fm_start = last_logical;
>> +	fiemap->fm_length = len;
>> +	fiemap->fm_extent_count = num_extents;
>>  
>> -		/* No more extents to map, exit */
>> -		if (!fiemap->fm_mapped_extents)
>> -			break;
>> +	ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
>> +	if (ret < 0) {
>> +		fprintf(stderr, "%s: ioctl(FS_IOC_FIEMAP) [\"%s\"]: "
>> +			"%s\n", progname, file->name, strerror(errno));
>> +		free(fiemap);
>> +		exitcode = 1;
>> +		return 0;
>> +	}
>> +
>> +	for (i = 0; i < fiemap->fm_mapped_extents; i++) {
>> +		struct fiemap_extent	*extent;
>>  
>> -		for (i = 0; i < fiemap->fm_mapped_extents; i++) {
>> -			struct fiemap_extent	*extent;
>> -
>> -			extent = &fiemap->fm_extents[i];
>> -			if (vflag) {
>> -				if (cur_extent == 0) {
>> -					calc_print_format(fiemap, blocksize,
>> -							  &foff_w, &boff_w,
>> -							  &tot_w, &flg_w);
>> -				}
>> -
>> -				print_verbose(extent, blocksize, foff_w,
>> -					      boff_w, tot_w, flg_w,
>> -					      max_extents, &cur_extent,
>> -					      &last_logical);
>> -			} else
>> -				print_plain(extent, lflag, blocksize,
>> -					    max_extents, &cur_extent,
>> -					    &last_logical);
>> -
>> -			if (extent->fe_flags & FIEMAP_EXTENT_LAST) {
>> -				last = 1;
>> -				break;
>> +		extent = &fiemap->fm_extents[i];
>> +		if (vflag) {
>> +			if (cur_extent == 0) {
>> +				calc_print_format(fiemap, &foff_w, &boff_w,
>> +						  &tot_w, &flg_w);
>>  			}
>>  
>> -			if ((cur_extent + 1) == max_extents)
>> -				break;
>> -		}
>> -	}
>> +			print_verbose(extent, foff_w, boff_w, tot_w, flg_w,
>> +				      &cur_extent, &last_logical);
>> +		} else
>> +			print_plain(extent, lflag, &cur_extent, &last_logical);
>>  
>> -	if ((cur_extent + 1) == max_extents)
>> -		goto out;
>> +		if (max_extents && cur_extent == max_extents)
>> +			goto out;
>> +
>> +		if (extent->fe_flags & FIEMAP_EXTENT_LAST)
>> +			last_extent = true;
>>  
>> -	memset(&st, 0, sizeof(st));
>> -	if (fstat(file->fd, &st)) {
>> -		fprintf(stderr, "%s: fstat failed: %s\n", progname,
>> -			strerror(errno));
>> -		free(fiemap);
>> -		exitcode = 1;
>> -		return 0;
>>  	}
>>  
>> -	if (cur_extent && last_logical < st.st_size) {
>> -		char	lbuf[32];
>> +	llast = last_logical / blocksize;
>> +	if (!last_extent && llast < end_offset) {
>> +		char lbuf[32];
>> +		__u64 difference = end_offset - llast;
>> +		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", llast, llast + difference);
>>  
>> -		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:",
>> -			 last_logical / blocksize, (st.st_size / blocksize) - 1);
>>  		if (vflag) {
>>  			printf("%4d: %-*s %-*s %*llu\n", cur_extent,
>>  			       foff_w, lbuf, boff_w, _("hole"), tot_w,
>> -			       (st.st_size - last_logical) / blocksize);
>> +			       difference);
>>  		} else {
>>  			printf("\t%d: %s %s", cur_extent, lbuf,
>>  			       _("hole"));
>>  			if (lflag)
>> -				printf(_(" %llu blocks\n"),
>> -				       (st.st_size - last_logical) / blocksize);
>> +				printf(_(" %llu blocks\n"), len / blocksize);
>>  			else
>>  				printf("\n");
>>  		}
>>  	}
>> -
>>  out:
>>  	free(fiemap);
>>  	return 0;
>> @@ -350,7 +365,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]]");
> 
> should probably be "start_offset" so it doesn't look like 2 args.
> Or maybe better, just "offset" to match the manpage.

Ack

> 
>>  	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.
> 
> Perhaps mention that offset & len can take the standard unit suffixes.

Ack
> 
> -Eric
> 
>>  .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
>>
> 


Thanks for the review, I will be incorporating it and reposting.

[PATCH 1/4] fiemap: Move global variables out of function scope

[Nikolay Borisov]

Move the blocksize and max_extent variables to the top of the file since they
are globals. Also blocksize never changes to mark it const. Also stop passing
max_extents around and refer to it directly.

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

diff --git a/io/fiemap.c b/io/fiemap.c
index 75e882057362..d1584aba7818 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -24,6 +24,8 @@
 #include "io.h"
 
 static cmdinfo_t fiemap_cmd;
+static const __u64 blocksize = 512;
+static int max_extents = 0;
 
 static void
 fiemap_help(void)
@@ -57,7 +59,6 @@ print_verbose(
 	int			boff_w,
 	int			tot_w,
 	int			flg_w,
-	int			max_extents,
 	int			*cur_extent,
 	__u64			*last_logical)
 {
@@ -113,7 +114,6 @@ print_plain(
 	struct fiemap_extent	*extent,
 	int			lflag,
 	int			blocksize,
-	int			max_extents,
 	int			*cur_extent,
 	__u64			*last_logical)
 {
@@ -165,7 +165,7 @@ calc_print_format(
 	int			*tot_w,
 	int			*flg_w)
 {
-	int 			i;
+	int			i;
 	char			lbuf[32];
 	char			bbuf[32];
 	__u64			logical;
@@ -199,7 +199,6 @@ fiemap_f(
 	char		**argv)
 {
 	struct fiemap	*fiemap;
-	int		max_extents = 0;
 	int		num_extents = 32;
 	int		last = 0;
 	int		lflag = 0;
@@ -214,7 +213,6 @@ fiemap_f(
 	int		boff_w = 16;
 	int		tot_w = 5;	/* 5 since its just one number */
 	int		flg_w = 5;
-	__u64		blocksize = 512;
 	__u64		last_logical = 0;
 	struct stat	st;
 
@@ -288,12 +286,10 @@ fiemap_f(
 
 				print_verbose(extent, blocksize, foff_w,
 					      boff_w, tot_w, flg_w,
-					      max_extents, &cur_extent,
-					      &last_logical);
+					      &cur_extent, &last_logical);
 			} else
 				print_plain(extent, lflag, blocksize,
-					    max_extents, &cur_extent,
-					    &last_logical);
+					    &cur_extent, &last_logical);
 
 			if (extent->fe_flags & FIEMAP_EXTENT_LAST) {
 				last = 1;
-- 
2.7.4

[PATCH 2/4] fiemap: Introduce get_extent_count

[Nikolay Borisov]

This function will be used in future patches and will aid in implementing
ranged fiemap queries

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

diff --git a/io/fiemap.c b/io/fiemap.c
index d1584aba7818..30c49112e089 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -193,6 +193,22 @@ calc_print_format(
 	}
 }
 
+/* Use ssize so that we can return also an error code in case the ioctl fails */
+static ssize_t
+get_extent_count(int fd, __u64 startoffset, __u64 len)
+{
+       struct fiemap fiemap = { 0 } ;
+       fiemap.fm_start = startoffset;
+       fiemap.fm_length = len;
+       if (ioctl(fd, FS_IOC_FIEMAP, (unsigned long)&fiemap) < 0) {
+               fprintf(stderr, _("%s: Failed to query fiemap extent count.\n"),
+                       progname);
+               return -1;
+       }
+
+       return fiemap.fm_mapped_extents;
+}
+
 int
 fiemap_f(
 	int		argc,
-- 
2.7.4

Re: [PATCH 2/4] fiemap: Introduce get_extent_count

[Darrick J. Wong]

On Wed, Aug 23, 2017 at 06:11:20PM +0300, Nikolay Borisov wrote:
> This function will be used in future patches and will aid in implementing
> ranged fiemap queries
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  io/fiemap.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/io/fiemap.c b/io/fiemap.c
> index d1584aba7818..30c49112e089 100644
> --- a/io/fiemap.c
> +++ b/io/fiemap.c
> @@ -193,6 +193,22 @@ calc_print_format(
>  	}
>  }
>  
> +/* Use ssize so that we can return also an error code in case the ioctl fails */
> +static ssize_t
> +get_extent_count(int fd, __u64 startoffset, __u64 len)

Argument indentation -- this should be:

static ssize_t
get_extent_count(
	int		fd
	__u64		startoffset,
	__u64		len)
{

to match the rest of the file.

> +{
> +       struct fiemap fiemap = { 0 } ;

Needs space between variable declaration and code.

> +       fiemap.fm_start = startoffset;
> +       fiemap.fm_length = len;
> +       if (ioctl(fd, FS_IOC_FIEMAP, (unsigned long)&fiemap) < 0) {
> +               fprintf(stderr, _("%s: Failed to query fiemap extent count.\n"),
> +                       progname);

perror/strerror to print the exact error code?

--D

> +               return -1;
> +       }
> +
> +       return fiemap.fm_mapped_extents;
> +}
> +
>  int
>  fiemap_f(
>  	int		argc,
> -- 
> 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

[PATCH 3/4] fiemap: Simplify internals of fiemap_f.

[Nikolay Borisov]

So far fiemap used some rather convoluted logic to terminate the iteration and
calculate the number of extents to pass to fm_extent_count. Simplify this by:

* Get the whole number of extents that this file has and keep iterating until
we've printed all extents

* Always use a hard-coded batch of 32 extents so we don't have to worry about
any extra calculations

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

diff --git a/io/fiemap.c b/io/fiemap.c
index 30c49112e089..ef54b265ab91 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -23,6 +23,8 @@
 #include "init.h"
 #include "io.h"
 
+#define EXTENT_BATCH 32
+
 static cmdinfo_t fiemap_cmd;
 static const __u64 blocksize = 512;
 static int max_extents = 0;
@@ -95,7 +97,7 @@ print_verbose(
 		memset(lbuf, 0, sizeof(lbuf));
 	}
 
-	if ((*cur_extent + 1) == max_extents)
+	if (*cur_extent == max_extents)
 		return;
 
 	snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", lstart,
@@ -137,7 +139,7 @@ print_plain(
 		(*cur_extent)++;
 	}
 
-	if ((*cur_extent + 1) == max_extents)
+	if (*cur_extent == max_extents)
 		return;
 
 	printf("\t%d: [%llu..%llu]: %llu..%llu", *cur_extent,
@@ -215,7 +217,7 @@ fiemap_f(
 	char		**argv)
 {
 	struct fiemap	*fiemap;
-	int		num_extents = 32;
+	int		num_extents;
 	int		last = 0;
 	int		lflag = 0;
 	int		vflag = 0;
@@ -251,10 +253,15 @@ fiemap_f(
 		}
 	}
 
-	if (max_extents)
-		num_extents = min(num_extents, max_extents);
+	ret = get_extent_count(file->fd, last_logical, -1);
+	if (ret < 0) {
+		exitcode = 1;
+		return 0;
+	}
+	num_extents = ret;
+
 	map_size = sizeof(struct fiemap) +
-		(num_extents * sizeof(struct fiemap_extent));
+		(EXTENT_BATCH * sizeof(struct fiemap_extent));
 	fiemap = malloc(map_size);
 	if (!fiemap) {
 		fprintf(stderr, _("%s: malloc of %d bytes failed.\n"),
@@ -265,16 +272,14 @@ fiemap_f(
 
 	printf("%s:\n", file->name);
 
-	while (!last && ((cur_extent + 1) != max_extents)) {
-		if (max_extents)
-			num_extents = min(num_extents,
-					  max_extents - (cur_extent + 1));
+	while (!last && num_extents) {
 
+		/* Query a batch worth of extents */
 		memset(fiemap, 0, map_size);
 		fiemap->fm_flags = fiemap_flags;
 		fiemap->fm_start = last_logical;
 		fiemap->fm_length = -1LL;
-		fiemap->fm_extent_count = num_extents;
+		fiemap->fm_extent_count = EXTENT_BATCH;
 
 		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
 		if (ret < 0) {
@@ -307,17 +312,17 @@ fiemap_f(
 				print_plain(extent, lflag, blocksize,
 					    &cur_extent, &last_logical);
 
-			if (extent->fe_flags & FIEMAP_EXTENT_LAST) {
+			if (extent->fe_flags & FIEMAP_EXTENT_LAST ||
+			    cur_extent == max_extents) {
 				last = 1;
 				break;
 			}
-
-			if ((cur_extent + 1) == max_extents)
-				break;
 		}
+
+		num_extents -= fiemap->fm_mapped_extents;
 	}
 
-	if ((cur_extent + 1) == max_extents)
+	if (cur_extent == max_extents)
 		goto out;
 
 	memset(&st, 0, sizeof(st));
-- 
2.7.4

Re: [PATCH 3/4] fiemap: Simplify internals of fiemap_f.

[Eric Sandeen]

On 8/23/17 10:11 AM, Nikolay Borisov wrote:
> So far fiemap used some rather convoluted logic to terminate the iteration and
> calculate the number of extents to pass to fm_extent_count. Simplify this by:
> 
> * Get the whole number of extents that this file has and keep iterating until
> we've printed all extents
> 
> * Always use a hard-coded batch of 32 extents so we don't have to worry about
> any extra calculations

As discussed on IRC, these types of changes:

> -	if ((*cur_extent + 1) == max_extents)
> +	if (*cur_extent == max_extents)
>  		return;

are a functional change not described in the changelog above; they should be
in their own patch with their own changelog, explaining why the test was off by one,
and what this fixes.  This stuff is complex enough that it's not obvious to the
casual reader now, and certainly won't be a few years later.  ;)

Thanks,
-Eric

 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  io/fiemap.c | 37 +++++++++++++++++++++----------------
>  1 file changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/io/fiemap.c b/io/fiemap.c
> index 30c49112e089..ef54b265ab91 100644
> --- a/io/fiemap.c
> +++ b/io/fiemap.c
> @@ -23,6 +23,8 @@
>  #include "init.h"
>  #include "io.h"
>  
> +#define EXTENT_BATCH 32
> +
>  static cmdinfo_t fiemap_cmd;
>  static const __u64 blocksize = 512;
>  static int max_extents = 0;
> @@ -95,7 +97,7 @@ print_verbose(
>  		memset(lbuf, 0, sizeof(lbuf));
>  	}
>  
> -	if ((*cur_extent + 1) == max_extents)
> +	if (*cur_extent == max_extents)
>  		return;
>  
>  	snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", lstart,
> @@ -137,7 +139,7 @@ print_plain(
>  		(*cur_extent)++;
>  	}
>  
> -	if ((*cur_extent + 1) == max_extents)
> +	if (*cur_extent == max_extents)
>  		return;
>  
>  	printf("\t%d: [%llu..%llu]: %llu..%llu", *cur_extent,
> @@ -215,7 +217,7 @@ fiemap_f(
>  	char		**argv)
>  {
>  	struct fiemap	*fiemap;
> -	int		num_extents = 32;
> +	int		num_extents;
>  	int		last = 0;
>  	int		lflag = 0;
>  	int		vflag = 0;
> @@ -251,10 +253,15 @@ fiemap_f(
>  		}
>  	}
>  
> -	if (max_extents)
> -		num_extents = min(num_extents, max_extents);
> +	ret = get_extent_count(file->fd, last_logical, -1);
> +	if (ret < 0) {
> +		exitcode = 1;
> +		return 0;
> +	}
> +	num_extents = ret;
> +
>  	map_size = sizeof(struct fiemap) +
> -		(num_extents * sizeof(struct fiemap_extent));
> +		(EXTENT_BATCH * sizeof(struct fiemap_extent));
>  	fiemap = malloc(map_size);
>  	if (!fiemap) {
>  		fprintf(stderr, _("%s: malloc of %d bytes failed.\n"),
> @@ -265,16 +272,14 @@ fiemap_f(
>  
>  	printf("%s:\n", file->name);
>  
> -	while (!last && ((cur_extent + 1) != max_extents)) {
> -		if (max_extents)
> -			num_extents = min(num_extents,
> -					  max_extents - (cur_extent + 1));
> +	while (!last && num_extents) {
>  
> +		/* Query a batch worth of extents */
>  		memset(fiemap, 0, map_size);
>  		fiemap->fm_flags = fiemap_flags;
>  		fiemap->fm_start = last_logical;
>  		fiemap->fm_length = -1LL;
> -		fiemap->fm_extent_count = num_extents;
> +		fiemap->fm_extent_count = EXTENT_BATCH;
>  
>  		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
>  		if (ret < 0) {
> @@ -307,17 +312,17 @@ fiemap_f(
>  				print_plain(extent, lflag, blocksize,
>  					    &cur_extent, &last_logical);
>  
> -			if (extent->fe_flags & FIEMAP_EXTENT_LAST) {
> +			if (extent->fe_flags & FIEMAP_EXTENT_LAST ||
> +			    cur_extent == max_extents) {
>  				last = 1;
>  				break;
>  			}
> -
> -			if ((cur_extent + 1) == max_extents)
> -				break;
>  		}
> +
> +		num_extents -= fiemap->fm_mapped_extents;
>  	}
>  
> -	if ((cur_extent + 1) == max_extents)
> +	if (cur_extent == max_extents)
>  		goto out;
>  
>  	memset(&st, 0, sizeof(st));
> 

Re: [PATCH 3/4] fiemap: Simplify internals of fiemap_f.

[Eric Sandeen]

On 8/23/17 10:51 AM, Eric Sandeen wrote:
> On 8/23/17 10:11 AM, Nikolay Borisov wrote:
>> So far fiemap used some rather convoluted logic to terminate the iteration and
>> calculate the number of extents to pass to fm_extent_count. Simplify this by:
>>
>> * Get the whole number of extents that this file has and keep iterating until
>> we've printed all extents
>>
>> * Always use a hard-coded batch of 32 extents so we don't have to worry about
>> any extra calculations
> 
> As discussed on IRC, these types of changes:
> 
>> -	if ((*cur_extent + 1) == max_extents)
>> +	if (*cur_extent == max_extents)
>>  		return;
> 
> are a functional change not described in the changelog above; they should be
> in their own patch with their own changelog, explaining why the test was off by one,
> and what this fixes.  This stuff is complex enough that it's not obvious to the
> casual reader now, and certainly won't be a few years later.  ;)

Ok, so this is fixing the "-n" argument, to actually print $ARG extents
instead of $ARG - 1 extents.

instead of today:

# xfs_io -c "fiemap -n 4" holefile
holefile:
	0: [0..7]: hole
	1: [8..15]: 172560120..172560127
	2: [16..23]: hole

now it does more expected:

# io/xfs_io -c "fiemap -n 4" holefile
holefile:
	0: [0..7]: hole
	1: [8..15]: 172560120..172560127
	2: [16..23]: hole
	3: [24..31]: 172560136..172560143

so it probably just needs a doc fix along w/ this behavior fix.

(note, bmap behaves the same way, except properly):

# xfs_io -c "bmap -n 4" holefile
holefile:
	0: [0..7]: hole
	1: [8..15]: 172560120..172560127
	2: [16..23]: hole
	3: [24..31]: 172560136..172560143

Thanks,
-Eric

 
> Thanks,
> -Eric
>

[PATCH 4/4] fiemap: Add support for ranged query

[Nikolay Borisov]

Introduce two optional arguments which can be used to perform fiemap queries
for a particular range in a file. Those are 'offset' and 'length' they can be
used like so:

xfs_io -c "fiemap 0 12k" - query for extents covering the first 12kb of the
target file.

Now that such queries are supposed also modify the logic for printing the last
hole to only cover the range which is asked. So if we ask for 0-10kb and the
range 8k-12k is actually a whole, then limit the last whole only to this range:

So for a file which has the following contents :

|-----hole-------|-------data--------|-----hole-----|
0                8k                  12k            16k

The output would be:

xfs_io -c "fiemap -v 0 13k" test-dir/fragmented-file
test-dir/fragmented-file:
 EXT: FILE-OFFSET      BLOCK-RANGE          TOTAL FLAGS
   0: [0..15]:         hole                    16
   1: [16..23]:        897847296..897847303     8   0x0
   2: [24..25]:        hole                     2

Furthermore in cases where the queried range is covered by a whole then the
existing while() loop would have never executed, due to num_exents = 0. Fix this
by converting it to a do {} while ()

_

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 io/fiemap.c       | 75 +++++++++++++++++++++++++++++++++++++++----------------
 man/man8/xfs_io.8 |  6 +++--
 2 files changed, 57 insertions(+), 24 deletions(-)

diff --git a/io/fiemap.c b/io/fiemap.c
index ef54b265ab91..2e03a81dc57a 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -27,7 +27,7 @@
 
 static cmdinfo_t fiemap_cmd;
 static const __u64 blocksize = 512;
-static int max_extents = 0;
+static int max_extents = -1;
 
 static void
 fiemap_help(void)
@@ -38,6 +38,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"
@@ -231,9 +232,14 @@ 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		start_offset = 0, last_logical = 0;
+	__u64		len = -1;
+	__u64		end_offset = 0, llast;
+	size_t		fsblocksize, fssectsize;
 	struct stat	st;
 
+	init_cvtnum(&fsblocksize, &fssectsize);
+
 	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
 		switch (c) {
 		case 'a':
@@ -253,7 +259,41 @@ fiemap_f(
 		}
 	}
 
-	ret = get_extent_count(file->fd, last_logical, -1);
+
+	if (optind < argc) {
+	        off64_t start = cvtnum(fsblocksize, fssectsize, argv[optind]);
+	        if (start_offset < 0) {
+	                printf("non-numeric offset argument -- %s\n", argv[optind]);
+			exitcode = 1;
+	                return 0;
+	        }
+	        last_logical = start_offset = start;
+	        optind++;
+	}
+
+	if (optind < argc) {
+	        off64_t length = cvtnum(fsblocksize, fssectsize, argv[optind]);
+	        if (length < 0) {
+	                printf("non-numeric len argument -- %s\n", argv[optind]);
+			exitcode = 1;
+	                return 0;
+	        }
+	        len = length;
+		end_offset = (start_offset + len) / blocksize;
+	}
+
+	memset(&st, 0, sizeof(st));
+	if (fstat(file->fd, &st)) {
+		fprintf(stderr, "%s: fstat failed: %s\n", progname,
+			strerror(errno));
+		exitcode = 1;
+		return 0;
+	}
+
+	if (!end_offset)
+		end_offset = (start_offset + st.st_size) / blocksize;
+
+	ret = get_extent_count(file->fd, last_logical, len);
 	if (ret < 0) {
 		exitcode = 1;
 		return 0;
@@ -272,13 +312,12 @@ fiemap_f(
 
 	printf("%s:\n", file->name);
 
-	while (!last && num_extents) {
-
+	do {
 		/* Query a batch worth of extents */
 		memset(fiemap, 0, map_size);
 		fiemap->fm_flags = fiemap_flags;
 		fiemap->fm_start = last_logical;
-		fiemap->fm_length = -1LL;
+		fiemap->fm_length = len - (last_logical - start_offset);
 		fiemap->fm_extent_count = EXTENT_BATCH;
 
 		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
@@ -320,35 +359,27 @@ fiemap_f(
 		}
 
 		num_extents -= fiemap->fm_mapped_extents;
-	}
+	} while (!last && num_extents);
 
 	if (cur_extent == max_extents)
 		goto out;
 
-	memset(&st, 0, sizeof(st));
-	if (fstat(file->fd, &st)) {
-		fprintf(stderr, "%s: fstat failed: %s\n", progname,
-			strerror(errno));
-		free(fiemap);
-		exitcode = 1;
-		return 0;
-	}
-
-	if (cur_extent && last_logical < st.st_size) {
+	llast = last_logical / blocksize;
+	if (cur_extent && llast < end_offset) {
 		char	lbuf[32];
+		__u64 difference = end_offset - llast;
 
 		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:",
-			 last_logical / blocksize, (st.st_size / blocksize) - 1);
+			 llast, llast + difference - 1);
 		if (vflag) {
 			printf("%4d: %-*s %-*s %*llu\n", cur_extent,
 			       foff_w, lbuf, boff_w, _("hole"), tot_w,
-			       (st.st_size - last_logical) / blocksize);
+			       difference);
 		} else {
 			printf("\t%d: %s %s", cur_extent, lbuf,
 			       _("hole"));
 			if (lflag)
-				printf(_(" %llu blocks\n"),
-				       (st.st_size - last_logical) / blocksize);
+				printf(_(" %llu blocks\n"), len / blocksize);
 			else
 				printf("\n");
 		}
@@ -367,7 +398,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 [lenght]]");
 	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..9b57aed1d8d6 100644
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -295,11 +295,13 @@ 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. It also supports
+the standard unit suffixes.
 .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 4/4] fiemap: Add support for ranged query

[Eric Sandeen]

On 8/23/17 10:11 AM, Nikolay Borisov wrote:
> Introduce two optional arguments which can be used to perform fiemap queries
> for a particular range in a file. Those are 'offset' and 'length' they can be
> used like so:
> 
> xfs_io -c "fiemap 0 12k" - query for extents covering the first 12kb of the
> target file.
> 
> Now that such queries are supposed also modify the logic for printing the last
> hole to only cover the range which is asked. So if we ask for 0-10kb and the
> range 8k-12k is actually a whole, then limit the last whole only to this range:
> 
> So for a file which has the following contents :
> 
> |-----hole-------|-------data--------|-----hole-----|
> 0                8k                  12k            16k
> 
> The output would be:
> 
> xfs_io -c "fiemap -v 0 13k" test-dir/fragmented-file
> test-dir/fragmented-file:
>  EXT: FILE-OFFSET      BLOCK-RANGE          TOTAL FLAGS
>    0: [0..15]:         hole                    16
>    1: [16..23]:        897847296..897847303     8   0x0
>    2: [24..25]:        hole                     2
> 
> Furthermore in cases where the queried range is covered by a whole then the
> existing while() loop would have never executed, due to num_exents = 0. Fix this
> by converting it to a do {} while ()
> 
> _
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  io/fiemap.c       | 75 +++++++++++++++++++++++++++++++++++++++----------------
>  man/man8/xfs_io.8 |  6 +++--
>  2 files changed, 57 insertions(+), 24 deletions(-)
> 
> diff --git a/io/fiemap.c b/io/fiemap.c
> index ef54b265ab91..2e03a81dc57a 100644
> --- a/io/fiemap.c
> +++ b/io/fiemap.c
> @@ -27,7 +27,7 @@
>  
>  static cmdinfo_t fiemap_cmd;
>  static const __u64 blocksize = 512;
> -static int max_extents = 0;
> +static int max_extents = -1;
>  
>  static void
>  fiemap_help(void)
> @@ -38,6 +38,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"
> @@ -231,9 +232,14 @@ 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		start_offset = 0, last_logical = 0;
> +	__u64		len = -1;
> +	__u64		end_offset = 0, llast;
> +	size_t		fsblocksize, fssectsize;
>  	struct stat	st;
>  
> +	init_cvtnum(&fsblocksize, &fssectsize);
> +
>  	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
>  		switch (c) {
>  		case 'a':
> @@ -253,7 +259,41 @@ fiemap_f(
>  		}
>  	}
>  
> -	ret = get_extent_count(file->fd, last_logical, -1);
> +
> +	if (optind < argc) {
> +	        off64_t start = cvtnum(fsblocksize, fssectsize, argv[optind]);
> +	        if (start_offset < 0) {

This is testing the wrong var.

Ok, I got totally distracted on this; looking to see if it can't just be
cleaned up wholesale... it's still so full of weird special cases :/

-Eric

Re: [PATCH 1/4] fiemap: Move global variables out of function scope

[Eric Sandeen]

On 8/23/17 10:11 AM, Nikolay Borisov wrote:
> Move the blocksize and max_extent variables to the top of the file since they
> are globals. Also blocksize never changes to mark it const. Also stop passing
> max_extents around and refer to it directly.

I think this is fine, though it could probably go further.  If blocksize is
always 512, we could just use the BTOBBT() macro, which converts bytes
to "basic blocks" (512 units), and do away with the variable
entirely...



> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  io/fiemap.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/io/fiemap.c b/io/fiemap.c
> index 75e882057362..d1584aba7818 100644
> --- a/io/fiemap.c
> +++ b/io/fiemap.c
> @@ -24,6 +24,8 @@
>  #include "io.h"
>  
>  static cmdinfo_t fiemap_cmd;
> +static const __u64 blocksize = 512;
> +static int max_extents = 0;
>  
>  static void
>  fiemap_help(void)
> @@ -57,7 +59,6 @@ print_verbose(
>  	int			boff_w,
>  	int			tot_w,
>  	int			flg_w,
> -	int			max_extents,
>  	int			*cur_extent,
>  	__u64			*last_logical)
>  {
> @@ -113,7 +114,6 @@ print_plain(
>  	struct fiemap_extent	*extent,
>  	int			lflag,
>  	int			blocksize,
> -	int			max_extents,
>  	int			*cur_extent,
>  	__u64			*last_logical)
>  {
> @@ -165,7 +165,7 @@ calc_print_format(
>  	int			*tot_w,
>  	int			*flg_w)
>  {
> -	int 			i;
> +	int			i;
>  	char			lbuf[32];
>  	char			bbuf[32];
>  	__u64			logical;
> @@ -199,7 +199,6 @@ fiemap_f(
>  	char		**argv)
>  {
>  	struct fiemap	*fiemap;
> -	int		max_extents = 0;
>  	int		num_extents = 32;
>  	int		last = 0;
>  	int		lflag = 0;
> @@ -214,7 +213,6 @@ fiemap_f(
>  	int		boff_w = 16;
>  	int		tot_w = 5;	/* 5 since its just one number */
>  	int		flg_w = 5;
> -	__u64		blocksize = 512;
>  	__u64		last_logical = 0;
>  	struct stat	st;
>  
> @@ -288,12 +286,10 @@ fiemap_f(
>  
>  				print_verbose(extent, blocksize, foff_w,
>  					      boff_w, tot_w, flg_w,
> -					      max_extents, &cur_extent,
> -					      &last_logical);
> +					      &cur_extent, &last_logical);
>  			} else
>  				print_plain(extent, lflag, blocksize,
> -					    max_extents, &cur_extent,
> -					    &last_logical);
> +					    &cur_extent, &last_logical);
>  
>  			if (extent->fe_flags & FIEMAP_EXTENT_LAST) {
>  				last = 1;
> 

Re: [PATCH 1/4] fiemap: Move global variables out of function scope

[Eric Sandeen]

On 8/23/17 10:11 AM, Nikolay Borisov wrote:
> Move the blocksize and max_extent variables to the top of the file since they
> are globals. Also blocksize never changes to mark it const. Also stop passing
> max_extents around and refer to it directly.

Having looked at this for way too long, here are things that I think could be
done, in more or less this order as separate patches, but rearrange or
add/remove as you see fit...

- Get rid of the blocksize variable, and use BTOBBT()
- Turn num_extents into #define EXTENT_BATCH 32, fixed query map size
- make max_extents global if you like
- Clean up the control loop(s) and exit point(s), there are now exits on:
  + fm_mapped_extents == 0 (we got none from our query)
  + FIEMAP_EXTENT_LAST set (kernel tells us this one is last)
  + max_extents hit (we hit the user's limit)
  + end of requested range hit (after range is implemented)
  ... there's a lot in there that is messy. :/
- Fix the "-n" behavior (a minimally as possible) and update xfs_bmap(8) to match

finally:

- Implement your range query (I think this can be done by simply
  modifying the initial fm_start and fm_length, no?  Or, by only
  modifying fm_start, and breaking out of the loop when we retrieve
  an extent that goes beyond the end of the requested region?)

Other things I noticed:

- print_plain and print_verbose are pretty nasty the way they
  hide increments of cur_extent & last_logical.  I think it would
  be better to:

   + pass cur_extent & last_logical by value
   + return nr of extents printed, so the caller can increment cur_extent
   + update last_logical in fiemap_f after, not in, the print functions

- The last hunk that handles a hole at EOF is ick; something like this
  may be better, re-using the print functions with a special "EOF
  extent"; the print functions would then return after printing a hole
  if they are handed a 0 length extent with LAST set, or something.

        /* Handle a hole at EOF if there is one */
        if (cur_extent && last_logical < st.st_size) {
                struct fiemap_extent hole = { 0 };

                /* special 0-length extent to show formatters it's hole @ EOF */
                hole.fe_logical = st.st_size;
                hole.fe_length = 0;
                hole.fe_flags = FIEMAP_EXTENT_LAST;

                if (vflag)
                        print_verbose(&hole, foff_w, boff_w, tot_w, flg_w,
                                      max_extents, cur_extent, last_logical);
                else
                        print_plain(&hole, lflag,
                                    max_extents, cur_extent, last_logical);
        }