Re: [PATCH 7/9] xfs: Add attr context to log item

[Brian Foster <bfoster@redhat.com>]

On Wed, Apr 24, 2019 at 08:25:33AM -0700, Darrick J. Wong wrote:
> On Wed, Apr 24, 2019 at 08:17:48AM -0400, Brian Foster wrote:
> > On Tue, Apr 23, 2019 at 09:10:16PM -0700, Darrick J. Wong wrote:
> > > Sorry I'm late back to the party...
> > > 
> > > On Tue, Apr 23, 2019 at 07:24:40PM -0700, Allison Henderson wrote:
> > > > 
> > > > On 4/23/19 6:20 AM, Brian Foster wrote:
> > > > > On Mon, Apr 22, 2019 at 03:01:27PM -0700, Allison Henderson wrote:
> > > > > > 
> > > > > > 
> > > > > > On 4/22/19 6:03 AM, Brian Foster wrote:
> > > > > > > On Fri, Apr 12, 2019 at 03:50:34PM -0700, Allison Henderson wrote:
> > > > > > > > This patch modifies xfs_attr_item to store a xfs_da_args, a xfs_buf pointer
> > > > > > > > and a new state type. We will use these in the next patch when
> > > > > > > > we modify xfs_set_attr_args to roll transactions by returning EAGAIN.
> > > > > > > > Because the subroutines of this function modify the contents of these
> > > > > > > > structures, we need to find a place to store them where they remain
> > > > > > > > instantiated across multiple calls to xfs_set_attr_args.
> > > > > > > > 
> > > > > > > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> > > > > > > > ---
> > > > > > > 
> > > > > > > I see Darrick has already commented on the whole state thing. I'll
> > > > > > > probably have to grok the next patch to comment further, but just a
> > > > > > > couple initial thoughts:
> > > > > > > 
> > > > > > > First, I hit a build failure with this patch. It looks like there's a
> > > > > > > missed include in the scrub code:
> > > > > > > 
> > > > > > >     ...
> > > > > > >     CC [M]  fs/xfs/scrub/repair.o
> > > > > > > In file included from fs/xfs/scrub/repair.c:32:
> > > > > > > fs/xfs/libxfs/xfs_attr.h:105:21: error: field ‘xattri_args’ has incomplete type
> > > > > > >     struct xfs_da_args xattri_args;   /* args context */
> > > > > > Hmm, ok.  I'll get that corrected, I probably need to clean out my workspace
> > > > > > and build from scratch.
> > > > > > 
> > > > > > >     ...
> > > > > > > 
> > > > > > > Second, the commit log suggests that the states will reflect the current
> > > > > > > transaction roll points (i.e., establishing re-entry points down in
> > > > > > > xfs_attr_set_args(). I'm kind of wondering if we should break these
> > > > > > > xattr set sub-sequences down into smaller helper functions (refactoring
> > > > > > > the existing code as we go) such that the mechanism could technically be
> > > 
> > > I had had the thought of "why not just give each step of setting an
> > > attribute its own log item, so we don't have to have this STATE_NNN
> > > business?" but then realized that will generate an insane amount of
> > > boilerplate, and you're already close to a better solution, so I shut up
> > > to think harder. :)
> > > 
> > 
> > The thought of separating things down into smaller "ops" popped into my
> > head (not necessarily separate/smaller log items), but I hadn't really
> > thought it through to this point...
> > 
> > > > > > > used deferred or not. Re: the previous thought on whether to defer xattr
> > > > > > > removes or not, there might also be cases where there's not a need to
> > > > > > > defer xattr sets.
> > > > > > > 
> > > > > > > E.g., taking a quick peek into the next patch, the state 1 case in
> > > > > > > xfs_attr_try_sf_addname() is actually a transaction commit, which I
> > > > > > > think means we're done. We'd have done an attr memory allocation,
> > > > > > > deferred op and transaction roll where none was necessary so it might
> > > > > > > not be worth it to defer in that scenario. Hmm, it also looks like we
> > > > > > > return -EAGAIN in places where we've not actually done any work, like if
> > > > > > > a shortform add attempt returns -ENOSPC (or the -EAGAIN return before we
> > > > > > > even attempt the sf add). That kind of looks like a waste of transaction
> > > > > > > rolls and further suggests it might be cleaner to break this whole path
> > > > > > > down into helpers and put it back together in a way more conducive to
> > > > > > > deferred operations.
> > > 
> > > Er, agreed:
> > > 
> > > > > > Yes, this area is a bit of a wart the way it is right now.  I think you're
> > > > > > right in that ultimately we may end up having to do a lot of refactoring in
> > > > > > order to have more efficient "re-entry points".  The state machine is hard
> > > > > > to get into subroutines, so it's limited in use in the top level function.
> > > 
> > > So my current understanding of the problem is that we have this big old
> > > xfs_attr_set_args function that does multiple responsibilities requiring
> > > transaction rolls, which we can't do directly inside a ->finish_item
> > > handler:
> > > 
> > >  1. If no attr fork, add one.
> > >  2. If shortform attr fork, try to put it in the sf area.
> > >  3. If shortform attr fork and out of space, convert to leaf format.
> > >  4. Add attr to leaf/node attr tree.
> > > 
> > 
> > And there are a bunch of tx rolls down in the #4 codepath that this
> > series currently just tosses away. I'm not quite sure how appropriate
> > that is, but I also don't think we necessarily need to preserve each and
> > every transaction roll as implemented by the current code.
> > 
> > IOW, I think it absolutely makes sense to step back from the current
> > behavior and reassess the best/required places to roll xattr ops in
> > progress as well as the transaction reservation itself.
> 
> Yes, it would help to make a list of every small step that could
> possibly be required to set an attribute.  That will help narrow down
> how many defer op pieces are needed.
> 
> Another thought I had is that having the finish_item continually logging
> a new intent with the latest state means that we can free the old intent
> item, which helps us avoid the problem of pinning the log tail at that
> first intent item while we scramble around doing a whole lot of rolling
> and other work to get to the done item.
> 
> > > So how about this: refactor each of these pieces into a separate
> > > function, then add a separate XFS_ATTR_OP_FLAGS_* value for each of
> > > these little pieces.  xfs_trans_attr() can call the appropriate little
> > > function for the OP_FLAG and xfs_attr_finish_item can figure out which
> > > state comes next based on the return value.
> > > 
> > > By directly mapping distinct OP_FLAGS to each piece of the attr setting
> > > puzzle, you can use the existing "roll and come back" part of the defer
> > > ops machinery.
> > > 
> > > If _finish_item thinks we're done then we just exit.  Otherwise, store
> > > the new state in the (struct xfs_attr_item *) parameter passed into
> > > _finish_item and return -EAGAIN, which puts the defer item back on the
> > > defer op list, logs a new xattr intent with the new state, rolls the
> > > transaction, and tries to finish the attr again.  I think you've already
> > > done this last part.
> > > 
> > 
> > That sounds plausible to me. One concern I have is that I think we
> > should try to avoid creating more unnecessary complexity in the dfops
> > state mechanism simply to accommodate a messy xattr implementation. For
> > example, consider the following sequence for a simple set of an xattr
> > that requires leaf format and remote value block(s):
> > 
> > - try sf add
> > - returns -ENOSPC, convert to leaf and roll tx
> > - attempt to add the xattr (xfs_attr_leaf_addname())
> > 	- if -ENOSPC, convert to node and call xfs_attr_node_addname()
> > 	- else call xfs_attr3_leaf_add_work()
> > 		- add entry
> > 		- if remoteval, set INCOMPLETE
> > - roll tx
> > - if remoteval, call xfs_attr_rmtval_set()
> > 	- block allocation, tx roll loop
> > 	- copy remote value into bufs, xfs_bwrite()
> > - if remoteval, xfs_attr3_leaf_clearflag()
> > 	- clear INCOMPLETE
> > 	- update/log rmt pointers
> > 	- roll tx
> > 
> > I'm wondering 1.) how much of this is necessary with an intent based
> > implementation and 2.) how much of this can be refactored to not require
> > complex state tracking.
> > 
> > For example, all of the format conversions that occur before we actually
> > make any modifications associated with the xattr (i.e., -ENOSPC returns
> > from the current format) seem to me could easily be performed and
> > immediately return -EAGAIN without any state tracking. The retry should
> > pick up the current format of the fork and retry there. Thus, ISTM we
> > could drop the whole xfs_attr_leaf_addname() -> xfs_attr3_leaf_to_node()
> > -> xfs_attr_node_addname() codepath in favor of a format conversion and
> > -EAGAIN retry that calls directly into xfs_attr_node_addname().
> 
> That had been my other thought -- in theory we keep the inode locked
> across all the transaction rolls, so we could auto-detect what we need
> to do.
> 

Indeed, at the very least it might reduce the number of "on-disk" state
markers we have to define.

> > Once we have leaf format and we're doing remote block allocation, how
> > much could we get away with by re-looking up the entry, finding that
> > we're still short of remote blocks and performing another
> > xfs_bmapi_write() -> -EAGAIN cycle until we're good to copy in the xattr
> > value?
> > 
> > What about all this INCOMPLETE stuff? Do we even need that with an
> > intent based implementation?
> 
> No.  AFAIK the INCOMPLETE flag exists to hide attrs from userspace until
> we're totally done setting them up, and is therefore unnecessary with an
> intent implementation.  Repair zaps any INCOMPLETE attrs it finds.
> 
> > My understanding was that was because we
> > had to roll the transaction and thus could leave an incomplete xattr on
> > disk. I haven't looked too far into it so perhaps there's more to it
> > than that, but if not and this is no longer a problem with an intent
> > based implementation then perhaps much of that code and associated tx
> > rolls can be bypassed as well.
> 
> Getting rid of the INCOMPLETE wonkiness would be the strongest argument
> for switching the regular attr manipulation paths to use intents, though
> we'd have to toggle it with some feature or other.
> 
> (Some feature or other being parent pointers, or possibly just migrating
> the free space tracking parts of dir3 to a "new" attr4 format for better
> speed.)
> 
> > This is not to say that we won't require any such state tracking as
> > you've described above. The whole block allocation thing above may
> > require a state marker to get around attempts to set the xattr name
> > again and get back to the remote value block allocation code. It also
> > looks like we can do post xattr set format changes (i.e., node -> leaf,
> > leaf -> sf) that might require something like that to make sure we don't
> > go an retry an xattr set we've already completed. The point is just that
> > I'd prefer that we explore how much we can simplify this mess of an
> > implementation as much as possible (the above is all very handwavy)
> > first to reduce the state tracking complexity, particularly if these
> > states end up written to the log via the intent.
> > 
> > Hmm, I'm starting to think that maybe what we really need to do here is
> > step back from the code and logically map out what these states and the
> > resulting operation flow needs to be, particularly since there are so
> > many variations between different format conversions, renames, remote
> > blocks, etc. Once we have this whole mess mapped out, coding it up
> > should be more of an effort in refactoring.
> 
> Yep.
> 
> > > xfs_attri_recover then becomes much simpler -- we're passed in the
> > > reconstructed log item from which we figure out which step we need to
> > > do.  We call xfs_trans_attr() to do that one step, but unlike
> > > _finish_item, we use the new state to construct a *new* attr intent and
> > > attach it to the transaction, then call xfs_defer_move at the end to
> > > move all the queued defer_ops to the parent_tp because log recovery
> > > requires us to recover all the incomplete log intent items before
> > > finishing any new ones that were created as part of recovery.
> > > 
> > > This does mean that we end up with dramatically separate code paths for
> > > defer ops attr setting vs. regular attr setting, but as you point out
> > > the parent pointer feature will give the new code paths plenty of exercise.
> > > Tying the new log intent items to a new feature bit is key to preventing
> > > old kernels from stumbling across our new intent items, so we needed to
> > > preserve the old attr set paths anyway.
> > > 
> > 
> > That's a good point wrt to the other discussion around the direct xattr
> > codepath. It sounds like we do need to keep that entire path around
> > regardless to support v4 filesystems and such. The current series just
> > unconditionally switches things over to deferred ops.
> 
> Er... yikes.  XFS cannot suddenly introduce new ondisk formats for
> existing filesystems.
> 

We were discussing whether to preserve the existing codepath with
respect to flexibility/efficiency, but the whole backwards compatibility
aspect just didn't register with me until you mentioned it. I think that
kind of makes that decision for us. :P

> > > Anyway, if this all seems confusing, you can track me down, because I
> > > wrote most of this system and therefore have forgotten all of
> > > it^W^W^W^W^Wam available to help. :)
> > > 
> > > > > > 
> > > > > > I was also starting to wonder if maybe I could do some refactoring in
> > > > > > xfs_defer_finish_noroll to capture the common code associated with the
> > > > > > -EAGAIN handling.  Then maybe we could make a function pointer that we can
> > > > > > pass through the finish_item interface.  The idea being that subroutines
> > > > > > could use the function pointer to cycle out the transaction when needed
> > > > > > instead of having to record states and back out like this. It'd be a new
> > > 
> > > The state tracking and rolling is already built into xfs_defer.c. :)
> > > 
> > > > > > parameter to pipe around, but it'd be more efficient than the state machine,
> > > > > > and less surgery in the refactor.  And maybe a blessing to any other
> > > > > > operations that might need to go through this transition in the future.
> > > > > > Thoughts?
> > > > > > 
> > > > > 
> > > > > That's an interesting idea. It still strikes me as a bit of a
> > > > > fallback/hack as opposed to organizing the code to properly fit into the
> > > > > dfops infrastructure, but it could be useful as a transient solution.
> > > > >  From a high level, it looks like we'd have to create a new intent, relog
> > > > > this item and all remaining items associated with the dfp to it, roll
> > > > > the tx, and finally create a done item associated with the intent in the
> > > > > new tx. You'd need access to the dfp for some of that, so it's not
> > > > > immediately clear to me that this ends up much easier than fixing up
> > > > > the xattr code.
> > > 
> > > (I think the code that handles EAGAIN being returned from finish_item
> > > does this for you....)
> > > 
> > 
> > Yeah, I'm not totally sure it's an ideal/feasible approach, but for the
> > sake of clarity I think what Allison is getting at is that if there was
> > a way to trigger a dfops -EAGAIN roll sequence via a callback/helper
> > function, we wouldn't need to refactor the xattr subsystem to have
> > -EAGAIN return points. Instead we could just invoke the callback at the
> > existing roll points and achieve the same behavior (in theory). It's
> > kind of like providing an inside-out xfs_defer_finish_noroll() -EAGAIN
> > implementation via a helper function for code down in ->finish_item().
> 
> <nod> I grok that, but wonder if we really can invoke a roll while in
> the middle of ->finish_item...?  Anyway, we can set aside my confusion
> for now because I really think we need to see a map of all the pieces 
> 

Ok, I'm not really sure either. It was just an idea to bat around with
the rest. I agree that an informal, logical map/breakdown is the best
next step here. That gives us something concrete to review and refine.

Brian

> --D
> 
> > Brian
> > 
> > > > > 
> > > > > BTW, if we did end up with something like that I'd probably prefer to
> > > > > see it as an exported dfops helper function as opposed to a function
> > > > > pointer being passed around, if possible.
> > > > > 
> > > > 
> > > > Alrighty, I think for now I may try to pursue something more like what you
> > > > proposed in the next patch and see where I get first.  Maybe I'll come back
> > > > to this later if for some reason it doesn't work out, but I think what you
> > > > have there is reasonable.
> > > 
> > > <nod>
> > > 
> > > --D
> > > 
> > > > 
> > > > Thanks again for the reviews!
> > > > Allison
> > > > 
> > > > > Brian
> > > > > 
> > > > > > Thanks again for the reviews!
> > > > > > 
> > > > > > Allison
> > > > > > 
> > > > > > > 
> > > > > > > Brian
> > > > > > > 
> > > > > > > 
> > > > > > > >    fs/xfs/libxfs/xfs_attr.h | 18 +++++++++++++++++-
> > > > > > > >    fs/xfs/scrub/common.c    |  2 ++
> > > > > > > >    fs/xfs/xfs_acl.c         |  2 ++
> > > > > > > >    fs/xfs/xfs_attr_item.c   |  2 +-
> > > > > > > >    fs/xfs/xfs_ioctl.c       |  2 ++
> > > > > > > >    fs/xfs/xfs_ioctl32.c     |  2 ++
> > > > > > > >    fs/xfs/xfs_iops.c        |  1 +
> > > > > > > >    fs/xfs/xfs_xattr.c       |  1 +
> > > > > > > >    8 files changed, 28 insertions(+), 2 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > > > > > > > index 974c963..4ce3b0a 100644
> > > > > > > > --- a/fs/xfs/libxfs/xfs_attr.h
> > > > > > > > +++ b/fs/xfs/libxfs/xfs_attr.h
> > > > > > > > @@ -77,6 +77,13 @@ typedef struct attrlist_ent {	/* data from attr_list() */
> > > > > > > >    	char	a_name[1];	/* attr name (NULL terminated) */
> > > > > > > >    } attrlist_ent_t;
> > > > > > > > +/* Attr state machine types */
> > > > > > > > +enum xfs_attr_state {
> > > > > > > > +	XFS_ATTR_STATE1 = 1,
> > > > > > > > +	XFS_ATTR_STATE2 = 2,
> > > > > > > > +	XFS_ATTR_STATE3 = 3,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > >    /*
> > > > > > > >     * List of attrs to commit later.
> > > > > > > >     */
> > > > > > > > @@ -88,7 +95,16 @@ struct xfs_attr_item {
> > > > > > > >    	void		  *xattri_name;	      /* attr name */
> > > > > > > >    	uint32_t	  xattri_name_len;    /* length of name */
> > > > > > > >    	uint32_t	  xattri_flags;       /* attr flags */
> > > > > > > > -	struct list_head  xattri_list;
> > > > > > > > +
> > > > > > > > +	/*
> > > > > > > > +	 * Delayed attr parameters that need to remain instantiated
> > > > > > > > +	 * across transaction rolls during the defer finish
> > > > > > > > +	 */
> > > > > > > > +	struct xfs_buf		*xattri_leaf_bp;  /* Leaf buf to release */
> > > > > > > > +	enum xfs_attr_state	xattri_state;	  /* state machine marker */
> > > > > > > > +	struct xfs_da_args	xattri_args;	  /* args context */
> > > > > > > > +
> > > > > > > > +	struct list_head	xattri_list;
> > > > > > > >    	/*
> > > > > > > >    	 * A byte array follows the header containing the file name and
> > > > > > > > diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> > > > > > > > index 0c54ff5..270c32e 100644
> > > > > > > > --- a/fs/xfs/scrub/common.c
> > > > > > > > +++ b/fs/xfs/scrub/common.c
> > > > > > > > @@ -30,6 +30,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/xfs_scrub.h"
> > > > > > > > diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> > > > > > > > index 142de8d..9b1b93e 100644
> > > > > > > > --- a/fs/xfs/xfs_acl.c
> > > > > > > > +++ b/fs/xfs/xfs_acl.c
> > > > > > > > @@ -10,6 +10,8 @@
> > > > > > > >    #include "xfs_mount.h"
> > > > > > > >    #include "xfs_inode.h"
> > > > > > > >    #include "xfs_acl.h"
> > > > > > > > +#include "xfs_da_format.h"
> > > > > > > > +#include "xfs_da_btree.h"
> > > > > > > >    #include "xfs_attr.h"
> > > > > > > >    #include "xfs_trace.h"
> > > > > > > >    #include <linux/slab.h>
> > > > > > > > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> > > > > > > > index 0ea19b4..36e6d1e 100644
> > > > > > > > --- a/fs/xfs/xfs_attr_item.c
> > > > > > > > +++ b/fs/xfs/xfs_attr_item.c
> > > > > > > > @@ -19,10 +19,10 @@
> > > > > > > >    #include "xfs_rmap.h"
> > > > > > > >    #include "xfs_inode.h"
> > > > > > > >    #include "xfs_icache.h"
> > > > > > > > -#include "xfs_attr.h"
> > > > > > > >    #include "xfs_shared.h"
> > > > > > > >    #include "xfs_da_format.h"
> > > > > > > >    #include "xfs_da_btree.h"
> > > > > > > > +#include "xfs_attr.h"
> > > > > > > >    static inline struct xfs_attri_log_item *ATTRI_ITEM(struct xfs_log_item *lip)
> > > > > > > >    {
> > > > > > > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > > > > > > > index ab341d6..c8728ca 100644
> > > > > > > > --- a/fs/xfs/xfs_ioctl.c
> > > > > > > > +++ b/fs/xfs/xfs_ioctl.c
> > > > > > > > @@ -16,6 +16,8 @@
> > > > > > > >    #include "xfs_rtalloc.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 5001dca..23f6990 100644
> > > > > > > > --- a/fs/xfs/xfs_ioctl32.c
> > > > > > > > +++ b/fs/xfs/xfs_ioctl32.c
> > > > > > > > @@ -21,6 +21,8 @@
> > > > > > > >    #include "xfs_fsops.h"
> > > > > > > >    #include "xfs_alloc.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 e73c21a..561c467 100644
> > > > > > > > --- a/fs/xfs/xfs_iops.c
> > > > > > > > +++ b/fs/xfs/xfs_iops.c
> > > > > > > > @@ -17,6 +17,7 @@
> > > > > > > >    #include "xfs_acl.h"
> > > > > > > >    #include "xfs_quota.h"
> > > > > > > >    #include "xfs_error.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 3013746..938e81d 100644
> > > > > > > > --- a/fs/xfs/xfs_xattr.c
> > > > > > > > +++ b/fs/xfs/xfs_xattr.c
> > > > > > > > @@ -11,6 +11,7 @@
> > > > > > > >    #include "xfs_mount.h"
> > > > > > > >    #include "xfs_da_format.h"
> > > > > > > >    #include "xfs_inode.h"
> > > > > > > > +#include "xfs_da_btree.h"
> > > > > > > >    #include "xfs_attr.h"
> > > > > > > >    #include "xfs_attr_leaf.h"
> > > > > > > >    #include "xfs_acl.h"
> > > > > > > > -- 
> > > > > > > > 2.7.4
> > > > > > > >