From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org, allison.henderson@oracle.com
Subject: Re: [PATCH 3/3] xfs: use XFS_IFORK_Q to determine the presence of an xattr fork
Date: Tue, 5 Jul 2022 15:59:14 -0700 [thread overview]
Message-ID: <YsTCQpAToP8NZuwX@magnolia> (raw)
In-Reply-To: <20220705224319.GF227878@dread.disaster.area>
On Wed, Jul 06, 2022 at 08:43:19AM +1000, Dave Chinner wrote:
> On Tue, Jul 05, 2022 at 03:09:51PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Modify xfs_ifork_ptr to return a NULL pointer if the caller asks for the
> > attribute fork but i_forkoff is zero. This eliminates the ambiguity
> > between i_forkoff and i_af.if_present, which should make it easier to
> > understand the lifetime of attr forks.
> >
> > While we're at it, remove the if_present checks around calls to
> > xfs_idestroy_fork and xfs_ifork_zap_attr since they can both handle attr
> > forks that have already been torn down.
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> > fs/xfs/libxfs/xfs_attr.c | 2 --
> > fs/xfs/libxfs/xfs_attr.h | 2 +-
> > fs/xfs/libxfs/xfs_bmap.c | 1 -
> > fs/xfs/libxfs/xfs_inode_buf.c | 1 -
> > fs/xfs/libxfs/xfs_inode_fork.c | 7 +------
> > fs/xfs/libxfs/xfs_inode_fork.h | 1 -
> > fs/xfs/xfs_attr_inactive.c | 11 ++++-------
> > fs/xfs/xfs_attr_list.c | 1 -
> > fs/xfs/xfs_icache.c | 8 +++-----
> > fs/xfs/xfs_inode.c | 5 ++---
> > fs/xfs/xfs_inode.h | 2 +-
> > 11 files changed, 12 insertions(+), 29 deletions(-)
> >
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 6b0d744e5947..002d7dea2190 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -69,8 +69,6 @@ xfs_inode_hasattr(
> > {
> > if (!XFS_IFORK_Q(ip))
> > return 0;
> > - if (!ip->i_af.if_present)
> > - return 0;
>
> I much prefer the "if (attr fork present)" style over the
> XFS_IFORK_Q() macro. I always have to go look up what all the whacky
> IFORK/DFORK macros actually mean, so if we are changing all this
> code can we wrap this in something like:
>
> static inline bool
> xfs_inode_has_attr_fork(struct xfs_inode *ip)
> {
> if (ip->i_forkoff)
> return true;
> return false;
> }
>
> And then xfs_inode_hasattr() becomes:
>
> if (!xfs_inode_has_attr_fork(ip)
> return false;
> if (ip->i_af.if_format == XFS_DINODE_FMT_EXTENTS &&
> ip->i_af.if_nextents == 0)
> return false;
> ....
Ok, done.
--D
> Cheers,
>
> Dave.
> > if (ip->i_af.if_format == XFS_DINODE_FMT_EXTENTS &&
> > ip->i_af.if_nextents == 0)
> > return 0;
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index 0f100db504ee..36371c3b9069 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -576,7 +576,7 @@ xfs_attr_init_add_state(struct xfs_da_args *args)
> > * context, i_af is guaranteed to exist. Hence if the attr fork is
> > * null, we were called from a pure remove operation and so we are done.
> > */
> > - if (!args->dp->i_af.if_present)
> > + if (!XFS_IFORK_Q(args->dp))
> > return XFS_DAS_DONE;
> >
> > args->op_flags |= XFS_DA_OP_ADDNAME;
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 4abb5b4cee0c..77ab5ac1fcbc 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -1041,7 +1041,6 @@ xfs_bmap_add_attrfork(
> > error = xfs_bmap_set_attrforkoff(ip, size, &version);
> > if (error)
> > goto trans_cancel;
> > - ASSERT(!ip->i_af.if_present);
> >
> > xfs_ifork_init_attr(ip, XFS_DINODE_FMT_EXTENTS, 0);
> > logflags = 0;
> > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> > index e203c80c1bf8..2aef27b042fe 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.c
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> > @@ -178,7 +178,6 @@ xfs_inode_from_disk(
> > xfs_failaddr_t fa;
> >
> > ASSERT(ip->i_cowfp == NULL);
> > - ASSERT(!ip->i_af.if_present);
> >
> > fa = xfs_dinode_verify(ip->i_mount, ip->i_ino, from);
> > if (fa) {
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > index 9d5141c21eee..b0370b837166 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > @@ -282,9 +282,6 @@ xfs_ifork_init_attr(
> > enum xfs_dinode_fmt format,
> > xfs_extnum_t nextents)
> > {
> > - ASSERT(!ip->i_af.if_present);
> > -
> > - ip->i_af.if_present = 1;
> > ip->i_af.if_format = format;
> > ip->i_af.if_nextents = nextents;
> > }
> > @@ -293,7 +290,6 @@ void
> > xfs_ifork_zap_attr(
> > struct xfs_inode *ip)
> > {
> > - ASSERT(ip->i_af.if_present);
> > ASSERT(ip->i_af.if_broot == NULL);
> > ASSERT(ip->i_af.if_u1.if_data == NULL);
> > ASSERT(ip->i_af.if_height == 0);
> > @@ -683,7 +679,6 @@ xfs_ifork_init_cow(
> >
> > ip->i_cowfp = kmem_cache_zalloc(xfs_ifork_cache,
> > GFP_NOFS | __GFP_NOFAIL);
> > - ip->i_cowfp->if_present = 1;
> > ip->i_cowfp->if_format = XFS_DINODE_FMT_EXTENTS;
> > }
> >
> > @@ -722,7 +717,7 @@ xfs_ifork_verify_local_attr(
> > struct xfs_ifork *ifp = &ip->i_af;
> > xfs_failaddr_t fa;
> >
> > - if (!ifp->if_present)
> > + if (!XFS_IFORK_Q(ip))
> > fa = __this_address;
> > else
> > fa = xfs_attr_shortform_verify(ip);
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index 63015e9cee14..0b912bbe4f4b 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -24,7 +24,6 @@ struct xfs_ifork {
> > xfs_extnum_t if_nextents; /* # of extents in this fork */
> > short if_broot_bytes; /* bytes allocated for root */
> > int8_t if_format; /* format of this fork */
> > - int8_t if_present; /* 1 if present */
> > };
> >
> > /*
> > diff --git a/fs/xfs/xfs_attr_inactive.c b/fs/xfs/xfs_attr_inactive.c
> > index dbe715fe92ce..ec20ad7a9001 100644
> > --- a/fs/xfs/xfs_attr_inactive.c
> > +++ b/fs/xfs/xfs_attr_inactive.c
> > @@ -362,12 +362,11 @@ xfs_attr_inactive(
> >
> > /*
> > * Invalidate and truncate the attribute fork extents. Make sure the
> > - * fork actually has attributes as otherwise the invalidation has no
> > + * fork actually has xattr blocks as otherwise the invalidation has no
> > * blocks to read and returns an error. In this case, just do the fork
> > * removal below.
> > */
> > - if (xfs_inode_hasattr(dp) &&
> > - dp->i_af.if_format != XFS_DINODE_FMT_LOCAL) {
> > + if (dp->i_af.if_nextents > 0) {
> > error = xfs_attr3_root_inactive(&trans, dp);
> > if (error)
> > goto out_cancel;
> > @@ -388,10 +387,8 @@ xfs_attr_inactive(
> > xfs_trans_cancel(trans);
> > out_destroy_fork:
> > /* kill the in-core attr fork before we drop the inode lock */
> > - if (dp->i_af.if_present) {
> > - xfs_idestroy_fork(&dp->i_af);
> > - xfs_ifork_zap_attr(dp);
> > - }
> > + xfs_idestroy_fork(&dp->i_af);
> > + xfs_ifork_zap_attr(dp);
> > if (lock_mode)
> > xfs_iunlock(dp, lock_mode);
> > return error;
> > diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> > index 6a832ee07916..99bbbe1a0e44 100644
> > --- a/fs/xfs/xfs_attr_list.c
> > +++ b/fs/xfs/xfs_attr_list.c
> > @@ -61,7 +61,6 @@ xfs_attr_shortform_list(
> > int sbsize, nsbuf, count, i;
> > int error = 0;
> >
> > - ASSERT(dp->i_af.if_present);
> > sf = (struct xfs_attr_shortform *)dp->i_af.if_u1.if_data;
> > ASSERT(sf != NULL);
> > if (!sf->hdr.count)
> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index e08dedfb7f9d..026c63234f8d 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -102,7 +102,6 @@ xfs_inode_alloc(
> > memset(&ip->i_af, 0, sizeof(ip->i_af));
> > ip->i_af.if_format = XFS_DINODE_FMT_EXTENTS;
> > memset(&ip->i_df, 0, sizeof(ip->i_df));
> > - ip->i_df.if_present = 1;
> > ip->i_flags = 0;
> > ip->i_delayed_blks = 0;
> > ip->i_diflags2 = mp->m_ino_geo.new_diflags2;
> > @@ -132,10 +131,9 @@ xfs_inode_free_callback(
> > break;
> > }
> >
> > - if (ip->i_af.if_present) {
> > - xfs_idestroy_fork(&ip->i_af);
> > - xfs_ifork_zap_attr(ip);
> > - }
> > + xfs_idestroy_fork(&ip->i_af);
> > + xfs_ifork_zap_attr(ip);
> > +
> > if (ip->i_cowfp) {
> > xfs_idestroy_fork(ip->i_cowfp);
> > kmem_cache_free(xfs_ifork_cache, ip->i_cowfp);
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index c9c26d782a38..2f559e16a4db 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -125,7 +125,7 @@ xfs_ilock_attr_map_shared(
> > {
> > uint lock_mode = XFS_ILOCK_SHARED;
> >
> > - if (ip->i_af.if_present && xfs_need_iread_extents(&ip->i_af))
> > + if (XFS_IFORK_Q(ip) && xfs_need_iread_extents(&ip->i_af))
> > lock_mode = XFS_ILOCK_EXCL;
> > xfs_ilock(ip, lock_mode);
> > return lock_mode;
> > @@ -1768,7 +1768,6 @@ xfs_inactive(
> > goto out;
> > }
> >
> > - ASSERT(!ip->i_af.if_present);
> > ASSERT(ip->i_forkoff == 0);
> >
> > /*
> > @@ -3488,7 +3487,7 @@ xfs_iflush(
> > if (ip->i_df.if_format == XFS_DINODE_FMT_LOCAL &&
> > xfs_ifork_verify_local_data(ip))
> > goto flush_out;
> > - if (ip->i_af.if_present &&
> > + if (XFS_IFORK_Q(ip) &&
> > ip->i_af.if_format == XFS_DINODE_FMT_LOCAL &&
> > xfs_ifork_verify_local_attr(ip))
> > goto flush_out;
> > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> > index 045bad62ac25..9bda01311c2f 100644
> > --- a/fs/xfs/xfs_inode.h
> > +++ b/fs/xfs/xfs_inode.h
> > @@ -86,7 +86,7 @@ xfs_ifork_ptr(
> > case XFS_DATA_FORK:
> > return &ip->i_df;
> > case XFS_ATTR_FORK:
> > - if (!ip->i_af.if_present)
> > + if (!XFS_IFORK_Q(ip))
> > return NULL;
> > return &ip->i_af;
> > case XFS_COW_FORK:
> >
> >
>
> --
> Dave Chinner
> david@fromorbit.com
next prev parent reply other threads:[~2022-07-05 22:59 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-05 22:09 [PATCHSET 0/3] xfs: make attr forks permanent Darrick J. Wong
2022-07-05 22:09 ` [PATCH 1/3] xfs: convert XFS_IFORK_PTR to a static inline helper Darrick J. Wong
2022-07-05 22:44 ` Dave Chinner
2022-07-05 22:09 ` [PATCH 2/3] xfs: make inode attribute forks a permanent part of struct xfs_inode Darrick J. Wong
2022-07-08 4:30 ` Dave Chinner
2022-07-05 22:09 ` [PATCH 3/3] xfs: use XFS_IFORK_Q to determine the presence of an xattr fork Darrick J. Wong
2022-07-05 22:43 ` Dave Chinner
2022-07-05 22:59 ` Darrick J. Wong [this message]
2022-07-05 23:18 ` [PATCH 4/3] xfs: replace XFS_IFORK_Q with a proper predicate function Darrick J. Wong
2022-07-06 0:18 ` Dave Chinner
2022-07-05 23:19 ` [PATCH 5/3] xfs: replace inode fork size macros with functions Darrick J. Wong
2022-07-06 0:25 ` Dave Chinner
2022-07-06 1:09 ` Darrick J. Wong
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=YsTCQpAToP8NZuwX@magnolia \
--to=djwong@kernel.org \
--cc=allison.henderson@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.