* [PATCH] misc/e4defrag: output extent's status(written or unwritten) @ 2014-09-23 10:41 Xiaoguang Wang 2014-09-23 19:02 ` Andreas Dilger 0 siblings, 1 reply; 13+ messages in thread From: Xiaoguang Wang @ 2014-09-23 10:41 UTC (permalink / raw) To: linux-ext4; +Cc: tytso, darrick.wong, Xiaoguang Wang Before this patch, the output woulld be(e4defrag -c -v testfile): <File> [ext 1]: start 33796: logical 0: len 1 [ext 2]: start 9267: logical 1: len 29 [ext 3]: start 9296: logical 30: len 1 [ext 4]: start 9297: logical 31: len 29 [ext 5]: start 9326: logical 60: len 1 [ext 6]: start 9327: logical 61: len 29 ... [ext 32]: start 9265: logical 800: len 1 [ext 33]: start 9266: logical 900: len 1 Total/best extents 33/1 Average size per extent 61 KB Fragmentation score 51 [0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag] This file (/mnt/xfstests/scratch/test.10) does not need defragmentation. Done. After this patch, the output would be: <File> [ext 1]: start 33796: logical 0: len 1 [written] [ext 2]: start 9267: logical 1: len 29 [unwritten] [ext 3]: start 9296: logical 30: len 1 [written] [ext 4]: start 9297: logical 31: len 29 [unwritten] [ext 5]: start 9326: logical 60: len 1 [written] [ext 6]: start 9327: logical 61: len 29 [unwritten] ... [ext 32]: start 9265: logical 800: len 1 [unwritten] [ext 33]: start 9266: logical 900: len 1 [unwritten] Total/best extents 33/1 Average size per extent 61 KB Fragmentation score 51 [0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag] This file (/mnt/xfstests/scratch/test.10) does not need defragmentation. Done. Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> --- misc/e4defrag.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/misc/e4defrag.c b/misc/e4defrag.c index d0eac60..b39e263 100644 --- a/misc/e4defrag.c +++ b/misc/e4defrag.c @@ -129,6 +129,7 @@ struct fiemap_extent_data { __u64 len; /* blocks count */ __u64 logical; /* start logical block number */ ext4_fsblk_t physical; /* start physical block number */ + __u32 fe_flags; /* FIEMAP_EXTENT_* flags for this extent */ }; struct fiemap_extent_list { @@ -831,6 +832,7 @@ static int get_file_extents(int fd, struct fiemap_extent_list **ext_list_head) / block_size; ext_list->data.len = ext_buf[i].fe_length / block_size; + ext_list->data.fe_flags = ext_buf[i].fe_flags; ret = insert_extent_by_physical( ext_list_head, ext_list); @@ -1170,12 +1172,16 @@ static int file_statistic(const char *file, const struct stat64 *buf, /* Print extents info */ do { + char *status = ext_list_tmp->data.fe_flags & + FIEMAP_EXTENT_UNWRITTEN ? + "unwritten" : "written"; count++; printf("[ext %d]:\tstart %llu:\tlogical " - "%llu:\tlen %llu\n", count, - ext_list_tmp->data.physical, - ext_list_tmp->data.logical, - ext_list_tmp->data.len); + "%llu:\tlen %llu \t[%s]\n", count, + ext_list_tmp->data.physical, + ext_list_tmp->data.logical, + ext_list_tmp->data.len, + status); ext_list_tmp = ext_list_tmp->next; } while (ext_list_tmp != logical_list_head); -- 1.8.2.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] misc/e4defrag: output extent's status(written or unwritten) 2014-09-23 10:41 [PATCH] misc/e4defrag: output extent's status(written or unwritten) Xiaoguang Wang @ 2014-09-23 19:02 ` Andreas Dilger 2014-09-24 1:11 ` Xiaoguang Wang 0 siblings, 1 reply; 13+ messages in thread From: Andreas Dilger @ 2014-09-23 19:02 UTC (permalink / raw) To: Xiaoguang Wang Cc: <linux-ext4@vger.kernel.org>, <tytso@mit.edu>, <darrick.wong@oracle.com> It would be nice if the output would match the format used by filefrag. Cheers, Andreas > On Sep 23, 2014, at 12:41, Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> wrote: > > Before this patch, the output woulld be(e4defrag -c -v testfile): > <File> > [ext 1]: start 33796: logical 0: len 1 > [ext 2]: start 9267: logical 1: len 29 > [ext 3]: start 9296: logical 30: len 1 > [ext 4]: start 9297: logical 31: len 29 > [ext 5]: start 9326: logical 60: len 1 > [ext 6]: start 9327: logical 61: len 29 > ... > [ext 32]: start 9265: logical 800: len 1 > [ext 33]: start 9266: logical 900: len 1 > > Total/best extents 33/1 > Average size per extent 61 KB > Fragmentation score 51 > [0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag] > This file (/mnt/xfstests/scratch/test.10) does not need defragmentation. > Done. > > After this patch, the output would be: > <File> > [ext 1]: start 33796: logical 0: len 1 [written] > [ext 2]: start 9267: logical 1: len 29 [unwritten] > [ext 3]: start 9296: logical 30: len 1 [written] > [ext 4]: start 9297: logical 31: len 29 [unwritten] > [ext 5]: start 9326: logical 60: len 1 [written] > [ext 6]: start 9327: logical 61: len 29 [unwritten] > ... > [ext 32]: start 9265: logical 800: len 1 [unwritten] > [ext 33]: start 9266: logical 900: len 1 [unwritten] > > Total/best extents 33/1 > Average size per extent 61 KB > Fragmentation score 51 > [0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag] > This file (/mnt/xfstests/scratch/test.10) does not need defragmentation. > Done. > > Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> > --- > misc/e4defrag.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/misc/e4defrag.c b/misc/e4defrag.c > index d0eac60..b39e263 100644 > --- a/misc/e4defrag.c > +++ b/misc/e4defrag.c > @@ -129,6 +129,7 @@ struct fiemap_extent_data { > __u64 len; /* blocks count */ > __u64 logical; /* start logical block number */ > ext4_fsblk_t physical; /* start physical block number */ > + __u32 fe_flags; /* FIEMAP_EXTENT_* flags for this extent */ > }; > > struct fiemap_extent_list { > @@ -831,6 +832,7 @@ static int get_file_extents(int fd, struct fiemap_extent_list **ext_list_head) > / block_size; > ext_list->data.len = ext_buf[i].fe_length > / block_size; > + ext_list->data.fe_flags = ext_buf[i].fe_flags; > > ret = insert_extent_by_physical( > ext_list_head, ext_list); > @@ -1170,12 +1172,16 @@ static int file_statistic(const char *file, const struct stat64 *buf, > > /* Print extents info */ > do { > + char *status = ext_list_tmp->data.fe_flags & > + FIEMAP_EXTENT_UNWRITTEN ? > + "unwritten" : "written"; > count++; > printf("[ext %d]:\tstart %llu:\tlogical " > - "%llu:\tlen %llu\n", count, > - ext_list_tmp->data.physical, > - ext_list_tmp->data.logical, > - ext_list_tmp->data.len); > + "%llu:\tlen %llu \t[%s]\n", count, > + ext_list_tmp->data.physical, > + ext_list_tmp->data.logical, > + ext_list_tmp->data.len, > + status); > ext_list_tmp = ext_list_tmp->next; > } while (ext_list_tmp != logical_list_head); > > -- > 1.8.2.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] misc/e4defrag: output extent's status(written or unwritten) 2014-09-23 19:02 ` Andreas Dilger @ 2014-09-24 1:11 ` Xiaoguang Wang 2014-09-24 13:44 ` Eric Sandeen 0 siblings, 1 reply; 13+ messages in thread From: Xiaoguang Wang @ 2014-09-24 1:11 UTC (permalink / raw) To: Andreas Dilger Cc: <linux-ext4@vger.kernel.org>, <tytso@mit.edu>, <darrick.wong@oracle.com> Hi, On 09/24/2014 03:02 AM, Andreas Dilger wrote: > It would be nice if the output would match the format used by filefrag. OK, will send a V2 version, thanks! Regards, Xiaoguang Wang > > Cheers, Andreas > >> On Sep 23, 2014, at 12:41, Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> wrote: >> >> Before this patch, the output woulld be(e4defrag -c -v testfile): >> <File> >> [ext 1]: start 33796: logical 0: len 1 >> [ext 2]: start 9267: logical 1: len 29 >> [ext 3]: start 9296: logical 30: len 1 >> [ext 4]: start 9297: logical 31: len 29 >> [ext 5]: start 9326: logical 60: len 1 >> [ext 6]: start 9327: logical 61: len 29 >> ... >> [ext 32]: start 9265: logical 800: len 1 >> [ext 33]: start 9266: logical 900: len 1 >> >> Total/best extents 33/1 >> Average size per extent 61 KB >> Fragmentation score 51 >> [0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag] >> This file (/mnt/xfstests/scratch/test.10) does not need defragmentation. >> Done. >> >> After this patch, the output would be: >> <File> >> [ext 1]: start 33796: logical 0: len 1 [written] >> [ext 2]: start 9267: logical 1: len 29 [unwritten] >> [ext 3]: start 9296: logical 30: len 1 [written] >> [ext 4]: start 9297: logical 31: len 29 [unwritten] >> [ext 5]: start 9326: logical 60: len 1 [written] >> [ext 6]: start 9327: logical 61: len 29 [unwritten] >> ... >> [ext 32]: start 9265: logical 800: len 1 [unwritten] >> [ext 33]: start 9266: logical 900: len 1 [unwritten] >> >> Total/best extents 33/1 >> Average size per extent 61 KB >> Fragmentation score 51 >> [0-30 no problem: 31-55 a little bit fragmented: 56- needs defrag] >> This file (/mnt/xfstests/scratch/test.10) does not need defragmentation. >> Done. >> >> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> >> --- >> misc/e4defrag.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/misc/e4defrag.c b/misc/e4defrag.c >> index d0eac60..b39e263 100644 >> --- a/misc/e4defrag.c >> +++ b/misc/e4defrag.c >> @@ -129,6 +129,7 @@ struct fiemap_extent_data { >> __u64 len; /* blocks count */ >> __u64 logical; /* start logical block number */ >> ext4_fsblk_t physical; /* start physical block number */ >> + __u32 fe_flags; /* FIEMAP_EXTENT_* flags for this extent */ >> }; >> >> struct fiemap_extent_list { >> @@ -831,6 +832,7 @@ static int get_file_extents(int fd, struct fiemap_extent_list **ext_list_head) >> / block_size; >> ext_list->data.len = ext_buf[i].fe_length >> / block_size; >> + ext_list->data.fe_flags = ext_buf[i].fe_flags; >> >> ret = insert_extent_by_physical( >> ext_list_head, ext_list); >> @@ -1170,12 +1172,16 @@ static int file_statistic(const char *file, const struct stat64 *buf, >> >> /* Print extents info */ >> do { >> + char *status = ext_list_tmp->data.fe_flags & >> + FIEMAP_EXTENT_UNWRITTEN ? >> + "unwritten" : "written"; >> count++; >> printf("[ext %d]:\tstart %llu:\tlogical " >> - "%llu:\tlen %llu\n", count, >> - ext_list_tmp->data.physical, >> - ext_list_tmp->data.logical, >> - ext_list_tmp->data.len); >> + "%llu:\tlen %llu \t[%s]\n", count, >> + ext_list_tmp->data.physical, >> + ext_list_tmp->data.logical, >> + ext_list_tmp->data.len, >> + status); >> ext_list_tmp = ext_list_tmp->next; >> } while (ext_list_tmp != logical_list_head); >> >> -- >> 1.8.2.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > . > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] misc/e4defrag: output extent's status(written or unwritten) 2014-09-24 1:11 ` Xiaoguang Wang @ 2014-09-24 13:44 ` Eric Sandeen 2014-10-06 11:04 ` Xiaoguang Wang 2014-10-06 11:05 ` [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP Xiaoguang Wang 0 siblings, 2 replies; 13+ messages in thread From: Eric Sandeen @ 2014-09-24 13:44 UTC (permalink / raw) To: Xiaoguang Wang, Andreas Dilger Cc: <linux-ext4@vger.kernel.org>, <tytso@mit.edu>, <darrick.wong@oracle.com> On 9/23/14 8:11 PM, Xiaoguang Wang wrote: > Hi, > > On 09/24/2014 03:02 AM, Andreas Dilger wrote: >> It would be nice if the output would match the format used by filefrag. > OK, will send a V2 version, thanks! And could they share code? I seem to remember that it's taken several iterations to get filefrag working properly, or some reason. -Eric > Regards, > Xiaoguang Wang > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] misc/e4defrag: output extent's status(written or unwritten) 2014-09-24 13:44 ` Eric Sandeen @ 2014-10-06 11:04 ` Xiaoguang Wang 2014-10-06 11:05 ` [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP Xiaoguang Wang 1 sibling, 0 replies; 13+ messages in thread From: Xiaoguang Wang @ 2014-10-06 11:04 UTC (permalink / raw) To: Eric Sandeen Cc: Andreas Dilger, <linux-ext4@vger.kernel.org>, <tytso@mit.edu>, <darrick.wong@oracle.com> Hi, On 09/24/2014 09:44 PM, Eric Sandeen wrote: > On 9/23/14 8:11 PM, Xiaoguang Wang wrote: >> Hi, >> >> On 09/24/2014 03:02 AM, Andreas Dilger wrote: >>> It would be nice if the output would match the format used by filefrag. >> OK, will send a V2 version, thanks! > > And could they share code? I seem to remember that it's taken several > iterations to get filefrag working properly, or some reason. Sorry for much late response, I was away from the internet for some time. Yes, we can pull some shared code together, but I found filefrag couldn't work correctly when using FIBMAP, I'd like to fix it first. Please check my next mail that fixes this bug, thanks! Regards, Xiaoguang Wang > > -Eric > > >> Regards, >> Xiaoguang Wang >> > > . > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP 2014-09-24 13:44 ` Eric Sandeen 2014-10-06 11:04 ` Xiaoguang Wang @ 2014-10-06 11:05 ` Xiaoguang Wang 2014-10-06 22:29 ` Andreas Dilger 2014-10-07 2:43 ` Darrick J. Wong 1 sibling, 2 replies; 13+ messages in thread From: Xiaoguang Wang @ 2014-10-06 11:05 UTC (permalink / raw) To: linux-ext4; +Cc: tytso, darrick.wong, Xiaoguang Wang When using FIBMAP and '-e' option is specified, the calculation for fiemap_extent is wrong, we wrongly updated fm_ext.fe_logical for every iteration, please see the code in the end of 'for' loop in fm_ext.fe_logical(). In an ext2 file system(block size is 1024 bytes), dd if=/dev/zero of=testfile bs=1k count=15 Using debugfs, corresponding physical blocks are "2049 2050 2051 2052 2053 2054 2055 2056 2057 2058 2059 2060 1025 2061 2062 2063", 1025 is this indirect block. Before this patch, filefrag's output would be: filefrag -B -e testfile Filesystem type is: ef53 Filesystem cylinder groups approximately 16 File size of testfile is 15360 (15 blocks of 1024 bytes) ext: logical_offset: physical_offset: length: expected: flags: 0: 1.. 2: 2050.. 2051: 2: 2051: merged 1: 3.. 4: 2052.. 2053: 2: 2053: merged 2: 5.. 6: 2054.. 2055: 2: 2055: merged 3: 7.. 8: 2056.. 2057: 2: 2057: merged 4: 9.. 10: 2058.. 2059: 2: 2059: merged 5: 11.. 12: 2060.. 2061: 2: 2062: merged 6: 13.. 14: 2062.. 2063: 2: 2063: merged,eof 7: 14.. 14: 2063.. 2063: 1: 2063: merged,eof This output is not reasonable. Fix this bug and try to make it readable. After this patch, the output would be: ./filefrag -B -e mntpoint/testfile Filesystem type is: ef53 Filesystem cylinder groups approximately 16 File size of mntpoint/testfile is 15360 (15 blocks of 1024 bytes) ext: logical_offset: physical_offset: length: expected: flags: 0: 0.. 11: 2049.. 2060: 12: 2062: merged 1: 12.. 14: 2061.. 2063: 3: 2063: merged,eof mntpoint/testfile: 2 extents found, perfection would be 1 extent Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> --- misc/filefrag.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/misc/filefrag.c b/misc/filefrag.c index c1a8684..e9f7e68 100644 --- a/misc/filefrag.c +++ b/misc/filefrag.c @@ -315,32 +315,41 @@ static int filefrag_fibmap(int fd, int blk_shift, int *num_extents, return rc; if (block == 0) continue; + count++; + if (*num_extents == 0) { (*num_extents)++; if (force_extent) { print_extent_header(); + fm_ext.fe_logical = logical; fm_ext.fe_physical = block * st->st_blksize; + fm_ext.fe_length = st->st_blksize; } + last_block = block; + continue; } - count++; - if (force_extent && last_block != 0 && - (block != last_block + 1 || - fm_ext.fe_logical + fm_ext.fe_length != logical)) { - print_extent_info(&fm_ext, *num_extents - 1, - (last_block + 1) * st->st_blksize, - blk_shift, st); - fm_ext.fe_length = 0; - (*num_extents)++; + + if (force_extent) { + if (block != last_block + 1 || + fm_ext.fe_length + fm_ext.fe_logical != logical) { + print_extent_info(&fm_ext, *num_extents - 1, + (last_block + 1) * + st->st_blksize, + blk_shift, st); + fm_ext.fe_logical = logical; + fm_ext.fe_physical = block * st->st_blksize; + fm_ext.fe_length = st->st_blksize; + (*num_extents)++; + } else { + fm_ext.fe_length += st->st_blksize; + } } else if (last_block && (block != last_block + 1)) { - if (verbose) + if (verbose) { printf("Discontinuity: Block %ld is at %lu (was " "%lu)\n", i, block, last_block + 1); - fm_ext.fe_length = 0; + } (*num_extents)++; } - fm_ext.fe_logical = logical; - fm_ext.fe_physical = block * st->st_blksize; - fm_ext.fe_length += st->st_blksize; last_block = block; } -- 1.8.2.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP 2014-10-06 11:05 ` [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP Xiaoguang Wang @ 2014-10-06 22:29 ` Andreas Dilger 2014-10-07 1:43 ` Xiaoguang Wang 2014-10-07 2:43 ` Darrick J. Wong 1 sibling, 1 reply; 13+ messages in thread From: Andreas Dilger @ 2014-10-06 22:29 UTC (permalink / raw) To: Xiaoguang Wang; +Cc: linux-ext4, tytso, darrick.wong [-- Attachment #1: Type: text/plain, Size: 4702 bytes --] On Oct 6, 2014, at 5:05 AM, Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> wrote: > When using FIBMAP and '-e' option is specified, the calculation for > fiemap_extent is wrong, we wrongly updated fm_ext.fe_logical for every iteration, please see the code in the end of 'for' loop in > fm_ext.fe_logical(). > > In an ext2 file system(block size is 1024 bytes), > dd if=/dev/zero of=testfile bs=1k count=15 > Using debugfs, corresponding physical blocks are "2049 2050 2051 2052 2053 2054 > 2055 2056 2057 2058 2059 2060 1025 2061 2062 2063", 1025 is this indirect block. > Before this patch, filefrag's output would be: > filefrag -B -e testfile > Filesystem type is: ef53 > Filesystem cylinder groups approximately 16 > File size of testfile is 15360 (15 blocks of 1024 bytes) > ext: logical_offset: physical_offset: length: expected: flags: > 0: 1.. 2: 2050.. 2051: 2: 2051: merged > 1: 3.. 4: 2052.. 2053: 2: 2053: merged > 2: 5.. 6: 2054.. 2055: 2: 2055: merged > 3: 7.. 8: 2056.. 2057: 2: 2057: merged > 4: 9.. 10: 2058.. 2059: 2: 2059: merged > 5: 11.. 12: 2060.. 2061: 2: 2062: merged > 6: 13.. 14: 2062.. 2063: 2: 2063: merged,eof > 7: 14.. 14: 2063.. 2063: 1: 2063: merged,eof > This output is not reasonable. Definitely agree. > Fix this bug and try to make it readable. After this patch, the output would be: > ./filefrag -B -e mntpoint/testfile > Filesystem type is: ef53 > Filesystem cylinder groups approximately 16 > File size of mntpoint/testfile is 15360 (15 blocks of 1024 bytes) > ext: logical_offset: physical_offset: length: expected: flags: > 0: 0.. 11: 2049.. 2060: 12: 2062: merged > 1: 12.. 14: 2061.. 2063: 3: 2063: merged,eof > mntpoint/testfile: 2 extents found, perfection would be 1 extent Why would this not be shown as a single extent: 0: 0..14: 2049..2063: 15: merged,eof It isn't clear why "expected" is 2062 instead of 2061. Cheers, Andreas > Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> > --- > misc/filefrag.c | 37 +++++++++++++++++++++++-------------- > 1 file changed, 23 insertions(+), 14 deletions(-) > > diff --git a/misc/filefrag.c b/misc/filefrag.c > index c1a8684..e9f7e68 100644 > --- a/misc/filefrag.c > +++ b/misc/filefrag.c > @@ -315,32 +315,41 @@ static int filefrag_fibmap(int fd, int blk_shift, int *num_extents, > return rc; > if (block == 0) > continue; > + count++; > + > if (*num_extents == 0) { > (*num_extents)++; > if (force_extent) { > print_extent_header(); > + fm_ext.fe_logical = logical; > fm_ext.fe_physical = block * st->st_blksize; > + fm_ext.fe_length = st->st_blksize; > } > + last_block = block; > + continue; > } > - count++; > - if (force_extent && last_block != 0 && > - (block != last_block + 1 || > - fm_ext.fe_logical + fm_ext.fe_length != logical)) { > - print_extent_info(&fm_ext, *num_extents - 1, > - (last_block + 1) * st->st_blksize, > - blk_shift, st); > - fm_ext.fe_length = 0; > - (*num_extents)++; > + > + if (force_extent) { > + if (block != last_block + 1 || > + fm_ext.fe_length + fm_ext.fe_logical != logical) { > + print_extent_info(&fm_ext, *num_extents - 1, > + (last_block + 1) * > + st->st_blksize, > + blk_shift, st); > + fm_ext.fe_logical = logical; > + fm_ext.fe_physical = block * st->st_blksize; > + fm_ext.fe_length = st->st_blksize; > + (*num_extents)++; > + } else { > + fm_ext.fe_length += st->st_blksize; > + } > } else if (last_block && (block != last_block + 1)) { > - if (verbose) > + if (verbose) { > printf("Discontinuity: Block %ld is at %lu (was " > "%lu)\n", i, block, last_block + 1); > - fm_ext.fe_length = 0; > + } > (*num_extents)++; > } > - fm_ext.fe_logical = logical; > - fm_ext.fe_physical = block * st->st_blksize; > - fm_ext.fe_length += st->st_blksize; > last_block = block; > } > > -- > 1.8.2.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, Andreas [-- Attachment #2: Message signed with OpenPGP using GPGMail --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP 2014-10-06 22:29 ` Andreas Dilger @ 2014-10-07 1:43 ` Xiaoguang Wang 0 siblings, 0 replies; 13+ messages in thread From: Xiaoguang Wang @ 2014-10-07 1:43 UTC (permalink / raw) To: Andreas Dilger; +Cc: linux-ext4, tytso, darrick.wong Hi, On 10/07/2014 06:29 AM, Andreas Dilger wrote: > On Oct 6, 2014, at 5:05 AM, Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> wrote: >> When using FIBMAP and '-e' option is specified, the calculation for >> fiemap_extent is wrong, we wrongly updated fm_ext.fe_logical for every iteration, please see the code in the end of 'for' loop in >> fm_ext.fe_logical(). >> >> In an ext2 file system(block size is 1024 bytes), >> dd if=/dev/zero of=testfile bs=1k count=15 >> Using debugfs, corresponding physical blocks are "2049 2050 2051 2052 2053 2054 >> 2055 2056 2057 2058 2059 2060 1025 2061 2062 2063", 1025 is this indirect block. >> Before this patch, filefrag's output would be: >> filefrag -B -e testfile >> Filesystem type is: ef53 >> Filesystem cylinder groups approximately 16 >> File size of testfile is 15360 (15 blocks of 1024 bytes) >> ext: logical_offset: physical_offset: length: expected: flags: >> 0: 1.. 2: 2050.. 2051: 2: 2051: merged >> 1: 3.. 4: 2052.. 2053: 2: 2053: merged >> 2: 5.. 6: 2054.. 2055: 2: 2055: merged >> 3: 7.. 8: 2056.. 2057: 2: 2057: merged >> 4: 9.. 10: 2058.. 2059: 2: 2059: merged >> 5: 11.. 12: 2060.. 2061: 2: 2062: merged >> 6: 13.. 14: 2062.. 2063: 2: 2063: merged,eof >> 7: 14.. 14: 2063.. 2063: 1: 2063: merged,eof >> This output is not reasonable. > > Definitely agree. > >> Fix this bug and try to make it readable. After this patch, the output would be: >> ./filefrag -B -e mntpoint/testfile >> Filesystem type is: ef53 >> Filesystem cylinder groups approximately 16 >> File size of mntpoint/testfile is 15360 (15 blocks of 1024 bytes) >> ext: logical_offset: physical_offset: length: expected: flags: >> 0: 0.. 11: 2049.. 2060: 12: 2062: merged >> 1: 12.. 14: 2061.. 2063: 3: 2063: merged,eof >> mntpoint/testfile: 2 extents found, perfection would be 1 extent > > Why would this not be shown as a single extent: > > 0: 0..14: 2049..2063: 15: merged,eof > > It isn't clear why "expected" is 2062 instead of 2061. This is because for a logical region, only all corresponding physical data blocks and possible indirect blocks are both continuous, we can say that it's an extent. There is such code in filefrag_fibmap(): if (is_ext2 && last_block) { if (((i - EXT2_DIRECT) % bpib) == 0) last_block++; if (((i - EXT2_DIRECT - bpib) % (bpib * bpib)) == 0) last_block++; if (((i - EXT2_DIRECT - bpib - bpib * bpib) % (((unsigned long long)bpib) * bpib * bpib)) == 0) last_block++; } For logical block 12, it starts to need an indirect block, and its expected physical block is 2062(expected indirect block is 2061), so we do not consider it continuous. Regards, Xiaoguang Wang > > Cheers, Andreas > >> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> >> --- >> misc/filefrag.c | 37 +++++++++++++++++++++++-------------- >> 1 file changed, 23 insertions(+), 14 deletions(-) >> >> diff --git a/misc/filefrag.c b/misc/filefrag.c >> index c1a8684..e9f7e68 100644 >> --- a/misc/filefrag.c >> +++ b/misc/filefrag.c >> @@ -315,32 +315,41 @@ static int filefrag_fibmap(int fd, int blk_shift, int *num_extents, >> return rc; >> if (block == 0) >> continue; >> + count++; >> + >> if (*num_extents == 0) { >> (*num_extents)++; >> if (force_extent) { >> print_extent_header(); >> + fm_ext.fe_logical = logical; >> fm_ext.fe_physical = block * st->st_blksize; >> + fm_ext.fe_length = st->st_blksize; >> } >> + last_block = block; >> + continue; >> } >> - count++; >> - if (force_extent && last_block != 0 && >> - (block != last_block + 1 || >> - fm_ext.fe_logical + fm_ext.fe_length != logical)) { >> - print_extent_info(&fm_ext, *num_extents - 1, >> - (last_block + 1) * st->st_blksize, >> - blk_shift, st); >> - fm_ext.fe_length = 0; >> - (*num_extents)++; >> + >> + if (force_extent) { >> + if (block != last_block + 1 || >> + fm_ext.fe_length + fm_ext.fe_logical != logical) { >> + print_extent_info(&fm_ext, *num_extents - 1, >> + (last_block + 1) * >> + st->st_blksize, >> + blk_shift, st); >> + fm_ext.fe_logical = logical; >> + fm_ext.fe_physical = block * st->st_blksize; >> + fm_ext.fe_length = st->st_blksize; >> + (*num_extents)++; >> + } else { >> + fm_ext.fe_length += st->st_blksize; >> + } >> } else if (last_block && (block != last_block + 1)) { >> - if (verbose) >> + if (verbose) { >> printf("Discontinuity: Block %ld is at %lu (was " >> "%lu)\n", i, block, last_block + 1); >> - fm_ext.fe_length = 0; >> + } >> (*num_extents)++; >> } >> - fm_ext.fe_logical = logical; >> - fm_ext.fe_physical = block * st->st_blksize; >> - fm_ext.fe_length += st->st_blksize; >> last_block = block; >> } >> >> -- >> 1.8.2.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > Cheers, Andreas > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP 2014-10-06 11:05 ` [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP Xiaoguang Wang 2014-10-06 22:29 ` Andreas Dilger @ 2014-10-07 2:43 ` Darrick J. Wong 2014-10-07 3:50 ` Xiaoguang Wang 1 sibling, 1 reply; 13+ messages in thread From: Darrick J. Wong @ 2014-10-07 2:43 UTC (permalink / raw) To: Xiaoguang Wang; +Cc: linux-ext4, tytso On Mon, Oct 06, 2014 at 07:05:39PM +0800, Xiaoguang Wang wrote: > When using FIBMAP and '-e' option is specified, the calculation for fiemap_extent > is wrong, we wrongly updated fm_ext.fe_logical for every iteration, please see the > code in the end of 'for' loop in fm_ext.fe_logical(). > > In an ext2 file system(block size is 1024 bytes), > dd if=/dev/zero of=testfile bs=1k count=15 > Using debugfs, corresponding physical blocks are "2049 2050 2051 2052 2053 2054 > 2055 2056 2057 2058 2059 2060 1025 2061 2062 2063", 1025 is this indirect block. > Before this patch, filefrag's output would be: > filefrag -B -e testfile > Filesystem type is: ef53 > Filesystem cylinder groups approximately 16 > File size of testfile is 15360 (15 blocks of 1024 bytes) > ext: logical_offset: physical_offset: length: expected: flags: > 0: 1.. 2: 2050.. 2051: 2: 2051: merged > 1: 3.. 4: 2052.. 2053: 2: 2053: merged > 2: 5.. 6: 2054.. 2055: 2: 2055: merged > 3: 7.. 8: 2056.. 2057: 2: 2057: merged > 4: 9.. 10: 2058.. 2059: 2: 2059: merged > 5: 11.. 12: 2060.. 2061: 2: 2062: merged > 6: 13.. 14: 2062.. 2063: 2: 2063: merged,eof > 7: 14.. 14: 2063.. 2063: 1: 2063: merged,eof > This output is not reasonable. It's just plain whacky. Why would logical offset 14 be listed twice? :) > Fix this bug and try to make it readable. After this patch, the output would be: > ./filefrag -B -e mntpoint/testfile > Filesystem type is: ef53 > Filesystem cylinder groups approximately 16 > File size of mntpoint/testfile is 15360 (15 blocks of 1024 bytes) > ext: logical_offset: physical_offset: length: expected: flags: > 0: 0.. 11: 2049.. 2060: 12: 2062: merged > 1: 12.. 14: 2061.. 2063: 3: 2063: merged,eof > mntpoint/testfile: 2 extents found, perfection would be 1 extent Where'd the indirect block end up, if not in the middle of 2049-2063? > Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> > --- > misc/filefrag.c | 37 +++++++++++++++++++++++-------------- > 1 file changed, 23 insertions(+), 14 deletions(-) > > diff --git a/misc/filefrag.c b/misc/filefrag.c > index c1a8684..e9f7e68 100644 > --- a/misc/filefrag.c > +++ b/misc/filefrag.c > @@ -315,32 +315,41 @@ static int filefrag_fibmap(int fd, int blk_shift, int *num_extents, > return rc; > if (block == 0) > continue; > + count++; > + > if (*num_extents == 0) { > (*num_extents)++; > if (force_extent) { > print_extent_header(); > + fm_ext.fe_logical = logical; > fm_ext.fe_physical = block * st->st_blksize; > + fm_ext.fe_length = st->st_blksize; > } > + last_block = block; > + continue; > } > - count++; > - if (force_extent && last_block != 0 && > - (block != last_block + 1 || > - fm_ext.fe_logical + fm_ext.fe_length != logical)) { > - print_extent_info(&fm_ext, *num_extents - 1, > - (last_block + 1) * st->st_blksize, > - blk_shift, st); > - fm_ext.fe_length = 0; > - (*num_extents)++; > + > + if (force_extent) { > + if (block != last_block + 1 || > + fm_ext.fe_length + fm_ext.fe_logical != logical) { > + print_extent_info(&fm_ext, *num_extents - 1, > + (last_block + 1) * > + st->st_blksize, > + blk_shift, st); > + fm_ext.fe_logical = logical; > + fm_ext.fe_physical = block * st->st_blksize; > + fm_ext.fe_length = st->st_blksize; > + (*num_extents)++; > + } else { > + fm_ext.fe_length += st->st_blksize; > + } > } else if (last_block && (block != last_block + 1)) { > - if (verbose) > + if (verbose) { > printf("Discontinuity: Block %ld is at %lu (was " > "%lu)\n", i, block, last_block + 1); > - fm_ext.fe_length = 0; > + } The { } is not needed for a single line if statement. > (*num_extents)++; > } > - fm_ext.fe_logical = logical; > - fm_ext.fe_physical = block * st->st_blksize; > - fm_ext.fe_length += st->st_blksize; > last_block = block; Otherwise, I think this looks decent. --D > } > > -- > 1.8.2.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP 2014-10-07 2:43 ` Darrick J. Wong @ 2014-10-07 3:50 ` Xiaoguang Wang 2014-10-07 3:59 ` Darrick J. Wong 0 siblings, 1 reply; 13+ messages in thread From: Xiaoguang Wang @ 2014-10-07 3:50 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-ext4, tytso Hi, On 10/07/2014 10:43 AM, Darrick J. Wong wrote: > On Mon, Oct 06, 2014 at 07:05:39PM +0800, Xiaoguang Wang wrote: >> When using FIBMAP and '-e' option is specified, the calculation for fiemap_extent >> is wrong, we wrongly updated fm_ext.fe_logical for every iteration, please see the >> code in the end of 'for' loop in fm_ext.fe_logical(). >> >> In an ext2 file system(block size is 1024 bytes), >> dd if=/dev/zero of=testfile bs=1k count=15 >> Using debugfs, corresponding physical blocks are "2049 2050 2051 2052 2053 2054 >> 2055 2056 2057 2058 2059 2060 1025 2061 2062 2063", 1025 is this indirect block. >> Before this patch, filefrag's output would be: >> filefrag -B -e testfile >> Filesystem type is: ef53 >> Filesystem cylinder groups approximately 16 >> File size of testfile is 15360 (15 blocks of 1024 bytes) >> ext: logical_offset: physical_offset: length: expected: flags: >> 0: 1.. 2: 2050.. 2051: 2: 2051: merged >> 1: 3.. 4: 2052.. 2053: 2: 2053: merged >> 2: 5.. 6: 2054.. 2055: 2: 2055: merged >> 3: 7.. 8: 2056.. 2057: 2: 2057: merged >> 4: 9.. 10: 2058.. 2059: 2: 2059: merged >> 5: 11.. 12: 2060.. 2061: 2: 2062: merged >> 6: 13.. 14: 2062.. 2063: 2: 2063: merged,eof >> 7: 14.. 14: 2063.. 2063: 1: 2063: merged,eof >> This output is not reasonable. > > It's just plain whacky. Why would logical offset 14 be listed twice? :) Because of the wrong code :) It updates fm_ext.fe_logical and fm_ext.fe_physical for every iteration :) and i think the original code is not that readable. > >> Fix this bug and try to make it readable. After this patch, the output would be: >> ./filefrag -B -e mntpoint/testfile >> Filesystem type is: ef53 >> Filesystem cylinder groups approximately 16 >> File size of mntpoint/testfile is 15360 (15 blocks of 1024 bytes) >> ext: logical_offset: physical_offset: length: expected: flags: >> 0: 0.. 11: 2049.. 2060: 12: 2062: merged >> 1: 12.. 14: 2061.. 2063: 3: 2063: merged,eof >> mntpoint/testfile: 2 extents found, perfection would be 1 extent > > Where'd the indirect block end up, if not in the middle of 2049-2063? Indirect block's information is used to judge whether logical region is continuous physically. For example, the above testfile's first indirect block is 1025, and will not shown in the output. > >> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> >> --- >> misc/filefrag.c | 37 +++++++++++++++++++++++-------------- >> 1 file changed, 23 insertions(+), 14 deletions(-) >> >> diff --git a/misc/filefrag.c b/misc/filefrag.c >> index c1a8684..e9f7e68 100644 >> --- a/misc/filefrag.c >> +++ b/misc/filefrag.c >> @@ -315,32 +315,41 @@ static int filefrag_fibmap(int fd, int blk_shift, int *num_extents, >> return rc; >> if (block == 0) >> continue; >> + count++; >> + >> if (*num_extents == 0) { >> (*num_extents)++; >> if (force_extent) { >> print_extent_header(); >> + fm_ext.fe_logical = logical; >> fm_ext.fe_physical = block * st->st_blksize; >> + fm_ext.fe_length = st->st_blksize; >> } >> + last_block = block; >> + continue; >> } >> - count++; >> - if (force_extent && last_block != 0 && >> - (block != last_block + 1 || >> - fm_ext.fe_logical + fm_ext.fe_length != logical)) { >> - print_extent_info(&fm_ext, *num_extents - 1, >> - (last_block + 1) * st->st_blksize, >> - blk_shift, st); >> - fm_ext.fe_length = 0; >> - (*num_extents)++; >> + >> + if (force_extent) { >> + if (block != last_block + 1 || >> + fm_ext.fe_length + fm_ext.fe_logical != logical) { >> + print_extent_info(&fm_ext, *num_extents - 1, >> + (last_block + 1) * >> + st->st_blksize, >> + blk_shift, st); >> + fm_ext.fe_logical = logical; >> + fm_ext.fe_physical = block * st->st_blksize; >> + fm_ext.fe_length = st->st_blksize; >> + (*num_extents)++; >> + } else { >> + fm_ext.fe_length += st->st_blksize; >> + } >> } else if (last_block && (block != last_block + 1)) { >> - if (verbose) >> + if (verbose) { >> printf("Discontinuity: Block %ld is at %lu (was " >> "%lu)\n", i, block, last_block + 1); >> - fm_ext.fe_length = 0; >> + } > > The { } is not needed for a single line if statement. OK, I'll send a new version to remove it, thanks! Regards, Xiaoguang Wang > >> (*num_extents)++; >> } >> - fm_ext.fe_logical = logical; >> - fm_ext.fe_physical = block * st->st_blksize; >> - fm_ext.fe_length += st->st_blksize; >> last_block = block; > > Otherwise, I think this looks decent. > > --D > >> } >> >> -- >> 1.8.2.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > . > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP 2014-10-07 3:50 ` Xiaoguang Wang @ 2014-10-07 3:59 ` Darrick J. Wong 2014-10-07 4:12 ` Xiaoguang Wang 0 siblings, 1 reply; 13+ messages in thread From: Darrick J. Wong @ 2014-10-07 3:59 UTC (permalink / raw) To: Xiaoguang Wang; +Cc: linux-ext4, tytso On Tue, Oct 07, 2014 at 11:50:34AM +0800, Xiaoguang Wang wrote: > Hi, > > On 10/07/2014 10:43 AM, Darrick J. Wong wrote: > > On Mon, Oct 06, 2014 at 07:05:39PM +0800, Xiaoguang Wang wrote: > >> When using FIBMAP and '-e' option is specified, the calculation for fiemap_extent > >> is wrong, we wrongly updated fm_ext.fe_logical for every iteration, please see the > >> code in the end of 'for' loop in fm_ext.fe_logical(). > >> > >> In an ext2 file system(block size is 1024 bytes), > >> dd if=/dev/zero of=testfile bs=1k count=15 > >> Using debugfs, corresponding physical blocks are "2049 2050 2051 2052 2053 2054 > >> 2055 2056 2057 2058 2059 2060 1025 2061 2062 2063", 1025 is this indirect block. > >> Before this patch, filefrag's output would be: > >> filefrag -B -e testfile > >> Filesystem type is: ef53 > >> Filesystem cylinder groups approximately 16 > >> File size of testfile is 15360 (15 blocks of 1024 bytes) > >> ext: logical_offset: physical_offset: length: expected: flags: > >> 0: 1.. 2: 2050.. 2051: 2: 2051: merged > >> 1: 3.. 4: 2052.. 2053: 2: 2053: merged > >> 2: 5.. 6: 2054.. 2055: 2: 2055: merged > >> 3: 7.. 8: 2056.. 2057: 2: 2057: merged > >> 4: 9.. 10: 2058.. 2059: 2: 2059: merged > >> 5: 11.. 12: 2060.. 2061: 2: 2062: merged > >> 6: 13.. 14: 2062.. 2063: 2: 2063: merged,eof > >> 7: 14.. 14: 2063.. 2063: 1: 2063: merged,eof > >> This output is not reasonable. > > > > It's just plain whacky. Why would logical offset 14 be listed twice? :) > > Because of the wrong code :) It updates fm_ext.fe_logical and fm_ext.fe_physical for every > iteration :) and i think the original code is not that readable. > > > >> Fix this bug and try to make it readable. After this patch, the output would be: > >> ./filefrag -B -e mntpoint/testfile > >> Filesystem type is: ef53 > >> Filesystem cylinder groups approximately 16 > >> File size of mntpoint/testfile is 15360 (15 blocks of 1024 bytes) > >> ext: logical_offset: physical_offset: length: expected: flags: > >> 0: 0.. 11: 2049.. 2060: 12: 2062: merged > >> 1: 12.. 14: 2061.. 2063: 3: 2063: merged,eof > >> mntpoint/testfile: 2 extents found, perfection would be 1 extent > > > > Where'd the indirect block end up, if not in the middle of 2049-2063? > > Indirect block's information is used to judge whether logical region is continuous physically. > For example, the above testfile's first indirect block is 1025, and will not shown in the output. 1025? Huh. Is this an old FS, or is there something wrong with the block allocator? --D > > > > >> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> > >> --- > >> misc/filefrag.c | 37 +++++++++++++++++++++++-------------- > >> 1 file changed, 23 insertions(+), 14 deletions(-) > >> > >> diff --git a/misc/filefrag.c b/misc/filefrag.c > >> index c1a8684..e9f7e68 100644 > >> --- a/misc/filefrag.c > >> +++ b/misc/filefrag.c > >> @@ -315,32 +315,41 @@ static int filefrag_fibmap(int fd, int blk_shift, int *num_extents, > >> return rc; > >> if (block == 0) > >> continue; > >> + count++; > >> + > >> if (*num_extents == 0) { > >> (*num_extents)++; > >> if (force_extent) { > >> print_extent_header(); > >> + fm_ext.fe_logical = logical; > >> fm_ext.fe_physical = block * st->st_blksize; > >> + fm_ext.fe_length = st->st_blksize; > >> } > >> + last_block = block; > >> + continue; > >> } > >> - count++; > >> - if (force_extent && last_block != 0 && > >> - (block != last_block + 1 || > >> - fm_ext.fe_logical + fm_ext.fe_length != logical)) { > >> - print_extent_info(&fm_ext, *num_extents - 1, > >> - (last_block + 1) * st->st_blksize, > >> - blk_shift, st); > >> - fm_ext.fe_length = 0; > >> - (*num_extents)++; > >> + > >> + if (force_extent) { > >> + if (block != last_block + 1 || > >> + fm_ext.fe_length + fm_ext.fe_logical != logical) { > >> + print_extent_info(&fm_ext, *num_extents - 1, > >> + (last_block + 1) * > >> + st->st_blksize, > >> + blk_shift, st); > >> + fm_ext.fe_logical = logical; > >> + fm_ext.fe_physical = block * st->st_blksize; > >> + fm_ext.fe_length = st->st_blksize; > >> + (*num_extents)++; > >> + } else { > >> + fm_ext.fe_length += st->st_blksize; > >> + } > >> } else if (last_block && (block != last_block + 1)) { > >> - if (verbose) > >> + if (verbose) { > >> printf("Discontinuity: Block %ld is at %lu (was " > >> "%lu)\n", i, block, last_block + 1); > >> - fm_ext.fe_length = 0; > >> + } > > > > The { } is not needed for a single line if statement. > > OK, I'll send a new version to remove it, thanks! > > Regards, > Xiaoguang Wang > > > >> (*num_extents)++; > >> } > >> - fm_ext.fe_logical = logical; > >> - fm_ext.fe_physical = block * st->st_blksize; > >> - fm_ext.fe_length += st->st_blksize; > >> last_block = block; > > > > Otherwise, I think this looks decent. > > > > --D > > > >> } > >> > >> -- > >> 1.8.2.1 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-ext4" 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-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP 2014-10-07 3:59 ` Darrick J. Wong @ 2014-10-07 4:12 ` Xiaoguang Wang 2014-10-07 8:31 ` [PATCH v2] " Xiaoguang Wang 0 siblings, 1 reply; 13+ messages in thread From: Xiaoguang Wang @ 2014-10-07 4:12 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-ext4, tytso Hi, On 10/07/2014 11:59 AM, Darrick J. Wong wrote: > On Tue, Oct 07, 2014 at 11:50:34AM +0800, Xiaoguang Wang wrote: >> Hi, >> >> On 10/07/2014 10:43 AM, Darrick J. Wong wrote: >>> On Mon, Oct 06, 2014 at 07:05:39PM +0800, Xiaoguang Wang wrote: >>>> When using FIBMAP and '-e' option is specified, the calculation for fiemap_extent >>>> is wrong, we wrongly updated fm_ext.fe_logical for every iteration, please see the >>>> code in the end of 'for' loop in fm_ext.fe_logical(). >>>> >>>> In an ext2 file system(block size is 1024 bytes), >>>> dd if=/dev/zero of=testfile bs=1k count=15 >>>> Using debugfs, corresponding physical blocks are "2049 2050 2051 2052 2053 2054 >>>> 2055 2056 2057 2058 2059 2060 1025 2061 2062 2063", 1025 is this indirect block. >>>> Before this patch, filefrag's output would be: >>>> filefrag -B -e testfile >>>> Filesystem type is: ef53 >>>> Filesystem cylinder groups approximately 16 >>>> File size of testfile is 15360 (15 blocks of 1024 bytes) >>>> ext: logical_offset: physical_offset: length: expected: flags: >>>> 0: 1.. 2: 2050.. 2051: 2: 2051: merged >>>> 1: 3.. 4: 2052.. 2053: 2: 2053: merged >>>> 2: 5.. 6: 2054.. 2055: 2: 2055: merged >>>> 3: 7.. 8: 2056.. 2057: 2: 2057: merged >>>> 4: 9.. 10: 2058.. 2059: 2: 2059: merged >>>> 5: 11.. 12: 2060.. 2061: 2: 2062: merged >>>> 6: 13.. 14: 2062.. 2063: 2: 2063: merged,eof >>>> 7: 14.. 14: 2063.. 2063: 1: 2063: merged,eof >>>> This output is not reasonable. >>> >>> It's just plain whacky. Why would logical offset 14 be listed twice? :) >> >> Because of the wrong code :) It updates fm_ext.fe_logical and fm_ext.fe_physical for every >> iteration :) and i think the original code is not that readable. >>> >>>> Fix this bug and try to make it readable. After this patch, the output would be: >>>> ./filefrag -B -e mntpoint/testfile >>>> Filesystem type is: ef53 >>>> Filesystem cylinder groups approximately 16 >>>> File size of mntpoint/testfile is 15360 (15 blocks of 1024 bytes) >>>> ext: logical_offset: physical_offset: length: expected: flags: >>>> 0: 0.. 11: 2049.. 2060: 12: 2062: merged >>>> 1: 12.. 14: 2061.. 2063: 3: 2063: merged,eof >>>> mntpoint/testfile: 2 extents found, perfection would be 1 extent >>> >>> Where'd the indirect block end up, if not in the middle of 2049-2063? >> >> Indirect block's information is used to judge whether logical region is continuous physically. >> For example, the above testfile's first indirect block is 1025, and will not shown in the output. > > 1025? Huh. Is this an old FS, or is there something wrong with the block > allocator? Yeah, it's strange. I created testfile multiple times, data blocks are basically continuous, but indirect block always not. It seems that block allocator does not work well. I have this test on a 128MB loop device(mkfs to ext2), OS: Fedora 19(3.9.5-301.fc19.x86_64) Regards, Xiaoguang Wang > > --D > >> >>> >>>> Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> >>>> --- >>>> misc/filefrag.c | 37 +++++++++++++++++++++++-------------- >>>> 1 file changed, 23 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/misc/filefrag.c b/misc/filefrag.c >>>> index c1a8684..e9f7e68 100644 >>>> --- a/misc/filefrag.c >>>> +++ b/misc/filefrag.c >>>> @@ -315,32 +315,41 @@ static int filefrag_fibmap(int fd, int blk_shift, int *num_extents, >>>> return rc; >>>> if (block == 0) >>>> continue; >>>> + count++; >>>> + >>>> if (*num_extents == 0) { >>>> (*num_extents)++; >>>> if (force_extent) { >>>> print_extent_header(); >>>> + fm_ext.fe_logical = logical; >>>> fm_ext.fe_physical = block * st->st_blksize; >>>> + fm_ext.fe_length = st->st_blksize; >>>> } >>>> + last_block = block; >>>> + continue; >>>> } >>>> - count++; >>>> - if (force_extent && last_block != 0 && >>>> - (block != last_block + 1 || >>>> - fm_ext.fe_logical + fm_ext.fe_length != logical)) { >>>> - print_extent_info(&fm_ext, *num_extents - 1, >>>> - (last_block + 1) * st->st_blksize, >>>> - blk_shift, st); >>>> - fm_ext.fe_length = 0; >>>> - (*num_extents)++; >>>> + >>>> + if (force_extent) { >>>> + if (block != last_block + 1 || >>>> + fm_ext.fe_length + fm_ext.fe_logical != logical) { >>>> + print_extent_info(&fm_ext, *num_extents - 1, >>>> + (last_block + 1) * >>>> + st->st_blksize, >>>> + blk_shift, st); >>>> + fm_ext.fe_logical = logical; >>>> + fm_ext.fe_physical = block * st->st_blksize; >>>> + fm_ext.fe_length = st->st_blksize; >>>> + (*num_extents)++; >>>> + } else { >>>> + fm_ext.fe_length += st->st_blksize; >>>> + } >>>> } else if (last_block && (block != last_block + 1)) { >>>> - if (verbose) >>>> + if (verbose) { >>>> printf("Discontinuity: Block %ld is at %lu (was " >>>> "%lu)\n", i, block, last_block + 1); >>>> - fm_ext.fe_length = 0; >>>> + } >>> >>> The { } is not needed for a single line if statement. >> >> OK, I'll send a new version to remove it, thanks! >> >> Regards, >> Xiaoguang Wang >>> >>>> (*num_extents)++; >>>> } >>>> - fm_ext.fe_logical = logical; >>>> - fm_ext.fe_physical = block * st->st_blksize; >>>> - fm_ext.fe_length += st->st_blksize; >>>> last_block = block; >>> >>> Otherwise, I think this looks decent. >>> >>> --D >>> >>>> } >>>> >>>> -- >>>> 1.8.2.1 >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" 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-ext4" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > . > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] filefrag: fix wrong extent count calculation when using FIBMAP 2014-10-07 4:12 ` Xiaoguang Wang @ 2014-10-07 8:31 ` Xiaoguang Wang 0 siblings, 0 replies; 13+ messages in thread From: Xiaoguang Wang @ 2014-10-07 8:31 UTC (permalink / raw) To: linux-ext4; +Cc: tytso, darrick.wong, Xiaoguang Wang When using FIBMAP and '-e' option is specified, the calculation for fiemap_extent is wrong, we wrongly updated fm_ext.fe_logical for every iteration, please see the code in the end of 'for' loop in fm_ext.fe_logical(). In an ext2 file system(block size is 1024 bytes), dd if=/dev/zero of=testfile bs=1k count=15 Using debugfs, corresponding physical blocks are "2049 2050 2051 2052 2053 2054 2055 2056 2057 2058 2059 2060 1025 2061 2062 2063", 1025 is this indirect block. Before this patch, filefrag's output would be: filefrag -B -e testfile Filesystem type is: ef53 Filesystem cylinder groups approximately 16 File size of testfile is 15360 (15 blocks of 1024 bytes) ext: logical_offset: physical_offset: length: expected: flags: 0: 1.. 2: 2050.. 2051: 2: 2051: merged 1: 3.. 4: 2052.. 2053: 2: 2053: merged 2: 5.. 6: 2054.. 2055: 2: 2055: merged 3: 7.. 8: 2056.. 2057: 2: 2057: merged 4: 9.. 10: 2058.. 2059: 2: 2059: merged 5: 11.. 12: 2060.. 2061: 2: 2062: merged 6: 13.. 14: 2062.. 2063: 2: 2063: merged,eof 7: 14.. 14: 2063.. 2063: 1: 2063: merged,eof This output is not reasonable. Fix this bug and try to make it readable. With this patch, the output would be: ./filefrag -B -e mntpoint/testfile Filesystem type is: ef53 Filesystem cylinder groups approximately 16 File size of mntpoint/testfile is 15360 (15 blocks of 1024 bytes) ext: logical_offset: physical_offset: length: expected: flags: 0: 0.. 11: 2049.. 2060: 12: 2062: merged 1: 12.. 14: 2061.. 2063: 3: 2063: merged,eof mntpoint/testfile: 2 extents found, perfection would be 1 extent Changes since v1->v2 - remove the unnecessary {} for if statement. Signed-off-by: Xiaoguang Wang <wangxg.fnst@cn.fujitsu.com> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --- misc/filefrag.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/misc/filefrag.c b/misc/filefrag.c index c1a8684..76e613f 100644 --- a/misc/filefrag.c +++ b/misc/filefrag.c @@ -315,32 +315,40 @@ static int filefrag_fibmap(int fd, int blk_shift, int *num_extents, return rc; if (block == 0) continue; + count++; + if (*num_extents == 0) { (*num_extents)++; if (force_extent) { print_extent_header(); + fm_ext.fe_logical = logical; fm_ext.fe_physical = block * st->st_blksize; + fm_ext.fe_length = st->st_blksize; } + last_block = block; + continue; } - count++; - if (force_extent && last_block != 0 && - (block != last_block + 1 || - fm_ext.fe_logical + fm_ext.fe_length != logical)) { - print_extent_info(&fm_ext, *num_extents - 1, - (last_block + 1) * st->st_blksize, - blk_shift, st); - fm_ext.fe_length = 0; - (*num_extents)++; + + if (force_extent) { + if (block != last_block + 1 || + fm_ext.fe_length + fm_ext.fe_logical != logical) { + print_extent_info(&fm_ext, *num_extents - 1, + (last_block + 1) * + st->st_blksize, + blk_shift, st); + fm_ext.fe_logical = logical; + fm_ext.fe_physical = block * st->st_blksize; + fm_ext.fe_length = st->st_blksize; + (*num_extents)++; + } else { + fm_ext.fe_length += st->st_blksize; + } } else if (last_block && (block != last_block + 1)) { if (verbose) printf("Discontinuity: Block %ld is at %lu (was " "%lu)\n", i, block, last_block + 1); - fm_ext.fe_length = 0; (*num_extents)++; } - fm_ext.fe_logical = logical; - fm_ext.fe_physical = block * st->st_blksize; - fm_ext.fe_length += st->st_blksize; last_block = block; } -- 1.8.2.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-10-07 9:00 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-23 10:41 [PATCH] misc/e4defrag: output extent's status(written or unwritten) Xiaoguang Wang 2014-09-23 19:02 ` Andreas Dilger 2014-09-24 1:11 ` Xiaoguang Wang 2014-09-24 13:44 ` Eric Sandeen 2014-10-06 11:04 ` Xiaoguang Wang 2014-10-06 11:05 ` [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP Xiaoguang Wang 2014-10-06 22:29 ` Andreas Dilger 2014-10-07 1:43 ` Xiaoguang Wang 2014-10-07 2:43 ` Darrick J. Wong 2014-10-07 3:50 ` Xiaoguang Wang 2014-10-07 3:59 ` Darrick J. Wong 2014-10-07 4:12 ` Xiaoguang Wang 2014-10-07 8:31 ` [PATCH v2] " Xiaoguang Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).