Re: [PATCH v7 13/19] xfs: Add delay ready attr remove routines

[Brian Foster <bfoster@redhat.com>]

On Tue, Feb 25, 2020 at 10:36:18PM -0700, Allison Collins wrote:
> 
> 
> On 2/25/20 6:34 AM, Brian Foster wrote:
> > On Mon, Feb 24, 2020 at 04:14:48PM -0700, Allison Collins wrote:
> > > On 2/24/20 8:25 AM, Brian Foster wrote:
> > > > On Sat, Feb 22, 2020 at 07:06:05PM -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.
> > > > > 
> > > > > Below is a state machine diagram for attr remove operations. The XFS_DAS_* states
> > > > > indicate places where the function would return -EAGAIN, and then immediately
> > > > > resume from after being recalled by the calling function.  States marked as a
> > > > > "subroutine state" indicate that they belong to a subroutine, and so the calling
> > > > > function needs to pass them back to that subroutine to allow it to finish where
> > > > > it left off. But they otherwise do not have a role in the calling function other
> > > > > than just passing through.
> > > > > 
> > > > >    xfs_attr_remove_iter()
> > > > >            XFS_DAS_RM_SHRINK     ─┐
> > > > >            (subroutine state)     │
> > > > >                                   │
> > > > >            XFS_DAS_RMTVAL_REMOVE ─┤
> > > > >            (subroutine state)     │
> > > > >                                   └─>xfs_attr_node_removename()
> > > > >                                                    │
> > > > >                                                    v
> > > > >                                            need to remove
> > > > >                                      ┌─n──  rmt blocks?
> > > > >                                      │             │
> > > > >                                      │             y
> > > > >                                      │             │
> > > > >                                      │             v
> > > > >                                      │  ┌─>XFS_DAS_RMTVAL_REMOVE
> > > > >                                      │  │          │
> > > > >                                      │  │          v
> > > > >                                      │  └──y── more blks
> > > > >                                      │         to remove?
> > > > >                                      │             │
> > > > >                                      │             n
> > > > >                                      │             │
> > > > >                                      │             v
> > > > >                                      │         need to
> > > > >                                      └─────> shrink tree? ─n─┐
> > > > >                                                    │         │
> > > > >                                                    y         │
> > > > >                                                    │         │
> > > > >                                                    v         │
> > > > >                                            XFS_DAS_RM_SHRINK │
> > > > >                                                    │         │
> > > > >                                                    v         │
> > > > >                                                   done <─────┘
> > > > > 
> > > > 
> > > > Wow. :P I guess I have nothing against verbose commit logs, but I wonder
> > > > how useful this level of documentation is for a patch that shouldn't
> > > > really change the existing flow of the operation.
> > > 
> > > Yes Darrick had requested a diagram in the last review, so I had put this
> > > together.  I wasnt sure where the best place to put it even was, so I put it
> > > here at least for now.  I have no idea if there is a limit on commit message
> > > length, but if there is, I'm pretty sure I blew right past it in this patch
> > > and the next.  Maybe if anything it can just be here for now while we work
> > > through things?
> > > 
> > 
> > No problem.. if it's useful it's good to have a record of out around
> > somewhere until the end result is more stabilized and we can determine
> > whether this warrants a permanent home somewhere in the code.
> > 
> > > > 
> > > > > Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> > > > > ---
> > > > >    fs/xfs/libxfs/xfs_attr.c     | 114 +++++++++++++++++++++++++++++++++++++------
> > > > >    fs/xfs/libxfs/xfs_attr.h     |   1 +
> > > > >    fs/xfs/libxfs/xfs_da_btree.h |  30 ++++++++++++
> > > > >    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, 141 insertions(+), 16 deletions(-)
> > > > > 
> > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > > > index 5d73bdf..cd3a3f7 100644
> > > > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > > > @@ -368,11 +368,60 @@ xfs_has_attr(
> > > > >     */
> > > > >    int
> > > > >    xfs_attr_remove_args(
> > > > > +	struct xfs_da_args	*args)
> > > > > +{
> > > > > +	int			error = 0;
> > > > > +	int			err2 = 0;
> > > > > +
> > > > > +	do {
> > > > > +		error = xfs_attr_remove_iter(args);
> > > > > +		if (error && error != -EAGAIN)
> > > > > +			goto out;
> > > > > +
> > > > 
> > > > I'm a little confused on the logic of this loop given that the only
> > > > caller commits the transaction (which also finishes dfops). IOW, it
> > > > seems we shouldn't ever need to finish/roll when error != -EAGAIN. If
> > > > that is the case, this can be simplified to something like:
> > > Well, we need to do it when error == -EAGAIN or 0, right? Which I think
> > > better imitates the defer_finish routines.  That's why a lot of the existing
> > > code that just finishes off with a transaction just sort of gets sawed off
> > > at the end. Otherwise they would need one more state just to return -EAGAIN
> > > as the last thing they have to do. Did that make sense?
> > > 
> > 
> > Hmm.. I could just be missing something or not far along enough in the
> > series. Can you point me at an example of where we need to finish/roll
> > before the caller of xfs_attr_remove_args() commits the transaction?
> > 
> Ok, in looking for an example, realized all such examples appear in the next
> patch ;-)  So maybe we can get away with simplifying it in this patch.
> 

Ah, Ok. Yeah, I think that would be best so long as it is correct, since
right now at least we have separate xfs_attr_[set|remove]_args() loop
functions and I didn't see any code that warranted the extra roll in the
remove path.

> For the next patch though, it's any place the roll/finish disappears, and an
> "return -EAGAIN" does not.  For example, at the end of
> xfs_attr_leaf_addname.
> 

I see, thanks. Hmmm... so I think that particular example is basically a
programming pattern thing moreso than a functional requirement. I.e.,
the current _clearflag() function clears the flag and rolls the
transaction perhaps simply so it can be reliably used in different
contexts. The use in the _addname() case is functionally spurious afaict
because we roll the transaction only to make no further changes and then
commit the final transaction in the higher level code.

I could see leaving the loop as is if this were the case for every exit
path back to xfs_attr_set_args(), but is that really the case? If not,
haven't we introduced a spurious roll for any zero return back to the
_args() function? I think it might be best to fix up the loop to not
roll on error == 0, explicitly plumb in the -EAGAIN in those spurious
cases like _addname() where we currently roll, and then come up with a
follow up patch to remove the ones that end up as spurious. That way
we're not conflating too much refactoring with functional change and can
review/document the functional change independently (i.e., if removing
one of those rolls ends up introducing a bug, we don't have to revert an
entire refactoring patch to restore original behavior).

Now that I think of it, the better option is probably to remove the
xfs_trans_roll_inode() call from _addname() first, before these patches
introduce the delay ready infrastructure, since it's already isolated as
spurious at that point. That should be a simple patch with a
clear/obvious explanation.

Brian

> > > > 
> > > > int
> > > > xfs_attr_remove_args(
> > > >           struct xfs_da_args      *args)
> > > > {
> > > >           int                     error;
> > > > 
> > > >           do {
> > > >                   error = xfs_attr_remove_iter(args);
> > > >                   if (error != -EAGAIN)
> > > >                           break;
> > > > 
> > > >                   if (args->dac.flags & XFS_DAC_FINISH_TRANS) {
> > > >                           args->dac.flags &= ~XFS_DAC_FINISH_TRANS;
> > > >                           error = xfs_defer_finish(&args->trans);
> > > >                           if (error)
> > > >                                   break;
> > > >                   }
> > > > 
> > > >                   error = xfs_trans_roll_inode(&args->trans, args->dp);
> > > >                   if (error)
> > > >                           break;
> > > >           } while (true);
> > > > 
> > > >           return error;
> > > > }
> > > > 
> > > > That has the added benefit of eliminating the whole err2 pattern, which
> > > > always strikes me as a landmine.
> > > > 
> > > > > +		if (args->dac.flags & XFS_DAC_FINISH_TRANS) {
> > > > 
> > > > BTW, _FINISH_TRANS also seems misnamed given that we finish deferred
> > > > operations, not necessarily the transaction. XFS_DAC_DEFER_FINISH?
> > > Sure, will update
> > > 
> > > > 
> > > > > +			args->dac.flags &= ~XFS_DAC_FINISH_TRANS;
> > > > > +
> > > > > +			err2 = xfs_defer_finish(&args->trans);
> > > > > +			if (err2) {
> > > > > +				error = err2;
> > > > > +				goto out;
> > > > > +			}
> > > > > +		}
> > > > > +
> > > > > +		err2 = xfs_trans_roll_inode(&args->trans, args->dp);
> > > > > +		if (err2) {
> > > > > +			error = err2;
> > > > > +			goto out;
> > > > > +		}
> > > > > +
> > > > > +	} while (error == -EAGAIN);
> > > > > +out:
> > > > > +	return error;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Remove the attribute specified in @args.
> > > > > + *
> > > > > + * This function may return -EAGAIN to signal that the transaction needs to be
> > > > > + * rolled.  Callers should continue calling this function until they receive a
> > > > > + * return value other than -EAGAIN.
> > > > > + */
> > > > > +int
> > > > > +xfs_attr_remove_iter(
> > > > >    	struct xfs_da_args      *args)
> > > > >    {
> > > > >    	struct xfs_inode	*dp = args->dp;
> > > > >    	int			error;
> > > > > +	/* State machine switch */
> > > > > +	switch (args->dac.dela_state) {
> > > > > +	case XFS_DAS_RM_SHRINK:
> > > > > +	case XFS_DAS_RMTVAL_REMOVE:
> > > > > +		goto node;
> > > > > +	default:
> > > > > +		break;
> > > > > +	}
> > > > > +
> > > > >    	if (!xfs_inode_hasattr(dp)) {
> > > > >    		error = -ENOATTR;
> > > > >    	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
> > > > > @@ -381,6 +430,7 @@ xfs_attr_remove_args(
> > > > >    	} else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> > > > >    		error = xfs_attr_leaf_removename(args);
> > > > >    	} else {
> > > > > +node:
> > > > >    		error = xfs_attr_node_removename(args);
> > > > >    	}
> > > > > @@ -895,9 +945,8 @@ xfs_attr_leaf_removename(
> > > > >    		/* bp is gone due to xfs_da_shrink_inode */
> > > > >    		if (error)
> > > > >    			return error;
> > > > > -		error = xfs_defer_finish(&args->trans);
> > > > > -		if (error)
> > > > > -			return error;
> > > > > +
> > > > > +		args->dac.flags |= XFS_DAC_FINISH_TRANS;
> > > > >    	}
> > > > >    	return 0;
> > > > >    }
> > > > > @@ -1218,6 +1267,11 @@ xfs_attr_node_addname(
> > > > >     * This will involve walking down the Btree, and may involve joining
> > > > >     * leaf nodes and even joining intermediate nodes up to and including
> > > > >     * the root node (a special case of an intermediate node).
> > > > > + *
> > > > > + * This routine is meant to function as either an inline or delayed operation,
> > > > > + * and may return -EAGAIN when the transaction needs to be rolled.  Calling
> > > > > + * functions will need to handle this, and recall the function until a
> > > > > + * successful error code is returned.
> > > > >     */
> > > > >    STATIC int
> > > > >    xfs_attr_node_removename(
> > > > > @@ -1230,10 +1284,24 @@ xfs_attr_node_removename(
> > > > >    	struct xfs_inode	*dp = args->dp;
> > > > >    	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_RMTVAL_REMOVE:
> > > > > +		goto rm_node_blks;
> > > > > +	case XFS_DAS_RM_SHRINK:
> > > > > +		goto rm_shrink;
> > > > > +	default:
> > > > > +		break;
> > > > > +	}
> > > > >    	error = xfs_attr_node_hasname(args, &state);
> > > > >    	if (error != -EEXIST)
> > > > >    		goto out;
> > > > > +	else
> > > > > +		error = 0;
> > > > 
> > > > This doesn't look necessary.
> > > Well, at this point error has to be -EEXIST.  Which is great because we need
> > > the attr to exist, but we dont want to return that as error for this
> > > function.  Which can happen if error is not otherwise set.
> > > 
> > 
> > AFAICT every codepath after this assigns error one way or another before
> > it's returned. There's another error = 0 assignment just before the out:
> > label.
> Ok, I see it.  Will remove.
> 
> > 
> > > > 
> > > > >    	/*
> > > > >    	 * If there is an out-of-line value, de-allocate the blocks.
> > > > > @@ -1243,6 +1311,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
> > > > > @@ -1261,13 +1337,21 @@ xfs_attr_node_removename(
> > > > >    		if (error)
> > > > >    			goto out;
> > > > > -		error = xfs_trans_roll_inode(&args->trans, args->dp);
> > > > > +		error = xfs_attr_rmtval_invalidate(args);
> > > > 
> > > > Remind me why we lose the above trans roll? I vaguely recall that this
> > > > was intentional, but I could be mistaken...
> > > I think we removed it in v5.  We used to have a  XFS_DAS_RM_INVALIDATE
> > > state, but then we reasoned that because these are just in-core changes, we
> > > didnt need it, so we eliminated this state entirely.
> > > 
> > > Maybe i just add a comment here?  Just as a reminder
> > > 
> > 
> > Ah, Ok. Normally I'd say document things like this in the commit log so
> > we don't lose track, though I don't know how much space we have there.
> > ;)
> Ok, I'll see if I can squeeze in a few more lines :-)
> 
> > 
> > > > 
> > > > >    		if (error)
> > > > >    			goto out;
> > > > > +	}
> > > > > -		error = xfs_attr_rmtval_remove(args);
> > > > > -		if (error)
> > > > > -			goto out;
> > > > > +rm_node_blks:
> > > > > +
> > > > > +	if (args->rmtblkno > 0) {
> > > > > +		error = xfs_attr_rmtval_unmap(args);
> > > > > +
> > > > > +		if (error) {
> > > > > +			if (error == -EAGAIN)
> > > > > +				args->dac.dela_state = XFS_DAS_RMTVAL_REMOVE;
> > > > 
> > > > Might be helpful for the code labels to match the state names. I.e., use
> > > > das_rmtval_remove: for the label above.
> > > Sure, I can update add the das prefix.
> > > 
> > > > 
> > > > > +			return error;
> > > > > +		}
> > > > >    		/*
> > > > >    		 * Refill the state structure with buffers, the prior calls
> > > > > @@ -1293,17 +1377,15 @@ xfs_attr_node_removename(
> > > > >    		error = xfs_da3_join(state);
> > > > >    		if (error)
> > > > >    			goto out;
> > > > > -		error = xfs_defer_finish(&args->trans);
> > > > > -		if (error)
> > > > > -			goto out;
> > > > > -		/*
> > > > > -		 * Commit the Btree join operation and start a new trans.
> > > > > -		 */
> > > > > -		error = xfs_trans_roll_inode(&args->trans, dp);
> > > > > -		if (error)
> > > > > -			goto out;
> > > > > +
> > > > > +		args->dac.flags |= XFS_DAC_FINISH_TRANS;
> > > > > +		args->dac.dela_state = XFS_DAS_RM_SHRINK;
> > > > > +		return -EAGAIN;
> > > > >    	}
> > > > > +rm_shrink:
> > > > > +	args->dac.dela_state = XFS_DAS_RM_SHRINK;
> > > > > +
> > > > 
> > > > There's an xfs_defer_finish() call further down this function. Should
> > > > that be replaced with the flag?
> > > > 
> > > > Finally, I mentioned in a previous review that this function should
> > > > probably be further broken down before fitting in the state management
> > > > stuff. It doesn't look like that happened so I've attached a diff that
> > > > is just intended to give an idea of what I mean by sectioning off the
> > > > hunks that might be able to break down into helpers. The helpers
> > > > wouldn't contain any state management, so we create a clear separation
> > > > between the state code and functional components.
> > > Yes, it's xfs_attr_node_shrink in patch 15.  I moved it to another patch to
> > > try and keep the activity in this one to a minimum.  Apologies if it
> > > surprised you!  And then i mistakenly had taken the XFS_DAC_FINISH_TRANS
> > > flag with it.  I meant to keep all the state machine stuff here.  Will fix!
> > > 
> > 
> > Ok, I might have just not got there yet.
> > 
> > > I think this initial
> > > > refactoring would make the introduction of state much more simple
> > > 
> > > I guess I didn't think people would be partial to introducing helpers before
> > > or after the state logic.  I put them after in this set because the states
> > > are visible now, so I though it would make the goal of modularizing code
> > > between the states more clear to folks.  Do you think I should move it back
> > > behind the state machine patches?
> > > 
> > 
> > I do think the refactoring should be done first. This does make it more
> > challenging for the developer (IMO) because I know I'd probably have to
> > hack around with the state bits to have a better idea of how to refactor
> > things in some cases, and then go back and retrofit the refactoring.
> > 
> > The advantage is that the heavy lifting in this series becomes agnostic
> > to the state bits. Refactoring patches are easier to review and we can
> > make progress because there's less of a need to carry those out of tree
> > through however many versions of the state code we'll need before
> > getting it merged. Once the code is sufficiently factored, the state
> > code should be much simpler to introduce and review since we hopefully
> > won't be jumping around into the middle of functions, multiple branches
> > of logic deep, etc.
> > 
> > (I see Dave commented similarly on a couple of the subsequent patches. I
> > 100% agree with the approach he describes there and that is similar to
> > what I was trying to describe with the diff I attached in my earlier
> > mail...)
> > 
> > Brian
> 
> Alrighty then, will move back.  Thanks, and thanks again for the reviews!!
> 
> Allison
> 
> > 
> > > (and
> > > > perhaps alleviate the need for the huge diagram).
> > > Well, I get the impression that people find the series sort of scary and
> > > maybe the diagrams help them a bit.  Maybe we can take them out later after
> > > people feel like they are comfortable with things?
> > > 
> > > It might also be
> > > > interesting to see how much of the result could be folded up further
> > > > into _removename_iter()...
> > > 
> > > Yes, I think that is the goal we're reaching for.  I will add the other
> > > helpers I see in your diff too.
> > > 
> > > Thanks for the reviews!
> > > Allison
> > > 
> > > > 
> > > > Brian
> > > > 
> > > > >    	/*
> > > > >    	 * If the result is small enough, push it all into the inode.
> > > > >    	 */
> > > > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > > > > index ce7b039..ea873a5 100644
> > > > > --- a/fs/xfs/libxfs/xfs_attr.h
> > > > > +++ b/fs/xfs/libxfs/xfs_attr.h
> > > > > @@ -155,6 +155,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 14f1be3..3c78498 100644
> > > > > --- a/fs/xfs/libxfs/xfs_da_btree.h
> > > > > +++ b/fs/xfs/libxfs/xfs_da_btree.h
> > > > > @@ -50,9 +50,39 @@ enum xfs_dacmp {
> > > > >    };
> > > > >    /*
> > > > > + * Enum values for xfs_delattr_context.da_state
> > > > > + *
> > > > > + * These values are used by delayed attribute operations to keep track  of where
> > > > > + * they were before they returned -EAGAIN.  A return code of -EAGAIN signals the
> > > > > + * calling function to roll the transaction, and then recall the subroutine to
> > > > > + * finish the operation.  The enum is then used by the subroutine to jump back
> > > > > + * to where it was and resume executing where it left off.
> > > > > + */
> > > > > +enum xfs_delattr_state {
> > > > > +	XFS_DAS_RM_SHRINK,	/* We are shrinking the tree */
> > > > > +	XFS_DAS_RMTVAL_REMOVE,	/* We are removing remote value blocks */
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Defines for xfs_delattr_context.flags
> > > > > + */
> > > > > +#define	XFS_DAC_FINISH_TRANS	0x1 /* indicates to finish the transaction */
> > > > > +
> > > > > +/*
> > > > > + * 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;
> > > > > +	unsigned int		flags;
> > > > > +	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 42ac847..d65e6d8 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 28c07c9..7c1d9da 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 769581a..d504f8f 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 74133a5..d8dc72d 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
> > > > > 
> > > > 
> > > 
> > 
>