Re: [PATCH V3 07/12] xfs: Rename inode's extent counter fields based on their width

From: Chandan Babu R <chandan.babu@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org, djwong@kernel.org
Subject: Re: [PATCH V3 07/12] xfs: Rename inode's extent counter fields based on their width
Date: Wed, 29 Sep 2021 22:33:23 +0530	[thread overview]
Message-ID: <87czors49w.fsf@debian-BULLSEYE-live-builder-AMD64> (raw)
In-Reply-To: <20210928040431.GP1756565@dread.disaster.area>

On 28 Sep 2021 at 09:34, Dave Chinner wrote:
> On Tue, Sep 28, 2021 at 09:46:37AM +1000, Dave Chinner wrote:
>> On Thu, Sep 16, 2021 at 03:36:42PM +0530, Chandan Babu R wrote:
>> > This commit renames extent counter fields in "struct xfs_dinode" and "struct
>> > xfs_log_dinode" based on the width of the fields. As of this commit, the
>> > 32-bit field will be used to count data fork extents and the 16-bit field will
>> > be used to count attr fork extents.
>> > 
>> > This change is done to enable a future commit to introduce a new 64-bit extent
>> > counter field.
>> > 
>> > Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>> > ---
>> >  fs/xfs/libxfs/xfs_format.h      |  8 ++++----
>> >  fs/xfs/libxfs/xfs_inode_buf.c   |  4 ++--
>> >  fs/xfs/libxfs/xfs_log_format.h  |  4 ++--
>> >  fs/xfs/scrub/inode_repair.c     |  4 ++--
>> >  fs/xfs/scrub/trace.h            | 14 +++++++-------
>> >  fs/xfs/xfs_inode_item.c         |  4 ++--
>> >  fs/xfs/xfs_inode_item_recover.c |  8 ++++----
>> >  7 files changed, 23 insertions(+), 23 deletions(-)
>> > 
>> > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
>> > index dba868f2c3e3..87c927d912f6 100644
>> > --- a/fs/xfs/libxfs/xfs_format.h
>> > +++ b/fs/xfs/libxfs/xfs_format.h
>> > @@ -802,8 +802,8 @@ typedef struct xfs_dinode {
>> >  	__be64		di_size;	/* number of bytes in file */
>> >  	__be64		di_nblocks;	/* # of direct & btree blocks used */
>> >  	__be32		di_extsize;	/* basic/minimum extent size for file */
>> > -	__be32		di_nextents;	/* number of extents in data fork */
>> > -	__be16		di_anextents;	/* number of extents in attribute fork*/
>> > +	__be32		di_nextents32;	/* number of extents in data fork */
>> > +	__be16		di_nextents16;	/* number of extents in attribute fork*/
>> 
>> 
>> Hmmm. Having the same field in the inode hold the extent count
>> for different inode forks based on a bit in the superblock means the
>> on-disk inode format is not self describing. i.e. we can't decode
>> the on-disk contents of an inode correctly without knowing whether a
>> specific feature bit is set in the superblock or not.
>
> Hmmmm - I just realised that there is an inode flag that indicates
> the format is different. It's jsut that most of the code doing
> conditional behaviour is using the superblock flag, not the inode
> flag as the conditional.
>
> So it is self describing, but I still don't like the way the same
> field is used for the different forks. It just feels like we are
> placing a landmine that we are going to forget about and step
> on in the future....
>

Sorry, I missed this response from you.

I agree with your suggestion. I will use the inode version number to help in
deciding which extent counter fields are valid for a specific inode.

-- 
chandan