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
