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

From: Dave Chinner <david@fromorbit.com>
To: Chandan Babu R <chandan.babu@oracle.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: Tue, 28 Sep 2021 14:04:31 +1000	[thread overview]
Message-ID: <20210928040431.GP1756565@dread.disaster.area> (raw)
In-Reply-To: <20210927234637.GM1756565@dread.disaster.area>

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com