[PATCH 0/6] Fiemap refactoring

[PATCH 0/6] Fiemap refactoring

[Nikolay Borisov]

Hello, 

Here are a bunch of patches resulting from review feedback on my earlier 
attempts to implement ranged fiemap lookup. All but the last patch are simple 
cleanup, aimed at making the code mode understandable and maintainable. They 
bear no functional changes. 

The last patch is a bug fix (at least I percieve it as a bug) for the -n 
argument behavior. More explanation in the patch commit log. 

I decided it's more prudent to put those cleanups as separate series since I've
run out of juice working on that and will perhaps come back to the actual 
ranged support later, yet I'd like to see those merged. 

Nikolay Borisov (6):
  fiemap: Remove blocksize variable
  fiemap: Make max_extents a global var
  fiemap: Eliminate num_extents
  fiemap: De-obfuscate last_logical and cur_extent manipulation
  fiemap: Factor out common code used for printing holes
  fiemap: Fix semantics of max_extents (-n arguments)

 io/fiemap.c | 174 ++++++++++++++++++++++++++++++------------------------------
 1 file changed, 86 insertions(+), 88 deletions(-)

-- 
2.7.4

[PATCH 1/6] fiemap: Remove blocksize variable

[Nikolay Borisov]

The blocksize variable was hardcoded to 512 bytes and was passed to various
functions. This introduced a lot of redundancy since we can just as well use
the BTOBBT macro. So let's do that and eliminate all usage of the blocksize var.
No functional changes.

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

diff --git a/io/fiemap.c b/io/fiemap.c
index 75e882057362..ed3a8be1dcc4 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -52,7 +52,6 @@ fiemap_help(void)
 static void
 print_verbose(
 	struct fiemap_extent	*extent,
-	int			blocksize,
 	int			foff_w,
 	int			boff_w,
 	int			tot_w,
@@ -69,10 +68,10 @@ print_verbose(
 	char			bbuf[48];
 	char			flgbuf[16];
 
-	llast = *last_logical / blocksize;
-	lstart = extent->fe_logical / blocksize;
-	len = extent->fe_length / blocksize;
-	block = extent->fe_physical / blocksize;
+	llast = BTOBBT(*last_logical);
+	lstart = BTOBBT(extent->fe_logical);
+	len = BTOBBT(extent->fe_length);
+	block = BTOBBT(extent->fe_physical);
 
 	memset(lbuf, 0, sizeof(lbuf));
 	memset(bbuf, 0, sizeof(bbuf));
@@ -112,7 +111,6 @@ static void
 print_plain(
 	struct fiemap_extent	*extent,
 	int			lflag,
-	int			blocksize,
 	int			max_extents,
 	int			*cur_extent,
 	__u64			*last_logical)
@@ -122,10 +120,10 @@ print_plain(
 	__u64			block;
 	__u64			len;
 
-	llast = *last_logical / blocksize;
-	lstart = extent->fe_logical / blocksize;
-	len = extent->fe_length / blocksize;
-	block = extent->fe_physical / blocksize;
+	llast = BTOBBT(*last_logical);
+	lstart = BTOBBT(extent->fe_logical);
+	len = BTOBBT(extent->fe_length);
+	block = BTOBBT(extent->fe_physical);
 
 	if (lstart != llast) {
 		printf("\t%d: [%llu..%llu]: hole", *cur_extent,
@@ -159,13 +157,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;
@@ -176,9 +173,9 @@ calc_print_format(
 	for (i = 0; i < fiemap->fm_mapped_extents; i++) {
 
 		extent = &fiemap->fm_extents[i];
-		logical = extent->fe_logical / blocksize;
-		len = extent->fe_length / blocksize;
-		block = extent->fe_physical / blocksize;
+		logical = BTOBBT(extent->fe_logical);
+		len = BTOBBT(extent->fe_length);
+		block = BTOBBT(extent->fe_physical);
 
 		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]", logical,
 			 logical + len - 1);
@@ -214,7 +211,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;
 
@@ -281,19 +277,17 @@ fiemap_f(
 			extent = &fiemap->fm_extents[i];
 			if (vflag) {
 				if (cur_extent == 0) {
-					calc_print_format(fiemap, blocksize,
-							  &foff_w, &boff_w,
-							  &tot_w, &flg_w);
+					calc_print_format(fiemap, &foff_w,
+							  &boff_w, &tot_w,
+							  &flg_w);
 				}
 
-				print_verbose(extent, blocksize, foff_w,
-					      boff_w, tot_w, flg_w,
-					      max_extents, &cur_extent,
+				print_verbose(extent, 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);
+				print_plain(extent, lflag, max_extents,
+					    &cur_extent, &last_logical);
 
 			if (extent->fe_flags & FIEMAP_EXTENT_LAST) {
 				last = 1;
@@ -321,17 +315,17 @@ fiemap_f(
 		char	lbuf[32];
 
 		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:",
-			 last_logical / blocksize, (st.st_size / blocksize) - 1);
+			 BTOBBT(last_logical), BTOBBT(st.st_size) - 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);
+			       BTOBBT(st.st_size - last_logical));
 		} else {
 			printf("\t%d: %s %s", cur_extent, lbuf,
 			       _("hole"));
 			if (lflag)
 				printf(_(" %llu blocks\n"),
-				       (st.st_size - last_logical) / blocksize);
+				       BTOBBT(st.st_size - last_logical));
 			else
 				printf("\n");
 		}
-- 
2.7.4

[PATCH 2/6] fiemap: Make max_extents a global var

[Nikolay Borisov]

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

diff --git a/io/fiemap.c b/io/fiemap.c
index ed3a8be1dcc4..d284248e4fe1 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -24,6 +24,7 @@
 #include "io.h"
 
 static cmdinfo_t fiemap_cmd;
+static int max_extents = 0;
 
 static void
 fiemap_help(void)
@@ -56,7 +57,6 @@ print_verbose(
 	int			boff_w,
 	int			tot_w,
 	int			flg_w,
-	int			max_extents,
 	int			*cur_extent,
 	__u64			*last_logical)
 {
@@ -111,7 +111,6 @@ static void
 print_plain(
 	struct fiemap_extent	*extent,
 	int			lflag,
-	int			max_extents,
 	int			*cur_extent,
 	__u64			*last_logical)
 {
@@ -196,7 +195,6 @@ fiemap_f(
 	char		**argv)
 {
 	struct fiemap	*fiemap;
-	int		max_extents = 0;
 	int		num_extents = 32;
 	int		last = 0;
 	int		lflag = 0;
@@ -283,11 +281,11 @@ fiemap_f(
 				}
 
 				print_verbose(extent, foff_w, boff_w, tot_w,
-					      flg_w, max_extents, &cur_extent,
+					      flg_w, &cur_extent,
 					      &last_logical);
 			} else
-				print_plain(extent, lflag, max_extents,
-					    &cur_extent, &last_logical);
+				print_plain(extent, lflag, &cur_extent,
+					    &last_logical);
 
 			if (extent->fe_flags & FIEMAP_EXTENT_LAST) {
 				last = 1;
-- 
2.7.4

[PATCH 3/6] fiemap: Eliminate num_extents

[Nikolay Borisov]

Fiemap has this rather convoluted logic to calculate the number of extents to
query. This introduces needless complexity with no real benefit. Remove
num_extents and instead hardcode the number of extents we query for in a single
go to 32. No functional changes

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

diff --git a/io/fiemap.c b/io/fiemap.c
index d284248e4fe1..a31db790c86d 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 int max_extents = 0;
 
@@ -195,7 +197,6 @@ fiemap_f(
 	char		**argv)
 {
 	struct fiemap	*fiemap;
-	int		num_extents = 32;
 	int		last = 0;
 	int		lflag = 0;
 	int		vflag = 0;
@@ -231,10 +232,8 @@ fiemap_f(
 		}
 	}
 
-	if (max_extents)
-		num_extents = min(num_extents, max_extents);
 	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"),
@@ -246,15 +245,12 @@ 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;
+		fiemap->fm_extent_count = EXTENT_BATCH;
 
 		ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap);
 		if (ret < 0) {
-- 
2.7.4

[PATCH 4/6] fiemap: De-obfuscate last_logical and cur_extent manipulation

[Nikolay Borisov]

last_logical and cur_extent are being passed by reference to the printing
functions and the in turn modify those variables. This makes it a bit harder to
reason about the code. So change the printing function to take those 2 arguemnts
by value and move the manipulation logic in fiemap_f. Furthermore, the printing
function now return the number of extents they have printed (either 1 or 2,
dependent on whether we've hit the -n limit). No functional changes

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

diff --git a/io/fiemap.c b/io/fiemap.c
index a31db790c86d..0f04b874fd5f 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -52,15 +52,15 @@ fiemap_help(void)
 "\n"));
 }
 
-static void
+static int
 print_verbose(
 	struct fiemap_extent	*extent,
 	int			foff_w,
 	int			boff_w,
 	int			tot_w,
 	int			flg_w,
-	int			*cur_extent,
-	__u64			*last_logical)
+	int			cur_extent,
+	__u64			last_logical)
 {
 	__u64			lstart;
 	__u64			llast;
@@ -70,7 +70,7 @@ print_verbose(
 	char			bbuf[48];
 	char			flgbuf[16];
 
-	llast = BTOBBT(*last_logical);
+	llast = BTOBBT(last_logical);
 	lstart = BTOBBT(extent->fe_logical);
 	len = BTOBBT(extent->fe_length);
 	block = BTOBBT(extent->fe_physical);
@@ -78,7 +78,7 @@ print_verbose(
 	memset(lbuf, 0, sizeof(lbuf));
 	memset(bbuf, 0, sizeof(bbuf));
 
-	if (*cur_extent == 0) {
+	if (cur_extent == 0) {
 		printf("%4s: %-*s %-*s %*s %*s\n", _("EXT"),
 			foff_w, _("FILE-OFFSET"),
 			boff_w, _("BLOCK-RANGE"),
@@ -89,57 +89,56 @@ print_verbose(
 	if (lstart != llast) {
 		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", llast,
 			 lstart - 1ULL);
-		printf("%4d: %-*s %-*s %*llu\n", *cur_extent, foff_w, lbuf,
+		printf("%4d: %-*s %-*s %*llu\n", cur_extent, foff_w, lbuf,
 		       boff_w, _("hole"), tot_w, lstart - llast);
-		(*cur_extent)++;
 		memset(lbuf, 0, sizeof(lbuf));
+		cur_extent++;
 	}
 
-	if ((*cur_extent + 1) == max_extents)
-		return;
+	if ((cur_extent + 1) == max_extents)
+		return 1;
 
 	snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", lstart,
 		 lstart + len - 1ULL);
 	snprintf(bbuf, sizeof(bbuf), "%llu..%llu", block, block + len - 1ULL);
 	snprintf(flgbuf, sizeof(flgbuf), "0x%x", extent->fe_flags);
-	printf("%4d: %-*s %-*s %*llu %*s\n", *cur_extent, foff_w, lbuf,
+	printf("%4d: %-*s %-*s %*llu %*s\n", cur_extent, foff_w, lbuf,
 	       boff_w, bbuf, tot_w, len, flg_w, flgbuf);
 
-	(*cur_extent)++;
-	*last_logical = extent->fe_logical + extent->fe_length;
+	return 2;
 }
 
-static void
+static int
 print_plain(
 	struct fiemap_extent	*extent,
 	int			lflag,
-	int			*cur_extent,
-	__u64			*last_logical)
+	int			cur_extent,
+	__u64			last_logical)
 {
 	__u64			lstart;
 	__u64			llast;
 	__u64			block;
 	__u64			len;
 
-	llast = BTOBBT(*last_logical);
+	llast = BTOBBT(last_logical);
 	lstart = BTOBBT(extent->fe_logical);
 	len = BTOBBT(extent->fe_length);
 	block = BTOBBT(extent->fe_physical);
 
 	if (lstart != llast) {
-		printf("\t%d: [%llu..%llu]: hole", *cur_extent,
+		printf("\t%d: [%llu..%llu]: hole", cur_extent,
 		       llast, lstart - 1ULL);
 		if (lflag)
 			printf(_(" %llu blocks\n"), lstart - llast);
 		else
 			printf("\n");
-		(*cur_extent)++;
+		cur_extent++;
 	}
 
-	if ((*cur_extent + 1) == max_extents)
-		return;
+	if ((cur_extent + 1) == max_extents)
+		return 1;
 
-	printf("\t%d: [%llu..%llu]: %llu..%llu", *cur_extent,
+	printf("\t%d: [%llu..%llu]: %llu..%llu", cur_extent,
 	       lstart, lstart + len - 1ULL, block,
 	       block + len - 1ULL);
 
@@ -147,8 +146,7 @@ print_plain(
 		printf(_(" %llu blocks\n"), len);
 	else
 		printf("\n");
-	(*cur_extent)++;
-	*last_logical = extent->fe_logical + extent->fe_length;
+	return 2;
 }
 
 /*
@@ -267,6 +265,7 @@ fiemap_f(
 
 		for (i = 0; i < fiemap->fm_mapped_extents; i++) {
 			struct fiemap_extent	*extent;
+			int num_printed = 0;
 
 			extent = &fiemap->fm_extents[i];
 			if (vflag) {
@@ -276,12 +275,17 @@ fiemap_f(
 							  &flg_w);
 				}
 
-				print_verbose(extent, foff_w, boff_w, tot_w,
-					      flg_w, &cur_extent,
-					      &last_logical);
+				num_printed = print_verbose(extent, foff_w,
+							    boff_w, tot_w,
+							    flg_w, cur_extent,
+							    last_logical);
 			} else
-				print_plain(extent, lflag, &cur_extent,
-					    &last_logical);
+				num_printed = print_plain(extent, lflag,
+							  cur_extent,
+							  last_logical);
+
+			cur_extent += num_printed;
+			last_logical = extent->fe_logical + extent->fe_length;
 
 			if (extent->fe_flags & FIEMAP_EXTENT_LAST) {
 				last = 1;
-- 
2.7.4

[PATCH 5/6] fiemap: Factor out common code used for printing holes

[Nikolay Borisov]

The code responsible for printing holes is scattered across 3 places:
plain print function, verbose print function and in the block handling EOF hole.
Introduce a new function factoring out the common code and replace the 3 sites
where the code is used with it. This reduces duplication and makes it apparent
when we are printing holes. No functional changes.

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

diff --git a/io/fiemap.c b/io/fiemap.c
index 0f04b874fd5f..44a64870d711 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -52,6 +52,36 @@ fiemap_help(void)
 "\n"));
 }
 
+static void
+print_hole(
+	   int		foff_w,
+	   int		boff_w,
+	   int		tot_w,
+	   int		cur_extent,
+	   int		lflag,
+	   bool		plain,
+	   __u64	llast,
+	   __u64	lstart)
+{
+	   char		lbuf[48];
+
+	   if (plain) {
+		printf("\t%d: [%llu..%llu]: hole", cur_extent,
+		       llast, lstart - 1ULL);
+		if (lflag)
+			printf(_(" %llu blocks\n"), lstart - llast);
+		else
+			printf("\n");
+	   } else {
+		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", llast,
+			 lstart - 1ULL);
+		printf("%4d: %-*s %-*s %*llu\n", cur_extent, foff_w, lbuf,
+		       boff_w, _("hole"), tot_w, lstart - llast);
+	   }
+
+
+}
+
 static int
 print_verbose(
 	struct fiemap_extent	*extent,
@@ -87,11 +117,8 @@ print_verbose(
 	}
 
 	if (lstart != llast) {
-		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", llast,
-			 lstart - 1ULL);
-		printf("%4d: %-*s %-*s %*llu\n", cur_extent, foff_w, lbuf,
-		       boff_w, _("hole"), tot_w, lstart - llast);
-		memset(lbuf, 0, sizeof(lbuf));
+		print_hole(foff_w, boff_w, tot_w, cur_extent, 0, false, llast,
+			   lstart);
 		cur_extent++;
 	}
 
@@ -126,12 +153,7 @@ print_plain(
 	block = BTOBBT(extent->fe_physical);
 
 	if (lstart != llast) {
-		printf("\t%d: [%llu..%llu]: hole", cur_extent,
-		       llast, lstart - 1ULL);
-		if (lflag)
-			printf(_(" %llu blocks\n"), lstart - llast);
-		else
-			printf("\n");
+		print_hole(0, 0, 0, cur_extent, lflag, true, llast, lstart);
 		cur_extent++;
 	}
 
@@ -309,25 +331,9 @@ fiemap_f(
 		return 0;
 	}
 
-	if (cur_extent && last_logical < st.st_size) {
-		char	lbuf[32];
-
-		snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:",
-			 BTOBBT(last_logical), BTOBBT(st.st_size) - 1);
-		if (vflag) {
-			printf("%4d: %-*s %-*s %*llu\n", cur_extent,
-			       foff_w, lbuf, boff_w, _("hole"), tot_w,
-			       BTOBBT(st.st_size - last_logical));
-		} else {
-			printf("\t%d: %s %s", cur_extent, lbuf,
-			       _("hole"));
-			if (lflag)
-				printf(_(" %llu blocks\n"),
-				       BTOBBT(st.st_size - last_logical));
-			else
-				printf("\n");
-		}
-	}
+	if (cur_extent && last_logical < st.st_size)
+		print_hole(foff_w, boff_w, tot_w, cur_extent, lflag, !vflag,
+			   BTOBBT(last_logical), BTOBBT(st.st_size));
 
 out:
 	free(fiemap);
-- 
2.7.4

[PATCH 6/6] fiemap: Fix semantics of max_extents (-n arguments)

[Nikolay Borisov]

Currently the semantics of the -n argument are a bit idiosyncratic. We want the
argument to be the limit of extents that are going to be output by the tool. This
is clearly broken now as evident from the following example on a fragmented file:

xfs_io -c "fiemap -v -n 5" 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..31]:        hole                     8
   3: [32..39]:        897851392..897851399     8   0x0

So we want at most 5 extents printed, yet we get 4. So we always print n - 1
extents.

With this modification the output looks like:

xfs_io -c "fiemap -v -n 5" 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..31]:        hole                     8
   3: [32..39]:        897851392..897851399     8   0x0
   4: [40..47]:        hole                     8

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

diff --git a/io/fiemap.c b/io/fiemap.c
index 44a64870d711..7b275275465d 100644
--- a/io/fiemap.c
+++ b/io/fiemap.c
@@ -122,7 +122,7 @@ print_verbose(
 		cur_extent++;
 	}
 
-	if ((cur_extent + 1) == max_extents)
+	if (cur_extent == max_extents)
 		return 1;
 
 	snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", lstart,
@@ -157,7 +157,7 @@ print_plain(
 		cur_extent++;
 	}
 
-	if ((cur_extent + 1) == max_extents)
+	if (cur_extent == max_extents)
 		return 1;
 
 	printf("\t%d: [%llu..%llu]: %llu..%llu", cur_extent,
@@ -264,7 +264,7 @@ fiemap_f(
 
 	printf("%s:\n", file->name);
 
-	while (!last && ((cur_extent + 1) != max_extents)) {
+	while (!last && (cur_extent != max_extents)) {
 
 		memset(fiemap, 0, map_size);
 		fiemap->fm_flags = fiemap_flags;
@@ -314,12 +314,12 @@ fiemap_f(
 				break;
 			}
 
-			if ((cur_extent + 1) == max_extents)
+			if (cur_extent == max_extents)
 				break;
 		}
 	}
 
-	if ((cur_extent + 1) == max_extents)
+	if (cur_extent  == max_extents)
 		goto out;
 
 	memset(&st, 0, sizeof(st));
-- 
2.7.4

Re: [PATCH 6/6] fiemap: Fix semantics of max_extents (-n arguments)

[Eric Sandeen]

On 8/24/17 6:47 AM, Nikolay Borisov wrote:
> Currently the semantics of the -n argument are a bit idiosyncratic. We want the
> argument to be the limit of extents that are going to be output by the tool. This
> is clearly broken now as evident from the following example on a fragmented file:
> 
> xfs_io -c "fiemap -v -n 5" 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..31]:        hole                     8
>    3: [32..39]:        897851392..897851399     8   0x0
> 
> So we want at most 5 extents printed, yet we get 4. So we always print n - 1
> extents.
> 
> With this modification the output looks like:
> 
> xfs_io -c "fiemap -v -n 5" 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..31]:        hole                     8
>    3: [32..39]:        897851392..897851399     8   0x0
>    4: [40..47]:        hole                     8
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  io/fiemap.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/io/fiemap.c b/io/fiemap.c
> index 44a64870d711..7b275275465d 100644
> --- a/io/fiemap.c
> +++ b/io/fiemap.c
> @@ -122,7 +122,7 @@ print_verbose(
>  		cur_extent++;
>  	}
>  
> -	if ((cur_extent + 1) == max_extents)
> +	if (cur_extent == max_extents)
>  		return 1;
>  
>  	snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", lstart,
> @@ -157,7 +157,7 @@ print_plain(
>  		cur_extent++;
>  	}
>  
> -	if ((cur_extent + 1) == max_extents)
> +	if (cur_extent == max_extents)
>  		return 1;
>  
>  	printf("\t%d: [%llu..%llu]: %llu..%llu", cur_extent,
> @@ -264,7 +264,7 @@ fiemap_f(
>  
>  	printf("%s:\n", file->name);
>  
> -	while (!last && ((cur_extent + 1) != max_extents)) {
> +	while (!last && (cur_extent != max_extents)) {

In my testing, for a bare fiemap (no -n) we get here with
cur_extent == max_extents == 0 and so nothing happens:

# xfs_io -c fiemap testfile
testfile:
#

(this causes every xfstest which invokes fiemap to fail).

I think initializing max_extents to -1 is a simple way to fix this.

>  
>  		memset(fiemap, 0, map_size);
>  		fiemap->fm_flags = fiemap_flags;
> @@ -314,12 +314,12 @@ fiemap_f(
>  				break;
>  			}
>  
> -			if ((cur_extent + 1) == max_extents)
> +			if (cur_extent == max_extents)
>  				break;
>  		}
>  	}
>  
> -	if ((cur_extent + 1) == max_extents)
> +	if (cur_extent  == max_extents)
                       ^ extra space ;)

Thanks,
-Eric


>  		goto out;
>  
>  	memset(&st, 0, sizeof(st));
> 

Re: [PATCH 6/6] fiemap: Fix semantics of max_extents (-n arguments)

[Nikolay Borisov]

On 24.08.2017 19:03, Eric Sandeen wrote:
> On 8/24/17 6:47 AM, Nikolay Borisov wrote:
>> Currently the semantics of the -n argument are a bit idiosyncratic. We want the
>> argument to be the limit of extents that are going to be output by the tool. This
>> is clearly broken now as evident from the following example on a fragmented file:
>>
>> xfs_io -c "fiemap -v -n 5" 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..31]:        hole                     8
>>    3: [32..39]:        897851392..897851399     8   0x0
>>
>> So we want at most 5 extents printed, yet we get 4. So we always print n - 1
>> extents.
>>
>> With this modification the output looks like:
>>
>> xfs_io -c "fiemap -v -n 5" 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..31]:        hole                     8
>>    3: [32..39]:        897851392..897851399     8   0x0
>>    4: [40..47]:        hole                     8
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  io/fiemap.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/io/fiemap.c b/io/fiemap.c
>> index 44a64870d711..7b275275465d 100644
>> --- a/io/fiemap.c
>> +++ b/io/fiemap.c
>> @@ -122,7 +122,7 @@ print_verbose(
>>  		cur_extent++;
>>  	}
>>  
>> -	if ((cur_extent + 1) == max_extents)
>> +	if (cur_extent == max_extents)
>>  		return 1;
>>  
>>  	snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", lstart,
>> @@ -157,7 +157,7 @@ print_plain(
>>  		cur_extent++;
>>  	}
>>  
>> -	if ((cur_extent + 1) == max_extents)
>> +	if (cur_extent == max_extents)
>>  		return 1;
>>  
>>  	printf("\t%d: [%llu..%llu]: %llu..%llu", cur_extent,
>> @@ -264,7 +264,7 @@ fiemap_f(
>>  
>>  	printf("%s:\n", file->name);
>>  
>> -	while (!last && ((cur_extent + 1) != max_extents)) {
>> +	while (!last && (cur_extent != max_extents)) {
> 
> In my testing, for a bare fiemap (no -n) we get here with
> cur_extent == max_extents == 0 and so nothing happens:
> 
> # xfs_io -c fiemap testfile
> testfile:
> #
> 
> (this causes every xfstest which invokes fiemap to fail).
> 
> I think initializing max_extents to -1 is a simple way to fix this.

Ah, you are right, I've come to the same conclusion during my testing of
earlier versions. Would you care to fold that fix if that's the only
problem found?

> 
>>  
>>  		memset(fiemap, 0, map_size);
>>  		fiemap->fm_flags = fiemap_flags;
>> @@ -314,12 +314,12 @@ fiemap_f(
>>  				break;
>>  			}
>>  
>> -			if ((cur_extent + 1) == max_extents)
>> +			if (cur_extent == max_extents)
>>  				break;
>>  		}
>>  	}
>>  
>> -	if ((cur_extent + 1) == max_extents)
>> +	if (cur_extent  == max_extents)
>                        ^ extra space ;)
> 
> Thanks,
> -Eric
> 
> 
>>  		goto out;
>>  
>>  	memset(&st, 0, sizeof(st));
>>
> 

Re: [PATCH 6/6] fiemap: Fix semantics of max_extents (-n arguments)

[Eric Sandeen]

On 8/24/17 11:06 AM, Nikolay Borisov wrote:
> 
> 
> On 24.08.2017 19:03, Eric Sandeen wrote:

...

>> In my testing, for a bare fiemap (no -n) we get here with
>> cur_extent == max_extents == 0 and so nothing happens:
>>
>> # xfs_io -c fiemap testfile
>> testfile:
>> #
>>
>> (this causes every xfstest which invokes fiemap to fail).
>>
>> I think initializing max_extents to -1 is a simple way to fix this.
> 
> Ah, you are right, I've come to the same conclusion during my testing of
> earlier versions. Would you care to fold that fix if that's the only
> problem found?

Sure, I can fix it on the way in.

-Eric

Re: [PATCH 6/6] fiemap: Fix semantics of max_extents (-n arguments)

[Darrick J. Wong]

On Thu, Aug 24, 2017 at 02:47:52PM +0300, Nikolay Borisov wrote:
> Currently the semantics of the -n argument are a bit idiosyncratic. We want the
> argument to be the limit of extents that are going to be output by the tool. This
> is clearly broken now as evident from the following example on a fragmented file:

Please update the documentation, since the xfs_io fiemap section refers
readers to xfs_bmap(8), which says:

"If this [-n] option is given, xfs_bmap obtains the extent list of the
file in groups of num_extents extents."

Which is no longer correct, because now -n limits the number of records
output, if I'm reading this patch correctly.  TBH I think -n for bmap is
also wrong...

The other patches leading up to this one have been
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> xfs_io -c "fiemap -v -n 5" 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..31]:        hole                     8
>    3: [32..39]:        897851392..897851399     8   0x0
> 
> So we want at most 5 extents printed, yet we get 4. So we always print n - 1
> extents.
> 
> With this modification the output looks like:
> 
> xfs_io -c "fiemap -v -n 5" 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..31]:        hole                     8
>    3: [32..39]:        897851392..897851399     8   0x0
>    4: [40..47]:        hole                     8
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  io/fiemap.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/io/fiemap.c b/io/fiemap.c
> index 44a64870d711..7b275275465d 100644
> --- a/io/fiemap.c
> +++ b/io/fiemap.c
> @@ -122,7 +122,7 @@ print_verbose(
>  		cur_extent++;
>  	}
>  
> -	if ((cur_extent + 1) == max_extents)
> +	if (cur_extent == max_extents)
>  		return 1;
>  
>  	snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", lstart,
> @@ -157,7 +157,7 @@ print_plain(
>  		cur_extent++;
>  	}
>  
> -	if ((cur_extent + 1) == max_extents)
> +	if (cur_extent == max_extents)
>  		return 1;
>  
>  	printf("\t%d: [%llu..%llu]: %llu..%llu", cur_extent,
> @@ -264,7 +264,7 @@ fiemap_f(
>  
>  	printf("%s:\n", file->name);
>  
> -	while (!last && ((cur_extent + 1) != max_extents)) {
> +	while (!last && (cur_extent != max_extents)) {
>  
>  		memset(fiemap, 0, map_size);
>  		fiemap->fm_flags = fiemap_flags;
> @@ -314,12 +314,12 @@ fiemap_f(
>  				break;
>  			}
>  
> -			if ((cur_extent + 1) == max_extents)
> +			if (cur_extent == max_extents)
>  				break;
>  		}
>  	}
>  
> -	if ((cur_extent + 1) == max_extents)
> +	if (cur_extent  == max_extents)
>  		goto out;
>  
>  	memset(&st, 0, sizeof(st));
> -- 
> 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 6/6] fiemap: Fix semantics of max_extents (-n arguments)

[Eric Sandeen]

On 8/24/17 12:51 PM, Darrick J. Wong wrote:
> On Thu, Aug 24, 2017 at 02:47:52PM +0300, Nikolay Borisov wrote:
>> Currently the semantics of the -n argument are a bit idiosyncratic. We want the
>> argument to be the limit of extents that are going to be output by the tool. This
>> is clearly broken now as evident from the following example on a fragmented file:
> 
> Please update the documentation, since the xfs_io fiemap section refers
> readers to xfs_bmap(8), which says:
> 
> "If this [-n] option is given, xfs_bmap obtains the extent list of the
> file in groups of num_extents extents."
> 
> Which is no longer correct, because now -n limits the number of records
> output, if I'm reading this patch correctly.  TBH I think -n for bmap is
> also wrong...

Yep.  That's fine as patch 7/6 I think - this patch doesn't make the
documentation any more wrong than it already is, but it does need to be
fixed in any case.

> The other patches leading up to this one have been
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks!  They looked good to me too modulo the one problem I found.

-Eric

[PATCH 7/6] xfs_bmap: fix -n documentation in manpage

[Eric Sandeen]

xfs_bmap's manpage mis-describes the behavior from the
-n option.  xfs_io's fiemap command references the xfs_bmap
manpage, and has the same problem:

-n does not change the query batch size, it limits the number
of extents displayed.

This has been true for 15+ years, so change the documentation
to match reality.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/man/man8/xfs_bmap.8 b/man/man8/xfs_bmap.8
index 098cfae..c725519 100644
--- a/man/man8/xfs_bmap.8
+++ b/man/man8/xfs_bmap.8
@@ -77,12 +77,11 @@ option is used.
 .BI \-n " num_extents"
 If this option is given,
 .B xfs_bmap
-obtains the extent list of the file in groups of
+will display at most
 .I num_extents
 extents. In the absence of
 .BR \-n ", " xfs_bmap
-queries the system for the number of extents in the file and uses that
-value to compute the group size.
+will display all extents in the file.
 .TP
 .B \-p
 If this option is used,

Re: [PATCH 7/6] xfs_bmap: fix -n documentation in manpage

[Darrick J. Wong]

On Thu, Aug 24, 2017 at 03:09:32PM -0500, Eric Sandeen wrote:
> xfs_bmap's manpage mis-describes the behavior from the
> -n option.  xfs_io's fiemap command references the xfs_bmap
> manpage, and has the same problem:
> 
> -n does not change the query batch size, it limits the number
> of extents displayed.
> 
> This has been true for 15+ years, so change the documentation
> to match reality.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

For patches 6 & 7,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> diff --git a/man/man8/xfs_bmap.8 b/man/man8/xfs_bmap.8
> index 098cfae..c725519 100644
> --- a/man/man8/xfs_bmap.8
> +++ b/man/man8/xfs_bmap.8
> @@ -77,12 +77,11 @@ option is used.
>  .BI \-n " num_extents"
>  If this option is given,
>  .B xfs_bmap
> -obtains the extent list of the file in groups of
> +will display at most
>  .I num_extents
>  extents. In the absence of
>  .BR \-n ", " xfs_bmap
> -queries the system for the number of extents in the file and uses that
> -value to compute the group size.
> +will display all extents in the file.
>  .TP
>  .B \-p
>  If this option is used,
> 
> --
> 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