From: Chandan Babu R <chandanrlinux@gmail.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org, david@fromorbit.com, bfoster@redhat.com
Subject: Re: [PATCH 2/3] xfs: Introduce xfs_dfork_nextents() helper
Date: Tue, 01 Sep 2020 19:48:30 +0530 [thread overview]
Message-ID: <1734571.3Z8NNOGFWv@garuda> (raw)
In-Reply-To: <20200831204832.GW6096@magnolia>
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@gmail.com>
> > ---
> > 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
next prev parent reply other threads:[~2020-09-01 14:37 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-31 13:00 [PATCH 0/3] xfs: Extend per-inode extent counters Chandan Babu R
2020-08-31 13:00 ` [PATCH 1/3] xfs: Introduce xfs_iext_max() helper Chandan Babu R
2020-08-31 20:43 ` Darrick J. Wong
2020-09-01 14:18 ` Chandan Babu R
2020-08-31 13:00 ` [PATCH 2/3] xfs: Introduce xfs_dfork_nextents() helper Chandan Babu R
2020-08-31 20:48 ` Darrick J. Wong
2020-09-01 14:18 ` Chandan Babu R [this message]
2020-08-31 13:00 ` [PATCH 3/3] xfs: Extend data/attr fork extent counter width Chandan Babu R
2020-08-31 21:13 ` Darrick J. Wong
2020-09-01 14:18 ` Chandan Babu R
2020-09-03 22:51 ` Dave Chinner
2020-09-04 8:57 ` Chandan Babu R
2020-09-04 15:51 ` Darrick J. Wong
2020-09-07 4:02 ` Chandan Babu R
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1734571.3Z8NNOGFWv@garuda \
--to=chandanrlinux@gmail.com \
--cc=bfoster@redhat.com \
--cc=darrick.wong@oracle.com \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.