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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux