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

[Allison Henderson <allison.henderson@oracle.com>]

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@oracle.com>
>> ---
>>   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@vger.kernel.org
>> 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@vger.kernel.org
> 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=