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
