[PATCH] misc/e4defrag: output extent's status(written or unwritten)

[PATCH] misc/e4defrag: output extent's status(written or unwritten)

[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

Re: [PATCH] misc/e4defrag: output extent's status(written or unwritten)

[Andreas Dilger]

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

Re: [PATCH] misc/e4defrag: output extent's status(written or unwritten)

[Xiaoguang Wang]

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
> .
> 

Re: [PATCH] misc/e4defrag: output extent's status(written or unwritten)

[Eric Sandeen]

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
>  

Re: [PATCH] misc/e4defrag: output extent's status(written or unwritten)

[Xiaoguang Wang]

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
>>  
> 
> .
> 

[PATCH] filefrag: fix wrong extent count calculation when using FIBMAP

[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

Re: [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP

[Andreas Dilger]

[-- 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 --]

Re: [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP

[Xiaoguang Wang]

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
> 
> 
> 
> 
> 

Re: [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP

[Darrick J. Wong]

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

Re: [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP

[Xiaoguang Wang]

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
> .
> 

Re: [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP

[Darrick J. Wong]

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

Re: [PATCH] filefrag: fix wrong extent count calculation when using FIBMAP

[Xiaoguang Wang]

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
> .
> 

[PATCH v2] filefrag: fix wrong extent count calculation when using FIBMAP

[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