On Tuesday 1 September 2020 2:18:32 AM IST Darrick J. Wong wrote:
> On Mon, Aug 31, 2020 at 06:30:09PM +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>
> > ---
> > fs/xfs/libxfs/xfs_format.h | 4 ----
> > fs/xfs/libxfs/xfs_inode_buf.c | 29 ++++++++++++++++++++++++-----
> > fs/xfs/libxfs/xfs_inode_buf.h | 2 ++
> > fs/xfs/libxfs/xfs_inode_fork.c | 10 +++++++---
> > fs/xfs/scrub/inode.c | 12 +++++++-----
> > 5 files changed, 40 insertions(+), 17 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> > index 31b7ece985bb..5f41e177dbda 100644
> > --- a/fs/xfs/libxfs/xfs_format.h
> > +++ b/fs/xfs/libxfs/xfs_format.h
> > @@ -992,10 +992,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/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> > index 5dcd71bfab2e..cce2aa99aad8 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.c
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> > @@ -368,9 +368,11 @@ xfs_dinode_verify_fork(
> > struct xfs_mount *mp,
> > int whichfork)
> > {
> > - uint32_t di_nextents = XFS_DFORK_NEXTENTS(dip, whichfork);
> > + uint32_t di_nextents;
> > xfs_extnum_t max_extents;
> >
> > + di_nextents = xfs_dfork_nextents(&mp->m_sb, dip, whichfork);
> > +
> > switch (XFS_DFORK_FORMAT(dip, whichfork)) {
> > case XFS_DINODE_FMT_LOCAL:
> > /*
> > @@ -401,6 +403,19 @@ xfs_dinode_verify_fork(
> > return NULL;
> > }
> >
> > +xfs_extnum_t
> > +xfs_dfork_nextents(struct xfs_sb *sbp, struct xfs_dinode *dip, int whichfork)
>
> The first arg probaly should be a (struct xfs_mount *) to save the
> callers some typing.
Hmm. I actually did try that ... but ended up failing. I don't recall the
reason now. I will implement this suggestion.
>
> > +{
> > + xfs_extnum_t nextents;
> > +
> > + if (whichfork == XFS_DATA_FORK)
> > + nextents = be32_to_cpu(dip->di_nextents);
> > + else
> > + nextents = be16_to_cpu(dip->di_anextents);
> > +
> > + return nextents;
>
> Would this be cleaner (as well as barf loudly on invalid args)?
>
> switch (whichfork) {
> case XFS_DATA_FORK:
> return be32_to_cpu(...);
> case XFS_ATTR_FORK:
> return be16_to_cpu(...);
> default:
> ASSERT(0);
> return 0;
> }
Yes, This is much better. I will include this in the next version of the
patchset.
>
>
> > +}
> > +
> > static xfs_failaddr_t
> > xfs_dinode_verify_forkoff(
> > struct xfs_dinode *dip,
> > @@ -437,6 +452,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;
> > @@ -467,10 +484,12 @@ xfs_dinode_verify(
> > if ((S_ISLNK(mode) || S_ISDIR(mode)) && di_size == 0)
> > return __this_address;
> >
> > + 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 &&
> > - be32_to_cpu(dip->di_nextents) + be16_to_cpu(dip->di_anextents) >
> > - be64_to_cpu(dip->di_nblocks))
> > + if (mode && nextents > nblocks)
> > return __this_address;
> >
> > if (mode && XFS_DFORK_BOFF(dip) > mp->m_sb.sb_inodesize)
> > @@ -527,7 +546,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/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> > index 6b08b9d060c2..a97caf675aaf 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.h
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> > @@ -59,5 +59,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);
>
> Please try to match the second-line indentation of the declaration above
> it. Or: two tabs is enough.
Sorry, I missed out on this one. I will fix this up.
>
> >
> > #endif /* __XFS_INODE_BUF_H__ */
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > index 3a084aea8f85..2ce80bb5c70a 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > @@ -10,6 +10,7 @@
> > #include "xfs_format.h"
> > #include "xfs_log_format.h"
> > #include "xfs_trans_resv.h"
> > +#include "xfs_sb.h"
> > #include "xfs_mount.h"
> > #include "xfs_inode.h"
> > #include "xfs_trans.h"
> > @@ -104,9 +105,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 nex = xfs_dfork_nextents(sbp, dip, whichfork);
> > int state = xfs_bmap_fork_to_state(whichfork);
> > - int nex = XFS_DFORK_NEXTENTS(dip, whichfork);
> > int size = nex * sizeof(xfs_bmbt_rec_t);
> > struct xfs_iext_cursor icur;
> > struct xfs_bmbt_rec *dp;
> > @@ -226,6 +228,7 @@ xfs_iformat_data_fork(
> > struct xfs_inode *ip,
> > struct xfs_dinode *dip)
> > {
> > + struct xfs_sb *sbp = &ip->i_mount->m_sb;
> > struct inode *inode = VFS_I(ip);
> > int error;
> >
> > @@ -234,7 +237,7 @@ xfs_iformat_data_fork(
> > * depend on it.
> > */
> > ip->i_df.if_format = dip->di_format;
> > - ip->i_df.if_nextents = be32_to_cpu(dip->di_nextents);
> > + ip->i_df.if_nextents = xfs_dfork_nextents(sbp, dip, XFS_DATA_FORK);
> >
> > switch (inode->i_mode & S_IFMT) {
> > case S_IFIFO:
> > @@ -286,6 +289,7 @@ xfs_iformat_attr_fork(
> > struct xfs_inode *ip,
> > struct xfs_dinode *dip)
> > {
> > + struct xfs_sb *sbp = &ip->i_mount->m_sb;
> > int error = 0;
> >
> > /*
> > @@ -296,7 +300,7 @@ xfs_iformat_attr_fork(
> > ip->i_afp->if_format = dip->di_aformat;
> > if (unlikely(ip->i_afp->if_format == 0)) /* pre IRIX 6.2 file system */
> > ip->i_afp->if_format = XFS_DINODE_FMT_EXTENTS;
> > - ip->i_afp->if_nextents = be16_to_cpu(dip->di_anextents);
> > + ip->i_afp->if_nextents = xfs_dfork_nextents(sbp, dip, XFS_ATTR_FORK);
> >
> > switch (ip->i_afp->if_format) {
> > case XFS_DINODE_FMT_LOCAL:
> > diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
> > index 6d483ab29e63..e428ad0eef03 100644
> > --- a/fs/xfs/scrub/inode.c
> > +++ b/fs/xfs/scrub/inode.c
> > @@ -354,7 +354,7 @@ xchk_dinode(
> > xchk_inode_extsize(sc, dip, ino, mode, flags);
> >
> > /* di_nextents */
> > - nextents = be32_to_cpu(dip->di_nextents);
> > + nextents = xfs_dfork_nextents(&mp->m_sb, dip, XFS_DATA_FORK);
> > fork_recs = XFS_DFORK_DSIZE(dip, mp) / sizeof(struct xfs_bmbt_rec);
> > switch (dip->di_format) {
> > case XFS_DINODE_FMT_EXTENTS:
> > @@ -371,10 +371,12 @@ xchk_dinode(
> > break;
> > }
> >
> > + nextents = xfs_dfork_nextents(&mp->m_sb, dip, XFS_ATTR_FORK);
>
> Might be a good time to change these all to "naextents"?
Sure, I will make the change.
>
> > +
> > /* di_forkoff */
> > if (XFS_DFORK_APTR(dip) >= (char *)dip + mp->m_sb.sb_inodesize)
> > xchk_ino_set_corrupt(sc, ino);
> > - if (dip->di_anextents != 0 && dip->di_forkoff == 0)
> > + if (nextents != 0 && dip->di_forkoff == 0)
> > xchk_ino_set_corrupt(sc, ino);
> > if (dip->di_forkoff == 0 && dip->di_aformat != XFS_DINODE_FMT_EXTENTS)
> > xchk_ino_set_corrupt(sc, ino);
> > @@ -386,7 +388,6 @@ xchk_dinode(
> > xchk_ino_set_corrupt(sc, ino);
> >
> > /* di_anextents */
> > - nextents = be16_to_cpu(dip->di_anextents);
> > fork_recs = XFS_DFORK_ASIZE(dip, mp) / sizeof(struct xfs_bmbt_rec);
> > switch (dip->di_aformat) {
> > case XFS_DINODE_FMT_EXTENTS:
> > @@ -464,6 +465,7 @@ xchk_inode_xref_bmap(
> > struct xfs_scrub *sc,
> > struct xfs_dinode *dip)
> > {
> > + xfs_mount_t *mp = sc->mp;
>
> Don't increase the usage of structure typedefs, please.
Sorry, I will replace it with 'struct xfs_mount'.
>
> --D
>
> > xfs_extnum_t nextents;
> > xfs_filblks_t count;
> > xfs_filblks_t acount;
> > @@ -477,14 +479,14 @@ xchk_inode_xref_bmap(
> > &nextents, &count);
> > if (!xchk_should_check_xref(sc, &error, NULL))
> > return;
> > - if (nextents < be32_to_cpu(dip->di_nextents))
> > + if (nextents < xfs_dfork_nextents(&mp->m_sb, dip, XFS_DATA_FORK))
> > xchk_ino_xref_set_corrupt(sc, sc->ip->i_ino);
> >
> > error = xfs_bmap_count_blocks(sc->tp, sc->ip, XFS_ATTR_FORK,
> > &nextents, &acount);
> > if (!xchk_should_check_xref(sc, &error, NULL))
> > return;
> > - if (nextents != be16_to_cpu(dip->di_anextents))
> > + if (nextents != xfs_dfork_nextents(&mp->m_sb, dip, XFS_ATTR_FORK))
> > xchk_ino_xref_set_corrupt(sc, sc->ip->i_ino);
> >
> > /* Check nblocks against the inode. */
>
--
chandan
