Re: [PATCH 2/2] Use fiemap internal infra-structure to handle FIBMAP

[Eric Sandeen <sandeen@sandeen.net>]

On 9/6/18 3:12 AM, Carlos Maiolino wrote:
>>> +
>>> +		/*
>>> +		 * Fiemap implementation of some filesystems will return the
>>> +		 * extent map beginning at the requested offset
>>> +		 * (fextent.fi_logical == start), while other FSes will return
>>> +		 * the beginning of the extent at fextent.fe_logical, even if
>>> +		 * the requested offset is somehwere in the middle of the
>>> +		 * extent. We must be sure to return the correct block block
>>> +		 * here in both cases.
>>> +		 */
>>> +		if (fextent.fe_logical != start)
>>> +			res = (fextent.fe_physical + start) >> inode->i_blkbits;
>> I don't think this is correct, is it?  You can't add the entire requested
>> logical file offset (start) to the resulting physical extent start (fe_physical).
>>
>> You'd need to add the delta, (start - fextent.fe_logical) to the physical extent
>> start to get the offset into the mapping, right?
>>
>> 		res = (fextent.fe_physical +
>> 		       (start - fextent.fe_logical)) >> inode->i_blkbits;
> I believe you're right, yes, when I wrote the calculation above I was thinking
> about "fe_logical is either 0 or equal start", and I tested it against
> multi-extents files. I really have no idea how it survived my tests and xfstests
> =/

Yeay... given the recent undetected FIBMAP breakage thanks to iomap conversion
in xfs, it appears that we could use better test coverage of this crappy old
interface.  ;)
 
-Eric