* [PATCH 1/5] xfs: fold xfs_attr_set_int into xfs_attr_set
2014-05-03 15:20 attr cleanups Christoph Hellwig
@ 2014-05-03 15:20 ` Christoph Hellwig
2014-05-05 20:21 ` Brian Foster
2014-05-03 15:20 ` [PATCH 2/5] xfs: fold xfs_attr_get_int into xfs_attr_get Christoph Hellwig
` (4 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2014-05-03 15:20 UTC (permalink / raw)
To: xfs
Plus various minor style fixes to the new xfs_attr_set.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_attr.c | 110 +++++++++++++++++++++--------------------------------
1 file changed, 44 insertions(+), 66 deletions(-)
diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index 01b6a01..eb3ae8f 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -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;
}
/*
--
1.7.10.4
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 1/5] xfs: fold xfs_attr_set_int into xfs_attr_set
2014-05-03 15:20 ` [PATCH 1/5] xfs: fold xfs_attr_set_int into xfs_attr_set Christoph Hellwig
@ 2014-05-05 20:21 ` Brian Foster
0 siblings, 0 replies; 22+ messages in thread
From: Brian Foster @ 2014-05-05 20:21 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Sat, May 03, 2014 at 05:20:11PM +0200, Christoph Hellwig wrote:
> Plus various minor style fixes to the new xfs_attr_set.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
Looks good to me.
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_attr.c | 110 +++++++++++++++++++++--------------------------------
> 1 file changed, 44 insertions(+), 66 deletions(-)
>
> diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
> index 01b6a01..eb3ae8f 100644
> --- a/fs/xfs/xfs_attr.c
> +++ b/fs/xfs/xfs_attr.c
> @@ -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;
> }
>
> /*
> --
> 1.7.10.4
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/5] xfs: fold xfs_attr_get_int into xfs_attr_get
2014-05-03 15:20 attr cleanups Christoph Hellwig
2014-05-03 15:20 ` [PATCH 1/5] xfs: fold xfs_attr_set_int into xfs_attr_set Christoph Hellwig
@ 2014-05-03 15:20 ` Christoph Hellwig
2014-05-05 20:21 ` Brian Foster
2014-05-03 15:20 ` [PATCH 3/5] xfs: fold xfs_attr_remove_int into xfs_attr_remove Christoph Hellwig
` (3 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2014-05-03 15:20 UTC (permalink / raw)
To: 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 <hch@lst.de>
---
fs/xfs/xfs_attr.c | 79 ++++++++++++++++++-----------------------------------
1 file changed, 27 insertions(+), 52 deletions(-)
diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index eb3ae8f..01f2267 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -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;
}
/*
--
1.7.10.4
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 2/5] xfs: fold xfs_attr_get_int into xfs_attr_get
2014-05-03 15:20 ` [PATCH 2/5] xfs: fold xfs_attr_get_int into xfs_attr_get Christoph Hellwig
@ 2014-05-05 20:21 ` Brian Foster
0 siblings, 0 replies; 22+ messages in thread
From: Brian Foster @ 2014-05-05 20:21 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Sat, May 03, 2014 at 05:20:12PM +0200, Christoph Hellwig wrote:
> 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 <hch@lst.de>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_attr.c | 79 ++++++++++++++++++-----------------------------------
> 1 file changed, 27 insertions(+), 52 deletions(-)
>
> diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
> index eb3ae8f..01f2267 100644
> --- a/fs/xfs/xfs_attr.c
> +++ b/fs/xfs/xfs_attr.c
> @@ -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;
> }
>
> /*
> --
> 1.7.10.4
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/5] xfs: fold xfs_attr_remove_int into xfs_attr_remove
2014-05-03 15:20 attr cleanups Christoph Hellwig
2014-05-03 15:20 ` [PATCH 1/5] xfs: fold xfs_attr_set_int into xfs_attr_set Christoph Hellwig
2014-05-03 15:20 ` [PATCH 2/5] xfs: fold xfs_attr_get_int into xfs_attr_get Christoph Hellwig
@ 2014-05-03 15:20 ` Christoph Hellwig
2014-05-05 20:21 ` Brian Foster
2014-05-03 15:20 ` [PATCH 4/5] xfs: simplify attr name setup Christoph Hellwig
` (2 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2014-05-03 15:20 UTC (permalink / raw)
To: 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 <hch@lst.de>
---
fs/xfs/xfs_attr.c | 97 +++++++++++++++++++----------------------------------
1 file changed, 35 insertions(+), 62 deletions(-)
diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index 01f2267..a96e27b 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -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);
@@ -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);
- /*
- * Decide on what work routines to call based on the inode size.
- */
if (!xfs_inode_hasattr(dp)) {
error = XFS_ERROR(ENOATTR);
- 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
*========================================================================*/
--
1.7.10.4
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 3/5] xfs: fold xfs_attr_remove_int into xfs_attr_remove
2014-05-03 15:20 ` [PATCH 3/5] xfs: fold xfs_attr_remove_int into xfs_attr_remove Christoph Hellwig
@ 2014-05-05 20:21 ` Brian Foster
2014-05-05 20:56 ` Dave Chinner
0 siblings, 1 reply; 22+ messages in thread
From: Brian Foster @ 2014-05-05 20:21 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Sat, May 03, 2014 at 05:20:13PM +0200, Christoph Hellwig wrote:
> 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 <hch@lst.de>
> ---
> fs/xfs/xfs_attr.c | 97 +++++++++++++++++++----------------------------------
> 1 file changed, 35 insertions(+), 62 deletions(-)
>
> diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
> index 01f2267..a96e27b 100644
> --- a/fs/xfs/xfs_attr.c
> +++ b/fs/xfs/xfs_attr.c
> @@ -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);
> @@ -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);
>
> - /*
> - * 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.
Brian
> - 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
> *========================================================================*/
> --
> 1.7.10.4
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 3/5] xfs: fold xfs_attr_remove_int into xfs_attr_remove
2014-05-05 20:21 ` Brian Foster
@ 2014-05-05 20:56 ` Dave Chinner
2014-05-05 21:08 ` Brian Foster
0 siblings, 1 reply; 22+ messages in thread
From: Dave Chinner @ 2014-05-05 20:56 UTC (permalink / raw)
To: Brian Foster; +Cc: Christoph Hellwig, xfs
On Mon, May 05, 2014 at 04:21:28PM -0400, Brian Foster wrote:
> On Sat, May 03, 2014 at 05:20:13PM +0200, Christoph Hellwig wrote:
> > 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 <hch@lst.de>
.....
> > @@ -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);
> >
> > - /*
> > - * 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.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 3/5] xfs: fold xfs_attr_remove_int into xfs_attr_remove
2014-05-05 20:56 ` Dave Chinner
@ 2014-05-05 21:08 ` Brian Foster
0 siblings, 0 replies; 22+ messages in thread
From: Brian Foster @ 2014-05-05 21:08 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Tue, May 06, 2014 at 06:56:53AM +1000, Dave Chinner wrote:
> On Mon, May 05, 2014 at 04:21:28PM -0400, Brian Foster wrote:
> > On Sat, May 03, 2014 at 05:20:13PM +0200, Christoph Hellwig wrote:
> > > 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 <hch@lst.de>
> .....
> > > @@ -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);
> > >
> > > - /*
> > > - * 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.
>
Sounds good.
Reviewed-by: Brian Foster <bfoster@redhat.com>
> Cheers,
>
> Dave.
>
> --
> Dave Chinner
> david@fromorbit.com
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/5] xfs: simplify attr name setup
2014-05-03 15:20 attr cleanups Christoph Hellwig
` (2 preceding siblings ...)
2014-05-03 15:20 ` [PATCH 3/5] xfs: fold xfs_attr_remove_int into xfs_attr_remove Christoph Hellwig
@ 2014-05-03 15:20 ` Christoph Hellwig
2014-05-05 20:21 ` Brian Foster
2014-05-03 15:20 ` [PATCH 5/5] xfs: pass struct da_args to xfs_attr_calc_size Christoph Hellwig
2014-05-03 16:04 ` attr cleanups Mark Tinguely
5 siblings, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2014-05-03 15:20 UTC (permalink / raw)
To: 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 <hch@lst.de>
---
fs/xfs/xfs_attr.c | 74 +++++++++++++++++++++--------------------------------
1 file changed, 29 insertions(+), 45 deletions(-)
diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index a96e27b..1208621 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -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
--
1.7.10.4
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 4/5] xfs: simplify attr name setup
2014-05-03 15:20 ` [PATCH 4/5] xfs: simplify attr name setup Christoph Hellwig
@ 2014-05-05 20:21 ` Brian Foster
0 siblings, 0 replies; 22+ messages in thread
From: Brian Foster @ 2014-05-05 20:21 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Sat, May 03, 2014 at 05:20:14PM +0200, Christoph Hellwig wrote:
> 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 <hch@lst.de>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_attr.c | 74 +++++++++++++++++++++--------------------------------
> 1 file changed, 29 insertions(+), 45 deletions(-)
>
> diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
> index a96e27b..1208621 100644
> --- a/fs/xfs/xfs_attr.c
> +++ b/fs/xfs/xfs_attr.c
> @@ -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
> --
> 1.7.10.4
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 5/5] xfs: pass struct da_args to xfs_attr_calc_size
2014-05-03 15:20 attr cleanups Christoph Hellwig
` (3 preceding siblings ...)
2014-05-03 15:20 ` [PATCH 4/5] xfs: simplify attr name setup Christoph Hellwig
@ 2014-05-03 15:20 ` Christoph Hellwig
2014-05-05 20:21 ` Brian Foster
2014-05-03 16:04 ` attr cleanups Mark Tinguely
5 siblings, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2014-05-03 15:20 UTC (permalink / raw)
To: xfs
And remove a very confused comment.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_attr.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index 1208621..86f482e 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -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)
--
1.7.10.4
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 5/5] xfs: pass struct da_args to xfs_attr_calc_size
2014-05-03 15:20 ` [PATCH 5/5] xfs: pass struct da_args to xfs_attr_calc_size Christoph Hellwig
@ 2014-05-05 20:21 ` Brian Foster
2014-05-06 8:06 ` [PATCH 5/5 v2] " Christoph Hellwig
0 siblings, 1 reply; 22+ messages in thread
From: Brian Foster @ 2014-05-05 20:21 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Sat, May 03, 2014 at 05:20:15PM +0200, Christoph Hellwig wrote:
> And remove a very confused comment.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_attr.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
> index 1208621..86f482e 100644
> --- a/fs/xfs/xfs_attr.c
> +++ b/fs/xfs/xfs_attr.c
> @@ -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)
> --
> 1.7.10.4
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH 5/5 v2] xfs: pass struct da_args to xfs_attr_calc_size
2014-05-05 20:21 ` Brian Foster
@ 2014-05-06 8:06 ` Christoph Hellwig
2014-05-06 9:09 ` Dave Chinner
0 siblings, 1 reply; 22+ messages in thread
From: Christoph Hellwig @ 2014-05-06 8:06 UTC (permalink / raw)
To: xfs; +Cc: Brian Foster
And remove a very confused comment.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
fs/xfs/xfs_attr.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
[updated for a minor conflict in for-next]
diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index d5c03ed..1fc1f06 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -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_attr3_rmt_blocks(mp, valuelen);
+ uint dblocks = xfs_attr3_rmt_blocks(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)
--
1.7.10.4
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 5/5 v2] xfs: pass struct da_args to xfs_attr_calc_size
2014-05-06 8:06 ` [PATCH 5/5 v2] " Christoph Hellwig
@ 2014-05-06 9:09 ` Dave Chinner
0 siblings, 0 replies; 22+ messages in thread
From: Dave Chinner @ 2014-05-06 9:09 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Brian Foster, xfs
On Tue, May 06, 2014 at 10:06:58AM +0200, Christoph Hellwig wrote:
> And remove a very confused comment.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> ---
> fs/xfs/xfs_attr.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> [updated for a minor conflict in for-next]
Already seen that on a local merge - I fix conflicts like this
when I merge the topic branch into for-next. :)
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: attr cleanups
2014-05-03 15:20 attr cleanups Christoph Hellwig
` (4 preceding siblings ...)
2014-05-03 15:20 ` [PATCH 5/5] xfs: pass struct da_args to xfs_attr_calc_size Christoph Hellwig
@ 2014-05-03 16:04 ` Mark Tinguely
2014-05-04 10:16 ` Christoph Hellwig
5 siblings, 1 reply; 22+ messages in thread
From: Mark Tinguely @ 2014-05-03 16:04 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On 05/03/14 10:20, Christoph Hellwig wrote:
> A couple random cleanups for the attr code that I had sitting in my tree
> for a long time.
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> 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.
--Mark.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: attr cleanups
2014-05-03 16:04 ` attr cleanups Mark Tinguely
@ 2014-05-04 10:16 ` Christoph Hellwig
2014-05-04 21:52 ` Dave Chinner
2014-05-05 13:24 ` Mark Tinguely
0 siblings, 2 replies; 22+ messages in thread
From: Christoph Hellwig @ 2014-05-04 10:16 UTC (permalink / raw)
To: Mark Tinguely; +Cc: Christoph Hellwig, xfs
On Sat, May 03, 2014 at 11:04:05AM -0500, Mark Tinguely wrote:
> 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.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: attr cleanups
2014-05-04 10:16 ` Christoph Hellwig
@ 2014-05-04 21:52 ` Dave Chinner
2014-05-05 13:24 ` Mark Tinguely
1 sibling, 0 replies; 22+ messages in thread
From: Dave Chinner @ 2014-05-04 21:52 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Mark Tinguely, xfs
On Sun, May 04, 2014 at 12:16:23PM +0200, Christoph Hellwig wrote:
> On Sat, May 03, 2014 at 11:04:05AM -0500, Mark Tinguely wrote:
> > 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.
*nod*
If we need to factor the code for new additions, then we should do
that appropriately for the new code when it arrives.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: attr cleanups
2014-05-04 10:16 ` Christoph Hellwig
2014-05-04 21:52 ` Dave Chinner
@ 2014-05-05 13:24 ` Mark Tinguely
2014-05-05 20:55 ` Dave Chinner
1 sibling, 1 reply; 22+ messages in thread
From: Mark Tinguely @ 2014-05-05 13:24 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On 05/04/14 05:16, Christoph Hellwig wrote:
> On Sat, May 03, 2014 at 11:04:05AM -0500, Mark Tinguely wrote:
>> 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.
>
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.
--Mark.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: attr cleanups
2014-05-05 13:24 ` Mark Tinguely
@ 2014-05-05 20:55 ` Dave Chinner
2014-05-06 19:40 ` Mark Tinguely
0 siblings, 1 reply; 22+ messages in thread
From: Dave Chinner @ 2014-05-05 20:55 UTC (permalink / raw)
To: Mark Tinguely; +Cc: Christoph Hellwig, xfs
On Mon, May 05, 2014 at 08:24:35AM -0500, Mark Tinguely wrote:
> On 05/04/14 05:16, Christoph Hellwig wrote:
> >On Sat, May 03, 2014 at 11:04:05AM -0500, Mark Tinguely wrote:
> >>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.
> >
>
> 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.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: attr cleanups
2014-05-05 20:55 ` Dave Chinner
@ 2014-05-06 19:40 ` Mark Tinguely
2014-05-06 20:51 ` Dave Chinner
0 siblings, 1 reply; 22+ messages in thread
From: Mark Tinguely @ 2014-05-06 19:40 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On 05/05/14 15:55, Dave Chinner wrote:
> On Mon, May 05, 2014 at 08:24:35AM -0500, Mark Tinguely wrote:
>> On 05/04/14 05:16, Christoph Hellwig wrote:
>>> On Sat, May 03, 2014 at 11:04:05AM -0500, Mark Tinguely wrote:
>>>> 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.
>>>
>>
>> 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.
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).
--Mark.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: attr cleanups
2014-05-06 19:40 ` Mark Tinguely
@ 2014-05-06 20:51 ` Dave Chinner
0 siblings, 0 replies; 22+ messages in thread
From: Dave Chinner @ 2014-05-06 20:51 UTC (permalink / raw)
To: Mark Tinguely; +Cc: Christoph Hellwig, xfs
On Tue, May 06, 2014 at 02:40:56PM -0500, Mark Tinguely wrote:
> On 05/05/14 15:55, Dave Chinner wrote:
> >On Mon, May 05, 2014 at 08:24:35AM -0500, Mark Tinguely wrote:
> >>On 05/04/14 05:16, Christoph Hellwig wrote:
> >>>On Sat, May 03, 2014 at 11:04:05AM -0500, Mark Tinguely wrote:
> >>>>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.
> >>>
> >>
> >>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.
>
> Right now xfs_attr_get(), xfs_attr_set, xfs_attr_remove() expect the
> name to be a NULL terminate character array.
Sure. The obvious solution is simply moving xfs_attr_name_to_xname()
to the callers and passing an xfs_name rather than a char *, as you
have pointed out:
> The internal versions of these functions have a more useful
> interface, they use a blob for the name (the array/length xfs_name
> structure).
... there is nothing that inherently makes those functions only
support character strings.
Like I said, it needs to become more like the directory code - we do
this "convert to opaque xfs_name" via xfs_dentry_to_name() at the
VFS entry layer for directory operations (see xfs_iops.c), and all
the code that uses xattrs should do the same...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 22+ messages in thread