| Message ID | 1503575272-28263-7-git-send-email-nborisov@suse.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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)); > -- 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
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)); >> > -- 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
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 -- 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
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 -- 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
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 -- 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
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));
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(-)