From: Allison Collins <allison.henderson@oracle.com>
To: Brian Foster <bfoster@redhat.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v5 13/14] xfs: Add delay ready attr remove routines
Date: Sat, 14 Dec 2019 12:21:03 -0700 [thread overview]
Message-ID: <27f6d1f2-8d7e-c759-31e1-6c4ac8c7ccad@oracle.com> (raw)
In-Reply-To: <20191213173046.GG43376@bfoster>
On 12/13/19 10:30 AM, Brian Foster wrote:
> On Wed, Dec 11, 2019 at 09:15:12PM -0700, Allison Collins wrote:
>> This patch modifies the attr remove 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_remove_args has become
>> xfs_attr_remove_iter, which uses a sort of state machine like switch
>> to keep track of where it was when EAGAIN was returned.
>> xfs_attr_node_removename has also been modified to use the switch,
>> and a new version of xfs_attr_remove_args consists of a simple loop
>> to refresh the transaction until the operation is completed.
>>
>> This patch also adds a new struct xfs_delattr_context, which we will
>> use to keep track of the current state of an attribute operation.
>> The new xfs_delattr_state enum is used to track various operations
>> that are in progress so that we know not to repeat them, and resume
>> where we left off before EAGAIN was returned to cycle out the
>> transaction. Other members take the place of local variables that
>> need to retain their values across multiple function recalls.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_attr.c | 127 ++++++++++++++++++++++++++++++++++++-------
>> fs/xfs/libxfs/xfs_attr.h | 1 +
>> fs/xfs/libxfs/xfs_da_btree.h | 16 ++++++
>> fs/xfs/scrub/common.c | 2 +
>> fs/xfs/xfs_acl.c | 2 +
>> fs/xfs/xfs_attr_list.c | 1 +
>> fs/xfs/xfs_ioctl.c | 2 +
>> fs/xfs/xfs_ioctl32.c | 2 +
>> fs/xfs/xfs_iops.c | 2 +
>> fs/xfs/xfs_xattr.c | 1 +
>> 10 files changed, 137 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index b5a5c84..726b75e 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
> ...
>> @@ -1206,12 +1249,29 @@ xfs_attr_node_removename(
>> struct xfs_buf *bp;
>> int retval, error, forkoff;
>> struct xfs_inode *dp = args->dp;
>> + int done = 0;
>>
>> trace_xfs_attr_node_removename(args);
>> + state = args->dac.da_state;
>> + blk = args->dac.blk;
>> +
>> + /* State machine switch */
>> + switch (args->dac.dela_state) {
>> + case XFS_DAS_RM_NODE_BLKS:
>> + goto rm_node_blks;
>> + case XFS_DAS_RM_INVALIDATE:
>> + goto rm_invalidate;
>> + case XFS_DAS_RM_SHRINK:
>> + goto rm_shrink;
>> + default:
>> + break;
>> + }
>
> I think this function could use at least a couple more prepatory
> refactoring patches before we introduce the state machine...
>
>>
>> error = xfs_attr_node_hasname(args, &state);
>> if (error != -EEXIST)
>> goto out;
>> + else
>> + error = 0;
>>
>> /*
>> * If there is an out-of-line value, de-allocate the blocks.
>> @@ -1221,6 +1281,14 @@ xfs_attr_node_removename(
>> blk = &state->path.blk[ state->path.active-1 ];
>> ASSERT(blk->bp != NULL);
>> ASSERT(blk->magic == XFS_ATTR_LEAF_MAGIC);
>> +
>> + /*
>> + * Store blk and state in the context incase we need to cycle out the
>> + * transaction
>> + */
>> + args->dac.blk = blk;
>> + args->dac.da_state = state;
>> +
>> if (args->rmtblkno > 0) {
>> /*
>> * Fill in disk block numbers in the state structure
>> @@ -1239,13 +1307,40 @@ xfs_attr_node_removename(
>> if (error)
>> goto out;
>>
>> - error = xfs_trans_roll_inode(&args->trans, args->dp);
>> - if (error)
>> - goto out;
>> + args->dac.dela_state = XFS_DAS_RM_INVALIDATE;
>> + return -EAGAIN;
>> + }
>
> The entire (args->rmtblkno > 0) branch above could be reduced into a
> helper function. BTW, does it matter whether the invalidate occurs
> before or after this particular transaction roll? It looks to me it just
> makes in-core changes. I'm wondering if we could just fold that in as
> well and eliminate that state entirely.
I assumed the reason for the roll here was the invalidate? But I
suppose if we only have in-core changes maybe it's not needed. I'll see
if I can remove this state, and tuck the above logic into a helper.
>
>> +
>> +rm_invalidate:
>> + args->dac.dela_state = XFS_DAS_RM_INVALIDATE;
>>
>> - error = xfs_attr_rmtval_remove(args);
>> + if (args->rmtblkno > 0) {
>> + error = xfs_attr_rmtval_invalidate(args);
>> if (error)
>> goto out;
>> + }
>> +
>> +rm_node_blks:
>> +
>> + args->dac.dela_state = XFS_DAS_RM_NODE_BLKS;
>> + if (args->rmtblkno > 0) {
>> + /*
>> + * Unmap value blocks for this attr. This is similar to
>> + * xfs_attr_rmtval_remove, but open coded here to return EAGAIN
>> + * for new transactions
>> + */
>> + while (!done && !error) {
>> + error = xfs_bunmapi(args->trans, args->dp,
>> + args->rmtblkno, args->rmtblkcnt,
>> + XFS_BMAPI_ATTRFORK, 1, &done);
>> + if (error)
>> + return error;
>> +
>> + if (!done) {
>> + args->dac.dela_state = XFS_DAS_RM_NODE_BLKS;
>> + return -EAGAIN;
>> + }
>> + }
>>
>
> The above could use the helper function treatment as well. E.g.,
> something like xfs_attr_rmtval_unmap() that has a *done param this
> function can check to determine whether to return -EAGAIN or proceed.
Sure, will do
>
>> /*
>> * Refill the state structure with buffers, the prior calls
>> @@ -1271,17 +1366,14 @@ xfs_attr_node_removename(
>> error = xfs_da3_join(state);
>> if (error)
>> goto out;
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - goto out;
>
> Hmm.. I think we might want to lift the xfs_defer_finish() call up into
> the iter() function rather than just drop it. Otherwise this changes
> behavior in that the transaction roll doesn't complete pending deferred
> operations.
Ok, I'll tack that on then
>
>> - /*
>> - * Commit the Btree join operation and start a new trans.
>> - */
>> - error = xfs_trans_roll_inode(&args->trans, dp);
>> - if (error)
>> - goto out;
>> +
>> + args->dac.dela_state = XFS_DAS_RM_SHRINK;
>> + return -EAGAIN;
>> }
>>
>> +rm_shrink:
>> + args->dac.dela_state = XFS_DAS_RM_SHRINK;
>> +
>> /*
>> * If the result is small enough, push it all into the inode.
>> */
>> @@ -1302,9 +1394,6 @@ xfs_attr_node_removename(
>> /* bp is gone due to xfs_da_shrink_inode */
>> if (error)
>> goto out;
>> - error = xfs_defer_finish(&args->trans);
>> - if (error)
>> - goto out;
>> } else
>> xfs_trans_brelse(args->trans, bp);
>> }
>
> Same deal here (and same fundamental comment for the next patch)..
> create a xfs_attr_node_shrink() or some such helper to make functions
> that handle state smaller and easier to follow once the state bits are
> introduced.
>
> Brian
Ok then, sounds good. Thanks again for all the reviews, I know it's
been a lot! I'll get all this stuff updated in the next version.
Allison
>
>> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
>> index 3b5dad4..f6ac571 100644
>> --- a/fs/xfs/libxfs/xfs_attr.h
>> +++ b/fs/xfs/libxfs/xfs_attr.h
>> @@ -152,6 +152,7 @@ int xfs_attr_set_args(struct xfs_da_args *args);
>> int xfs_attr_remove(struct xfs_inode *dp, struct xfs_name *name, int flags);
>> int xfs_has_attr(struct xfs_da_args *args);
>> int xfs_attr_remove_args(struct xfs_da_args *args);
>> +int xfs_attr_remove_iter(struct xfs_da_args *args);
>> int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
>> int flags, struct attrlist_cursor_kern *cursor);
>> bool xfs_attr_namecheck(const void *name, size_t length);
>> diff --git a/fs/xfs/libxfs/xfs_da_btree.h b/fs/xfs/libxfs/xfs_da_btree.h
>> index 580fb72..137ec29 100644
>> --- a/fs/xfs/libxfs/xfs_da_btree.h
>> +++ b/fs/xfs/libxfs/xfs_da_btree.h
>> @@ -49,10 +49,26 @@ enum xfs_dacmp {
>> XFS_CMP_CASE /* names are same but differ in case */
>> };
>>
>> +enum xfs_delattr_state {
>> + XFS_DAS_RM_INVALIDATE = 1, /* We are invalidating blocks */
>> + XFS_DAS_RM_SHRINK = 2, /* We are shrinking the tree */
>> + XFS_DAS_RM_NODE_BLKS = 3,/* We are removing node blocks */
>> +};
>> +
>> +/*
>> + * Context used for keeping track of delayed attribute operations
>> + */
>> +struct xfs_delattr_context {
>> + struct xfs_da_state *da_state;
>> + struct xfs_da_state_blk *blk;
>> + enum xfs_delattr_state dela_state;
>> +};
>> +
>> /*
>> * Structure to ease passing around component names.
>> */
>> typedef struct xfs_da_args {
>> + struct xfs_delattr_context dac;/* context used for delay attr ops */
>> struct xfs_da_geometry *geo; /* da block geometry */
>> struct xfs_name name; /* name, length and argument flags*/
>> uint8_t filetype; /* filetype of inode for directories */
>> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
>> index 1887605..9a649d1 100644
>> --- a/fs/xfs/scrub/common.c
>> +++ b/fs/xfs/scrub/common.c
>> @@ -24,6 +24,8 @@
>> #include "xfs_rmap_btree.h"
>> #include "xfs_log.h"
>> #include "xfs_trans_priv.h"
>> +#include "xfs_da_format.h"
>> +#include "xfs_da_btree.h"
>> #include "xfs_attr.h"
>> #include "xfs_reflink.h"
>> #include "scrub/scrub.h"
>> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
>> index 7b0e5b7..573e47e 100644
>> --- a/fs/xfs/xfs_acl.c
>> +++ b/fs/xfs/xfs_acl.c
>> @@ -10,6 +10,8 @@
>> #include "xfs_trans_resv.h"
>> #include "xfs_mount.h"
>> #include "xfs_inode.h"
>> +#include "xfs_da_format.h"
>> +#include "xfs_da_btree.h"
>> #include "xfs_attr.h"
>> #include "xfs_trace.h"
>> #include "xfs_error.h"
>> diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
>> index d37743b..881b9a4 100644
>> --- a/fs/xfs/xfs_attr_list.c
>> +++ b/fs/xfs/xfs_attr_list.c
>> @@ -12,6 +12,7 @@
>> #include "xfs_trans_resv.h"
>> #include "xfs_mount.h"
>> #include "xfs_da_format.h"
>> +#include "xfs_da_btree.h"
>> #include "xfs_inode.h"
>> #include "xfs_trans.h"
>> #include "xfs_bmap.h"
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index 4fc8698..a31753f 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -15,6 +15,8 @@
>> #include "xfs_iwalk.h"
>> #include "xfs_itable.h"
>> #include "xfs_error.h"
>> +#include "xfs_da_format.h"
>> +#include "xfs_da_btree.h"
>> #include "xfs_attr.h"
>> #include "xfs_bmap.h"
>> #include "xfs_bmap_util.h"
>> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
>> index c4c4f09..4b693e3 100644
>> --- a/fs/xfs/xfs_ioctl32.c
>> +++ b/fs/xfs/xfs_ioctl32.c
>> @@ -17,6 +17,8 @@
>> #include "xfs_itable.h"
>> #include "xfs_fsops.h"
>> #include "xfs_rtalloc.h"
>> +#include "xfs_da_format.h"
>> +#include "xfs_da_btree.h"
>> #include "xfs_attr.h"
>> #include "xfs_ioctl.h"
>> #include "xfs_ioctl32.h"
>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>> index e85bbf5..a2d299f 100644
>> --- a/fs/xfs/xfs_iops.c
>> +++ b/fs/xfs/xfs_iops.c
>> @@ -13,6 +13,8 @@
>> #include "xfs_inode.h"
>> #include "xfs_acl.h"
>> #include "xfs_quota.h"
>> +#include "xfs_da_format.h"
>> +#include "xfs_da_btree.h"
>> #include "xfs_attr.h"
>> #include "xfs_trans.h"
>> #include "xfs_trace.h"
>> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
>> index 5623682..8bdb972 100644
>> --- a/fs/xfs/xfs_xattr.c
>> +++ b/fs/xfs/xfs_xattr.c
>> @@ -10,6 +10,7 @@
>> #include "xfs_log_format.h"
>> #include "xfs_da_format.h"
>> #include "xfs_inode.h"
>> +#include "xfs_da_btree.h"
>> #include "xfs_attr.h"
>> #include "xfs_acl.h"
>>
>> --
>> 2.7.4
>>
>
next prev parent reply other threads:[~2019-12-14 19:23 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-12 4:14 [PATCH v5 00/14] xfs: Delay Ready Attributes Allison Collins
2019-12-12 4:15 ` [PATCH v5 01/14] xfs: Remove all strlen in all xfs_attr_* functions for attr names Allison Collins
2019-12-12 4:15 ` [PATCH v5 02/14] xfs: Replace attribute parameters with struct xfs_name Allison Collins
2019-12-13 13:07 ` Brian Foster
2019-12-14 6:57 ` Allison Collins
2019-12-12 4:15 ` [PATCH v5 03/14] xfs: Embed struct xfs_name in xfs_da_args Allison Collins
2019-12-12 4:15 ` [PATCH v5 04/14] xfs: Add xfs_has_attr and subroutines Allison Collins
2019-12-13 13:08 ` Brian Foster
2019-12-14 6:58 ` Allison Collins
2019-12-24 12:18 ` Christoph Hellwig
2019-12-25 4:21 ` Allison Collins
2020-01-21 22:30 ` Darrick J. Wong
2020-01-22 0:25 ` Allison Collins
2020-01-25 16:27 ` Allison Collins
2020-01-25 23:08 ` Christoph Hellwig
2020-01-26 4:03 ` Allison Collins
2019-12-12 4:15 ` [PATCH v5 05/14] xfs: Factor out new helper functions xfs_attr_rmtval_set Allison Collins
2019-12-24 12:14 ` Christoph Hellwig
2019-12-25 17:43 ` Allison Collins
2020-01-06 14:46 ` Brian Foster
2020-01-06 18:29 ` Allison Collins
2020-01-06 21:45 ` Darrick J. Wong
2020-01-06 23:33 ` Dave Chinner
2019-12-12 4:15 ` [PATCH v5 06/14] xfs: Factor up trans handling in xfs_attr3_leaf_flipflags Allison Collins
2019-12-24 12:16 ` Christoph Hellwig
2019-12-25 20:49 ` Allison Collins
2019-12-12 4:15 ` [PATCH v5 07/14] xfs: Factor out xfs_attr_leaf_addname helper Allison Collins
2019-12-13 14:15 ` Brian Foster
2019-12-14 6:58 ` Allison Collins
2019-12-24 12:22 ` Christoph Hellwig
2019-12-26 17:04 ` Allison Collins
2019-12-12 4:15 ` [PATCH v5 08/14] xfs: Factor up xfs_attr_try_sf_addname Allison Collins
2019-12-13 14:15 ` Brian Foster
2019-12-14 6:58 ` Allison Collins
2019-12-24 12:25 ` Christoph Hellwig
2019-12-27 3:23 ` Allison Collins
2019-12-12 4:15 ` [PATCH v5 09/14] xfs: Factor up trans roll from xfs_attr3_leaf_setflag Allison Collins
2019-12-24 12:26 ` Christoph Hellwig
2019-12-12 4:15 ` [PATCH v5 10/14] xfs: Factor out xfs_attr_rmtval_invalidate Allison Collins
2019-12-12 4:15 ` [PATCH v5 11/14] xfs: Factor up trans roll in xfs_attr3_leaf_clearflag Allison Collins
2019-12-24 12:28 ` Christoph Hellwig
2019-12-12 4:15 ` [PATCH v5 12/14] xfs: Check for -ENOATTR or -EEXIST Allison Collins
2019-12-13 14:16 ` Brian Foster
2019-12-14 7:56 ` Allison Collins
2019-12-12 4:15 ` [PATCH v5 13/14] xfs: Add delay ready attr remove routines Allison Collins
2019-12-13 17:30 ` Brian Foster
2019-12-14 19:21 ` Allison Collins [this message]
2019-12-24 12:30 ` Christoph Hellwig
2019-12-24 23:18 ` Allison Collins
2019-12-12 4:15 ` [PATCH v5 14/14] xfs: Add delay ready attr set routines Allison Collins
2019-12-24 12:02 ` [PATCH v5 00/14] xfs: Delay Ready Attributes Christoph Hellwig
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=27f6d1f2-8d7e-c759-31e1-6c4ac8c7ccad@oracle.com \
--to=allison.henderson@oracle.com \
--cc=bfoster@redhat.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;
as well as URLs for NNTP newsgroup(s).