Re: [PATCH] ext4,fiemap: Add inode offset for xattr fiemap

["wangjianjian (C)" <wangjianjian3@huawei.com>]

On 2025/12/19 7:36, Darrick J. Wong wrote:
> On Thu, Dec 18, 2025 at 09:05:57AM +0800, wangjianjian (C) wrote:
>> On 2025/12/18 0:35, Darrick J. Wong wrote:
>>> On Wed, Dec 17, 2025 at 04:47:08PM +0800, Wang Jianjian wrote:
>>>> For xattr in inode, need add inode offset in this block?
>>>> Also, there is one problem, if we have xattrs both in inode
>>>> and block, current implementation will only return xattr inode fiemap.
>>>> Is this by design?
>>>
>>> I don't think there's much value in reporting the inline xattrs via
>>> FIEMAP because user programs can't directly access that area anyway.
>>> The only reason (AFAICT) for reporting the external xattr block is for
>>> building a map of lost data given a report of localized media failure.
>> yes, I agree with this. however, current behavior is it will always
>> reporting inline xattr first. Do you think we should fix this?
> 
> Nah.  If there are no complaints, then let's leave it alone.
> It's not like the xattr structure has a meaningful byte position index.
sure, let's keep it as is.
> 
>>> (FIEMAP only being useful for debugging and after-the-shatter forensics)
>>>
>>>> Signed-off-by: Wang Jianjian <wangjianjian3@huawei.com>
>>>> ---
>>>>    fs/ext4/extents.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>>>> index 2cf5759ba689..a16bfc75345d 100644
>>>> --- a/fs/ext4/extents.c
>>>> +++ b/fs/ext4/extents.c
>>>> @@ -5043,6 +5043,7 @@ static int ext4_iomap_xattr_fiemap(struct inode *inode, struct iomap *iomap)
>>>>    		if (error)
>>>>    			return error;
>>>>    		physical = (__u64)iloc.bh->b_blocknr << blockbits;
>>>> +		physical += iloc.offset;
>>>
>>> Also it doesn't make sense to add the address of the external block to
>>> the inode offset.
>> IIUC, bh is the buffer head of the inode is in and iloc.offset is its offset
>> of this block.
> 
> Oh silly me.  Yes, that's more correct, though if you really wanted to
> be pedantic, you could also add in the distance from the start of the
> inode core to wherever the xattr data actually is.
I think bh->b_blocknr << blockbits has been the offset from the very 
begin of this FS. But as above said, since nobody cares this, let's keep 
it as is. Thanks for your reply.
>   dfd
> --D
> 
>>>
>>> --D
>>>
>>>>    		offset = EXT4_GOOD_OLD_INODE_SIZE +
>>>>    				EXT4_I(inode)->i_extra_isize;
>>>>    		physical += offset;
>>>> -- 
>>>> 2.34.1
>>>>
>>>>
>> -- 
>> Regards
>>
>>
-- 
Regards