[PATCH v3 1/2] fiemap: Factor out actual fiemap call code

[PATCH v3 1/2] fiemap: Factor out actual fiemap call code

[Nikolay Borisov]

This will be needed to in a subsequent patch to avoid code duplication

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
V3: No change
V2: No change
 io/fiemap.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/io/fiemap.c b/io/fiemap.c
index e6fd66da753d..08391f69d9bd 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -211,6 +211,31 @@ calc_print_format(
 	}
 }
 
+static int
+__fiemap(
+	 struct fiemap * fiemap,
+	 int		 mapsize,
+	 __u32		 flags,
+	 __u64		 start,
+	 __u64		 length) {
+
+	int ret;
+
+	memset(fiemap, 0, mapsize);
+	fiemap->fm_flags = flags;
+	fiemap->fm_start = start;
+	fiemap->fm_length = length;
+	fiemap->fm_extent_count = EXTENT_BATCH;
+	ret = ioctl(file->fd, FS_IOC_FIEMAP, fiemap);
+	if (ret < 0) {
+	        fprintf(stderr, "%s: ioctl(FS_IOC_FIEMAP) [\"%s\"]: "
+	                "%s\n", progname, file->name, strerror(errno));
+	        return ret;
+	}
+
+	return 0;
+}
+
 int
 fiemap_f(
 	int		argc,
@@ -266,19 +291,11 @@ fiemap_f(
 
 	while (!last && (cur_extent != max_extents)) {
 
-		memset(fiemap, 0, map_size);
-		fiemap->fm_flags = fiemap_flags;
-		fiemap->fm_start = last_logical;
-		fiemap->fm_length = -1LL;
-		fiemap->fm_extent_count = EXTENT_BATCH;
-
-		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
+		ret = __fiemap(fiemap, map_size, fiemap_flags, last_logical,
+			       -1LL);
 		if (ret < 0) {
-			fprintf(stderr, "%s: ioctl(FS_IOC_FIEMAP) [\"%s\"]: "
-				"%s\n", progname, file->name, strerror(errno));
-			free(fiemap);
 			exitcode = 1;
-			return 0;
+			goto out;
 		}
 
 		/* No more extents to map, exit */
-- 
2.7.4

[PATCH v3 2/2] fiemap: Implement ranged query

[Nikolay Borisov]

Currently the fiemap implementation of xfs_io doesn't support making ranged
queries. This patch implements the '-r' parameter, taking up to 2 arguments - 
the starting offset and the length of the region to be queried. This also 
requires changing of the final hole range is calculated. Namely, it's now being
done as [last_logical, logical addres of next extent] rather than being
statically determined by [last_logical, filesize].

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
V3: 
 * Fixed a bug where incorrect extent index was shown if we didn't print a 
 hole. This was due to statically returning 2 at the end of print_(plain|verbose)
 Now, the actual number of printed extents inside the 2 functions is used. 
 This bug is visible only with the -r parameter

 * Fixed a bug where 1 additional extent would be printed. This was a result of 
 the aforementioned bug fix, since now printing function can return 1 and still
 have printed an extent and no hole. This can happen when you use -r parameter,
 this is now fixed and a comment explaining it is put. 

 * Reworked the handling of the new params by making them arguments to the 
 -r parameter. 

V2:
 * Incorporated Daricks feedback - removed variables which weren't introduced
  until the next patch as a result the build with this patch was broken. This is 
  fixed now

 io/fiemap.c       | 119 +++++++++++++++++++++++++++++++++++++++++++++++-------
 man/man8/xfs_io.8 |   5 ++-
 2 files changed, 107 insertions(+), 17 deletions(-)

diff --git a/io/fiemap.c b/io/fiemap.c
index 08391f69d9bd..018f3d064ab2 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -27,6 +27,9 @@
 
 static cmdinfo_t fiemap_cmd;
 static int max_extents = -1;
+static __u64 covered_length = 0;
+static __u64 len = -1LL;
+static bool range_limit = false;
 
 static void
 fiemap_help(void)
@@ -79,7 +82,7 @@ print_hole(
 		       boff_w, _("hole"), tot_w, lstart - llast);
 	   }
 
-
+	   covered_length += BBTOB(lstart - llast);
 }
 
 static int
@@ -90,7 +93,8 @@ print_verbose(
 	int			tot_w,
 	int			flg_w,
 	int			cur_extent,
-	__u64			last_logical)
+	__u64			last_logical,
+	__u64			limit)
 {
 	__u64			lstart;
 	__u64			llast;
@@ -99,6 +103,7 @@ print_verbose(
 	char			lbuf[48];
 	char			bbuf[48];
 	char			flgbuf[16];
+	int			num_printed = 0;
 
 	llast = BTOBBT(last_logical);
 	lstart = BTOBBT(extent->fe_logical);
@@ -120,10 +125,11 @@ print_verbose(
 		print_hole(foff_w, boff_w, tot_w, cur_extent, 0, false, llast,
 			   lstart);
 		cur_extent++;
+		num_printed = 1;
 	}
 
-	if (cur_extent == max_extents)
-		return 1;
+	if (cur_extent == max_extents || (range_limit && covered_length >= limit))
+		return num_printed;
 
 	snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", lstart,
 		 lstart + len - 1ULL);
@@ -132,7 +138,9 @@ print_verbose(
 	printf("%4d: %-*s %-*s %*llu %*s\n", cur_extent, foff_w, lbuf,
 	       boff_w, bbuf, tot_w, len, flg_w, flgbuf);
 
-	return 2;
+	num_printed++;
+
+	return num_printed;
 }
 
 static int
@@ -140,12 +148,14 @@ print_plain(
 	struct fiemap_extent	*extent,
 	int			lflag,
 	int			cur_extent,
-	__u64			last_logical)
+	__u64			last_logical,
+	__u64			limit)
 {
 	__u64			lstart;
 	__u64			llast;
 	__u64			block;
 	__u64			len;
+	int			num_printed = 0;
 
 	llast = BTOBBT(last_logical);
 	lstart = BTOBBT(extent->fe_logical);
@@ -155,20 +165,23 @@ print_plain(
 	if (lstart != llast) {
 		print_hole(0, 0, 0, cur_extent, lflag, true, llast, lstart);
 		cur_extent++;
+		num_printed = 1;
 	}
 
-	if (cur_extent == max_extents)
-		return 1;
+	if (cur_extent == max_extents || (range_limit && covered_length >= limit))
+		return num_printed;
 
 	printf("\t%d: [%llu..%llu]: %llu..%llu", cur_extent,
 	       lstart, lstart + len - 1ULL, block,
 	       block + len - 1ULL);
 
+	num_printed++;
+
 	if (lflag)
 		printf(_(" %llu blocks\n"), len);
 	else
 		printf("\n");
-	return 2;
+	return num_printed;
 }
 
 /*
@@ -256,9 +269,12 @@ fiemap_f(
 	int		tot_w = 5;	/* 5 since its just one number */
 	int		flg_w = 5;
 	__u64		last_logical = 0;
+	size_t		fsblocksize, fssectsize;
 	struct stat	st;
 
-	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
+	init_cvtnum(&fsblocksize, &fssectsize);
+
+	while ((c = getopt(argc, argv, "aln:vr")) != EOF) {
 		switch (c) {
 		case 'a':
 			fiemap_flags |= FIEMAP_FLAG_XATTR;
@@ -272,6 +288,50 @@ fiemap_f(
 		case 'v':
 			vflag++;
 			break;
+		case 'r':
+			/* Parse the first option which is mandatory */
+			if (optind < argc && argv[optind][0] != '-') {
+				off64_t start_offset = cvtnum(fsblocksize,
+							      fssectsize,
+							      argv[optind]);
+				if (start_offset < 0) {
+					printf("non-numeric offset argument -- "
+					       "%s\n", argv[optind]);
+					return 0;
+				}
+				last_logical = start_offset;
+				optind++;
+			} else {
+				fprintf(stderr, _("Invalid offset argument for"
+						  " -r\n"));
+				exitcode = 1;
+				return 0;
+			}
+
+			if (optind < argc) {
+				/* first check if what follows doesn't begin
+				 * with '-' which means it would be a param and
+				 * not an argument
+				 */
+				if (argv[optind][0] == '-') {
+					optind--;
+					break;
+
+				}
+
+				off64_t length = cvtnum(fsblocksize,
+							fssectsize,
+							argv[optind]);
+				if (length < 0) {
+					printf("non-numeric len argument --"
+					       " %s\n", argv[optind]);
+					return 0;
+				}
+				len = length;
+				range_limit = true;
+			}
+			break;
+
 		default:
 			return command_usage(&fiemap_cmd);
 		}
@@ -317,14 +377,23 @@ fiemap_f(
 				num_printed = print_verbose(extent, foff_w,
 							    boff_w, tot_w,
 							    flg_w, cur_extent,
-							    last_logical);
+							    last_logical,
+							    len);
 			} else
 				num_printed = print_plain(extent, lflag,
 							  cur_extent,
-							  last_logical);
+							  last_logical,
+							  len);
 
 			cur_extent += num_printed;
 			last_logical = extent->fe_logical + extent->fe_length;
+			/* If num_printed > 0 then we dunno if we have printed
+			 * a hole or an extent and a hole but we don't really
+			 * care. Termination of the loop is still going to be
+			 * correct
+			 */
+			if (num_printed)
+				covered_length += extent->fe_length;
 
 			if (extent->fe_flags & FIEMAP_EXTENT_LAST) {
 				last = 1;
@@ -333,6 +402,9 @@ fiemap_f(
 
 			if (cur_extent == max_extents)
 				break;
+
+			if (range_limit && covered_length >= len)
+				goto out;
 		}
 	}
 
@@ -348,9 +420,26 @@ fiemap_f(
 		return 0;
 	}
 
-	if (cur_extent && last_logical < st.st_size)
+	if (last_logical < st.st_size &&
+	    (!range_limit || covered_length < len)) {
+		int end;
+
+		ret = __fiemap(fiemap, map_size, fiemap_flags, last_logical,
+			       st.st_size);
+		if (ret < 0) {
+			exitcode = 1;
+			goto out;
+		}
+
+		if (!fiemap->fm_mapped_extents)
+			end = BTOBBT(st.st_size);
+		else
+			end = BTOBBT(fiemap->fm_extents[0].fe_logical);
+
+
 		print_hole(foff_w, boff_w, tot_w, cur_extent, lflag, !vflag,
-			   BTOBBT(last_logical), BTOBBT(st.st_size));
+			   BTOBBT(last_logical), end);
+	}
 
 out:
 	free(fiemap);
@@ -365,7 +454,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] [-r 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 0fd9b951199c..5a00e02c94b7 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 " ] [ \-r " 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

[PATCH v3 1/2] generic: Adjust generic test outputs for new fiemap imeplementation

[Nikolay Borisov]

With the new  range query support for the fiemap command,
the command also started printing hole extent for files which
consist of only a hole. So adjust generic test output accordingly.
Furthermore, this change breaks tests which are executed with a version
of xfs_io that doesn't support fiemap's range query. Fix this by implementing a
function which will fixup the output of tests which are broken by emulating
the output on older xfs_io versions

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

V3: 
 * Squash the test adjusments as well as the implementation of fiemap_range_fixup
 in a single patch

 * Add coment explaining the need for _fiemap_range_fixup
 common/punch          | 23 +++++++++++++++++++++++
 tests/generic/012.out |  1 +
 tests/generic/016.out |  1 +
 tests/generic/021.out |  2 ++
 tests/generic/022.out |  2 ++
 tests/generic/058.out |  1 +
 tests/generic/060.out |  1 +
 tests/generic/061.out |  1 +
 tests/generic/063.out |  1 +
 tests/generic/255.out |  6 ++++++
 tests/generic/316.out |  6 ++++++
 11 files changed, 45 insertions(+)

diff --git a/common/punch b/common/punch
index c4ed261..9c39183 100644
--- a/common/punch
+++ b/common/punch
@@ -218,6 +218,27 @@ _filter_fiemap()
 	_coalesce_extents
 }
 
+#This function allows for tests which print the fiemap of a
+#file, that consists of only a hole to pass when executed with 
+#older versions of xfs_io's fiemap that didn't print anything for
+#such files
+_fiemap_range_fixup()
+{
+	#check if we support ranged query
+	$XFS_IO_PROG -c "help fiemap" | head -n 1 | grep -q "[offset [len]]"
+	local range_sup=$?
+	#check if the file consists of a single hole only
+	echo "$1" | grep -q "^0\: \[.*\]\: hole$"
+	local sole_hole=$?
+	local filesize="$(((`stat -c %s $1` / 512) - 1))"
+	local output_line_num=`$XFS_IO_PROG -c 'fiemap' $testfile | wc -l`
+
+	if [ $range_sup -eq 1 ] && [ $sole_hole -eq 1  ] && [ $output_line_num -eq 1 ]
+	then
+		echo "0: [0..$filesize]: hole"
+	fi
+}
+
 _filter_fiemap_flags()
 {
 	$AWK_PROG '
@@ -363,6 +384,7 @@ _test_generic_punch()
 	$XFS_IO_PROG -f -c "truncate $_20k" \
 		-c "$zero_cmd $_4k $_8k" \
 		-c "$map_cmd -v" $testfile | $filter_cmd
+	_fiemap_range_fixup $testfile
 	[ $? -ne 0 ] && die_now
 	_md5_checksum $testfile
 
@@ -470,6 +492,7 @@ _test_generic_punch()
 		-c "pwrite $_8k $_4k" $sync_cmd \
 		-c "$zero_cmd $_4k $_12k" \
 		-c "$map_cmd -v" $testfile | $filter_cmd
+	_fiemap_range_fixup $testfile
 	[ $? -ne 0 ] && die_now
 	_md5_checksum $testfile
 
diff --git a/tests/generic/012.out b/tests/generic/012.out
index ffbf8a3..8045471 100644
--- a/tests/generic/012.out
+++ b/tests/generic/012.out
@@ -1,5 +1,6 @@
 QA output created by 012
 	1. into a hole
+0: [0..95]: hole
 f4f35d60b3cc18aaa6d8d92f0cd3708a
 	2. into allocated space
 0: [0..95]: extent
diff --git a/tests/generic/016.out b/tests/generic/016.out
index c45a44a..1371ce7 100644
--- a/tests/generic/016.out
+++ b/tests/generic/016.out
@@ -1,5 +1,6 @@
 QA output created by 016
 	1. into a hole
+0: [0..95]: hole
 f4f35d60b3cc18aaa6d8d92f0cd3708a
 	2. into allocated space
 0: [0..95]: extent
diff --git a/tests/generic/021.out b/tests/generic/021.out
index 1137741..791b78a 100644
--- a/tests/generic/021.out
+++ b/tests/generic/021.out
@@ -1,5 +1,6 @@
 QA output created by 021
 	1. into a hole
+0: [0..95]: hole
 f4f35d60b3cc18aaa6d8d92f0cd3708a
 	2. into allocated space
 0: [0..95]: extent
@@ -34,6 +35,7 @@ f4f35d60b3cc18aaa6d8d92f0cd3708a
 1: [64..95]: hole
 d8f51c20223dbce5c7c90db87bc221b0
 	10. hole -> data -> hole
+0: [0..63]: hole
 bb7df04e1b0a2570657527a7e108ae23
 	11. data -> hole -> data
 0: [0..63]: extent
diff --git a/tests/generic/022.out b/tests/generic/022.out
index fbffa59..6dbc192 100644
--- a/tests/generic/022.out
+++ b/tests/generic/022.out
@@ -1,5 +1,6 @@
 QA output created by 022
 	1. into a hole
+0: [0..95]: hole
 f4f35d60b3cc18aaa6d8d92f0cd3708a
 	2. into allocated space
 0: [0..95]: extent
@@ -34,6 +35,7 @@ f4f35d60b3cc18aaa6d8d92f0cd3708a
 1: [64..95]: hole
 d8f51c20223dbce5c7c90db87bc221b0
 	10. hole -> data -> hole
+0: [0..63]: hole
 bb7df04e1b0a2570657527a7e108ae23
 	11. data -> hole -> data
 0: [0..63]: extent
diff --git a/tests/generic/058.out b/tests/generic/058.out
index b15308d..3bbc2a4 100644
--- a/tests/generic/058.out
+++ b/tests/generic/058.out
@@ -1,5 +1,6 @@
 QA output created by 058
 	1. into a hole
+0: [0..55]: hole
 cf845a781c107ec1346e849c9dd1b7e8
 	2. into allocated space
 0: [0..7]: extent
diff --git a/tests/generic/060.out b/tests/generic/060.out
index 909b578..210af74 100644
--- a/tests/generic/060.out
+++ b/tests/generic/060.out
@@ -1,5 +1,6 @@
 QA output created by 060
 	1. into a hole
+0: [0..55]: hole
 cf845a781c107ec1346e849c9dd1b7e8
 	2. into allocated space
 0: [0..7]: extent
diff --git a/tests/generic/061.out b/tests/generic/061.out
index 78d6c6d..6d95680 100644
--- a/tests/generic/061.out
+++ b/tests/generic/061.out
@@ -1,5 +1,6 @@
 QA output created by 061
 	1. into a hole
+0: [0..55]: hole
 cf845a781c107ec1346e849c9dd1b7e8
 	2. into allocated space
 0: [0..7]: extent
diff --git a/tests/generic/063.out b/tests/generic/063.out
index d828ff6..10db43f 100644
--- a/tests/generic/063.out
+++ b/tests/generic/063.out
@@ -1,5 +1,6 @@
 QA output created by 063
 	1. into a hole
+0: [0..55]: hole
 cf845a781c107ec1346e849c9dd1b7e8
 	2. into allocated space
 0: [0..7]: extent
diff --git a/tests/generic/255.out b/tests/generic/255.out
index 217ef3e..441fde8 100644
--- a/tests/generic/255.out
+++ b/tests/generic/255.out
@@ -1,5 +1,6 @@
 QA output created by 255
 	1. into a hole
+0: [0..39]: hole
 daa100df6e6711906b61c9ab5aa16032
 	2. into allocated space
 0: [0..7]: extent
@@ -42,6 +43,7 @@ daa100df6e6711906b61c9ab5aa16032
 3: [32..39]: hole
 cc63069677939f69a6e8f68cae6a6dac
 	10. hole -> data -> hole
+0: [0..39]: hole
 daa100df6e6711906b61c9ab5aa16032
 	11. data -> hole -> data
 0: [0..7]: extent
@@ -79,6 +81,7 @@ eecb7aa303d121835de05028751d301c
 0000400 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
 *
 	1. into a hole
+0: [0..39]: hole
 daa100df6e6711906b61c9ab5aa16032
 	2. into allocated space
 0: [0..7]: extent
@@ -121,6 +124,7 @@ daa100df6e6711906b61c9ab5aa16032
 3: [32..39]: hole
 cc63069677939f69a6e8f68cae6a6dac
 	10. hole -> data -> hole
+0: [0..39]: hole
 daa100df6e6711906b61c9ab5aa16032
 	11. data -> hole -> data
 0: [0..7]: extent
@@ -158,6 +162,7 @@ eecb7aa303d121835de05028751d301c
 0000400 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
 *
 	1. into a hole
+0: [0..39]: hole
 daa100df6e6711906b61c9ab5aa16032
 	2. into allocated space
 0: [0..7]: extent
@@ -240,6 +245,7 @@ eecb7aa303d121835de05028751d301c
 0000400 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
 *
 	1. into a hole
+0: [0..39]: hole
 daa100df6e6711906b61c9ab5aa16032
 	2. into allocated space
 0: [0..7]: extent
diff --git a/tests/generic/316.out b/tests/generic/316.out
index 383f0d1..5506198 100644
--- a/tests/generic/316.out
+++ b/tests/generic/316.out
@@ -1,5 +1,6 @@
 QA output created by 316
 	1. into a hole
+0: [0..39]: hole
 daa100df6e6711906b61c9ab5aa16032
 	2. into allocated space
 0: [0..7]: extent
@@ -16,6 +17,7 @@ cc63069677939f69a6e8f68cae6a6dac
 1: [8..39]: hole
 1b3779878366498b28c702ef88c4a773
 	10. hole -> data -> hole
+0: [0..39]: hole
 daa100df6e6711906b61c9ab5aa16032
 	11. data -> hole -> data
 0: [0..7]: extent
@@ -43,6 +45,7 @@ eecb7aa303d121835de05028751d301c
 0000400 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
 *
 	1. into a hole
+0: [0..39]: hole
 daa100df6e6711906b61c9ab5aa16032
 	2. into allocated space
 0: [0..7]: extent
@@ -59,6 +62,7 @@ cc63069677939f69a6e8f68cae6a6dac
 1: [8..39]: hole
 1b3779878366498b28c702ef88c4a773
 	10. hole -> data -> hole
+0: [0..39]: hole
 daa100df6e6711906b61c9ab5aa16032
 	11. data -> hole -> data
 0: [0..7]: extent
@@ -86,6 +90,7 @@ eecb7aa303d121835de05028751d301c
 0000400 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
 *
 	1. into a hole
+0: [0..39]: hole
 daa100df6e6711906b61c9ab5aa16032
 	2. into allocated space
 0: [0..7]: extent
@@ -133,6 +138,7 @@ eecb7aa303d121835de05028751d301c
 0000400 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
 *
 	1. into a hole
+0: [0..39]: hole
 daa100df6e6711906b61c9ab5aa16032
 	2. into allocated space
 0: [0..7]: extent
-- 
2.7.4

[PATCH v3 2/2] xfs: initial fiemap range query test

[Nikolay Borisov]

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

V3: 
 * Added tests for -r parameter error handling
 tests/xfs/900     |  98 ++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/900.out | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 222 insertions(+)
 create mode 100755 tests/xfs/900
 create mode 100644 tests/xfs/900.out

diff --git a/tests/xfs/900 b/tests/xfs/900
new file mode 100755
index 0000000..570f7d6
--- /dev/null
+++ b/tests/xfs/900
@@ -0,0 +1,98 @@
+#! /bin/bash
+# FS QA Test No. 900
+#-----------------------------------------------------------------------
+# Copyright (c) 2017 SUSE Linux Products GmbH. All Rights Reserved.
+# Author: Nikolay Borisov <nborisov@suse.com>
+#
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/punch
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+_require_xfs_io_command "falloc"
+_require_xfs_io_command "fiemap" "-r 0 4k"
+
+_scratch_mkfs > $seqres.full 2>&1
+_scratch_mount || _fail "mount failure"
+
+file=$SCRATCH_MNT/testfile
+$XFS_IO_PROG -f -c "falloc 0 256k" $file 
+for i in {0..31}; do $XFS_IO_PROG -c "fpunch $(($i*8))k 4k" $file; done
+
+# Query 1 data extent between 4k..8k range
+echo "Basic data extent"
+$XFS_IO_PROG -c "fiemap -v -r 4k 4k" $file | _filter_fiemap 
+
+# Query data and hole extent
+echo "Data + Hole"
+$XFS_IO_PROG -c "fiemap -v -r 4k 5k" $file | _filter_fiemap
+
+echo "Hole + Data + Hole"
+$XFS_IO_PROG -c "fiemap -v -r 8k 10k" $file | _filter_fiemap
+
+echo "Beginning with a hole"
+$XFS_IO_PROG -c "fiemap -v -r 0 3k" $file | _filter_fiemap
+
+# Query for 0..160k that's 40 extents, more than the EXTENT_BATCH
+echo "Query more than 32 extents"
+$XFS_IO_PROG -c "fiemap -v -r 0 160k" $file | _filter_fiemap
+
+echo "Larger query than file size"
+$XFS_IO_PROG -c "fiemap -v -r 0 300k" $file | _filter_fiemap
+
+echo "Parameter parsing tests"
+
+#check everything without the first hole, should print 65
+$XFS_IO_PROG -c "fiemap -v -r 4k" $file | wc -l
+
+#check correct handling of [-r offset] followed by another param
+$XFS_IO_PROG -c "fiemap -v -r 4k -n 5" $file | wc -l
+
+#check correct handling of [-r offset len] followed by another param
+$XFS_IO_PROG -c "fiemap -v -r 4k 12k -n 2" $file | wc -l
+
+#check incorrect offset arg. should return error
+$XFS_IO_PROG -c "fiemap -v -r -n 1" $file
+$XFS_IO_PROG -c "fiemap -v -r" $file
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/900.out b/tests/xfs/900.out
new file mode 100644
index 0000000..fa25565
--- /dev/null
+++ b/tests/xfs/900.out
@@ -0,0 +1,124 @@
+QA output created by 900
+Basic data extent
+0: [8..15]: unwritten
+Data + Hole
+0: [8..15]: unwritten
+1: [16..23]: hole
+Hole + Data + Hole
+0: [16..23]: hole
+1: [24..31]: unwritten
+2: [32..39]: hole
+Beginning with a hole
+0: [0..7]: hole
+Query more than 32 extents
+0: [0..7]: hole
+1: [8..15]: unwritten
+2: [16..23]: hole
+3: [24..31]: unwritten
+4: [32..39]: hole
+5: [40..47]: unwritten
+6: [48..55]: hole
+7: [56..63]: unwritten
+8: [64..71]: hole
+9: [72..79]: unwritten
+10: [80..87]: hole
+11: [88..95]: unwritten
+12: [96..103]: hole
+13: [104..111]: unwritten
+14: [112..119]: hole
+15: [120..127]: unwritten
+16: [128..135]: hole
+17: [136..143]: unwritten
+18: [144..151]: hole
+19: [152..159]: unwritten
+20: [160..167]: hole
+21: [168..175]: unwritten
+22: [176..183]: hole
+23: [184..191]: unwritten
+24: [192..199]: hole
+25: [200..207]: unwritten
+26: [208..215]: hole
+27: [216..223]: unwritten
+28: [224..231]: hole
+29: [232..239]: unwritten
+30: [240..247]: hole
+31: [248..255]: unwritten
+32: [256..263]: hole
+33: [264..271]: unwritten
+34: [272..279]: hole
+35: [280..287]: unwritten
+36: [288..295]: hole
+37: [296..303]: unwritten
+38: [304..311]: hole
+39: [312..319]: unwritten
+Larger query than file size
+0: [0..7]: hole
+1: [8..15]: unwritten
+2: [16..23]: hole
+3: [24..31]: unwritten
+4: [32..39]: hole
+5: [40..47]: unwritten
+6: [48..55]: hole
+7: [56..63]: unwritten
+8: [64..71]: hole
+9: [72..79]: unwritten
+10: [80..87]: hole
+11: [88..95]: unwritten
+12: [96..103]: hole
+13: [104..111]: unwritten
+14: [112..119]: hole
+15: [120..127]: unwritten
+16: [128..135]: hole
+17: [136..143]: unwritten
+18: [144..151]: hole
+19: [152..159]: unwritten
+20: [160..167]: hole
+21: [168..175]: unwritten
+22: [176..183]: hole
+23: [184..191]: unwritten
+24: [192..199]: hole
+25: [200..207]: unwritten
+26: [208..215]: hole
+27: [216..223]: unwritten
+28: [224..231]: hole
+29: [232..239]: unwritten
+30: [240..247]: hole
+31: [248..255]: unwritten
+32: [256..263]: hole
+33: [264..271]: unwritten
+34: [272..279]: hole
+35: [280..287]: unwritten
+36: [288..295]: hole
+37: [296..303]: unwritten
+38: [304..311]: hole
+39: [312..319]: unwritten
+40: [320..327]: hole
+41: [328..335]: unwritten
+42: [336..343]: hole
+43: [344..351]: unwritten
+44: [352..359]: hole
+45: [360..367]: unwritten
+46: [368..375]: hole
+47: [376..383]: unwritten
+48: [384..391]: hole
+49: [392..399]: unwritten
+50: [400..407]: hole
+51: [408..415]: unwritten
+52: [416..423]: hole
+53: [424..431]: unwritten
+54: [432..439]: hole
+55: [440..447]: unwritten
+56: [448..455]: hole
+57: [456..463]: unwritten
+58: [464..471]: hole
+59: [472..479]: unwritten
+60: [480..487]: hole
+61: [488..495]: unwritten
+62: [496..503]: hole
+63: [504..511]: unwritten
+Parameter parsing tests
+65
+7
+4
+Invalid offset argument for -r
+Invalid offset argument for -r
-- 
2.7.4

Re: [PATCH v3 2/2] fiemap: Implement ranged query

[Dave Chinner]

On Mon, Nov 13, 2017 at 05:47:53PM +0200, Nikolay Borisov wrote:
> Currently the fiemap implementation of xfs_io doesn't support making ranged
> queries. This patch implements the '-r' parameter, taking up to 2 arguments - 
> the starting offset and the length of the region to be queried. This also 
> requires changing of the final hole range is calculated. Namely, it's now being
> done as [last_logical, logical addres of next extent] rather than being
> statically determined by [last_logical, filesize].
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> V3: 
>  * Fixed a bug where incorrect extent index was shown if we didn't print a 
>  hole. This was due to statically returning 2 at the end of print_(plain|verbose)
>  Now, the actual number of printed extents inside the 2 functions is used. 
>  This bug is visible only with the -r parameter
> 
>  * Fixed a bug where 1 additional extent would be printed. This was a result of 
>  the aforementioned bug fix, since now printing function can return 1 and still
>  have printed an extent and no hole. This can happen when you use -r parameter,
>  this is now fixed and a comment explaining it is put. 
> 
>  * Reworked the handling of the new params by making them arguments to the 
>  -r parameter. 
> 
> V2:
>  * Incorporated Daricks feedback - removed variables which weren't introduced
>   until the next patch as a result the build with this patch was broken. This is 
>   fixed now
.....

> @@ -256,9 +269,12 @@ fiemap_f(
>  	int		tot_w = 5;	/* 5 since its just one number */
>  	int		flg_w = 5;
>  	__u64		last_logical = 0;
> +	size_t		fsblocksize, fssectsize;
>  	struct stat	st;
>  
> -	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
> +	init_cvtnum(&fsblocksize, &fssectsize);
> +
> +	while ((c = getopt(argc, argv, "aln:vr")) != EOF) {

Ok, you're not telling gotopt that "-r" has parameters, so....

>  		switch (c) {
>  		case 'a':
>  			fiemap_flags |= FIEMAP_FLAG_XATTR;
> @@ -272,6 +288,50 @@ fiemap_f(
>  		case 'v':
>  			vflag++;
>  			break;
> +		case 'r':
> +			/* Parse the first option which is mandatory */
> +			if (optind < argc && argv[optind][0] != '-') {
> +				off64_t start_offset = cvtnum(fsblocksize,
> +							      fssectsize,
> +							      argv[optind]);
> +				if (start_offset < 0) {
> +					printf("non-numeric offset argument -- "
> +					       "%s\n", argv[optind]);
> +					return 0;
> +				}
> +				last_logical = start_offset;
> +				optind++;
> +			} else {
> +				fprintf(stderr, _("Invalid offset argument for"
> +						  " -r\n"));
> +				exitcode = 1;
> +				return 0;
> +			}
> +
> +			if (optind < argc) {
> +				/* first check if what follows doesn't begin
> +				 * with '-' which means it would be a param and
> +				 * not an argument
> +				 */
> +				if (argv[optind][0] == '-') {
> +					optind--;
> +					break;
> +
> +				}
> +
> +				off64_t length = cvtnum(fsblocksize,
> +							fssectsize,
> +							argv[optind]);
> +				if (length < 0) {
> +					printf("non-numeric len argument --"
> +					       " %s\n", argv[optind]);
> +					return 0;
> +				}
> +				len = length;
> +				range_limit = true;
> +			}
> +			break;

.... this is pretty nasty because you're having to check if the next
option should be parsed by the main loop or not. This assumes that
getopt is giving us the options in the order they were presented on
the command line, which is not a good assumption to make as glibc
can mutate the order as it parses the listi of know options and
arguments.

Given that "-r" is the only option that has parameters, then this
can be massively simplified just by noting we've seen the rflag, and
leaving the non-arg parameter parsing to after the end of the loop.
i.e.:


		case 'r':
			range_limit = true;
			break;
......

	if (!range_limit) {
		/* no extra parameters */
		if (optind != argc)
			usage();
	} else if (optind != argc - 2) {
		/* wrong number of parameters for rflag */
		usage();
	} else {
		/* parse range variables */
		offset = cvtnum(fsblocksize, fssectsize, argv[optind++]);
		length = cvtnum(fsblocksize, fssectsize, argv[optind]);
		if (offset < 0 || length < 0) {
			/* invalid options */
			usage();
		}
	}



>  
>  			cur_extent += num_printed;
>  			last_logical = extent->fe_logical + extent->fe_length;
> +			/* If num_printed > 0 then we dunno if we have printed
> +			 * a hole or an extent and a hole but we don't really
> +			 * care. Termination of the loop is still going to be
> +			 * correct
> +			 */

/*
 * Please use the standard comment format.
 */

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v3 2/2] fiemap: Implement ranged query

[Nikolay Borisov]

On 13.11.2017 23:44, Dave Chinner wrote:
> On Mon, Nov 13, 2017 at 05:47:53PM +0200, Nikolay Borisov wrote:
>> Currently the fiemap implementation of xfs_io doesn't support making ranged
>> queries. This patch implements the '-r' parameter, taking up to 2 arguments - 
>> the starting offset and the length of the region to be queried. This also 
>> requires changing of the final hole range is calculated. Namely, it's now being
>> done as [last_logical, logical addres of next extent] rather than being
>> statically determined by [last_logical, filesize].
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>> V3: 
>>  * Fixed a bug where incorrect extent index was shown if we didn't print a 
>>  hole. This was due to statically returning 2 at the end of print_(plain|verbose)
>>  Now, the actual number of printed extents inside the 2 functions is used. 
>>  This bug is visible only with the -r parameter
>>
>>  * Fixed a bug where 1 additional extent would be printed. This was a result of 
>>  the aforementioned bug fix, since now printing function can return 1 and still
>>  have printed an extent and no hole. This can happen when you use -r parameter,
>>  this is now fixed and a comment explaining it is put. 
>>
>>  * Reworked the handling of the new params by making them arguments to the 
>>  -r parameter. 
>>
>> V2:
>>  * Incorporated Daricks feedback - removed variables which weren't introduced
>>   until the next patch as a result the build with this patch was broken. This is 
>>   fixed now
> .....
> 
>> @@ -256,9 +269,12 @@ fiemap_f(
>>  	int		tot_w = 5;	/* 5 since its just one number */
>>  	int		flg_w = 5;
>>  	__u64		last_logical = 0;
>> +	size_t		fsblocksize, fssectsize;
>>  	struct stat	st;
>>  
>> -	while ((c = getopt(argc, argv, "aln:v")) != EOF) {
>> +	init_cvtnum(&fsblocksize, &fssectsize);
>> +
>> +	while ((c = getopt(argc, argv, "aln:vr")) != EOF) {
> 
> Ok, you're not telling gotopt that "-r" has parameters, so....
> 
>>  		switch (c) {
>>  		case 'a':
>>  			fiemap_flags |= FIEMAP_FLAG_XATTR;
>> @@ -272,6 +288,50 @@ fiemap_f(
>>  		case 'v':
>>  			vflag++;
>>  			break;
>> +		case 'r':
>> +			/* Parse the first option which is mandatory */
>> +			if (optind < argc && argv[optind][0] != '-') {
>> +				off64_t start_offset = cvtnum(fsblocksize,
>> +							      fssectsize,
>> +							      argv[optind]);
>> +				if (start_offset < 0) {
>> +					printf("non-numeric offset argument -- "
>> +					       "%s\n", argv[optind]);
>> +					return 0;
>> +				}
>> +				last_logical = start_offset;
>> +				optind++;
>> +			} else {
>> +				fprintf(stderr, _("Invalid offset argument for"
>> +						  " -r\n"));
>> +				exitcode = 1;
>> +				return 0;
>> +			}
>> +
>> +			if (optind < argc) {
>> +				/* first check if what follows doesn't begin
>> +				 * with '-' which means it would be a param and
>> +				 * not an argument
>> +				 */
>> +				if (argv[optind][0] == '-') {
>> +					optind--;
>> +					break;
>> +
>> +				}
>> +
>> +				off64_t length = cvtnum(fsblocksize,
>> +							fssectsize,
>> +							argv[optind]);
>> +				if (length < 0) {
>> +					printf("non-numeric len argument --"
>> +					       " %s\n", argv[optind]);
>> +					return 0;
>> +				}
>> +				len = length;
>> +				range_limit = true;
>> +			}
>> +			break;
> 
> .... this is pretty nasty because you're having to check if the next
> option should be parsed by the main loop or not. This assumes that
> getopt is giving us the options in the order they were presented on
> the command line, which is not a good assumption to make as glibc
> can mutate the order as it parses the listi of know options and
> arguments.
> 
> Given that "-r" is the only option that has parameters, then this
> can be massively simplified just by noting we've seen the rflag, and
> leaving the non-arg parameter parsing to after the end of the loop.
> i.e.:


You are right this is a bit ugly, but it seems more consistend to me,
rather than something like

xfs_io -c "fiemap -r -n 10 -v 4k 10k" for example. And the reason why
I'm using this hackish way and not declaring r as r: in getopt is
because getopt doesn't recognise when a parameter takes more than 1
argument.

> 
> 
> 		case 'r':
> 			range_limit = true;
> 			break;
> ......
> 
> 	if (!range_limit) {
> 		/* no extra parameters */
> 		if (optind != argc)
> 			usage();
> 	} else if (optind != argc - 2) {
> 		/* wrong number of parameters for rflag */
> 		usage();
> 	} else {
> 		/* parse range variables */
> 		offset = cvtnum(fsblocksize, fssectsize, argv[optind++]);
> 		length = cvtnum(fsblocksize, fssectsize, argv[optind]);
> 		if (offset < 0 || length < 0) {
> 			/* invalid options */
> 			usage();
> 		}
> 	}

True, this is very simple but it imposes the constraints that if we want
to use -r then we must absolutely give 2 args always, which is not a big
deal but it seems a bit limiting. If that's what you desire then I'm
fine doing it that way but it just seems a bit iffy. Given that the -r
method has tests which ensure that parsing is correct I'm more inclined
to have the more consistent -r followed by 1 or 2 arguments.

> 
> 
> 
>>  
>>  			cur_extent += num_printed;
>>  			last_logical = extent->fe_logical + extent->fe_length;
>> +			/* If num_printed > 0 then we dunno if we have printed
>> +			 * a hole or an extent and a hole but we don't really
>> +			 * care. Termination of the loop is still going to be
>> +			 * correct
>> +			 */
> 
> /*
>  * Please use the standard comment format.
>  */
> 
> Cheers,
> 
> Dave.
> 

Re: [PATCH v3 2/2] fiemap: Implement ranged query

[Eric Sandeen]

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

Sorry for chiming in so late after all the go-rounds, but:

Why not just drop -r entirely, and make fiemap go into ranged mode iff a
range is specified at the end of the command, i.e.:

fiemap [ -alv ] [ -n nx ] [ offset length ]

and if no offset/length is specified, map the whole file.   You can parse
the optional last 2 arguments just like pread, write, sync_range, sendfile,
or a host of other commands already do (though some are not optional.)

There are /many/ other xfs_io commands that take offset & length at the
end, so this usage should be very familiar to users.

-Eric

Re: [PATCH v3 2/2] fiemap: Implement ranged query

[Nikolay Borisov]

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

V2 was like that but it required some changing to the generic code which
detects the presence of this feature and Dave objected hence v3.


> 
> and if no offset/length is specified, map the whole file.   You can parse
> the optional last 2 arguments just like pread, write, sync_range, sendfile,
> or a host of other commands already do (though some are not optional.)
> 
> There are /many/ other xfs_io commands that take offset & length at the
> end, so this usage should be very familiar to users.
> 
> -Eric
> 
> 

Re: [PATCH v3 2/2] fiemap: Implement ranged query

[Eric Sandeen]

On 11/14/17 12:32 AM, Nikolay Borisov wrote:
> 
> 
> On 14.11.2017 00:22, Eric Sandeen wrote:
>> On 11/13/17 4:05 PM, Nikolay Borisov wrote:

...

>>> You are right this is a bit ugly, but it seems more consistend to me,
>>> rather than something like
>>>
>>> xfs_io -c "fiemap -r -n 10 -v 4k 10k" for example. And the reason why
>>> I'm using this hackish way and not declaring r as r: in getopt is
>>> because getopt doesn't recognise when a parameter takes more than 1
>>> argument.
>>
>> Sorry for chiming in so late after all the go-rounds, but:
>>
>> Why not just drop -r entirely, and make fiemap go into ranged mode iff a
>> range is specified at the end of the command, i.e.:
>>
>> fiemap [ -alv ] [ -n nx ] [ offset length ]
> 
> V2 was like that but it required some changing to the generic code which
> detects the presence of this feature and Dave objected hence v3.

Ok, sorry for being so behind and rehashing old discussions.  Let me look
at that.

-Eric

Re: [PATCH v3 2/2] xfs: initial fiemap range query test

[Eric Sandeen]

On 11/13/17 9:50 AM, Nikolay Borisov wrote:
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> V3: 
>  * Added tests for -r parameter error handling
>  tests/xfs/900     |  98 ++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/900.out | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 222 insertions(+)
>  create mode 100755 tests/xfs/900
>  create mode 100644 tests/xfs/900.out
> 
> diff --git a/tests/xfs/900 b/tests/xfs/900
> new file mode 100755
> index 0000000..570f7d6
> --- /dev/null
> +++ b/tests/xfs/900
> @@ -0,0 +1,98 @@
> +#! /bin/bash
> +# FS QA Test No. 900
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2017 SUSE Linux Products GmbH. All Rights Reserved.
> +# Author: Nikolay Borisov <nborisov@suse.com>
> +#
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/punch
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic

This says generic but you have it under xfs/ tests, is that intentional?

> +_supported_os Linux
> +_require_scratch
> +_require_xfs_io_command "falloc"
> +_require_xfs_io_command "fiemap" "-r 0 4k"


> +
> +_scratch_mkfs > $seqres.full 2>&1

Do you need to explicitly mkfs w/ a 4k block size so all the
below tests on 4k boundaries will pass?

(IOWs does this work on 2k or 8k fs block size?)

-Eric

Re: [PATCH v3 2/2] xfs: initial fiemap range query test

[Darrick J. Wong]

On Mon, Nov 13, 2017 at 05:50:29PM +0200, Nikolay Borisov wrote:
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> V3: 
>  * Added tests for -r parameter error handling
>  tests/xfs/900     |  98 ++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/900.out | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 222 insertions(+)
>  create mode 100755 tests/xfs/900
>  create mode 100644 tests/xfs/900.out
> 
> diff --git a/tests/xfs/900 b/tests/xfs/900
> new file mode 100755
> index 0000000..570f7d6
> --- /dev/null
> +++ b/tests/xfs/900
> @@ -0,0 +1,98 @@
> +#! /bin/bash
> +# FS QA Test No. 900

Needs description of what the test is testing here.

> +#-----------------------------------------------------------------------
> +# Copyright (c) 2017 SUSE Linux Products GmbH. All Rights Reserved.
> +# Author: Nikolay Borisov <nborisov@suse.com>
> +#
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/punch
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_supported_os Linux
> +_require_scratch
> +_require_xfs_io_command "falloc"
> +_require_xfs_io_command "fiemap" "-r 0 4k"

Can you multiply everything by 16 (i.e. assume 64k fs blocks instead of
4k fs blocks) so that we can test machines with larger pages and larger
fs block sizes?

--D

> +
> +_scratch_mkfs > $seqres.full 2>&1
> +_scratch_mount || _fail "mount failure"
> +
> +file=$SCRATCH_MNT/testfile
> +$XFS_IO_PROG -f -c "falloc 0 256k" $file 
> +for i in {0..31}; do $XFS_IO_PROG -c "fpunch $(($i*8))k 4k" $file; done
> +
> +# Query 1 data extent between 4k..8k range
> +echo "Basic data extent"
> +$XFS_IO_PROG -c "fiemap -v -r 4k 4k" $file | _filter_fiemap 
> +
> +# Query data and hole extent
> +echo "Data + Hole"
> +$XFS_IO_PROG -c "fiemap -v -r 4k 5k" $file | _filter_fiemap
> +
> +echo "Hole + Data + Hole"
> +$XFS_IO_PROG -c "fiemap -v -r 8k 10k" $file | _filter_fiemap
> +
> +echo "Beginning with a hole"
> +$XFS_IO_PROG -c "fiemap -v -r 0 3k" $file | _filter_fiemap
> +
> +# Query for 0..160k that's 40 extents, more than the EXTENT_BATCH
> +echo "Query more than 32 extents"
> +$XFS_IO_PROG -c "fiemap -v -r 0 160k" $file | _filter_fiemap
> +
> +echo "Larger query than file size"
> +$XFS_IO_PROG -c "fiemap -v -r 0 300k" $file | _filter_fiemap
> +
> +echo "Parameter parsing tests"
> +
> +#check everything without the first hole, should print 65
> +$XFS_IO_PROG -c "fiemap -v -r 4k" $file | wc -l
> +
> +#check correct handling of [-r offset] followed by another param
> +$XFS_IO_PROG -c "fiemap -v -r 4k -n 5" $file | wc -l
> +
> +#check correct handling of [-r offset len] followed by another param
> +$XFS_IO_PROG -c "fiemap -v -r 4k 12k -n 2" $file | wc -l
> +
> +#check incorrect offset arg. should return error
> +$XFS_IO_PROG -c "fiemap -v -r -n 1" $file
> +$XFS_IO_PROG -c "fiemap -v -r" $file
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/900.out b/tests/xfs/900.out
> new file mode 100644
> index 0000000..fa25565
> --- /dev/null
> +++ b/tests/xfs/900.out
> @@ -0,0 +1,124 @@
> +QA output created by 900
> +Basic data extent
> +0: [8..15]: unwritten
> +Data + Hole
> +0: [8..15]: unwritten
> +1: [16..23]: hole
> +Hole + Data + Hole
> +0: [16..23]: hole
> +1: [24..31]: unwritten
> +2: [32..39]: hole
> +Beginning with a hole
> +0: [0..7]: hole
> +Query more than 32 extents
> +0: [0..7]: hole
> +1: [8..15]: unwritten
> +2: [16..23]: hole
> +3: [24..31]: unwritten
> +4: [32..39]: hole
> +5: [40..47]: unwritten
> +6: [48..55]: hole
> +7: [56..63]: unwritten
> +8: [64..71]: hole
> +9: [72..79]: unwritten
> +10: [80..87]: hole
> +11: [88..95]: unwritten
> +12: [96..103]: hole
> +13: [104..111]: unwritten
> +14: [112..119]: hole
> +15: [120..127]: unwritten
> +16: [128..135]: hole
> +17: [136..143]: unwritten
> +18: [144..151]: hole
> +19: [152..159]: unwritten
> +20: [160..167]: hole
> +21: [168..175]: unwritten
> +22: [176..183]: hole
> +23: [184..191]: unwritten
> +24: [192..199]: hole
> +25: [200..207]: unwritten
> +26: [208..215]: hole
> +27: [216..223]: unwritten
> +28: [224..231]: hole
> +29: [232..239]: unwritten
> +30: [240..247]: hole
> +31: [248..255]: unwritten
> +32: [256..263]: hole
> +33: [264..271]: unwritten
> +34: [272..279]: hole
> +35: [280..287]: unwritten
> +36: [288..295]: hole
> +37: [296..303]: unwritten
> +38: [304..311]: hole
> +39: [312..319]: unwritten
> +Larger query than file size
> +0: [0..7]: hole
> +1: [8..15]: unwritten
> +2: [16..23]: hole
> +3: [24..31]: unwritten
> +4: [32..39]: hole
> +5: [40..47]: unwritten
> +6: [48..55]: hole
> +7: [56..63]: unwritten
> +8: [64..71]: hole
> +9: [72..79]: unwritten
> +10: [80..87]: hole
> +11: [88..95]: unwritten
> +12: [96..103]: hole
> +13: [104..111]: unwritten
> +14: [112..119]: hole
> +15: [120..127]: unwritten
> +16: [128..135]: hole
> +17: [136..143]: unwritten
> +18: [144..151]: hole
> +19: [152..159]: unwritten
> +20: [160..167]: hole
> +21: [168..175]: unwritten
> +22: [176..183]: hole
> +23: [184..191]: unwritten
> +24: [192..199]: hole
> +25: [200..207]: unwritten
> +26: [208..215]: hole
> +27: [216..223]: unwritten
> +28: [224..231]: hole
> +29: [232..239]: unwritten
> +30: [240..247]: hole
> +31: [248..255]: unwritten
> +32: [256..263]: hole
> +33: [264..271]: unwritten
> +34: [272..279]: hole
> +35: [280..287]: unwritten
> +36: [288..295]: hole
> +37: [296..303]: unwritten
> +38: [304..311]: hole
> +39: [312..319]: unwritten
> +40: [320..327]: hole
> +41: [328..335]: unwritten
> +42: [336..343]: hole
> +43: [344..351]: unwritten
> +44: [352..359]: hole
> +45: [360..367]: unwritten
> +46: [368..375]: hole
> +47: [376..383]: unwritten
> +48: [384..391]: hole
> +49: [392..399]: unwritten
> +50: [400..407]: hole
> +51: [408..415]: unwritten
> +52: [416..423]: hole
> +53: [424..431]: unwritten
> +54: [432..439]: hole
> +55: [440..447]: unwritten
> +56: [448..455]: hole
> +57: [456..463]: unwritten
> +58: [464..471]: hole
> +59: [472..479]: unwritten
> +60: [480..487]: hole
> +61: [488..495]: unwritten
> +62: [496..503]: hole
> +63: [504..511]: unwritten
> +Parameter parsing tests
> +65
> +7
> +4
> +Invalid offset argument for -r
> +Invalid offset argument for -r
> -- 
> 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 v3 2/2] fiemap: Implement ranged query

[Dave Chinner]

On Tue, Nov 14, 2017 at 07:31:34AM -0600, Eric Sandeen wrote:
> 
> 
> On 11/14/17 12:32 AM, Nikolay Borisov wrote:
> > 
> > 
> > On 14.11.2017 00:22, Eric Sandeen wrote:
> >> On 11/13/17 4:05 PM, Nikolay Borisov wrote:
> 
> ...
> 
> >>> You are right this is a bit ugly, but it seems more consistend to me,
> >>> rather than something like
> >>>
> >>> xfs_io -c "fiemap -r -n 10 -v 4k 10k" for example. And the reason why
> >>> I'm using this hackish way and not declaring r as r: in getopt is
> >>> because getopt doesn't recognise when a parameter takes more than 1
> >>> argument.
> >>
> >> Sorry for chiming in so late after all the go-rounds, but:
> >>
> >> Why not just drop -r entirely, and make fiemap go into ranged mode iff a
> >> range is specified at the end of the command, i.e.:
> >>
> >> fiemap [ -alv ] [ -n nx ] [ offset length ]
> > 
> > V2 was like that but it required some changing to the generic code which
> > detects the presence of this feature and Dave objected hence v3.
> 
> Ok, sorry for being so behind and rehashing old discussions.  Let me look
> at that.

I suggested "-r" because of the fact that you can't detect optional
parameters in fstests without jumping through really nasty hoops
involving regex line noise.

Cheers,

Daves.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v3 2/2] fiemap: Implement ranged query

[Eric Sandeen]

On 11/14/17 2:52 PM, Dave Chinner wrote:
> On Tue, Nov 14, 2017 at 07:31:34AM -0600, Eric Sandeen wrote:
>>
>>
>> On 11/14/17 12:32 AM, Nikolay Borisov wrote:
>>>
>>>
>>> On 14.11.2017 00:22, Eric Sandeen wrote:
>>>> On 11/13/17 4:05 PM, Nikolay Borisov wrote:
>>
>> ...
>>
>>>>> You are right this is a bit ugly, but it seems more consistend to me,
>>>>> rather than something like
>>>>>
>>>>> xfs_io -c "fiemap -r -n 10 -v 4k 10k" for example. And the reason why
>>>>> I'm using this hackish way and not declaring r as r: in getopt is
>>>>> because getopt doesn't recognise when a parameter takes more than 1
>>>>> argument.
>>>>
>>>> Sorry for chiming in so late after all the go-rounds, but:
>>>>
>>>> Why not just drop -r entirely, and make fiemap go into ranged mode iff a
>>>> range is specified at the end of the command, i.e.:
>>>>
>>>> fiemap [ -alv ] [ -n nx ] [ offset length ]
>>>
>>> V2 was like that but it required some changing to the generic code which
>>> detects the presence of this feature and Dave objected hence v3.
>>
>> Ok, sorry for being so behind and rehashing old discussions.  Let me look
>> at that.
> 
> I suggested "-r" because of the fact that you can't detect optional
> parameters in fstests without jumping through really nasty hoops
> involving regex line noise.

Yeah, but I'd rather have nasty stuff in xfstests than in xfsprogs interfaces.
 
See also my suggestion to try mapping past EOF as a detection method.

-Eric

> Cheers,
> 
> Daves.
> 

Re: [PATCH v3 2/2] xfs: initial fiemap range query test

[Nikolay Borisov]

On 14.11.2017 16:36, Eric Sandeen wrote:
> 
> 
> On 11/13/17 9:50 AM, Nikolay Borisov wrote:
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>
>> V3: 
>>  * Added tests for -r parameter error handling
>>  tests/xfs/900     |  98 ++++++++++++++++++++++++++++++++++++++++++
>>  tests/xfs/900.out | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 222 insertions(+)
>>  create mode 100755 tests/xfs/900
>>  create mode 100644 tests/xfs/900.out
>>
>> diff --git a/tests/xfs/900 b/tests/xfs/900
>> new file mode 100755
>> index 0000000..570f7d6
>> --- /dev/null
>> +++ b/tests/xfs/900
>> @@ -0,0 +1,98 @@
>> +#! /bin/bash
>> +# FS QA Test No. 900
>> +#-----------------------------------------------------------------------
>> +# Copyright (c) 2017 SUSE Linux Products GmbH. All Rights Reserved.
>> +# Author: Nikolay Borisov <nborisov@suse.com>
>> +#
>> +#
>> +# This program is free software; you can redistribute it and/or
>> +# modify it under the terms of the GNU General Public License as
>> +# published by the Free Software Foundation.
>> +#
>> +# This program is distributed in the hope that it would be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write the Free Software Foundation,
>> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>> +#-----------------------------------------------------------------------
>> +#
>> +
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1	# failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +	cd /
>> +	rm -f $tmp.*
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/punch
>> +
>> +# remove previous $seqres.full before test
>> +rm -f $seqres.full
>> +
>> +# real QA test starts here
>> +
>> +# Modify as appropriate.
>> +_supported_fs generic>
> This says generic but you have it under xfs/ tests, is that intentional?

Nope, likely a copy-paste remnant. Will remove in next iteration

> 
>> +_supported_os Linux
>> +_require_scratch
>> +_require_xfs_io_command "falloc"
>> +_require_xfs_io_command "fiemap" "-r 0 4k"
> 
> 
>> +
>> +_scratch_mkfs > $seqres.full 2>&1
> 
> Do you need to explicitly mkfs w/ a 4k block size so all the
> below tests on 4k boundaries will pass?
> 
> (IOWs does this work on 2k or 8k fs block size?)
> 
> -Eric
>