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

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

[Wang Jianjian]

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?

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;
 		offset = EXT4_GOOD_OLD_INODE_SIZE +
 				EXT4_I(inode)->i_extra_isize;
 		physical += offset;
-- 
2.34.1

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

[Darrick J. Wong]

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.

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

--D

>  		offset = EXT4_GOOD_OLD_INODE_SIZE +
>  				EXT4_I(inode)->i_extra_isize;
>  		physical += offset;
> -- 
> 2.34.1
> 
> 

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

[wangjianjian (C)]

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?
> 
> (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.
> 
> --D
> 
>>   		offset = EXT4_GOOD_OLD_INODE_SIZE +
>>   				EXT4_I(inode)->i_extra_isize;
>>   		physical += offset;
>> -- 
>> 2.34.1
>>
>>
-- 
Regards

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

[Darrick J. Wong]

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.

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

--D

> > 
> > --D
> > 
> > >   		offset = EXT4_GOOD_OLD_INODE_SIZE +
> > >   				EXT4_I(inode)->i_extra_isize;
> > >   		physical += offset;
> > > -- 
> > > 2.34.1
> > > 
> > > 
> -- 
> Regards
> 
> 

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

[wangjianjian (C)]

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