From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Brian Foster <bfoster@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>, linux-xfs@vger.kernel.org
Subject: Re: [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper
Date: Mon, 16 Mar 2020 07:51:49 -0700 [thread overview]
Message-ID: <20200316145149.GA256767@magnolia> (raw)
In-Reply-To: <20200316131649.GE12313@bfoster>
On Mon, Mar 16, 2020 at 09:16:49AM -0400, Brian Foster wrote:
> On Thu, Mar 12, 2020 at 03:22:31PM +0100, Christoph Hellwig wrote:
> > Add a new wrapper to check if a file system supports the newer large
> > dinode format. Previously we uses xfs_sb_version_hascrc for that,
> > which is technically correct but a little confusing to read.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> > fs/xfs/libxfs/xfs_format.h | 5 +++++
> > fs/xfs/libxfs/xfs_ialloc.c | 4 ++--
> > fs/xfs/libxfs/xfs_inode_buf.c | 12 ++++++++----
> > fs/xfs/libxfs/xfs_trans_resv.c | 2 +-
> > fs/xfs/xfs_buf_item.c | 2 +-
> > fs/xfs/xfs_log_recover.c | 2 +-
> > 6 files changed, 18 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> > index cd814f99da28..a28bf6a978ad 100644
> > --- a/fs/xfs/libxfs/xfs_format.h
> > +++ b/fs/xfs/libxfs/xfs_format.h
> > @@ -497,6 +497,11 @@ static inline bool xfs_sb_version_hascrc(struct xfs_sb *sbp)
> > return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> > }
> >
>
> A comment would be useful here to indicate what this means (i.e., we can
> assume v3 inode format). The function name is a little vague too I
> suppose (will the inode ever get larger than large? :P). I wonder if
> something like _has_v3_inode() is more clear, but we can always change
> it easily enough and either way is better than hascrc() IMO.
>
> Brian
>
> > +static inline bool xfs_sb_version_has_large_dinode(struct xfs_sb *sbp)
/me agrees that this function ought to have a comment saying that
"large_dinode" means v3 inodes.
Not sure about larger than large inodes though -- the last di_flags2
could be made to mean "...and now expand structure by X bytes" whenever
we run out of space.
(The rest of the series looks ok to me)
--D
> > +{
> > + return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> > +}
> > +
> > static inline bool xfs_sb_version_has_pquotino(struct xfs_sb *sbp)
> > {
> > return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> > diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> > index b4a404278935..6adffaa68fb8 100644
> > --- a/fs/xfs/libxfs/xfs_ialloc.c
> > +++ b/fs/xfs/libxfs/xfs_ialloc.c
> > @@ -304,7 +304,7 @@ xfs_ialloc_inode_init(
> > * That means for v3 inode we log the entire buffer rather than just the
> > * inode cores.
> > */
> > - if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > + if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> > version = 3;
> > ino = XFS_AGINO_TO_INO(mp, agno, XFS_AGB_TO_AGINO(mp, agbno));
> >
> > @@ -2872,7 +2872,7 @@ xfs_ialloc_setup_geometry(
> > * cannot change the behavior.
> > */
> > igeo->inode_cluster_size_raw = XFS_INODE_BIG_CLUSTER_SIZE;
> > - if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > + if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> > int new_size = igeo->inode_cluster_size_raw;
> >
> > new_size *= mp->m_sb.sb_inodesize / XFS_DINODE_MIN_SIZE;
> > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> > index 17e88a8c8353..a5aa2f220c28 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.c
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> > @@ -44,14 +44,18 @@ xfs_inobp_check(
> > }
> > #endif
> >
> > +/*
> > + * v4 and earlier file systems only support the small dinode, and must use the
> > + * v1 or v2 inode formats. v5 file systems support a larger dinode, and must
> > + * use the v3 inode format.
> > + */
> > bool
> > xfs_dinode_good_version(
> > struct xfs_mount *mp,
> > __u8 version)
> > {
> > - if (xfs_sb_version_hascrc(&mp->m_sb))
> > + if (xfs_sb_version_has_large_dinode(&mp->m_sb))
> > return version == 3;
> > -
> > return version == 1 || version == 2;
> > }
> >
> > @@ -454,7 +458,7 @@ xfs_dinode_verify(
> >
> > /* Verify v3 integrity information first */
> > if (dip->di_version >= 3) {
> > - if (!xfs_sb_version_hascrc(&mp->m_sb))
> > + if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
> > return __this_address;
> > if (!xfs_verify_cksum((char *)dip, mp->m_sb.sb_inodesize,
> > XFS_DINODE_CRC_OFF))
> > @@ -629,7 +633,7 @@ xfs_iread(
> >
> > /* shortcut IO on inode allocation if possible */
> > if ((iget_flags & XFS_IGET_CREATE) &&
> > - xfs_sb_version_hascrc(&mp->m_sb) &&
> > + xfs_sb_version_has_large_dinode(&mp->m_sb) &&
> > !(mp->m_flags & XFS_MOUNT_IKEEP)) {
> > VFS_I(ip)->i_generation = prandom_u32();
> > ip->i_d.di_version = 3;
> > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > index 7a9c04920505..294e23d47912 100644
> > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > @@ -187,7 +187,7 @@ xfs_calc_inode_chunk_res(
> > XFS_FSB_TO_B(mp, 1));
> > if (alloc) {
> > /* icreate tx uses ordered buffers */
> > - if (xfs_sb_version_hascrc(&mp->m_sb))
> > + if (xfs_sb_version_has_large_dinode(&mp->m_sb))
> > return res;
> > size = XFS_FSB_TO_B(mp, 1);
> > }
> > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> > index 663810e6cd59..d004ae3455d7 100644
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -345,7 +345,7 @@ xfs_buf_item_format(
> > * occurs during recovery.
> > */
> > if (bip->bli_flags & XFS_BLI_INODE_BUF) {
> > - if (xfs_sb_version_hascrc(&lip->li_mountp->m_sb) ||
> > + if (xfs_sb_version_has_large_dinode(&lip->li_mountp->m_sb) ||
> > !((bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF) &&
> > xfs_log_item_in_current_chkpt(lip)))
> > bip->__bli_format.blf_flags |= XFS_BLF_INODE_BUF;
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 6abc0863c9c3..e5e976b5cc11 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -2997,7 +2997,7 @@ xlog_recover_inode_pass2(
> > * superblock flag to determine whether we need to look at di_flushiter
> > * to skip replay when the on disk inode is newer than the log one
> > */
> > - if (!xfs_sb_version_hascrc(&mp->m_sb) &&
> > + if (!xfs_sb_version_has_large_dinode(&mp->m_sb) &&
> > ldip->di_flushiter < be16_to_cpu(dip->di_flushiter)) {
> > /*
> > * Deal with the wrap case, DI_MAX_FLUSH is less
> > --
> > 2.24.1
> >
>
next prev parent reply other threads:[~2020-03-16 14:51 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-12 14:22 remove the di_version icdicone field v2 Christoph Hellwig
2020-03-12 14:22 ` [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper Christoph Hellwig
2020-03-16 13:16 ` Brian Foster
2020-03-16 14:47 ` Christoph Hellwig
2020-03-16 14:51 ` Darrick J. Wong [this message]
2020-03-16 14:46 ` Chandan Rajendra
2020-03-12 14:22 ` [PATCH 2/5] xfs: only check the superblock version for dinode size calculation Christoph Hellwig
2020-03-16 13:17 ` Brian Foster
2020-03-16 14:48 ` Christoph Hellwig
2020-03-16 14:47 ` Chandan Rajendra
2020-03-12 14:22 ` [PATCH 3/5] xfs: simplify a check in xfs_ioctl_setattr_check_cowextsize Christoph Hellwig
2020-03-16 13:17 ` Brian Foster
2020-03-16 14:48 ` Chandan Rajendra
2020-03-12 14:22 ` [PATCH 4/5] xfs: simplify di_flags2 inheritance in xfs_ialloc Christoph Hellwig
2020-03-16 13:17 ` Brian Foster
2020-03-16 14:48 ` Chandan Rajendra
2020-03-12 14:22 ` [PATCH 5/5] xfs: remove the di_version field from struct icdinode Christoph Hellwig
2020-03-16 13:17 ` Brian Foster
2020-03-16 14:49 ` Chandan Rajendra
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=20200316145149.GA256767@magnolia \
--to=darrick.wong@oracle.com \
--cc=bfoster@redhat.com \
--cc=hch@lst.de \
--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.