Re: [PATCH 2/2] xfs: Extend xattr extent counter to 32-bits

[Chandan Rajendra <chandan@linux.ibm.com>]

On Monday, April 27, 2020 1:09 PM Christoph Hellwig wrote: 
> FYI, I have had a series in the works for a while but not quite 
> finished yet that moves the in-memory nextents and format fields
> into the ifork structure.  I feared this might conflict badly, but
> so far this seems relatively harmless.  Note that your patch creates
> some not so nice layout in struct xfs_icdinode, so maybe I need to
> rush and finish that series ASAP.
> 
> > +static inline int32_t XFS_DFORK_NEXTENTS(struct xfs_sb *sbp,
> > +					struct xfs_dinode *dip, int whichfork)
> > +{
> > +	int32_t anextents;
> > +
> > +	if (whichfork == XFS_DATA_FORK)
> > +		return be32_to_cpu((dip)->di_nextents);
> > +
> > +	anextents = be16_to_cpu((dip)->di_anextents_lo);
> > +	if (xfs_sb_version_has_v3inode(sbp))
> > +		anextents |= ((u32)(be16_to_cpu((dip)->di_anextents_hi)) << 16);
> > +
> > +	return anextents;
> 
> No need for any of the braces around dip.  Also this funcion really
> deserves a proper lower case name now, and probably should be moved out
> of line.

Sure, I will implement that.

> 
> >  typedef uint32_t	xfs_extlen_t;	/* extent length in blocks */
> >  typedef uint32_t	xfs_agnumber_t;	/* allocation group number */
> >  typedef int32_t		xfs_extnum_t;	/* # of extents in a file */
> > -typedef int16_t		xfs_aextnum_t;	/* # extents in an attribute fork */
> > +typedef int32_t		xfs_aextnum_t;	/* # extents in an attribute fork */
> 
> We can just retire xfs_aextnum_t.  It only has 4 uses anyway.
> 
> > @@ -327,7 +327,7 @@ xfs_inode_to_log_dinode(
> >  	to->di_nblocks = from->di_nblocks;
> >  	to->di_extsize = from->di_extsize;
> >  	to->di_nextents = from->di_nextents;
> > -	to->di_anextents = from->di_anextents;
> > +	to->di_anextents_lo = ((u32)(from->di_anextents)) & 0xffff;
> 
> No need for any of the casting here.

Ok.

> 
> > @@ -3044,7 +3045,14 @@ xlog_recover_inode_pass2(
> >  			goto out_release;
> >  		}
> >  	}
> > -	if (unlikely(ldip->di_nextents + ldip->di_anextents > ldip->di_nblocks)){
> > +
> > +	nextents = ldip->di_anextents_lo;
> > +	if (xfs_sb_version_has_v3inode(&mp->m_sb))
> > +		nextents |= ((u32)(ldip->di_anextents_hi) << 16);
> > +
> > +	nextents += ldip->di_nextents;
> 
> Little helpers to get/set the attr extents in the log inode would be nice.
>

Ok. I will implement the helper functions.

> 
> Last but not least:  This seems like a feature flag we could just lazily
> set once needed, similar to attr2.
> 

Yes, I will implement this change before posting the next version of the
patchset.

-- 
chandan