| Message ID | 1503501082-16983-3-git-send-email-nborisov@suse.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 8/23/17 10:11 AM, Nikolay Borisov wrote: > So far fiemap used some rather convoluted logic to terminate the iteration and > calculate the number of extents to pass to fm_extent_count. Simplify this by: > > * Get the whole number of extents that this file has and keep iterating until > we've printed all extents > > * Always use a hard-coded batch of 32 extents so we don't have to worry about > any extra calculations As discussed on IRC, these types of changes: > - if ((*cur_extent + 1) == max_extents) > + if (*cur_extent == max_extents) > return; are a functional change not described in the changelog above; they should be in their own patch with their own changelog, explaining why the test was off by one, and what this fixes. This stuff is complex enough that it's not obvious to the casual reader now, and certainly won't be a few years later. ;) Thanks, -Eric > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > io/fiemap.c | 37 +++++++++++++++++++++---------------- > 1 file changed, 21 insertions(+), 16 deletions(-) > > diff --git a/io/fiemap.c b/io/fiemap.c > index 30c49112e089..ef54b265ab91 100644 > --- a/io/fiemap.c > +++ b/io/fiemap.c > @@ -23,6 +23,8 @@ > #include "init.h" > #include "io.h" > > +#define EXTENT_BATCH 32 > + > static cmdinfo_t fiemap_cmd; > static const __u64 blocksize = 512; > static int max_extents = 0; > @@ -95,7 +97,7 @@ print_verbose( > memset(lbuf, 0, sizeof(lbuf)); > } > > - if ((*cur_extent + 1) == max_extents) > + if (*cur_extent == max_extents) > return; > > snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", lstart, > @@ -137,7 +139,7 @@ print_plain( > (*cur_extent)++; > } > > - if ((*cur_extent + 1) == max_extents) > + if (*cur_extent == max_extents) > return; > > printf("\t%d: [%llu..%llu]: %llu..%llu", *cur_extent, > @@ -215,7 +217,7 @@ fiemap_f( > char **argv) > { > struct fiemap *fiemap; > - int num_extents = 32; > + int num_extents; > int last = 0; > int lflag = 0; > int vflag = 0; > @@ -251,10 +253,15 @@ fiemap_f( > } > } > > - if (max_extents) > - num_extents = min(num_extents, max_extents); > + ret = get_extent_count(file->fd, last_logical, -1); > + if (ret < 0) { > + exitcode = 1; > + return 0; > + } > + num_extents = ret; > + > map_size = sizeof(struct fiemap) + > - (num_extents * sizeof(struct fiemap_extent)); > + (EXTENT_BATCH * sizeof(struct fiemap_extent)); > fiemap = malloc(map_size); > if (!fiemap) { > fprintf(stderr, _("%s: malloc of %d bytes failed.\n"), > @@ -265,16 +272,14 @@ fiemap_f( > > printf("%s:\n", file->name); > > - while (!last && ((cur_extent + 1) != max_extents)) { > - if (max_extents) > - num_extents = min(num_extents, > - max_extents - (cur_extent + 1)); > + while (!last && num_extents) { > > + /* Query a batch worth of extents */ > memset(fiemap, 0, map_size); > fiemap->fm_flags = fiemap_flags; > fiemap->fm_start = last_logical; > fiemap->fm_length = -1LL; > - fiemap->fm_extent_count = num_extents; > + fiemap->fm_extent_count = EXTENT_BATCH; > > ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap); > if (ret < 0) { > @@ -307,17 +312,17 @@ fiemap_f( > print_plain(extent, lflag, blocksize, > &cur_extent, &last_logical); > > - if (extent->fe_flags & FIEMAP_EXTENT_LAST) { > + if (extent->fe_flags & FIEMAP_EXTENT_LAST || > + cur_extent == max_extents) { > last = 1; > break; > } > - > - if ((cur_extent + 1) == max_extents) > - break; > } > + > + num_extents -= fiemap->fm_mapped_extents; > } > > - if ((cur_extent + 1) == max_extents) > + if (cur_extent == max_extents) > goto out; > > memset(&st, 0, sizeof(st)); > -- 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/23/17 10:51 AM, Eric Sandeen wrote: > On 8/23/17 10:11 AM, Nikolay Borisov wrote: >> So far fiemap used some rather convoluted logic to terminate the iteration and >> calculate the number of extents to pass to fm_extent_count. Simplify this by: >> >> * Get the whole number of extents that this file has and keep iterating until >> we've printed all extents >> >> * Always use a hard-coded batch of 32 extents so we don't have to worry about >> any extra calculations > > As discussed on IRC, these types of changes: > >> - if ((*cur_extent + 1) == max_extents) >> + if (*cur_extent == max_extents) >> return; > > are a functional change not described in the changelog above; they should be > in their own patch with their own changelog, explaining why the test was off by one, > and what this fixes. This stuff is complex enough that it's not obvious to the > casual reader now, and certainly won't be a few years later. ;) Ok, so this is fixing the "-n" argument, to actually print $ARG extents instead of $ARG - 1 extents. instead of today: # xfs_io -c "fiemap -n 4" holefile holefile: 0: [0..7]: hole 1: [8..15]: 172560120..172560127 2: [16..23]: hole now it does more expected: # io/xfs_io -c "fiemap -n 4" holefile holefile: 0: [0..7]: hole 1: [8..15]: 172560120..172560127 2: [16..23]: hole 3: [24..31]: 172560136..172560143 so it probably just needs a doc fix along w/ this behavior fix. (note, bmap behaves the same way, except properly): # xfs_io -c "bmap -n 4" holefile holefile: 0: [0..7]: hole 1: [8..15]: 172560120..172560127 2: [16..23]: hole 3: [24..31]: 172560136..172560143 Thanks, -Eric > Thanks, > -Eric > -- 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 30c49112e089..ef54b265ab91 100644 --- a/io/fiemap.c +++ b/io/fiemap.c @@ -23,6 +23,8 @@ #include "init.h" #include "io.h" +#define EXTENT_BATCH 32 + static cmdinfo_t fiemap_cmd; static const __u64 blocksize = 512; static int max_extents = 0; @@ -95,7 +97,7 @@ print_verbose( memset(lbuf, 0, sizeof(lbuf)); } - if ((*cur_extent + 1) == max_extents) + if (*cur_extent == max_extents) return; snprintf(lbuf, sizeof(lbuf), "[%llu..%llu]:", lstart, @@ -137,7 +139,7 @@ print_plain( (*cur_extent)++; } - if ((*cur_extent + 1) == max_extents) + if (*cur_extent == max_extents) return; printf("\t%d: [%llu..%llu]: %llu..%llu", *cur_extent, @@ -215,7 +217,7 @@ fiemap_f( char **argv) { struct fiemap *fiemap; - int num_extents = 32; + int num_extents; int last = 0; int lflag = 0; int vflag = 0; @@ -251,10 +253,15 @@ fiemap_f( } } - if (max_extents) - num_extents = min(num_extents, max_extents); + ret = get_extent_count(file->fd, last_logical, -1); + if (ret < 0) { + exitcode = 1; + return 0; + } + num_extents = ret; + map_size = sizeof(struct fiemap) + - (num_extents * sizeof(struct fiemap_extent)); + (EXTENT_BATCH * sizeof(struct fiemap_extent)); fiemap = malloc(map_size); if (!fiemap) { fprintf(stderr, _("%s: malloc of %d bytes failed.\n"), @@ -265,16 +272,14 @@ fiemap_f( printf("%s:\n", file->name); - while (!last && ((cur_extent + 1) != max_extents)) { - if (max_extents) - num_extents = min(num_extents, - max_extents - (cur_extent + 1)); + while (!last && num_extents) { + /* Query a batch worth of extents */ memset(fiemap, 0, map_size); fiemap->fm_flags = fiemap_flags; fiemap->fm_start = last_logical; fiemap->fm_length = -1LL; - fiemap->fm_extent_count = num_extents; + fiemap->fm_extent_count = EXTENT_BATCH; ret = ioctl(file->fd, FS_IOC_FIEMAP, (unsigned long)fiemap); if (ret < 0) { @@ -307,17 +312,17 @@ fiemap_f( print_plain(extent, lflag, blocksize, &cur_extent, &last_logical); - if (extent->fe_flags & FIEMAP_EXTENT_LAST) { + if (extent->fe_flags & FIEMAP_EXTENT_LAST || + cur_extent == max_extents) { last = 1; break; } - - if ((cur_extent + 1) == max_extents) - break; } + + num_extents -= fiemap->fm_mapped_extents; } - if ((cur_extent + 1) == max_extents) + if (cur_extent == max_extents) goto out; memset(&st, 0, sizeof(st));
So far fiemap used some rather convoluted logic to terminate the iteration and calculate the number of extents to pass to fm_extent_count. Simplify this by: * Get the whole number of extents that this file has and keep iterating until we've printed all extents * Always use a hard-coded batch of 32 extents so we don't have to worry about any extra calculations Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- io/fiemap.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-)