attr cleanups

Plus various minor style fixes to the new xfs_attr_set.

Signed-off-by Christoph Hellwig <[email protected]>

---

 fs/xfs/xfs_attr.c |  110 +++++++++++++++++++++--------------------------------
 1 file changed, 44 insertions(+), 66 deletions(-)

fs/xfs/xfs_attr.c +44 -66

index 01b6a01..eb3ae8f 100644
@@ -221,26 +221,32 @@ xfs_attr_calc_size(
 	return nblks;
 }
 
-STATIC int
-xfs_attr_set_int(
-	struct xfs_inode *dp,
-	struct xfs_name	*name,
-	unsigned char	*value,
-	int		valuelen,
-	int		flags)
+int
+xfs_attr_set(
+	struct xfs_inode	*dp,
+	const unsigned char	*name,
+	unsigned char		*value,
+	int			valuelen,
+	int			flags)
 {
-	xfs_da_args_t		args;
-	xfs_fsblock_t		firstblock;
-	xfs_bmap_free_t		flist;
-	int			error, err2, committed;
 	struct xfs_mount	*mp = dp->i_mount;
+	struct xfs_da_args	args;
+	struct xfs_bmap_free	flist;
 	struct xfs_trans_res	tres;
+	struct xfs_name		xname;
+	xfs_fsblock_t		firstblock;
 	int			rsvd = (flags & ATTR_ROOT) != 0;
-	int			local;
+	int			error, err2, committed, local;
+
+	XFS_STATS_INC(xs_attr_set);
+
+	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
+		return EIO;
+
+	error = xfs_attr_name_to_xname(&xname, name);
+	if (error)
+		return error;
 
-	/*
-	 * Attach the dquots to the inode.
-	 */
 	error = xfs_qm_dqattach(dp, 0);
 	if (error)
 		return error;

@@ -251,18 +257,16 @@ xfs_attr_set_int(
 	 */
 	if (XFS_IFORK_Q(dp) == 0) {
 		int sf_size = sizeof(xfs_attr_sf_hdr_t) +
-			      XFS_ATTR_SF_ENTSIZE_BYNAME(name->len, valuelen);
+			      XFS_ATTR_SF_ENTSIZE_BYNAME(xname.len, valuelen);
 
-		if ((error = xfs_bmap_add_attrfork(dp, sf_size, rsvd)))
-			return(error);
+		error = xfs_bmap_add_attrfork(dp, sf_size, rsvd);
+		if (error)
+			return error;
 	}
 
-	/*
-	 * Fill in the arg structure for this request.
-	 */
-	memset((char *)&args, 0, sizeof(args));
-	args.name = name->name;
-	args.namelen = name->len;
+	memset(&args, 0, sizeof(args));
+	args.name = xname.name;
+	args.namelen = xname.len;
 	args.value = value;
 	args.valuelen = valuelen;
 	args.flags = flags;

@@ -274,7 +278,7 @@ xfs_attr_set_int(
 	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
 
 	/* Size is now blocks for attribute data */
-	args.total = xfs_attr_calc_size(dp, name->len, valuelen, &local);
+	args.total = xfs_attr_calc_size(dp, xname.len, valuelen, &local);
 
 	/*
 	 * Start our first transaction of the day.

@@ -303,7 +307,7 @@ xfs_attr_set_int(
 	error = xfs_trans_reserve(args.trans, &tres, args.total, 0);
 	if (error) {
 		xfs_trans_cancel(args.trans, 0);
-		return(error);
+		return error;
 	}
 	xfs_ilock(dp, XFS_ILOCK_EXCL);
 

@@ -313,7 +317,7 @@ xfs_attr_set_int(
 	if (error) {
 		xfs_iunlock(dp, XFS_ILOCK_EXCL);
 		xfs_trans_cancel(args.trans, XFS_TRANS_RELEASE_LOG_RES);
-		return (error);
+		return error;
 	}
 
 	xfs_trans_ijoin(args.trans, dp, 0);

@@ -322,9 +326,9 @@ xfs_attr_set_int(
 	 * 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))) {
+	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).

@@ -349,9 +353,8 @@ xfs_attr_set_int(
 			 * the transaction goes to disk before returning
 			 * to the user.
 			 */
-			if (mp->m_flags & XFS_MOUNT_WSYNC) {
+			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,

@@ -361,7 +364,7 @@ xfs_attr_set_int(
 						 XFS_TRANS_RELEASE_LOG_RES);
 			xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
-			return(error == 0 ? err2 : error);
+			return error ? error : err2;
 		}
 
 		/*

@@ -399,22 +402,19 @@ xfs_attr_set_int(
 
 	}
 
-	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
+	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
 		error = xfs_attr_leaf_addname(&args);
-	} else {
+	else
 		error = xfs_attr_node_addname(&args);
-	}
-	if (error) {
+	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) {
+	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);

@@ -426,37 +426,15 @@ xfs_attr_set_int(
 	error = xfs_trans_commit(args.trans, XFS_TRANS_RELEASE_LOG_RES);
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
-	return(error);
+	return error;
 
 out:
-	if (args.trans)
+	if (args.trans) {
 		xfs_trans_cancel(args.trans,
 			XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
+	}
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
-	return(error);
-}
-
-int
-xfs_attr_set(
-	xfs_inode_t	*dp,
-	const unsigned char *name,
-	unsigned char	*value,
-	int		valuelen,
-	int		flags)
-{
-	int             error;
-	struct xfs_name	xname;
-
-	XFS_STATS_INC(xs_attr_set);
-
-	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
-		return (EIO);
-
-	error = xfs_attr_name_to_xname(&xname, name);
-	if (error)
-		return error;
-
-	return xfs_attr_set_int(dp, &xname, value, valuelen, flags);
+	return error;
 }
 
 /*

Signature · 6 lines

1.7.10.4

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

This allows doing an unlocked check if an attr for is present at all and slightly reduce the lock hold time if we actually do an attr get.

Plus various minor style fixes to the new xfs_attr_get.

Signed-off-by Christoph Hellwig <[email protected]>

---

 fs/xfs/xfs_attr.c |   79 ++++++++++++++++++-----------------------------------
 1 file changed, 27 insertions(+), 52 deletions(-)

fs/xfs/xfs_attr.c +27 -52

index eb3ae8f..01f2267 100644
@@ -106,26 +106,34 @@ xfs_inode_hasattr(
  * Overall external interface routines.
  *========================================================================*/
 
-STATIC int
-xfs_attr_get_int(
+int
+xfs_attr_get(
 	struct xfs_inode	*ip,
-	struct xfs_name		*name,
+	const unsigned char	*name,
 	unsigned char		*value,
 	int			*valuelenp,
 	int			flags)
 {
-	xfs_da_args_t   args;
-	int             error;
+	struct xfs_da_args	args;
+	struct xfs_name		xname;
+	uint			lock_mode;
+	int			error;
+
+	XFS_STATS_INC(xs_attr_get);
+
+	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
+		return EIO;
 
 	if (!xfs_inode_hasattr(ip))
 		return ENOATTR;
 
-	/*
-	 * Fill in the arg structure for this request.
-	 */
-	memset((char *)&args, 0, sizeof(args));
-	args.name = name->name;
-	args.namelen = name->len;
+	error = xfs_attr_name_to_xname(&xname, name);
+	if (error)
+		return error;
+
+	memset(&args, 0, sizeof(args));
+	args.name = xname.name;
+	args.namelen = xname.len;
 	args.value = value;
 	args.valuelen = *valuelenp;
 	args.flags = flags;

@@ -133,52 +141,19 @@ xfs_attr_get_int(
 	args.dp = ip;
 	args.whichfork = XFS_ATTR_FORK;
 
-	/*
-	 * Decide on what work routines to call based on the inode size.
-	 */
-	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
+	lock_mode = xfs_ilock_attr_map_shared(ip);
+	if (!xfs_inode_hasattr(ip))
+		error = ENOATTR;
+	else if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL)
 		error = xfs_attr_shortform_getvalue(&args);
-	} else if (xfs_bmap_one_block(ip, XFS_ATTR_FORK)) {
+	else if (xfs_bmap_one_block(ip, XFS_ATTR_FORK))
 		error = xfs_attr_leaf_get(&args);
-	} else {
+	else
 		error = xfs_attr_node_get(&args);
-	}
+	xfs_iunlock(ip, lock_mode);
 
-	/*
-	 * Return the number of bytes in the value to the caller.
-	 */
 	*valuelenp = args.valuelen;
-
-	if (error == EEXIST)
-		error = 0;
-	return(error);
-}
-
-int
-xfs_attr_get(
-	xfs_inode_t	*ip,
-	const unsigned char *name,
-	unsigned char	*value,
-	int		*valuelenp,
-	int		flags)
-{
-	int		error;
-	struct xfs_name	xname;
-	uint		lock_mode;
-
-	XFS_STATS_INC(xs_attr_get);
-
-	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
-		return(EIO);
-
-	error = xfs_attr_name_to_xname(&xname, name);
-	if (error)
-		return error;
-
-	lock_mode = xfs_ilock_attr_map_shared(ip);
-	error = xfs_attr_get_int(ip, &xname, value, valuelenp, flags);
-	xfs_iunlock(ip, lock_mode);
-	return(error);
+	return error == EEXIST ? 0 : error;
 }
 
 /*

Signature · 6 lines

1.7.10.4

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Also remove a useless ilock roundtrip for the first attr fork check, it's racy anyway and we redo it later under the ilock before we start the removal.

Plus various minor style fixes to the new xfs_attr_remove.

Signed-off-by Christoph Hellwig <[email protected]>

---
 fs/xfs/xfs_attr.c |   97 +++++++++++++++++++----------------------------------
 1 file changed, 35 insertions(+), 62 deletions(-)

fs/xfs/xfs_attr.c +27 -17

index 01f2267..a96e27b 100644
@@ -416,21 +416,34 @@ out:
  * Generic handler routine to remove a name from an attribute list.
  * Transitions attribute list from Btree to shortform as necessary.
  */
-STATIC int
-xfs_attr_remove_int(xfs_inode_t *dp, struct xfs_name *name, int flags)
+int
+xfs_attr_remove(
+	struct xfs_inode	*dp,
+	const unsigned char	*name,
+	int			flags)
 {
-	xfs_da_args_t	args;
-	xfs_fsblock_t	firstblock;
-	xfs_bmap_free_t	flist;
-	int		error;
-	xfs_mount_t	*mp = dp->i_mount;
+	struct xfs_mount	*mp = dp->i_mount;
+	struct xfs_da_args	args;
+	struct xfs_bmap_free	flist;
+	struct xfs_name		xname;
+	xfs_fsblock_t		firstblock;
+	int			error;
 
-	/*
-	 * Fill in the arg structure for this request.
-	 */
-	memset((char *)&args, 0, sizeof(args));
-	args.name = name->name;
-	args.namelen = name->len;
+	XFS_STATS_INC(xs_attr_remove);
+
+	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
+		return EIO;
+
+	if (!xfs_inode_hasattr(dp))
+		return ENOATTR;
+
+	error = xfs_attr_name_to_xname(&xname, name);
+	if (error)
+		return error;
+
+	memset(&args, 0, sizeof(args));
+	args.name = xname.name;
+	args.namelen = xname.len;
 	args.flags = flags;
 	args.hashval = xfs_da_hashname(args.name, args.namelen);
 	args.dp = dp;

@@ -446,9 +459,6 @@ xfs_attr_remove_int(xfs_inode_t *dp, struct xfs_name *name, int flags)
 	 */
 	args.op_flags = XFS_DA_OP_OKNOENT;
 
-	/*
-	 * Attach the dquots to the inode.
-	 */
 	error = xfs_qm_dqattach(dp, 0);
 	if (error)
 		return error;

@@ -477,7 +487,7 @@ xfs_attr_remove_int(xfs_inode_t *dp, struct xfs_name *name, int flags)
 				  XFS_ATTRRM_SPACE_RES(mp), 0);
 	if (error) {
 		xfs_trans_cancel(args.trans, 0);
-		return(error);
+		return error;
 	}
 
 	xfs_ilock(dp, XFS_ILOCK_EXCL);

fs/xfs/xfs_attr.c +8 -42

-		goto out;
-	}
-	if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
+	} 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);
-		if (error) {
-			goto out;
-		}
 	} 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) {
+
+	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) {
+	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);

@@ -527,45 +528,17 @@ xfs_attr_remove_int(xfs_inode_t *dp, struct xfs_name *name, int flags)
 	error = xfs_trans_commit(args.trans, XFS_TRANS_RELEASE_LOG_RES);
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
 
-	return(error);
+	return error;
 
 out:
-	if (args.trans)
+	if (args.trans) {
 		xfs_trans_cancel(args.trans,
 			XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_ABORT);
-	xfs_iunlock(dp, XFS_ILOCK_EXCL);
-	return(error);
-}
-
-int
-xfs_attr_remove(
-	xfs_inode_t	*dp,
-	const unsigned char *name,
-	int		flags)
-{
-	int		error;
-	struct xfs_name	xname;
-
-	XFS_STATS_INC(xs_attr_remove);
-
-	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
-		return (EIO);
-
-	error = xfs_attr_name_to_xname(&xname, name);
-	if (error)
-		return error;
-
-	xfs_ilock(dp, XFS_ILOCK_SHARED);
-	if (!xfs_inode_hasattr(dp)) {
-		xfs_iunlock(dp, XFS_ILOCK_SHARED);
-		return XFS_ERROR(ENOATTR);
 	}
-	xfs_iunlock(dp, XFS_ILOCK_SHARED);
-
-	return xfs_attr_remove_int(dp, &xname, flags);
+	xfs_iunlock(dp, XFS_ILOCK_EXCL);
+	return error;
 }
 
-
 /*========================================================================
  * External routines when attribute list is inside the inode
  *========================================================================*/

Signature · 6 lines

1.7.10.4

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 6 lines · Christoph Hellwig

Also remove a useless ilock roundtrip for the first attr fork check, it's racy anyway and we redo it later under the ilock before we start the removal.

Plus various minor style fixes to the new xfs_attr_remove.

Signed-off-by Christoph Hellwig <[email protected]>

Quoted 4 lines

fs/xfs/xfs_attr.c +0 -0

@@ -487,35 +497,26 @@ xfs_attr_remove_int(xfs_inode_t *dp, struct xfs_name *name, int flags)
 	 */
 	xfs_trans_ijoin(args.trans, dp, 0);
 

fs/xfs/xfs_attr.c xfs_attr_remove_int(xfs_inode_t *dp, struct xfs_name *name, int flags) · line 501

Quoted 5 lines

fs/xfs/xfs_attr.c +0 -3

-	/*
-	 * Decide on what work routines to call based on the inode size.
-	 */
 	if (!xfs_inode_hasattr(dp)) {
 		error = XFS_ERROR(ENOATTR);

I suppose we probably want to nuke the XFS_ERROR() while we're here..? Otherwise, it looks good.

Quoted 7 lines · Brian Foster

Quoted 6 lines · Christoph Hellwig

Also remove a useless ilock roundtrip for the first attr fork check, it's racy anyway and we redo it later under the ilock before we start the removal.

Plus various minor style fixes to the new xfs_attr_remove.

Signed-off-by Christoph Hellwig <[email protected]>

.....

Quoted 4 lines

Quoted 4 lines

fs/xfs/xfs_attr.c +0 -0

@@ -487,35 +497,26 @@ xfs_attr_remove_int(xfs_inode_t *dp, struct xfs_name *name, int flags)
 	 */
 	xfs_trans_ijoin(args.trans, dp, 0);
 

Quoted 8 lines

fs/xfs/xfs_attr.c xfs_attr_remove_int(xfs_inode_t *dp, struct xfs_name *name, int flags) · line 501

Quoted 5 lines

fs/xfs/xfs_attr.c +0 -3

-	/*
-	 * Decide on what work routines to call based on the inode size.
-	 */
 	if (!xfs_inode_hasattr(dp)) {
 		error = XFS_ERROR(ENOATTR);

I suppose we probably want to nuke the XFS_ERROR() while we're here..? Otherwise, it looks good.

No need, I'll do that at the end of the dev cycle for everything.

Cheers,

Dave.

Signature · 7 lines

Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Replace xfs_attr_name_to_xname with a new xfs_attr_args_init helper that sets up the basic da_args structure without using a temporary xfs_name structure.

Signed-off-by Christoph Hellwig <[email protected]>

---

 fs/xfs/xfs_attr.c |   74 +++++++++++++++++++++--------------------------------
 1 file changed, 29 insertions(+), 45 deletions(-)

fs/xfs/xfs_attr.c +29 -45

index a96e27b..1208621 100644
@@ -77,17 +77,26 @@ STATIC int xfs_attr_refillstate(xfs_da_state_t *state);
 
 
 STATIC int
-xfs_attr_name_to_xname(
-	struct xfs_name	*xname,
-	const unsigned char *aname)
+xfs_attr_args_init(
+	struct xfs_da_args	*args,
+	struct xfs_inode	*dp,
+	const unsigned char	*name,
+	int			flags)
 {
-	if (!aname)
+
+	if (!name)
 		return EINVAL;
-	xname->name = aname;
-	xname->len = strlen((char *)aname);
-	if (xname->len >= MAXNAMELEN)
+
+	memset(args, 0, sizeof(*args));
+	args->whichfork = XFS_ATTR_FORK;
+	args->dp = dp;
+	args->flags = flags;
+	args->name = name;
+	args->namelen = strlen((const char *)name);
+	if (args->namelen >= MAXNAMELEN)
 		return EFAULT;		/* match IRIX behaviour */
 
+	args->hashval = xfs_da_hashname(args->name, args->namelen);
 	return 0;
 }
 

@@ -115,7 +124,6 @@ xfs_attr_get(
 	int			flags)
 {
 	struct xfs_da_args	args;
-	struct xfs_name		xname;
 	uint			lock_mode;
 	int			error;
 

@@ -127,19 +135,12 @@ xfs_attr_get(
 	if (!xfs_inode_hasattr(ip))
 		return ENOATTR;
 
-	error = xfs_attr_name_to_xname(&xname, name);
+	error = xfs_attr_args_init(&args, ip, name, flags);
 	if (error)
 		return error;
 
-	memset(&args, 0, sizeof(args));
-	args.name = xname.name;
-	args.namelen = xname.len;
 	args.value = value;
 	args.valuelen = *valuelenp;
-	args.flags = flags;
-	args.hashval = xfs_da_hashname(args.name, args.namelen);
-	args.dp = ip;
-	args.whichfork = XFS_ATTR_FORK;
 
 	lock_mode = xfs_ilock_attr_map_shared(ip);
 	if (!xfs_inode_hasattr(ip))

@@ -208,7 +209,6 @@ xfs_attr_set(
 	struct xfs_da_args	args;
 	struct xfs_bmap_free	flist;
 	struct xfs_trans_res	tres;
-	struct xfs_name		xname;
 	xfs_fsblock_t		firstblock;
 	int			rsvd = (flags & ATTR_ROOT) != 0;
 	int			error, err2, committed, local;

@@ -218,10 +218,19 @@ xfs_attr_set(
 	if (XFS_FORCED_SHUTDOWN(dp->i_mount))
 		return EIO;
 
-	error = xfs_attr_name_to_xname(&xname, name);
+	error = xfs_attr_args_init(&args, dp, name, flags);
 	if (error)
 		return error;
 
+	args.value = value;
+	args.valuelen = valuelen;
+	args.firstblock = &firstblock;
+	args.flist = &flist;
+	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
+
+	/* Size is now blocks for attribute data */
+	args.total = xfs_attr_calc_size(dp, args.namelen, valuelen, &local);
+
 	error = xfs_qm_dqattach(dp, 0);
 	if (error)
 		return error;

@@ -232,29 +241,13 @@ xfs_attr_set(
 	 */
 	if (XFS_IFORK_Q(dp) == 0) {
 		int sf_size = sizeof(xfs_attr_sf_hdr_t) +
-			      XFS_ATTR_SF_ENTSIZE_BYNAME(xname.len, valuelen);
+			XFS_ATTR_SF_ENTSIZE_BYNAME(args.namelen, valuelen);
 
 		error = xfs_bmap_add_attrfork(dp, sf_size, rsvd);
 		if (error)
 			return error;
 	}
 
-	memset(&args, 0, sizeof(args));
-	args.name = xname.name;
-	args.namelen = xname.len;
-	args.value = value;
-	args.valuelen = valuelen;
-	args.flags = flags;
-	args.hashval = xfs_da_hashname(args.name, args.namelen);
-	args.dp = dp;
-	args.firstblock = &firstblock;
-	args.flist = &flist;
-	args.whichfork = XFS_ATTR_FORK;
-	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
-
-	/* Size is now blocks for attribute data */
-	args.total = xfs_attr_calc_size(dp, xname.len, valuelen, &local);
-
 	/*
 	 * Start our first transaction of the day.
 	 *

@@ -425,7 +418,6 @@ xfs_attr_remove(
 	struct xfs_mount	*mp = dp->i_mount;
 	struct xfs_da_args	args;
 	struct xfs_bmap_free	flist;
-	struct xfs_name		xname;
 	xfs_fsblock_t		firstblock;
 	int			error;
 

@@ -437,20 +429,12 @@ xfs_attr_remove(
 	if (!xfs_inode_hasattr(dp))
 		return ENOATTR;
 
-	error = xfs_attr_name_to_xname(&xname, name);
+	error = xfs_attr_args_init(&args, dp, name, flags);
 	if (error)
 		return error;
 
-	memset(&args, 0, sizeof(args));
-	args.name = xname.name;
-	args.namelen = xname.len;
-	args.flags = flags;
-	args.hashval = xfs_da_hashname(args.name, args.namelen);
-	args.dp = dp;
 	args.firstblock = &firstblock;
 	args.flist = &flist;
-	args.total = 0;
-	args.whichfork = XFS_ATTR_FORK;
 
 	/*
 	 * we have no control over the attribute names that userspace passes us

Signature · 6 lines

1.7.10.4

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

And remove a very confused comment.

Signed-off-by Christoph Hellwig <[email protected]>

---

 fs/xfs/xfs_attr.c |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

fs/xfs/xfs_attr.c +5 -9

index 1208621..86f482e 100644
@@ -162,12 +162,10 @@ xfs_attr_get(
  */
 STATIC int
 xfs_attr_calc_size(
-	struct xfs_inode 	*ip,
-	int			namelen,
-	int			valuelen,
+	struct xfs_da_args	*args,
 	int			*local)
 {
-	struct xfs_mount 	*mp = ip->i_mount;
+	struct xfs_mount	*mp = args->dp->i_mount;
 	int			size;
 	int			nblks;
 

@@ -175,7 +173,7 @@ xfs_attr_calc_size(
 	 * Determine space new attribute will use, and if it would be
 	 * "local" or "remote" (note: local != inline).
 	 */
-	size = xfs_attr_leaf_newentsize(namelen, valuelen,
+	size = xfs_attr_leaf_newentsize(args->namelen, args->valuelen,
 					mp->m_sb.sb_blocksize, local);
 
 	nblks = XFS_DAENTER_SPACE_RES(mp, XFS_ATTR_FORK);

@@ -189,7 +187,7 @@ xfs_attr_calc_size(
 		 * Out of line attribute, cannot double split, but
 		 * make room for the attribute value itself.
 		 */
-		uint	dblocks = XFS_B_TO_FSB(mp, valuelen);
+		uint	dblocks = XFS_B_TO_FSB(mp, args->valuelen);
 		nblks += dblocks;
 		nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK);
 	}

@@ -227,9 +225,7 @@ xfs_attr_set(
 	args.firstblock = &firstblock;
 	args.flist = &flist;
 	args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
-
-	/* Size is now blocks for attribute data */
-	args.total = xfs_attr_calc_size(dp, args.namelen, valuelen, &local);
+	args.total = xfs_attr_calc_size(&args, &local);
 
 	error = xfs_qm_dqattach(dp, 0);
 	if (error)

Signature · 6 lines

1.7.10.4

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

And remove a very confused comment.

Signed-off-by Christoph Hellwig <[email protected]>

Reviewed-by Brian Foster <[email protected]>

---
 fs/xfs/xfs_attr.c |   14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

[updated for a minor conflict in for-next]

A couple random cleanups for the attr code that I had sitting in my tree for a long time.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Depends on how parent inode pointers are implemented, this folding the internal version of get and set attributes could be undone.

Quoted 2 lines · Mark Tinguely

Depends on how parent inode pointers are implemented, this folding the internal version of get and set attributes could be undone.

We might have to introduce _locked version at that point. But I'd like to keep the xfs_name removal and other assorted cleanups.

Quoted 2 lines · Mark Tinguely

Depends on how parent inode pointers are implemented, this folding the internal version of get and set attributes could be undone.

We might have to introduce _locked version at that point. But I'd like to keep the xfs_name removal and other assorted cleanups.

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · Mark Tinguely

Depends on how parent inode pointers are implemented, this folding the internal version of get and set attributes could be undone.

Quoted 3 lines

We might have to introduce _locked version at that point. But I'd like to keep the xfs_name removal and other assorted cleanups.

locking is only one issue, xfs_attr_(get/set/remove) are asciii only whereas the xfs_attr_(get/set/remove)_int versions are more generic. I am thinking of not just parent inode pointers but a non-ascii character set.

Quoted 5 lines · Mark Tinguely

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · Mark Tinguely

Depends on how parent inode pointers are implemented, this folding the internal version of get and set attributes could be undone.

Quoted 8 lines

Quoted 3 lines

We might have to introduce _locked version at that point. But I'd like to keep the xfs_name removal and other assorted cleanups.

locking is only one issue, xfs_attr_(get/set/remove) are asciii only whereas the xfs_attr_(get/set/remove)_int versions are more generic. I am thinking of not just parent inode pointers but a non-ascii character set.

I fail to see how they are different. The attribute name is just an opaque binary blob - only when it is compared externally does it have any meaning at all.

Character sets are meaningless unless you are doing case manipulation, in which case we would need to apply the same treatment as the directory code deep in the internal attribute cod. i.e It needs case aware compare and hash algorithms. However, the outer layers are completely unchanged - they just pass through the blob that was passed to them...

Cheers,

Dave.

Quoted 26 lines · Dave Chinner

Quoted 5 lines · Mark Tinguely

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · Mark Tinguely

Depends on how parent inode pointers are implemented, this folding the internal version of get and set attributes could be undone.

Quoted 8 lines

Quoted 3 lines

We might have to introduce _locked version at that point. But I'd like to keep the xfs_name removal and other assorted cleanups.

locking is only one issue, xfs_attr_(get/set/remove) are asciii only whereas the xfs_attr_(get/set/remove)_int versions are more generic. I am thinking of not just parent inode pointers but a non-ascii character set.

I fail to see how they are different. The attribute name is just an opaque binary blob - only when it is compared externally does it have any meaning at all.

Character sets are meaningless unless you are doing case manipulation, in which case we would need to apply the same treatment as the directory code deep in the internal attribute cod. i.e It needs case aware compare and hash algorithms. However, the outer layers are completely unchanged - they just pass through the blob that was passed to them...

Quoted 3 lines

Cheers,

Dave.

Right now xfs_attr_get(), xfs_attr_set, xfs_attr_remove() expect the name to be a NULL terminate character array.

The internal versions of these functions have a more useful interface, they use a blob for the name (the array/length xfs_name structure).