Re: [PATCH 01/17] Add helper functions xfs_attr_set_args and xfs_attr_remove_args

[Allison Henderson <allison.henderson@xxxxxxxxxx>]

On 10/19/2017 01:03 PM, Darrick J. Wong wrote:

> On Wed, Oct 18, 2017 at 03:55:17PM -0700, Allison Henderson wrote:
> > These sub-routines set or remove the attributes specified in
> > @args. We will use this later for setting parent pointers as a
> > deferred attribute operation.
> >
> > Signed-off-by: Allison Henderson<allison.henderson@xxxxxxxxxx>
> > ---
> >   fs/xfs/libxfs/xfs_attr.c | 322 +++++++++++++++++++++++++++--------------------
> >   fs/xfs/xfs_attr.h        |   2 +
> >   2 files changed, 189 insertions(+), 135 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 6249c92..b00ec1f 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -203,6 +203,185 @@ xfs_attr_calc_size(
> >   	return nblks;
> >   }
> >   
> > +/*
> > + * set the attribute specified in @args. In the case of the parent attribute
> > + * being set, we do not want to roll the transaction on shortform-to-leaf
> > + * conversion, as the attribute must be added in the same transaction as the
> > + * parent directory modifications. Hence @roll_trans needs to be set
> > + * appropriately to control whether the transaction is committed during this
> > + * function.
> Hmm... shouldn't the deferred attribute set code take care of all the
> conversions/attr fork expansions/whatever is necessary to cram in the
> parent pointer?  So in theory all the parent pointer updates should look
> like this:
>
> xfs_defer_init(&dfops...);
> xfs_some_name_creating_operation(tp, ...);
>
> if (hasparent) {
> 	xfs_parent_set(tp, ip);
> }
> xfs_defer_finish(&tp, &dfops);
> xfs_trans_commit(tp);
>
> xfs_parent_set() would then be:
>
> if (fits in inode) {
> 	xfs_attr_set_sf(ip, key, value);
> 	xfs_log_attr_area();
> 	return;
> }
>
> xfs_attr_do_conversions_if_needed(dfops);
> xfs_attr_set(dfops);
>
> ...and if there's a really good reason to try to cram things in, we can
> add those later?  As I scanned the series, that was what kept coming up
> in my head -- just tell the xfs_parent.c code to set a parent pointer;
> it can figure out if there's sufficient space to put it directly into
> the inode and log that, or we need something else that actually requires
> the deferred ops mechanism then do that.
>
> (Maybe I'm just fixating on xfs_parent_create...)

Ok, I think may have caused some confusion by not revising the comments
for these functions.  xfs_attr_remove_args and xfs_attr_set_args are
sort of subsets of code that came out of xfs_attr_remove and xfs_attr_set.
In the first revision of the set, the link/unlink/rename operations called
them directly to set up the parent pointers.  Now these two functions
are called through xfs_trans_attr.

But you've still made very good points because I forgot that xfs_create
path uses xfs_parent_create not xfs_parent_add.  So I think the pseudo
code you are proposing makes sense.  I will see if I can revise the
create path a bit.  Thanks for the catch!
> Higher level questions about robustness: if we try to set a parent
> ptr and the attr name already exists, do we error out?  If we try to
> remove a parent ptr and there's no attr name, do we error out?
Erroring out seems logical sense these these would not be normal
conditions?

> I think
> you've enough here to start thinking what changes need to be made to
> xfs_repair to validate & fix the parent pointers, and what test cases
> (and possibly ioctls) will need to be constructed to verify that we
> actually get the parent pointers we want.
>
> --D
Right, I will put together some basic add/remove/rename tests to
add to xfstests.

Thank you for the very thorough review!!

> > + */
> > +int
> > +xfs_attr_set_args(
> > +	struct xfs_da_args	*args,
> > +	int			flags,
> > +	bool			roll_trans)
> > +{
> > +	struct xfs_inode	*dp = args->dp;
> > +	struct xfs_mount        *mp = dp->i_mount;
> > +	struct xfs_trans_res    tres;
> > +	int			rsvd = 0;
> > +	int			error = 0;
> > +
> > +	tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
> > +			 M_RES(mp)->tr_attrsetrt.tr_logres * args->total;
> > +	tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> > +	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> > +
> > +	/*
> > +	 * Root fork attributes can use reserved data blocks for this
> > +	 * operation if necessary
> > +	 */
> > +	error = xfs_trans_alloc(mp, &tres, args->total, 0,
> > +				rsvd ? XFS_TRANS_RESERVE : 0, &args->trans);
> > +	if (error)
> > +		goto out;
> > +
> > +	error = xfs_trans_reserve_quota_nblks(args->trans, dp, args->total, 0,
> > +					      rsvd ? XFS_QMOPT_RES_REGBLKS |
> > +						     XFS_QMOPT_FORCE_RES :
> > +						     XFS_QMOPT_RES_REGBLKS);
> > +	if (error)
> > +		goto out;
> > +
> > +	xfs_trans_ijoin(args->trans, dp, 0);
> > +	/*
> > +	 * If the attribute list is non-existent or a shortform list,
> > +	 * upgrade it to a single-leaf-block attribute list.
> > +	 */
> > +	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).
> > +		 */
> > +		if (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS)
> > +			xfs_attr_shortform_create(args);
> > +
> > +		/*
> > +		 * Try to add the attr to the attribute list in the inode.
> > +		 */
> > +		error = xfs_attr_shortform_addname(args);
> > +		if (error != -ENOSPC) {
> > +			ASSERT(args->trans);
> > +			if (!error && (flags & ATTR_KERNOTIME) == 0)
> > +				xfs_trans_ichgtime(args->trans, dp,
> > +						   XFS_ICHGTIME_CHG);
> > +			goto out;
> > +		}
> > +
> > +		/*
> > +		 * It won't fit in the shortform, transform to a leaf block.
> > +		 * GROT: another possible req'mt for a double-split btree op.
> > +		 */
> > +		error = xfs_attr_shortform_to_leaf(args);
> > +		if (error)
> > +			goto out;
> > +		xfs_defer_ijoin(args->dfops, dp);
> > +		if (roll_trans) {
> > +			error = xfs_defer_finish(&args->trans, args->dfops);
> > +			if (error) {
> > +				args->trans = NULL;
> > +				goto out;
> > +			}
> > +
> > +			/*
> > +			 * Commit the leaf transformation.  We'll need another
> > +			 * (linked) transaction to add the new attribute to the
> > +			 * leaf.
> > +			 */
> > +			error = xfs_trans_roll_inode(&args->trans, dp);
> > +			if (error)
> > +				goto out;
> > +		}
> > +	}
> > +
> > +	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> > +		error = xfs_attr_leaf_addname(args);
> > +	else
> > +		error = xfs_attr_node_addname(args);
> > +	if (error)
> > +		goto out;
> > +
> > +	if ((flags & ATTR_KERNOTIME) == 0)
> > +		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
> > +
> > +	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE);
> > +out:
> > +	return error;
> > +}
> > +
> > +/*
> > + * Remove the attribute specified in @args.
> > + */
> > +int
> > +xfs_attr_remove_args(
> > +	struct xfs_da_args      *args,
> > +	int			flags)
> > +{
> > +	struct xfs_inode	*dp = args->dp;
> > +	struct xfs_mount	*mp = dp->i_mount;
> > +	int			error;
> > +	int                     rsvd = 0;
> > +
> > +	error = xfs_qm_dqattach_locked(dp, 0);
> > +	if (error)
> > +		return error;
> > +
> > +	/*
> > +	 * Root fork attributes can use reserved data blocks for this
> > +	 * operation if necessary
> > +	 */
> > +	if (flags & ATTR_ROOT)
> > +		rsvd = XFS_TRANS_RESERVE;
> > +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_attrrm,
> > +		XFS_ATTRRM_SPACE_RES(mp), 0, rsvd, &args->trans);
> > +
> > +	if (error)
> > +		goto out;
> > +
> > +	/*
> > +	 * No need to make quota reservations here. We expect to release some
> > +	 * blocks not allocate in the common case.
> > +	 */
> > +	xfs_trans_ijoin(args->trans, dp, 0);
> > +
> > +	if (!xfs_inode_hasattr(dp)) {
> > +		error = -ENOATTR;
> > +	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
> > +		ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
> > +		error = xfs_attr_shortform_remove(args);
> > +	} else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> > +		error = xfs_attr_leaf_removename(args);
> > +	} else {
> > +		error = xfs_attr_node_removename(args);
> > +	}
> > +
> > +	if (error)
> > +		goto out;
> > +
> > +	/*
> > +	 * If this is a synchronous mount, make sure that the
> > +	 * transaction goes to disk before returning to the user.
> > +	 */
> > +	if (mp->m_flags & XFS_MOUNT_WSYNC)
> > +		xfs_trans_set_sync(args->trans);
> > +
> > +	if ((flags & ATTR_KERNOTIME) == 0)
> > +		xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
> > +
> > +	xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE);
> > +
> > +	return error;
> > +
> > +out:
> > +	if (args->trans)
> > +		xfs_trans_cancel(args->trans);
> > +
> > +	return error;
> > +}
> > +
> >   int
> >   xfs_attr_set(
> >   	struct xfs_inode	*dp,
> > @@ -214,10 +393,9 @@ xfs_attr_set(
> >   	struct xfs_mount	*mp = dp->i_mount;
> >   	struct xfs_da_args	args;
> >   	struct xfs_defer_ops	dfops;
> > -	struct xfs_trans_res	tres;
> >   	xfs_fsblock_t		firstblock;
> >   	int			rsvd = (flags & ATTR_ROOT) != 0;
> > -	int			error, err2, local;
> > +	int			error, local;
> >   
> >   	XFS_STATS_INC(mp, xs_attr_set);
> >   
> > @@ -252,106 +430,11 @@ xfs_attr_set(
> >   			return error;
> >   	}
> >   
> > -	tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres +
> > -			 M_RES(mp)->tr_attrsetrt.tr_logres * args.total;
> > -	tres.tr_logcount = XFS_ATTRSET_LOG_COUNT;
> > -	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> > -
> > -	/*
> > -	 * Root fork attributes can use reserved data blocks for this
> > -	 * operation if necessary
> > -	 */
> > -	error = xfs_trans_alloc(mp, &tres, args.total, 0,
> > -			rsvd ? XFS_TRANS_RESERVE : 0, &args.trans);
> > -	if (error)
> > -		return error;
> > -
> >   	xfs_ilock(dp, XFS_ILOCK_EXCL);
> > -	error = xfs_trans_reserve_quota_nblks(args.trans, dp, args.total, 0,
> > -				rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES :
> > -				       XFS_QMOPT_RES_REGBLKS);
> > -	if (error) {
> > -		xfs_iunlock(dp, XFS_ILOCK_EXCL);
> > -		xfs_trans_cancel(args.trans);
> > -		return error;
> > -	}
> > -
> > -	xfs_trans_ijoin(args.trans, dp, 0);
> > -
> > -	/*
> > -	 * If the attribute list is non-existent or a shortform list,
> > -	 * upgrade it to a single-leaf-block attribute list.
> > -	 */
> > -	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).
> > -		 */
> > -		if (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS)
> > -			xfs_attr_shortform_create(&args);
> > -
> > -		/*
> > -		 * Try to add the attr to the attribute list in
> > -		 * the inode.
> > -		 */
> > -		error = xfs_attr_shortform_addname(&args);
> > -		if (error != -ENOSPC) {
> > -			/*
> > -			 * Commit the shortform mods, and we're done.
> > -			 * NOTE: this is also the error path (EEXIST, etc).
> > -			 */
> > -			ASSERT(args.trans != NULL);
> > -
> > -			/*
> > -			 * If this is a synchronous mount, make sure that
> > -			 * the transaction goes to disk before returning
> > -			 * to the user.
> > -			 */
> > -			if (mp->m_flags & XFS_MOUNT_WSYNC)
> > -				xfs_trans_set_sync(args.trans);
> > -
> > -			if (!error && (flags & ATTR_KERNOTIME) == 0) {
> > -				xfs_trans_ichgtime(args.trans, dp,
> > -							XFS_ICHGTIME_CHG);
> > -			}
> > -			err2 = xfs_trans_commit(args.trans);
> > -			xfs_iunlock(dp, XFS_ILOCK_EXCL);
> > -
> > -			return error ? error : err2;
> > -		}
> > -
> > -		/*
> > -		 * It won't fit in the shortform, transform to a leaf block.
> > -		 * GROT: another possible req'mt for a double-split btree op.
> > -		 */
> > -		xfs_defer_init(args.dfops, args.firstblock);
> > -		error = xfs_attr_shortform_to_leaf(&args);
> > -		if (error)
> > -			goto out_defer_cancel;
> > -		xfs_defer_ijoin(args.dfops, dp);
> > -		error = xfs_defer_finish(&args.trans, args.dfops);
> > -		if (error)
> > -			goto out_defer_cancel;
> > -
> > -		/*
> > -		 * Commit the leaf transformation.  We'll need another (linked)
> > -		 * transaction to add the new attribute to the leaf.
> > -		 */
> > -
> > -		error = xfs_trans_roll_inode(&args.trans, dp);
> > -		if (error)
> > -			goto out;
> > -
> > -	}
> > -
> > -	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> > -		error = xfs_attr_leaf_addname(&args);
> > -	else
> > -		error = xfs_attr_node_addname(&args);
> > +	xfs_defer_init(args.dfops, args.firstblock);
> > +	error = xfs_attr_set_args(&args, flags, true);
> >   	if (error)
> > -		goto out;
> > +		goto out_defer_cancel;
> >   
> >   	/*
> >   	 * If this is a synchronous mount, make sure that the
> > @@ -360,9 +443,6 @@ xfs_attr_set(
> >   	if (mp->m_flags & XFS_MOUNT_WSYNC)
> >   		xfs_trans_set_sync(args.trans);
> >   
> > -	if ((flags & ATTR_KERNOTIME) == 0)
> > -		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
> > -
> >   	/*
> >   	 * Commit the last in the sequence of transactions.
> >   	 */
> > @@ -374,10 +454,6 @@ xfs_attr_set(
> >   
> >   out_defer_cancel:
> >   	xfs_defer_cancel(&dfops);
> > -	args.trans = NULL;
> > -out:
> > -	if (args.trans)
> > -		xfs_trans_cancel(args.trans);
> >   	xfs_iunlock(dp, XFS_ILOCK_EXCL);
> >   	return error;
> >   }
> > @@ -417,38 +493,15 @@ xfs_attr_remove(
> >   	 */
> >   	args.op_flags = XFS_DA_OP_OKNOENT;
> >   
> > -	error = xfs_qm_dqattach(dp, 0);
> > -	if (error)
> > -		return error;
> > -
> > -	/*
> > -	 * Root fork attributes can use reserved data blocks for this
> > -	 * operation if necessary
> > -	 */
> > -	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_attrrm,
> > -			XFS_ATTRRM_SPACE_RES(mp), 0,
> > -			(flags & ATTR_ROOT) ? XFS_TRANS_RESERVE : 0,
> > -			&args.trans);
> > -	if (error)
> > -		return error;
> > -
> >   	xfs_ilock(dp, XFS_ILOCK_EXCL);
> >   	/*
> >   	 * No need to make quota reservations here. We expect to release some
> >   	 * blocks not allocate in the common case.
> >   	 */
> >   	xfs_trans_ijoin(args.trans, dp, 0);
> > +	xfs_defer_init(args.dfops, args.firstblock);
> >   
> > -	if (!xfs_inode_hasattr(dp)) {
> > -		error = -ENOATTR;
> > -	} else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
> > -		ASSERT(dp->i_afp->if_flags & XFS_IFINLINE);
> > -		error = xfs_attr_shortform_remove(&args);
> > -	} else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> > -		error = xfs_attr_leaf_removename(&args);
> > -	} else {
> > -		error = xfs_attr_node_removename(&args);
> > -	}
> > +	error = xfs_attr_remove_args(&args, flags);
> >   
> >   	if (error)
> >   		goto out;
> > @@ -460,9 +513,6 @@ xfs_attr_remove(
> >   	if (mp->m_flags & XFS_MOUNT_WSYNC)
> >   		xfs_trans_set_sync(args.trans);
> >   
> > -	if ((flags & ATTR_KERNOTIME) == 0)
> > -		xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG);
> > -
> >   	/*
> >   	 * Commit the last in the sequence of transactions.
> >   	 */
> > @@ -473,6 +523,8 @@ xfs_attr_remove(
> >   	return error;
> >   
> >   out:
> > +	xfs_defer_cancel(&dfops);
> > +
> >   	if (args.trans)
> >   		xfs_trans_cancel(args.trans);
> >   	xfs_iunlock(dp, XFS_ILOCK_EXCL);
> > diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h
> > index 5d5a5e2..8542606 100644
> > --- a/fs/xfs/xfs_attr.h
> > +++ b/fs/xfs/xfs_attr.h
> > @@ -149,7 +149,9 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name,
> >   		 unsigned char *value, int *valuelenp, int flags);
> >   int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name,
> >   		 unsigned char *value, int valuelen, int flags);
> > +int xfs_attr_set_args(struct xfs_da_args *args, int flags, bool roll_trans);
> >   int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags);
> > +int xfs_attr_remove_args(struct xfs_da_args *args, int flags);
> >   int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize,
> >   		  int flags, struct attrlist_cursor_kern *cursor);
> >   
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message tomajordomo@xxxxxxxxxxxxxxx
> > More majordomo info athttps://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=XFp4B05bcXkJ0dhYaFjd3F8telP01COkBp9cI7mKLb4&m=aicitOc3doix8VyB_l-FSSD7nPvIu7TIiw0VVfKnBhc&s=JKZ0ozAg9jwyNpJK-9-53aT_kpKIedFceJDGJpDQh3U&e=  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message tomajordomo@xxxxxxxxxxxxxxx
> More majordomo info athttps://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=XFp4B05bcXkJ0dhYaFjd3F8telP01COkBp9cI7mKLb4&m=aicitOc3doix8VyB_l-FSSD7nPvIu7TIiw0VVfKnBhc&s=JKZ0ozAg9jwyNpJK-9-53aT_kpKIedFceJDGJpDQh3U&e=  

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html