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