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

[Chandan Babu R <chandan.babu@oracle.com>]

On 30 Sep 2021 at 10:01, Dave Chinner wrote:
> On Thu, Sep 30, 2021 at 10:40:15AM +1000, Dave Chinner wrote:
>> On Wed, Sep 29, 2021 at 10:33:23PM +0530, Chandan Babu R wrote:
>> > 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.
>> 
>> No, don't do something I suggested with a flawed understanding of
>> the code.
>> 
>> Just because *I* suggest something, it means you have to make that
>> change. That is reacting to *who* said something, not *what was
>> said*.
>> 
>> So, I may have reservations about the way the storage definitions
>> are being redefined, but if I had a valid, technical argument I
>> could give right now I would have said so directly. I can't put my
>> finger on why this worries me in this case but didn't for something
>> like, say, the BIGTIME feature which redefined the contents of
>> various fields in the inode.
>> 
>> IOWs, I haven't really had time to think and go back over the rest
>> of the patchset since I realised my mistake and determine if that
>> changes what I think about this, so don't go turning the patchset
>> upside just because *I suggested something*.
>
> So, looking over the patchset more, I think I understand my feeling
> a bit better. Inconsistency is a big part of it.
>
> The in-memory extent counts are held in the struct xfs_inode_fork
> and not the inode. The type is a xfs_extcnt_t - it's not a size
> dependent type. Indeed, there are actually no users of the
> xfs_aextcnt_t variable in XFS at all any more. It should be removed.
>
> What this means is that in-memory inode extent counting just doesn't
> discriminate between inode fork types. They are all 64 bit counters,
> and all the limits applied to them should be 64 bit types. Even the
> checks for overflow are abstracted away by
> xfs_iext_count_may_overflow(), so none of the extent manipulation
> code has any idea there are different types and limits in the
> on-disk format.
>
> That's good.
>
> The only place the actual type matters is when looking at the raw
> disk inode and, unfortunately, that's where it gets messy. Anything
> accessing the on-disk inode directly has to look at inode version
> number, and an inode feature flag to interpret the inode format
> correctly.  That format is then reflected in an in-memory inode
> feature flag, and then there's the superblock feature flag on top of
> that to indicate that there are NREXT64 format inodes in the
> filesystem.
>
> Then there's implied dynamic upgrades of the on-disk inode format.
> We see that being implied in xfs_inode_to_disk_iext_counters() and
> xfs_trans_log_inode() but the filesystem format can't be changed
> dynamically. i.e. we can't create new NREXT64 inodes if the
> superblock flag is not set, so there is no code in this patchset
> that I can see that provides a trigger for a dynamic upgrade to
> start. IOWs, the filesystem has to be taken offline to change the
> superblock feature bit, and the setup of the default NREXT64 inode
> flag at mount time re-inforces this.
>
> With this in mind, I started to see inconsistent use of inode
> feature flag vs superblock feature flag to determine on-disk inode
> extent count limits. e.g. look at xfs_iext_count_may_overflow() and
> xfs_iext_max_nextents(). Both of these are determining the maximum
> number of extents that are valid for an inode, and they look at the
> -superblock feature bit- to determine the limits.
>
> This only works if all inodes in the filesystem have the same
> format, which is not true if we are doing dynamic upgrades of the
> inode features. The most obvious case here is that scrub needs to
> determine the layout and limits based on the current feature bits in
> the inode, not the superblock feature bit.
>
> Then we have to look at how the upgrade is performed - by changing
> the in-memory inode flag during xfs_trans_log_inode() when the inode
> is dirtied. When we are modifying the inode for extent allocation,
> we check the extent count limits on the inode *before* we dirty the
> inode. Hence the only way an "upgrade at overflow thresholds" can
> actually work is if we don't use the inode flag for determining
> limits but instead use the sueprblock feature bit limits. But as
> I've already pointed out, that leads to other problems.
>
> When we are converting an inode format, we currently do it when the
> inode is first brought into memory and read from disk (i.e.
> xfs_inode_from_disk()). We do the full conversion at this point in
> time, such that if the inode is dirtied in memory all the correct
> behaviour for the new format occurs and the writeback is done in the
> new format.
>
> This would allow xfs_iext_count_may_overflow/xfs_iext_max_nextents
> to actually return the correct limits for the inode as it is being
> modified and not have to rely on superblock feature bits. If the
> inode is not being modified, then the in-memory format changes are
> discarded when the inode is reclaimed from memory and nothing
> changes on disk.
>
> This means that once we've read the inode in from disk and set up
> ip->i_diflags2 according to the superblock feature bit, we can use
> the in-memory inode flag -everywhere- we need to find and/or check
> limits during modifications. Yes, I know that the BIGTIME upgrade
> path does this, but that doesn't have limits that prevent
> modifications from taking place before we can log the inode and set
> the BIGTIME flag....
>

Ok. The above solution looks logically correct. I haven't been able to come up
with a scenario where the solution wouldn't work. I will implement it and see
if anything breaks.

> So, yeah, I think the biggest problem I've been having is that the
> way the inode flags, the limits and the on-disk format is juggled
> has resulted in me taking some time to understand where the problems
> lie. Cleaning up the initialisation, conversion and consistency in
> using the inode flags rather thant he superblock flag will go a long
> way to addressing my concerns
>
> ---
>
> FWIW, I also think doing something like this would help make the
> code be easier to read and confirm that it is obviously correct when
> reading it:
>
> 	__be32          di_gid;         /* owner's group id */
> 	__be32          di_nlink;       /* number of links to file */
> 	__be16          di_projid_lo;   /* lower part of owner's project id */
> 	__be16          di_projid_hi;   /* higher part owner's project id */
> 	union {
> 		__be64	di_big_dextcnt;	/* NREXT64 data extents */
> 		__u8	di_v3_pad[8];	/* !NREXT64 V3 inode zeroed space */
> 		struct {
> 			__u8	di_v2_pad[6];	/* V2 inode zeroed space */
> 			__be16	di_flushiter;	/* V2 inode incremented on flush */
> 		};
> 	};
> 	xfs_timestamp_t di_atime;       /* time last accessed */
> 	xfs_timestamp_t di_mtime;       /* time last modified */
> 	xfs_timestamp_t di_ctime;       /* time created/inode modified */
> 	__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 */
> 	union {
> 		struct {
> 			__be32	di_big_aextcnt; /* NREXT64 attr extents */
> 			__be16	di_nrext64_pad;	/* NREXT64 unused, zero */
> 		};
> 		struct {
> 			__be32	di_nextents;    /* !NREXT64 data extents */
> 			__be16	di_anextents;   /* !NREXT64 attr extents */
> 		}
> 	}
> 	__u8            di_forkoff;     /* attr fork offs, <<3 for 64b align */
> 	__s8            di_aformat;     /* format of attr fork's data */
> ...
>
> Then we get something like:
>
> static inline void
> xfs_inode_to_disk_iext_counters(
>        struct xfs_inode        *ip,
>        struct xfs_dinode       *to)
> {
>        if (xfs_inode_has_nrext64(ip)) {
>                to->di_big_dextent_cnt = cpu_to_be64(xfs_ifork_nextents(&ip->i_df));
>                to->di_big_anextents = cpu_to_be32(xfs_ifork_nextents(ip->i_afp));
>                to->di_nrext64_pad = 0;
>        } else {
>                to->di_nextents = cpu_to_be32(xfs_ifork_nextents(&ip->i_df));
>                to->di_anextents = cpu_to_be16(xfs_ifork_nextents(ip->i_afp));
>        }
> }
>
> This is now obvious that we are writing to the correct fields
> in the inode for the feature bits that are set, and we don't need
> to zero the di_big_dextcnt field because that's been taken care of
> by the existing di_v2_pad/flushiter zeroing. That bit could probably
> be improved by unwinding and open coding this in xfs_inode_to_disk(),
> but I think what I'm proposing should be obvious now...
>

Yes, the explaination provided by you is very clear. I will implement these
suggestions.

-- 
chandan