On Tue, Sep 01, 2020 at 07:47:41PM +0530, Chandan Babu R wrote: > On Tuesday 1 September 2020 2:24:26 AM IST Darrick J. Wong wrote: > > On Mon, Aug 31, 2020 at 06:31:00PM +0530, Chandan Babu R wrote: > > > This commit replaces the macro XFS_DFORK_NEXTENTS() with the helper > > > function xfs_dfork_nextents(). As of this commit, xfs_dfork_nextents() > > > returns the same value as XFS_DFORK_NEXTENTS(). A future commit which > > > extends inode's extent counter fields will add more logic to this > > > helper. > > > > > > This commit also replaces direct accesses to xfs_dinode->di_[a]nextents > > > with calls to xfs_dfork_nextents(). > > > > > > No functional changes have been made. > > > > > > Signed-off-by: Chandan Babu R <chandanrlinux@xxxxxxxxx> > > > --- > > > db/bmap.c | 6 +++--- > > > db/btdump.c | 4 ++-- > > > db/check.c | 2 +- > > > db/frag.c | 8 ++++--- > > > db/inode.c | 14 ++++++------ > > > db/metadump.c | 4 ++-- > > > libxfs/xfs_format.h | 4 ---- > > > libxfs/xfs_inode_buf.c | 26 ++++++++++++++++------ > > > libxfs/xfs_inode_buf.h | 2 ++ > > > libxfs/xfs_inode_fork.c | 3 ++- > > > repair/attr_repair.c | 2 +- > > > repair/dinode.c | 48 +++++++++++++++++++++++------------------ > > > repair/prefetch.c | 2 +- > > > 13 files changed, 74 insertions(+), 51 deletions(-) > > > > > > diff --git a/db/bmap.c b/db/bmap.c > > > index fdc70e95..9800a909 100644 > > > --- a/db/bmap.c > > > +++ b/db/bmap.c > > > @@ -68,7 +68,7 @@ bmap( > > > ASSERT(fmt == XFS_DINODE_FMT_LOCAL || fmt == XFS_DINODE_FMT_EXTENTS || > > > fmt == XFS_DINODE_FMT_BTREE); > > > if (fmt == XFS_DINODE_FMT_EXTENTS) { > > > - nextents = XFS_DFORK_NEXTENTS(dip, whichfork); > > > + nextents = xfs_dfork_nextents(&mp->m_sb, dip, whichfork); > > > xp = (xfs_bmbt_rec_t *)XFS_DFORK_PTR(dip, whichfork); > > > for (ep = xp; ep < &xp[nextents] && n < nex; ep++) { > > > if (!bmap_one_extent(ep, &curoffset, eoffset, &n, bep)) > > > @@ -158,9 +158,9 @@ bmap_f( > > > push_cur(); > > > set_cur_inode(iocur_top->ino); > > > dip = iocur_top->data; > > > - if (be32_to_cpu(dip->di_nextents)) > > > + if (xfs_dfork_nextents(&mp->m_sb, dip, XFS_DATA_FORK)) > > > > Suggestion: Shift these kinds of changes to a separate patch to minimize > > the amount of non-libxfs changes in a patch that will (eventually) be > > ported from the kernel. Ideally, the only changes to db/ and repair/ > > and mkfs/ would be the ones that are necessary to avoid breaking the > > build. > > > > Once you've separated the other conversions (like this one here) into a > > separate patch, we can review that as a separate refactoring change to > > userspace. > > So the changes should be split into two patches -- One patch containing > conversion changes to code inside libxfs and the other one for non-libxfs > code. Please correct me if my understanding is incorrect. I usually try to split the responsibilities in this manner: "libxfs: prepare for FROB API change" -- make whatever change I need to so that the next patch doesn't become insanely difficult. This patch is optional. "xfs: change FROB API to frob" -- this is a strict backport of changes from the kernel git tree to xfsprogs. The only changes outside of libxfs/ are fixes whatever tool breakage happens. "xfs_db: support new FROB" -- add whatever new code you need to add to xfs_db to support the new frob "xfs_repair: support new FROB" -- same thing with repair <repeat with the other tools> "mkfs: support new FROB" -- same thing with mkfs "xfs: officially enable FROB feature" -- make it so that libxfs will actually recognize whatever new feature you're adding, if you're adding one. --D > > > > The reason for this ofc is that when the maintainers run libxfs-apply to > > pull in the kernel patches, they're totally going to miss things like > > this conversion unless you make them an explicit separate change. > > > > FWIW the conversions themselves mostly look ok... > > > > > dfork = 1; > > > - if (be16_to_cpu(dip->di_anextents)) > > > + if (xfs_dfork_nextents(&mp->m_sb, dip, XFS_ATTR_FORK)) > > > afork = 1; > > > pop_cur(); > > > } > > > diff --git a/db/btdump.c b/db/btdump.c > > > index 920f595b..9ced71d4 100644 > > > --- a/db/btdump.c > > > +++ b/db/btdump.c > > > @@ -166,13 +166,13 @@ dump_inode( > > > > > > dip = iocur_top->data; > > > if (attrfork) { > > > - if (!dip->di_anextents || > > > + if (!xfs_dfork_nextents(&mp->m_sb, dip, XFS_ATTR_FORK) || > > > dip->di_aformat != XFS_DINODE_FMT_BTREE) { > > > dbprintf(_("attr fork not in btree format\n")); > > > return 0; > > > } > > > } else { > > > - if (!dip->di_nextents || > > > + if (!xfs_dfork_nextents(&mp->m_sb, dip, XFS_DATA_FORK) || > > > dip->di_format != XFS_DINODE_FMT_BTREE) { > > > dbprintf(_("data fork not in btree format\n")); > > > return 0; > > > diff --git a/db/check.c b/db/check.c > > > index 12c03b6d..2d1823a4 100644 > > > --- a/db/check.c > > > +++ b/db/check.c > > > @@ -2686,7 +2686,7 @@ process_exinode( > > > xfs_bmbt_rec_t *rp; > > > > > > rp = (xfs_bmbt_rec_t *)XFS_DFORK_PTR(dip, whichfork); > > > - *nex = XFS_DFORK_NEXTENTS(dip, whichfork); > > > + *nex = xfs_dfork_nextents(&mp->m_sb, dip, whichfork); > > > if (*nex < 0 || *nex > XFS_DFORK_SIZE(dip, mp, whichfork) / > > > sizeof(xfs_bmbt_rec_t)) { > > > if (!sflag || id->ilist) > > > diff --git a/db/frag.c b/db/frag.c > > > index 1cfc6c2c..20fb1306 100644 > > > --- a/db/frag.c > > > +++ b/db/frag.c > > > @@ -262,9 +262,11 @@ process_exinode( > > > int whichfork) > > > { > > > xfs_bmbt_rec_t *rp; > > > + xfs_extnum_t nextents; > > > > > > rp = (xfs_bmbt_rec_t *)XFS_DFORK_PTR(dip, whichfork); > > > - process_bmbt_reclist(rp, XFS_DFORK_NEXTENTS(dip, whichfork), extmapp); > > > + nextents = xfs_dfork_nextents(&mp->m_sb, dip, whichfork); > > > + process_bmbt_reclist(rp, nextents, extmapp); > > > } > > > > > > static void > > > @@ -273,9 +275,9 @@ process_fork( > > > int whichfork) > > > { > > > extmap_t *extmap; > > > - int nex; > > > + xfs_extnum_t nex; > > > > > > - nex = XFS_DFORK_NEXTENTS(dip, whichfork); > > > + nex = xfs_dfork_nextents(&mp->m_sb, dip, whichfork); > > > if (!nex) > > > return; > > > extmap = extmap_alloc(nex); > > > diff --git a/db/inode.c b/db/inode.c > > > index 0cff9d63..3853092c 100644 > > > --- a/db/inode.c > > > +++ b/db/inode.c > > > @@ -271,7 +271,7 @@ inode_a_bmx_count( > > > return 0; > > > ASSERT((char *)XFS_DFORK_APTR(dip) - (char *)dip == byteize(startoff)); > > > return dip->di_aformat == XFS_DINODE_FMT_EXTENTS ? > > > - be16_to_cpu(dip->di_anextents) : 0; > > > + xfs_dfork_nextents(&mp->m_sb, dip, XFS_ATTR_FORK) : 0; > > > } > > > > > > static int > > > @@ -325,6 +325,7 @@ inode_a_size( > > > { > > > xfs_attr_shortform_t *asf; > > > xfs_dinode_t *dip; > > > + xfs_extnum_t nextents; > > > > > > ASSERT(startoff == 0); > > > ASSERT(idx == 0); > > > @@ -334,8 +335,8 @@ inode_a_size( > > > asf = (xfs_attr_shortform_t *)XFS_DFORK_APTR(dip); > > > return bitize(be16_to_cpu(asf->hdr.totsize)); > > > case XFS_DINODE_FMT_EXTENTS: > > > - return (int)be16_to_cpu(dip->di_anextents) * > > > - bitsz(xfs_bmbt_rec_t); > > > + nextents = xfs_dfork_nextents(&mp->m_sb, dip, XFS_ATTR_FORK); > > > + return (int)(nextents * bitsz(xfs_bmbt_rec_t)); > > > case XFS_DINODE_FMT_BTREE: > > > return bitize((int)XFS_DFORK_ASIZE(dip, mp)); > > > default: > > > @@ -496,7 +497,7 @@ inode_u_bmx_count( > > > dip = obj; > > > ASSERT((char *)XFS_DFORK_DPTR(dip) - (char *)dip == byteize(startoff)); > > > return dip->di_format == XFS_DINODE_FMT_EXTENTS ? > > > - be32_to_cpu(dip->di_nextents) : 0; > > > + xfs_dfork_nextents(&mp->m_sb, dip, XFS_DATA_FORK) : 0; > > > } > > > > > > static int > > > @@ -582,6 +583,7 @@ inode_u_size( > > > int idx) > > > { > > > xfs_dinode_t *dip; > > > + xfs_extnum_t nextents; > > > > > > ASSERT(startoff == 0); > > > ASSERT(idx == 0); > > > @@ -592,8 +594,8 @@ inode_u_size( > > > case XFS_DINODE_FMT_LOCAL: > > > return bitize((int)be64_to_cpu(dip->di_size)); > > > case XFS_DINODE_FMT_EXTENTS: > > > - return (int)be32_to_cpu(dip->di_nextents) * > > > - bitsz(xfs_bmbt_rec_t); > > > + nextents = xfs_dfork_nextents(&mp->m_sb, dip, XFS_DATA_FORK); > > > + return (int)(nextents * bitsz(xfs_bmbt_rec_t)); > > > case XFS_DINODE_FMT_BTREE: > > > return bitize((int)XFS_DFORK_DSIZE(dip, mp)); > > > case XFS_DINODE_FMT_UUID: > > > diff --git a/db/metadump.c b/db/metadump.c > > > index e5cb3aa5..6a6757a2 100644 > > > --- a/db/metadump.c > > > +++ b/db/metadump.c > > > @@ -2282,7 +2282,7 @@ process_exinode( > > > > > > whichfork = (itype == TYP_ATTR) ? XFS_ATTR_FORK : XFS_DATA_FORK; > > > > > > - nex = XFS_DFORK_NEXTENTS(dip, whichfork); > > > + nex = xfs_dfork_nextents(&mp->m_sb, dip, whichfork); > > > used = nex * sizeof(xfs_bmbt_rec_t); > > > if (nex < 0 || used > XFS_DFORK_SIZE(dip, mp, whichfork)) { > > > if (show_warnings) > > > @@ -2335,7 +2335,7 @@ static int > > > process_dev_inode( > > > xfs_dinode_t *dip) > > > { > > > - if (XFS_DFORK_NEXTENTS(dip, XFS_DATA_FORK)) { > > > + if (xfs_dfork_nextents(&mp->m_sb, dip, XFS_DATA_FORK)) { > > > if (show_warnings) > > > print_warning("inode %llu has unexpected extents", > > > (unsigned long long)cur_ino); > > > diff --git a/libxfs/xfs_format.h b/libxfs/xfs_format.h > > > index a738cd8b..188deada 100644 > > > --- a/libxfs/xfs_format.h > > > +++ b/libxfs/xfs_format.h > > > @@ -993,10 +993,6 @@ enum xfs_dinode_fmt { > > > ((w) == XFS_DATA_FORK ? \ > > > (dip)->di_format : \ > > > (dip)->di_aformat) > > > -#define XFS_DFORK_NEXTENTS(dip,w) \ > > > - ((w) == XFS_DATA_FORK ? \ > > > - be32_to_cpu((dip)->di_nextents) : \ > > > - be16_to_cpu((dip)->di_anextents)) > > > > > > /* > > > * For block and character special files the 32bit dev_t is stored at the > > > diff --git a/libxfs/xfs_inode_buf.c b/libxfs/xfs_inode_buf.c > > > index ae71a19e..d5584372 100644 > > > --- a/libxfs/xfs_inode_buf.c > > > +++ b/libxfs/xfs_inode_buf.c > > > @@ -362,9 +362,10 @@ xfs_dinode_verify_fork( > > > struct xfs_mount *mp, > > > int whichfork) > > > { > > > - uint32_t di_nextents = XFS_DFORK_NEXTENTS(dip, whichfork); > > > xfs_extnum_t max_extents; > > > + uint32_t di_nextents; > > > > > > + di_nextents = xfs_dfork_nextents(&mp->m_sb, dip, whichfork); > > > > > > switch (XFS_DFORK_FORMAT(dip, whichfork)) { > > > case XFS_DINODE_FMT_LOCAL: > > > @@ -396,6 +397,15 @@ xfs_dinode_verify_fork( > > > return NULL; > > > } > > > > > > +xfs_extnum_t > > > +xfs_dfork_nextents(struct xfs_sb *sbp, struct xfs_dinode *dip, int whichfork) > > > +{ > > > + if (whichfork == XFS_DATA_FORK) > > > + return be32_to_cpu(dip->di_nextents); > > > + else > > > + return be16_to_cpu(dip->di_anextents); > > > +} > > > + > > > static xfs_failaddr_t > > > xfs_dinode_verify_forkoff( > > > struct xfs_dinode *dip, > > > @@ -432,6 +442,8 @@ xfs_dinode_verify( > > > uint16_t flags; > > > uint64_t flags2; > > > uint64_t di_size; > > > + xfs_extnum_t nextents; > > > + int64_t nblocks; > > > > > > if (dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC)) > > > return __this_address; > > > @@ -462,10 +474,12 @@ xfs_dinode_verify( > > > if ((S_ISLNK(mode) || S_ISDIR(mode)) && di_size == 0) > > > return __this_address; > > > > > > - /* Fork checks carried over from xfs_iformat_fork */ > > > - if (mode && > > > - be32_to_cpu(dip->di_nextents) + be16_to_cpu(dip->di_anextents) > > > > - be64_to_cpu(dip->di_nblocks)) > > > + nextents = xfs_dfork_nextents(&mp->m_sb, dip, XFS_DATA_FORK); > > > + nextents += xfs_dfork_nextents(&mp->m_sb, dip, XFS_ATTR_FORK); > > > + nblocks = be64_to_cpu(dip->di_nblocks); > > > + > > > + /* Fork checks carried over from xfs_iformat_fork */ > > > + if (mode && nextents > nblocks) > > > return __this_address; > > > > > > if (mode && XFS_DFORK_BOFF(dip) > mp->m_sb.sb_inodesize) > > > @@ -522,7 +536,7 @@ xfs_dinode_verify( > > > default: > > > return __this_address; > > > } > > > - if (dip->di_anextents) > > > + if (xfs_dfork_nextents(&mp->m_sb, dip, XFS_ATTR_FORK)) > > > return __this_address; > > > } > > > > > > diff --git a/libxfs/xfs_inode_buf.h b/libxfs/xfs_inode_buf.h > > > index 9b373dcf..f97b3428 100644 > > > --- a/libxfs/xfs_inode_buf.h > > > +++ b/libxfs/xfs_inode_buf.h > > > @@ -71,5 +71,7 @@ xfs_failaddr_t xfs_inode_validate_extsize(struct xfs_mount *mp, > > > xfs_failaddr_t xfs_inode_validate_cowextsize(struct xfs_mount *mp, > > > uint32_t cowextsize, uint16_t mode, uint16_t flags, > > > uint64_t flags2); > > > +xfs_extnum_t xfs_dfork_nextents(struct xfs_sb *sbp, struct xfs_dinode *dip, > > > + int whichfork); > > > > > > #endif /* __XFS_INODE_BUF_H__ */ > > > diff --git a/libxfs/xfs_inode_fork.c b/libxfs/xfs_inode_fork.c > > > index 80ba6c12..8c32f993 100644 > > > --- a/libxfs/xfs_inode_fork.c > > > +++ b/libxfs/xfs_inode_fork.c > > > @@ -205,9 +205,10 @@ xfs_iformat_extents( > > > int whichfork) > > > { > > > struct xfs_mount *mp = ip->i_mount; > > > + struct xfs_sb *sbp = &mp->m_sb; > > > struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork); > > > int state = xfs_bmap_fork_to_state(whichfork); > > > - int nex = XFS_DFORK_NEXTENTS(dip, whichfork); > > > + xfs_extnum_t nex = xfs_dfork_nextents(sbp, dip, whichfork); > > > int size = nex * sizeof(xfs_bmbt_rec_t); > > > struct xfs_iext_cursor icur; > > > struct xfs_bmbt_rec *dp; > > > diff --git a/repair/attr_repair.c b/repair/attr_repair.c > > > index 6cec0f70..b6ca564b 100644 > > > --- a/repair/attr_repair.c > > > +++ b/repair/attr_repair.c > > > @@ -1083,7 +1083,7 @@ process_longform_attr( > > > bno = blkmap_get(blkmap, 0); > > > if (bno == NULLFSBLOCK) { > > > if (dip->di_aformat == XFS_DINODE_FMT_EXTENTS && > > > - be16_to_cpu(dip->di_anextents) == 0) > > > + xfs_dfork_nextents(&mp->m_sb, dip, XFS_ATTR_FORK) == 0) > > > > ^ > > This should /not/ be indented so that it lines up with the if body. > > Sorry about that. I will fix it up. > > -- > chandan > > >