All of lore.kernel.org
 help / color / mirror / Atom feed
From: Brian Foster <bfoster@redhat.com>
To: Allison Henderson <allison.henderson@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v16 08/11] xfs: Hoist xfs_attr_leaf_addname
Date: Mon, 5 Apr 2021 09:15:29 -0400	[thread overview]
Message-ID: <YGsNcQ0fhh4YWMA1@bfoster> (raw)
In-Reply-To: <f227aed8-1ef5-61fe-64c1-0173936dc1c8@oracle.com>

On Fri, Apr 02, 2021 at 02:01:08AM -0700, Allison Henderson wrote:
> 
> 
> On 4/1/21 8:43 AM, Brian Foster wrote:
> > On Thu, Mar 25, 2021 at 05:33:05PM -0700, Allison Henderson wrote:
> > > This patch hoists xfs_attr_leaf_addname into the calling function.  The
> > > goal being to get all the code that will require state management into
> > > the same scope. This isn't particuarly aesthetic right away, but it is a
> > > preliminary step to merging in the state machine code.
> > > 
> > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >   fs/xfs/libxfs/xfs_attr.c | 209 ++++++++++++++++++++++-------------------------
> > >   1 file changed, 96 insertions(+), 113 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > index 5b5410f..16f10ac 100644
> > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > ...
> > > @@ -729,115 +821,6 @@ xfs_attr_leaf_try_add(
> > >   	return retval;
> > >   }
> > > -
> > > -/*
> > > - * Add a name to the leaf attribute list structure
> > > - *
> > > - * This leaf block cannot have a "remote" value, we only call this routine
> > > - * if bmap_one_block() says there is only one block (ie: no remote blks).
> > > - */
> > > -STATIC int
> > > -xfs_attr_leaf_addname(
> > > -	struct xfs_da_args	*args)
> > > -{
> > > -	int			error, forkoff;
> > > -	struct xfs_buf		*bp = NULL;
> > > -	struct xfs_inode	*dp = args->dp;
> > > -
> > > -	trace_xfs_attr_leaf_addname(args);
> > > -
> > 
> > Did this tracepoint disappear for a reason?
> I thought the trace made sense to mark the entry of this function, but then
> when hoisted, looked sort of out of place.  It certainly wouldn't hurt it to
> put it back if people prefer.  I don't see it used anywhere else, and I
> don't think the calling function has it's own trace scheme either?  Should I
> translate trace_xfs_attr_leaf_addname to
> trace_xfs_attr_set_args and hoist it up?
> 

Ok. I have no strong preference, I just noticed it missing. FWIW, it
appears to be unused as of the end of the series so this patch should
probably just drop it from xfs_trace.h if the removal was intentional.

Brian

> Allison
> 
> > 
> > Brian
> > 
> > > -	error = xfs_attr_leaf_try_add(args, bp);
> > > -	if (error)
> > > -		return error;
> > > -
> > > -	/*
> > > -	 * Commit the transaction that added the attr name so that
> > > -	 * later routines can manage their own transactions.
> > > -	 */
> > > -	error = xfs_trans_roll_inode(&args->trans, dp);
> > > -	if (error)
> > > -		return error;
> > > -
> > > -	/*
> > > -	 * If there was an out-of-line value, allocate the blocks we
> > > -	 * identified for its storage and copy the value.  This is done
> > > -	 * after we create the attribute so that we don't overflow the
> > > -	 * maximum size of a transaction and/or hit a deadlock.
> > > -	 */
> > > -	if (args->rmtblkno > 0) {
> > > -		error = xfs_attr_rmtval_set(args);
> > > -		if (error)
> > > -			return error;
> > > -	}
> > > -
> > > -	if (!(args->op_flags & XFS_DA_OP_RENAME)) {
> > > -		/*
> > > -		 * Added a "remote" value, just clear the incomplete flag.
> > > -		 */
> > > -		if (args->rmtblkno > 0)
> > > -			error = xfs_attr3_leaf_clearflag(args);
> > > -
> > > -		return error;
> > > -	}
> > > -
> > > -	/*
> > > -	 * If this is an atomic rename operation, we must "flip" the incomplete
> > > -	 * flags on the "new" and "old" attribute/value pairs so that one
> > > -	 * disappears and one appears atomically.  Then we must remove the "old"
> > > -	 * attribute/value pair.
> > > -	 *
> > > -	 * In a separate transaction, set the incomplete flag on the "old" attr
> > > -	 * and clear the incomplete flag on the "new" attr.
> > > -	 */
> > > -
> > > -	error = xfs_attr3_leaf_flipflags(args);
> > > -	if (error)
> > > -		return error;
> > > -	/*
> > > -	 * Commit the flag value change and start the next trans in series.
> > > -	 */
> > > -	error = xfs_trans_roll_inode(&args->trans, args->dp);
> > > -	if (error)
> > > -		return error;
> > > -
> > > -	/*
> > > -	 * Dismantle the "old" attribute/value pair by removing a "remote" value
> > > -	 * (if it exists).
> > > -	 */
> > > -	xfs_attr_restore_rmt_blk(args);
> > > -
> > > -	if (args->rmtblkno) {
> > > -		error = xfs_attr_rmtval_invalidate(args);
> > > -		if (error)
> > > -			return error;
> > > -
> > > -		error = xfs_attr_rmtval_remove(args);
> > > -		if (error)
> > > -			return error;
> > > -	}
> > > -
> > > -	/*
> > > -	 * Read in the block containing the "old" attr, then remove the "old"
> > > -	 * attr from that block (neat, huh!)
> > > -	 */
> > > -	error = xfs_attr3_leaf_read(args->trans, args->dp, args->blkno,
> > > -				   &bp);
> > > -	if (error)
> > > -		return error;
> > > -
> > > -	xfs_attr3_leaf_remove(bp, args);
> > > -
> > > -	/*
> > > -	 * If the result is small enough, shrink it all into the inode.
> > > -	 */
> > > -	forkoff = xfs_attr_shortform_allfit(bp, dp);
> > > -	if (forkoff)
> > > -		error = xfs_attr3_leaf_to_shortform(bp, args, forkoff);
> > > -		/* bp is gone due to xfs_da_shrink_inode */
> > > -
> > > -	return error;
> > > -}
> > > -
> > >   /*
> > >    * Return EEXIST if attr is found, or ENOATTR if not
> > >    */
> > > -- 
> > > 2.7.4
> > > 
> > 
> 


  reply	other threads:[~2021-04-05 13:15 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-26  0:32 [PATCH v16 00/11] xfs: Delay Ready Attributes Allison Henderson
2021-03-26  0:32 ` [PATCH v16 01/11] xfs: Reverse apply 72b97ea40d Allison Henderson
2021-03-29  8:44   ` Chandan Babu R
2021-04-02  9:47     ` Allison Henderson
2021-04-01 15:43   ` Brian Foster
2021-04-02  9:49     ` Allison Henderson
2021-03-26  0:32 ` [PATCH v16 02/11] xfs: Add xfs_attr_node_remove_cleanup Allison Henderson
2021-03-26  0:33 ` [PATCH v16 03/11] xfs: Hoist xfs_attr_set_shortform Allison Henderson
2021-03-29  9:21   ` Chandan Babu R
2021-04-02  9:47     ` Allison Henderson
2021-03-26  0:33 ` [PATCH v16 04/11] xfs: Add helper xfs_attr_set_fmt Allison Henderson
2021-03-29  9:37   ` Chandan Babu R
2021-04-02  9:47     ` Allison Henderson
2021-03-26  0:33 ` [PATCH v16 05/11] xfs: Separate xfs_attr_node_addname and xfs_attr_node_addname_clear_incomplete Allison Henderson
2021-03-29 14:42   ` Chandan Babu R
2021-04-02  9:00     ` Allison Henderson
2021-03-26  0:33 ` [PATCH v16 06/11] xfs: Add helper xfs_attr_node_addname_find_attr Allison Henderson
2021-04-02  4:05   ` Chandan Babu R
2021-04-02  9:50     ` Allison Henderson
2021-03-26  0:33 ` [PATCH v16 07/11] xfs: Hoist xfs_attr_node_addname Allison Henderson
2021-04-02  4:26   ` Chandan Babu R
2021-04-02  9:01     ` Allison Henderson
2021-03-26  0:33 ` [PATCH v16 08/11] xfs: Hoist xfs_attr_leaf_addname Allison Henderson
2021-04-01 15:43   ` Brian Foster
2021-04-02  9:01     ` Allison Henderson
2021-04-05 13:15       ` Brian Foster [this message]
2021-04-02  4:40   ` Chandan Babu R
2021-04-02  9:50     ` Allison Henderson
2021-03-26  0:33 ` [PATCH v16 09/11] xfs: Hoist node transaction handling Allison Henderson
2021-04-02  5:04   ` Chandan Babu R
2021-04-02  9:51     ` Allison Henderson
2021-03-26  0:33 ` [PATCH v16 10/11] xfs: Add delay ready attr remove routines Allison Henderson
2021-04-01 16:55   ` Brian Foster
2021-04-02  9:42     ` Allison Henderson
2021-04-05 13:15       ` Brian Foster
2021-04-02  7:59   ` Chandan Babu R
2021-04-02  9:45     ` Allison Henderson
2021-03-26  0:33 ` [PATCH v16 11/11] xfs: Add delay ready attr set routines Allison Henderson
2021-04-01 16:57   ` Brian Foster
2021-04-02  9:01     ` Allison Henderson
2021-04-05 13:17       ` Brian Foster
2021-04-14 18:14         ` Allison Henderson
2021-03-29 21:59 ` [PATCH v16 00/11] xfs: Delay Ready Attributes Darrick J. Wong
2021-04-02  9:49   ` Allison Henderson

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=YGsNcQ0fhh4YWMA1@bfoster \
    --to=bfoster@redhat.com \
    --cc=allison.henderson@oracle.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.