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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux