From: Chandan Babu R <chandanrlinux@gmail.com>
To: Allison Henderson <allison.henderson@oracle.com>
Cc: "Darrick J. Wong" <darrick.wong@oracle.com>, linux-xfs@vger.kernel.org
Subject: Re: [PATCH v13 03/10] xfs: Add delay ready attr set routines
Date: Fri, 13 Nov 2020 14:46:05 +0530 [thread overview]
Message-ID: <20322189.3UhRF6KiVz@garuda> (raw)
In-Reply-To: <f386c199-3bb8-f6b7-6ef7-1e08d2b35b8a@oracle.com>
On Friday 13 November 2020 7:03:13 AM IST Allison Henderson wrote:
>
> On 11/10/20 2:57 PM, Darrick J. Wong wrote:
> > On Tue, Oct 27, 2020 at 07:02:55PM +0530, Chandan Babu R wrote:
> >> On Friday 23 October 2020 12:04:28 PM IST Allison Henderson wrote:
> >>> This patch modifies the attr set routines to be delay ready. This means
> >>> they no longer roll or commit transactions, but instead return -EAGAIN
> >>> to have the calling routine roll and refresh the transaction. In this
> >>> series, xfs_attr_set_args has become xfs_attr_set_iter, which uses a
> >>> state machine like switch to keep track of where it was when EAGAIN was
> >>> returned. See xfs_attr.h for a more detailed diagram of the states.
> >>>
> >>> Two new helper functions have been added: xfs_attr_rmtval_set_init and
> >>> xfs_attr_rmtval_set_blk. They provide a subset of logic similar to
> >>> xfs_attr_rmtval_set, but they store the current block in the delay attr
> >>> context to allow the caller to roll the transaction between allocations.
> >>> This helps to simplify and consolidate code used by
> >>> xfs_attr_leaf_addname and xfs_attr_node_addname. xfs_attr_set_args has
> >>> now become a simple loop to refresh the transaction until the operation
> >>> is completed. Lastly, xfs_attr_rmtval_remove is no longer used, and is
> >>> removed.
> >>
> >> One nit. xfs_attr_rmtval_remove()'s prototype declaration needs to be removed
> >> from xfs_attr_remote.h.
> Alrighty, will pull out
>
> >>
> >>>
> >>> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> >>> ---
> >>> fs/xfs/libxfs/xfs_attr.c | 370 ++++++++++++++++++++++++++--------------
> >>> fs/xfs/libxfs/xfs_attr.h | 126 +++++++++++++-
> >>> fs/xfs/libxfs/xfs_attr_remote.c | 99 +++++++----
> >>> fs/xfs/libxfs/xfs_attr_remote.h | 4 +
> >>> fs/xfs/xfs_trace.h | 1 -
> >>> 5 files changed, 439 insertions(+), 161 deletions(-)
> >>>
> >>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> >>> index 6ca94cb..95c98d7 100644
> >>> --- a/fs/xfs/libxfs/xfs_attr.c
> >>> +++ b/fs/xfs/libxfs/xfs_attr.c
> >>> @@ -44,7 +44,7 @@ STATIC int xfs_attr_shortform_addname(xfs_da_args_t *args);
> >>> * Internal routines when attribute list is one block.
> >>> */
> >>> STATIC int xfs_attr_leaf_get(xfs_da_args_t *args);
> >>> -STATIC int xfs_attr_leaf_addname(xfs_da_args_t *args);
> >>> +STATIC int xfs_attr_leaf_addname(struct xfs_delattr_context *dac);
> >>> STATIC int xfs_attr_leaf_removename(xfs_da_args_t *args);
> >>> STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
> >>>
> >>> @@ -52,12 +52,15 @@ STATIC int xfs_attr_leaf_hasname(struct xfs_da_args *args, struct xfs_buf **bp);
> >>> * Internal routines when attribute list is more than one block.
> >>> */
> >>> STATIC int xfs_attr_node_get(xfs_da_args_t *args);
> >>> -STATIC int xfs_attr_node_addname(xfs_da_args_t *args);
> >>> +STATIC int xfs_attr_node_addname(struct xfs_delattr_context *dac);
> >>> STATIC int xfs_attr_node_removename_iter(struct xfs_delattr_context *dac);
> >>> STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
> >>> struct xfs_da_state **state);
> >>> STATIC int xfs_attr_fillstate(xfs_da_state_t *state);
> >>> STATIC int xfs_attr_refillstate(xfs_da_state_t *state);
> >>> +STATIC int xfs_attr_leaf_try_add(struct xfs_da_args *args, struct xfs_buf *bp);
> >>> +STATIC int xfs_attr_set_iter(struct xfs_delattr_context *dac,
> >>> + struct xfs_buf **leaf_bp);
> >>>
> >>> int
> >>> xfs_inode_hasattr(
> >>> @@ -218,8 +221,11 @@ xfs_attr_is_shortform(
> >>>
> >>> /*
> >>> * Attempts to set an attr in shortform, or converts short form to leaf form if
> >>> - * there is not enough room. If the attr is set, the transaction is committed
> >>> - * and set to NULL.
> >>> + * there is not enough room. This function is meant to operate as a helper
> >>> + * routine to the delayed attribute functions. It returns -EAGAIN to indicate
> >>> + * that the calling function should roll the transaction, and then proceed to
> >>> + * add the attr in leaf form. This subroutine does not expect to be recalled
> >>> + * again like the other delayed attr routines do.
> >>> */
> >>> STATIC int
> >>> xfs_attr_set_shortform(
> >>> @@ -227,16 +233,16 @@ xfs_attr_set_shortform(
> >>> struct xfs_buf **leaf_bp)
> >>> {
> >>> struct xfs_inode *dp = args->dp;
> >>> - int error, error2 = 0;
> >>> + int error = 0;
> >>>
> >>> /*
> >>> * Try to add the attr to the attribute list in the inode.
> >>> */
> >>> error = xfs_attr_try_sf_addname(dp, args);
> >>> +
> >>> + /* Should only be 0, -EEXIST or ENOSPC */
> >>> if (error != -ENOSPC) {
> >>> - error2 = xfs_trans_commit(args->trans);
> >>> - args->trans = NULL;
> >>> - return error ? error : error2;
> >>> + return error;
> >>> }
> >>> /*
> >>> * It won't fit in the shortform, transform to a leaf block. GROT:
> >>> @@ -249,18 +255,10 @@ xfs_attr_set_shortform(
> >>> /*
> >>> * Prevent the leaf buffer from being unlocked so that a concurrent AIL
> >>> * push cannot grab the half-baked leaf buffer and run into problems
> >>> - * with the write verifier. Once we're done rolling the transaction we
> >>> - * can release the hold and add the attr to the leaf.
> >>> + * with the write verifier.
> >>> */
> >>> xfs_trans_bhold(args->trans, *leaf_bp);
> >>> - error = xfs_defer_finish(&args->trans);
> >>> - xfs_trans_bhold_release(args->trans, *leaf_bp);
> >>> - if (error) {
> >>> - xfs_trans_brelse(args->trans, *leaf_bp);
> >>> - return error;
> >>> - }
> >>> -
> >>> - return 0;
> >>> + return -EAGAIN;
> >>> }
> >>>
> >>> /*
> >>> @@ -268,7 +266,7 @@ xfs_attr_set_shortform(
> >>> * also checks for a defer finish. Transaction is finished and rolled as
> >>> * needed, and returns true of false if the delayed operation should continue.
> >>> */
> >>> -int
> >>> +STATIC int
> >>> xfs_attr_trans_roll(
> >>> struct xfs_delattr_context *dac)
> >>> {
> >>> @@ -297,61 +295,130 @@ int
> >>> xfs_attr_set_args(
> >>> struct xfs_da_args *args)
> >>> {
> >>> - struct xfs_inode *dp = args->dp;
> >>> - struct xfs_buf *leaf_bp = NULL;
> >>> - int error = 0;
> >>> + struct xfs_buf *leaf_bp = NULL;
> >>> + int error = 0;
> >>> + struct xfs_delattr_context dac = {
> >>> + .da_args = args,
> >>> + };
> >>> +
> >>> + do {
> >>> + error = xfs_attr_set_iter(&dac, &leaf_bp);
> >>> + if (error != -EAGAIN)
> >>> + break;
> >>> +
> >>> + error = xfs_attr_trans_roll(&dac);
> >>> + if (error)
> >>> + return error;
> >>> +
> >>> + if (leaf_bp) {
> >>> + xfs_trans_bjoin(args->trans, leaf_bp);
> >>> + xfs_trans_bhold(args->trans, leaf_bp);
> >>> + }
> >>
> >> When xfs_attr_set_iter() causes a "short form" attribute list to be converted
> >> to "leaf form", leaf_bp would point to an xfs_buf which has been added to the
> >> transaction and also XFS_BLI_HOLD flag is set on the buffer (last statement in
> >> xfs_attr_set_shortform()). XFS_BLI_HOLD flag makes sure that the new
> >> transaction allocated by xfs_attr_trans_roll() would continue to have leaf_bp
> >> in the transaction's item list. Hence I think the above calls to
> >> xfs_trans_bjoin() and xfs_trans_bhold() are not required.
> Sorry, I just noticed Chandans commentary for this patch. Apologies. I
> think we can get away with out this now, but yes this routine disappears
> at the end of the set now. Will clean out anyway for bisecting reasons
> though. :-)
No problem. As an aside, I stopped reviewing the patchset after I noticed
Brian's review comments for "[PATCH v13 02/10] xfs: Add delay ready attr
remove routines" suggesting some more code refactoring work.
>
> >
> > I /think/ the defer ops will rejoin the buffer each time it rolls, which
> > means that xfs_attr_trans_roll returns with the buffer already joined to
> > the transaction? And I think you're right that the bhold isn't needed,
> > because holding is dictated by the lower levels (i.e. _set_iter).
> >
> >> Please let me know if I am missing something obvious here.
> >
> > The entire function goes away by the end of the series. :)
> >
> > --D
> >
> >>
> >>
> >>
> >>
>
--
chandan
next prev parent reply other threads:[~2020-11-13 9:16 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-23 6:34 [PATCH v13 00/10] xfs: Delayed Attributes Allison Henderson
2020-10-23 6:34 ` [PATCH v13 01/10] xfs: Add helper xfs_attr_node_remove_step Allison Henderson
2020-10-27 7:03 ` Chandan Babu R
2020-10-27 22:23 ` Allison Henderson
2020-10-27 12:15 ` Brian Foster
2020-10-27 15:33 ` Allison Henderson
2020-11-10 23:12 ` Darrick J. Wong
2020-11-13 1:38 ` Allison Henderson
2020-10-23 6:34 ` [PATCH v13 02/10] xfs: Add delay ready attr remove routines Allison Henderson
2020-10-27 9:59 ` Chandan Babu R
2020-10-27 15:32 ` Allison Henderson
2020-10-28 12:04 ` Chandan Babu R
2020-10-29 1:29 ` Allison Henderson
2020-11-14 0:53 ` Darrick J. Wong
2020-10-27 12:16 ` Brian Foster
2020-10-27 22:27 ` Allison Henderson
2020-10-28 12:28 ` Brian Foster
2020-10-29 1:03 ` Allison Henderson
2020-11-10 23:15 ` Darrick J. Wong
2020-11-10 23:43 ` Darrick J. Wong
2020-11-11 0:28 ` Dave Chinner
2020-11-13 4:00 ` Allison Henderson
2020-11-13 3:43 ` Allison Henderson
2020-11-14 1:18 ` Darrick J. Wong
2020-11-16 5:12 ` Allison Henderson
2020-10-23 6:34 ` [PATCH v13 03/10] xfs: Add delay ready attr set routines Allison Henderson
2020-10-27 13:32 ` Chandan Babu R
2020-11-10 21:57 ` Darrick J. Wong
2020-11-13 1:33 ` Allison Henderson
2020-11-13 9:16 ` Chandan Babu R [this message]
2020-11-13 17:12 ` Allison Henderson
2020-11-14 1:20 ` Darrick J. Wong
2020-11-10 23:10 ` Darrick J. Wong
2020-11-13 1:38 ` Allison Henderson
2020-11-14 1:35 ` Darrick J. Wong
2020-11-16 5:25 ` Allison Henderson
2020-10-23 6:34 ` [PATCH v13 04/10] xfs: Rename __xfs_attr_rmtval_remove Allison Henderson
2020-10-23 6:34 ` [PATCH v13 05/10] xfs: Set up infastructure for deferred attribute operations Allison Henderson
2020-11-10 21:51 ` Darrick J. Wong
2020-11-11 3:44 ` Darrick J. Wong
2020-11-13 17:06 ` Allison Henderson
2020-11-13 1:32 ` Allison Henderson
2020-11-14 2:00 ` Darrick J. Wong
2020-11-16 7:41 ` Allison Henderson
2020-10-23 6:34 ` [PATCH v13 06/10] xfs: Add xfs_attr_set_deferred and xfs_attr_remove_deferred Allison Henderson
2020-11-10 20:15 ` Darrick J. Wong
2020-11-13 1:27 ` Allison Henderson
2020-11-14 2:03 ` Darrick J. Wong
2020-10-23 6:34 ` [PATCH v13 07/10] xfs: Add feature bit XFS_SB_FEAT_INCOMPAT_LOG_DELATTR Allison Henderson
2020-11-10 20:10 ` Darrick J. Wong
2020-11-13 1:27 ` Allison Henderson
2020-11-19 2:36 ` Darrick J. Wong
2020-11-19 4:01 ` Allison Henderson
2020-10-23 6:34 ` [PATCH v13 08/10] xfs: Enable delayed attributes Allison Henderson
2020-10-23 6:34 ` [PATCH v13 09/10] xfs: Remove unused xfs_attr_*_args Allison Henderson
2020-11-10 20:07 ` Darrick J. Wong
2020-11-13 1:27 ` Allison Henderson
2020-10-23 6:34 ` [PATCH v13 10/10] xfs: Add delayed attributes error tag 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=20322189.3UhRF6KiVz@garuda \
--to=chandanrlinux@gmail.com \
--cc=allison.henderson@oracle.com \
--cc=darrick.wong@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox