Re: [PATCH 8/9] xfs: Roll delayed attr operations by returning EAGAIN

[Allison Henderson <allison.henderson@xxxxxxxxxx>]

On 4/23/19 7:19 AM, Brian Foster wrote:
> On Fri, Apr 12, 2019 at 03:50:35PM -0700, Allison Henderson wrote:
> > This patch modifies xfs_attr_set_args to return -EAGAIN
> > when a transaction needs to be rolled.  All functions
> > currently calling xfs_attr_set_args are modified to use
> > the deferred attr operation, or handle the -EAGAIN return
> > code
> >
> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> > ---
> >   fs/xfs/libxfs/xfs_attr.c | 62 ++++++++++++++++++++++++++++++++++++++++--------
> >   fs/xfs/libxfs/xfs_attr.h |  2 +-
> >   fs/xfs/xfs_attr_item.c   | 41 +++++++++++++++++++++++++++-----
> >   fs/xfs/xfs_trans.h       |  2 ++
> >   fs/xfs/xfs_trans_attr.c  | 56 +++++++++++++++++++++++++------------------
> >   5 files changed, 123 insertions(+), 40 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 0042708..4ddd86b 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -236,10 +236,37 @@ int
> >   xfs_attr_set_args(
> >   	struct xfs_da_args	*args,
> >   	struct xfs_buf          **leaf_bp,
> > +	enum xfs_attr_state	*state,
> >   	bool			roll_trans)
> >   {
> >   	struct xfs_inode	*dp = args->dp;
> >   	int			error = 0;
> > +	int			sf_size;
> > +
> > +	switch (*state) {
> > +	case (XFS_ATTR_STATE1):
> > +		goto state1;
> > +	case (XFS_ATTR_STATE2):
> > +		goto state2;
> > +	case (XFS_ATTR_STATE3):
> > +		goto state3;
> > +	}
> > +
> > +	/*
> > +	 * New inodes may not have an attribute fork yet. So set the attribute
> > +	 * fork appropriately
> > +	 */
> > +	if (XFS_IFORK_Q((args->dp)) == 0) {
> > +		sf_size = sizeof(struct xfs_attr_sf_hdr) +
> > +		     XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen, args->valuelen);
> > +		xfs_bmap_set_attrforkoff(args->dp, sf_size, NULL);
> > +		args->dp->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP);
> > +		args->dp->i_afp->if_flags = XFS_IFEXTENTS;
> > +	}
> > +
> > +	*state = XFS_ATTR_STATE1;
> > +	return -EAGAIN;
>
> As noted previously, this return seems unnecessary since we've not done
> anything in the transaction to this point.
>
> > +state1:
> >   
> >   	/*
> >   	 * If the attribute list is non-existent or a shortform list,
> > @@ -248,7 +275,6 @@ xfs_attr_set_args(
> >   	if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
> >   	    (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> >   	     dp->i_d.di_anextents == 0)) {
> > -
> >   		/*
> >   		 * Build initial attribute list (if required).
> >   		 */
> > @@ -262,6 +288,9 @@ xfs_attr_set_args(
> >   		if (error != -ENOSPC)
> >   			return error;
> >   
> > +		*state = XFS_ATTR_STATE2;
> > +		return -EAGAIN;
> > +state2:
>
> Here we've failed the sf add but not yet done the conversion, which
> means we've still not done anything in the transaction. I suspect we
> should probably convert to leaf and then return -EAGAIN.
>
> >   		/*
> >   		 * It won't fit in the shortform, transform to a leaf block.
> >   		 * GROT: another possible req'mt for a double-split btree op.
> > @@ -270,14 +299,14 @@ xfs_attr_set_args(
> >   		if (error)
> >   			return error;
> >   
> > -		if (roll_trans) {
> > -			/*
> > -			 * 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.
> > -			 */
> > -			xfs_trans_bhold(args->trans, *leaf_bp);
> > +		/*
> > +		 * 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.
> > +		 */
> > +		xfs_trans_bhold(args->trans, *leaf_bp);
> >   
> > +		if (roll_trans) {
> >   			error = xfs_defer_finish(&args->trans);
> >   			if (error)
> >   				return error;
> > @@ -293,6 +322,12 @@ xfs_attr_set_args(
> >   			xfs_trans_bjoin(args->trans, *leaf_bp);
> >   			*leaf_bp = NULL;
> >   		}
> > +
> > +		*state = XFS_ATTR_STATE3;
> > +		return -EAGAIN;
> > +state3:
>
> Hmm, and this appears to be the last place we return -EAGAIN from the
> set code. Am I following this correctly that we basically expect any of
> the other rolls down in xfs_attr_[leaf|node]_addname() to go away in
> deferred context? If so, why is that?

Well it looks like the calling function (xfs_defer_finish_noroll) is 
taking care of logging and rolling the transactions for us, so we need 
to not be doing that twice.  And then the same routines are used later 
during the log recovery too.

> That aside, I'm wondering whether we need the whole state thing to track
> this. For example, why not have a high level flow as something like the
> following?
>
> xfs_attr_set_args()
> {
> 	...
> 	if (local format) {
> 		error = xfs_attr_try_sf_addname(dp, args);
> 		if (error == -ENOSPC) {
> 			error = xfs_attr_shortform_to_leaf(args, leaf_bp);
> 			return -EAGAIN;
> 		} else
> 			return error;
> 	} else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> 		error = xfs_attr_leaf_addname(args);
> 	} else {
> 		error = xfs_attr_node_addname(args);
> 	}
> }
>
> Of course, this may need further changes if we do end up incorporating
> the rolls down in the leaf/node functions. Perhaps we could pull apart
> those functions such that we -EAGAIN on the conversions required to
> address -ENOSPC returns. That might provide a natural boundary to
> re-enter the top-level function without the need for a state machine, at
> least for any rolls that occurs before we actually do an attr op
> (post-op rolls may very well require more state to incorporate).
> Thoughts?

Alrighty, I think that sounds like a good approach.  I'll try to re 
factor this area as you suggest and see where I get.  Maybe I can 
isolate the areas that handle the transactions as we talked about 
earlier too.

Thanks again for the reviews!  They are very helpful!
Allison

> Brian
>
> > +		if (*leaf_bp != NULL)
> > +			xfs_trans_brelse(args->trans, *leaf_bp);
> >   	}
> >   
> >   	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> > @@ -419,7 +454,9 @@ xfs_attr_set(
> >   		goto out_trans_cancel;
> >   
> >   	xfs_trans_ijoin(args.trans, dp, 0);
> > -	error = xfs_attr_set_args(&args, &leaf_bp, true);
> > +
> > +	error = xfs_attr_set_deferred(dp, args.trans, name, namelen,
> > +			value, valuelen, flags);
> >   	if (error)
> >   		goto out_release_leaf;
> >   	if (!args.trans) {
> > @@ -554,8 +591,13 @@ xfs_attr_remove(
> >   	 */
> >   	xfs_trans_ijoin(args.trans, dp, 0);
> >   
> > -	error = xfs_attr_remove_args(&args, true);
> > +	error = xfs_has_attr(&args);
> > +	if (error)
> > +		goto out;
> > +
> >   
> > +	error = xfs_attr_remove_deferred(dp, args.trans,
> > +			name, namelen, flags);
> >   	if (error)
> >   		goto out;
> >   
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index 4ce3b0a..da95e69 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -181,7 +181,7 @@ int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name,
> >   		 size_t namelen, unsigned char *value, int valuelen,
> >   		 int flags);
> >   int xfs_attr_set_args(struct xfs_da_args *args, struct xfs_buf **leaf_bp,
> > -		 bool roll_trans);
> > +		 enum xfs_attr_state *state, bool roll_trans);
> >   int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name,
> >   		    size_t namelen, int flags);
> >   int xfs_has_attr(struct xfs_da_args *args);
> > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> > index 36e6d1e..292d608 100644
> > --- a/fs/xfs/xfs_attr_item.c
> > +++ b/fs/xfs/xfs_attr_item.c
> > @@ -464,8 +464,11 @@ xfs_attri_recover(
> >   	struct xfs_attri_log_format	*attrp;
> >   	struct xfs_trans_res		tres;
> >   	int				local;
> > -	int				error = 0;
> > +	int				error, err2 = 0;
> >   	int				rsvd = 0;
> > +	enum xfs_attr_state		state = 0;
> > +	struct xfs_buf			*leaf_bp = NULL;
> > +
> >   
> >   	ASSERT(!test_bit(XFS_ATTRI_RECOVERED, &attrip->flags));
> >   
> > @@ -540,14 +543,40 @@ xfs_attri_recover(
> >   	xfs_ilock(ip, XFS_ILOCK_EXCL);
> >   
> >   	xfs_trans_ijoin(args.trans, ip, 0);
> > -	error = xfs_trans_attr(&args, attrdp, attrp->alfi_op_flags);
> > -	if (error)
> > -		goto abort_error;
> >   
> > +	do {
> > +		leaf_bp = NULL;
> > +
> > +		error = xfs_trans_attr(&args, attrdp, &leaf_bp, &state,
> > +				attrp->alfi_op_flags);
> > +		if (error && error != -EAGAIN)
> > +			goto abort_error;
> > +
> > +		xfs_trans_log_inode(args.trans, ip,
> > +				XFS_ILOG_CORE | XFS_ILOG_ADATA);
> > +
> > +		err2 = xfs_trans_commit(args.trans);
> > +		if (err2) {
> > +			error = err2;
> > +			goto abort_error;
> > +		}
> > +
> > +		if (error == -EAGAIN) {
> > +			err2 = xfs_trans_alloc(mp, &tres, args.total, 0,
> > +				XFS_TRANS_PERM_LOG_RES, &args.trans);
> > +			if (err2) {
> > +				error = err2;
> > +				goto abort_error;
> > +			}
> > +			xfs_trans_ijoin(args.trans, ip, 0);
> > +		}
> > +
> > +	} while (error == -EAGAIN);
> > +
> > +	if (leaf_bp)
> > +		xfs_trans_brelse(args.trans, leaf_bp);
> >   
> >   	set_bit(XFS_ATTRI_RECOVERED, &attrip->flags);
> > -	xfs_trans_log_inode(args.trans, ip, XFS_ILOG_CORE | XFS_ILOG_ADATA);
> > -	error = xfs_trans_commit(args.trans);
> >   	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >   	return error;
> >   
> > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> > index 7bb9d8e..c785cd7 100644
> > --- a/fs/xfs/xfs_trans.h
> > +++ b/fs/xfs/xfs_trans.h
> > @@ -239,6 +239,8 @@ xfs_trans_get_attrd(struct xfs_trans *tp,
> >   		    struct xfs_attri_log_item *attrip);
> >   int xfs_trans_attr(struct xfs_da_args *args,
> >   		   struct xfs_attrd_log_item *attrdp,
> > +		   struct xfs_buf **leaf_bp,
> > +		   void *state,
> >   		   uint32_t attr_op_flags);
> >   
> >   int		xfs_trans_commit(struct xfs_trans *);
> > diff --git a/fs/xfs/xfs_trans_attr.c b/fs/xfs/xfs_trans_attr.c
> > index 3679348..a3339ea 100644
> > --- a/fs/xfs/xfs_trans_attr.c
> > +++ b/fs/xfs/xfs_trans_attr.c
> > @@ -56,10 +56,11 @@ int
> >   xfs_trans_attr(
> >   	struct xfs_da_args		*args,
> >   	struct xfs_attrd_log_item	*attrdp,
> > +	struct xfs_buf			**leaf_bp,
> > +	void				*state,
> >   	uint32_t			op_flags)
> >   {
> >   	int				error;
> > -	struct xfs_buf			*leaf_bp = NULL;
> >   
> >   	error = xfs_qm_dqattach_locked(args->dp, 0);
> >   	if (error)
> > @@ -68,7 +69,8 @@ xfs_trans_attr(
> >   	switch (op_flags) {
> >   	case XFS_ATTR_OP_FLAGS_SET:
> >   		args->op_flags |= XFS_DA_OP_ADDNAME;
> > -		error = xfs_attr_set_args(args, &leaf_bp, false);
> > +		error = xfs_attr_set_args(args, leaf_bp,
> > +				(enum xfs_attr_state *)state, false);
> >   		break;
> >   	case XFS_ATTR_OP_FLAGS_REMOVE:
> >   		ASSERT(XFS_IFORK_Q((args->dp)));
> > @@ -78,11 +80,6 @@ xfs_trans_attr(
> >   		error = -EFSCORRUPTED;
> >   	}
> >   
> > -	if (error) {
> > -		if (leaf_bp)
> > -			xfs_trans_brelse(args->trans, leaf_bp);
> > -	}
> > -
> >   	/*
> >   	 * Mark the transaction dirty, even on error. This ensures the
> >   	 * transaction is aborted, which:
> > @@ -184,27 +181,40 @@ xfs_attr_finish_item(
> >   	char				*name_value;
> >   	int				error;
> >   	int				local;
> > -	struct xfs_da_args		args;
> > +	struct xfs_da_args		*args;
> >   
> >   	attr = container_of(item, struct xfs_attr_item, xattri_list);
> > -	name_value = ((char *)attr) + sizeof(struct xfs_attr_item);
> > -
> > -	error = xfs_attr_args_init(&args, attr->xattri_ip, name_value,
> > -				   attr->xattri_name_len, attr->xattri_flags);
> > -	if (error)
> > -		goto out;
> > +	args = &attr->xattri_args;
> > +
> > +	if (attr->xattri_state == 0) {
> > +		/* Only need to initialize args context once */
> > +		name_value = ((char *)attr) + sizeof(struct xfs_attr_item);
> > +		error = xfs_attr_args_init(args, attr->xattri_ip, name_value,
> > +					   attr->xattri_name_len,
> > +					   attr->xattri_flags);
> > +		if (error)
> > +			goto out;
> > +
> > +		args->hashval = xfs_da_hashname(args->name, args->namelen);
> > +		args->value = &name_value[attr->xattri_name_len];
> > +		args->valuelen = attr->xattri_value_len;
> > +		args->op_flags = XFS_DA_OP_OKNOENT;
> > +		args->total = xfs_attr_calc_size(args, &local);
> > +		attr->xattri_leaf_bp = NULL;
> > +	}
> >   
> > -	args.hashval = xfs_da_hashname(args.name, args.namelen);
> > -	args.value = &name_value[attr->xattri_name_len];
> > -	args.valuelen = attr->xattri_value_len;
> > -	args.op_flags = XFS_DA_OP_OKNOENT;
> > -	args.total = xfs_attr_calc_size(&args, &local);
> > -	args.trans = tp;
> > +	/*
> > +	 * Always reset trans after EAGAIN cycle
> > +	 * since the transaction is new
> > +	 */
> > +	args->trans = tp;
> >   
> > -	error = xfs_trans_attr(&args, done_item,
> > -			attr->xattri_op_flags);
> > +	error = xfs_trans_attr(args, done_item, &attr->xattri_leaf_bp,
> > +			&attr->xattri_state, attr->xattri_op_flags);
> >   out:
> > -	kmem_free(attr);
> > +	if (error != -EAGAIN)
> > +		kmem_free(attr);
> > +
> >   	return error;
> >   }
> >   
> > --
> > 2.7.4