xfs: allocate xattr buffer on demand
v3 cover xfs: allocate xattr buffer on demand
Updated series to address Christophs revierw comments. V2 can be found here:
https://lore.kernel.org/linux-xfs/[email protected]/T/#t
v3:
- fixed typoes and stray mods in patch 1
- split patch two into an indent removal patch and a consolidation patch.
- split patch three in a consolidation pathc and a patch to add allocation on demand.
- various other minor cleanups.
PATCH 1/5 xfs: make attr lookup returns consistent
From: Dave Chinner <[email protected]>
Shortform, leaf and remote value attr value retrieval return different values for success. This makes it more complex to handle actual errors xfs_attr_get() as some errors mean success and some mean failure. Make the return values consistent for success and failure consistent for all attribute formats.
--- fs/xfs/libxfs/xfs_attr.c | 55 +++++++++++++++++++++------------ fs/xfs/libxfs/xfs_attr_leaf.c | 15 ++++++--- fs/xfs/libxfs/xfs_attr_remote.c | 2 ++ fs/xfs/scrub/attr.c | 2 -- 4 files changed, 48 insertions(+), 26 deletions(-)
fs/xfs/libxfs/xfs_attr.c +35 -20
@@ -97,7 +97,10 @@ xfs_inode_hasattr(* Overall external interface routines. *========================================================================*/ -/* Retrieve an extended attribute and its value. Must have ilock. */ +/* +* Retrieve an extended attribute and its value. Must have ilock. +* Returns 0 on successful retrieval, otherwise an error. +*/ int xfs_attr_get_ilocked( struct xfs_inode *ip,
@@ -147,7 +150,7 @@ xfs_attr_get(xfs_iunlock (ip, lock_mode); *valuelenp = args.valuelen; -return error == -EEXIST ?0 : error; +return error; }/*
@@ -768,6 +771,8 @@ xfs_attr_leaf_removename(* * This leaf block cannot have a "remote" value, we only call this routine * if bmap_one_block() says there is only one block (ie: no remote blks). +* +* Returns 0 on successful retrieval, otherwise an error. */ STATIC intxfs_attr_leaf_get (xfs_da_args_t *args)
@@ -789,10 +794,15 @@ xfs_attr_leaf_get(xfs_da_args_t *args) } error =xfs_attr3_leaf_getvalue (bp, args);xfs_trans_brelse (args->trans, bp); -if (!error && (args->rmtblkno >0 ) && !(args->flags &ATTR_KERNOVAL )) { - error =xfs_attr_rmtval_get (args); - } -return error; +if (error) +return error; + +/* check if we have to retrieve a remote attribute to get the value */ +if (args->flags &ATTR_KERNOVAL ) +return 0 ; +if (!args->rmtblkno) +return 0 ; +return xfs_attr_rmtval_get (args); }/*========================================================================
@@ -1268,11 +1278,13 @@ xfs_attr_refillstate(xfs_da_state_t *state) }/* -* Look up a filename in a node attribute list. +* Retrieve the attribute data from a node attribute list. * * This routine gets called for any attribute fork that has more than one * block, ie: both true Btree attr lists and for single-leaf-blocks with * "remote" values taking up more blocks. +* +* Returns 0 on successful retrieval, otherwise an error. */ STATIC intxfs_attr_node_get (xfs_da_args_t *args)
@@ -1294,24 +1306,27 @@ xfs_attr_node_get(xfs_da_args_t *args) error =xfs_da3_node_lookup_int (state, &retval);if (error) { retval = error; - }else if (retval == -EEXIST ) { - blk = &state->path.blk[ state->path.active-1 ]; -ASSERT (blk->bp !=NULL ); -ASSERT (blk->magic ==XFS_ATTR_LEAF_MAGIC ); - -/* -* Get the value, local or "remote" -*/ - retval =xfs_attr3_leaf_getvalue (blk->bp, args); -if (!retval && (args->rmtblkno >0 ) - && !(args->flags &ATTR_KERNOVAL )) { - retval =xfs_attr_rmtval_get (args); - } + goto out_release; } +if (retval != -EEXIST ) + goto out_release; + +/* +* Get the value, local or "remote" +*/ + blk = &state->path.blk[state->path.active -1 ]; + retval =xfs_attr3_leaf_getvalue (blk->bp, args); +if (retval) + goto out_release; +if (args->flags &ATTR_KERNOVAL ) + goto out_release; +if (args->rmtblkno >0 ) + retval =xfs_attr_rmtval_get (args);/* * If not in a transaction, we have to release all the buffers. */ +out_release:for (i =0 ; i < state->path.active; i++) {xfs_trans_brelse (args->trans, state->path.blk[i].bp); state->path.blk[i].bp =NULL ;
fs/xfs/libxfs/xfs_attr_leaf.c +11 -4
@@ -720,9 +720,12 @@ xfs_attr_shortform_lookup(xfs_da_args_t *args) }/* -* Look up a name in a shortform attribute list structure. +* Retreive the attribute value and length. +* +* If ATTR_KERNOVAL is specified, only the length needs to be returned. +* Unlike a lookup, we only return an error if the attribute does not +* exist or we can't retrieve the value. */ -/*ARGSUSED*/ int xfs_attr_shortform_getvalue (xfs_da_args_t *args) {
@@ -743,7 +746,7 @@ xfs_attr_shortform_getvalue(xfs_da_args_t *args)continue ;if (args->flags &ATTR_KERNOVAL ) { args->valuelen = sfe->valuelen; -return -EEXIST ; +return 0 ; }if (args->valuelen < sfe->valuelen) { args->valuelen = sfe->valuelen;
@@ -752,7 +755,7 @@ xfs_attr_shortform_getvalue(xfs_da_args_t *args) args->valuelen = sfe->valuelen;memcpy (args->value, &sfe->nameval[args->namelen], args->valuelen); -return -EEXIST ; +return 0 ; }return -ENOATTR ; }
@@ -2350,6 +2353,10 @@ xfs_attr3_leaf_lookup_int(/* * Get the value associated with an attribute name from a leaf attribute * list structure. +* +* If ATTR_KERNOVAL is specified, only the length needs to be returned. +* Unlike a lookup, we only return an error if the attribute does not +* exist or we can't retrieve the value. */ int xfs_attr3_leaf_getvalue(
fs/xfs/libxfs/xfs_attr_remote.c +2 -0
@@ -358,6 +358,8 @@ xfs_attr_rmtval_copyin(/* * Read the value associated with an attribute from the out-of-line buffer * that we stored it in. +* +* Returns 0 on successful retrieval, otherwise an error. */ int xfs_attr_rmtval_get(
fs/xfs/scrub/attr.c +0 -2
@@ -163,8 +163,6 @@ xchk_xattr_listent( args.valuelen = valuelen; error =xfs_attr_get_ilocked (context->dp, &args); -if (error == -EEXIST ) - error =0 ;if (!xchk_fblock_process_error (sx->sc,XFS_ATTR_FORK , args.blkno, &error)) goto fail_xref;
Signature · 1 line
2.23.0.rc1
Re: [PATCH 1/5] xfs: make attr lookup returns consistent
Quoted 6 lines · Dave Chinner
+/*
+ * Retrieve an extended attribute and its value. Must have ilock.
+ * Returns 0 on successful retrieval, otherwise an error.
+ */
int
xfs_attr_get_ilocked(
This just updates a comment on xfs_attr_get_ilocked, no other change, so looks good.
fs/xfs/libxfs/xfs_attr.c xfs_attr_get( · line 154
Quoted 9 lines
fs/xfs/libxfs/xfs_attr.c +1
-1struct xfs_inode *ip,@@ -147,7 +150,7 @@ xfs_attr_get(xfs_iunlock (ip, lock_mode); *valuelenp = args.valuelen; -return error == -EEXIST ?0 : error; +return error; }
This drops a conversion from -EEXIST to 0 at the very top of the callchain.
fs/xfs/libxfs/xfs_attr.c xfs_attr_leaf_get(xfs_da_args_t *args) · line 805
Quoted 13 lines
fs/xfs/libxfs/xfs_attr.c +9
-4-if (!error && (args->rmtblkno >0 ) && !(args->flags &ATTR_KERNOVAL )) { - error =xfs_attr_rmtval_get (args); - } -return error; +if (error) +return error; + +/* check if we have to retrieve a remote attribute to get the value */ +if (args->flags &ATTR_KERNOVAL ) +return 0 ; +if (!args->rmtblkno) +return 0 ; +return xfs_attr_rmtval_get (args);
No change at all here, just a more verbose style.
Quoted 30 lines
STATIC int
xfs_attr_node_get(xfs_da_args_t *args)fs/xfs/libxfs/xfs_attr.c +10
-13@@ -1294,24 +1306,27 @@ xfs_attr_node_get(xfs_da_args_t *args) error =xfs_da3_node_lookup_int (state, &retval);if (error) { retval = error; - }else if (retval == -EEXIST ) { - blk = &state->path.blk[ state->path.active-1 ]; -ASSERT (blk->bp !=NULL ); -ASSERT (blk->magic ==XFS_ATTR_LEAF_MAGIC ); - -/* -* Get the value, local or "remote" -*/ - retval =xfs_attr3_leaf_getvalue (blk->bp, args); -if (!retval && (args->rmtblkno >0 ) - && !(args->flags &ATTR_KERNOVAL )) { - retval =xfs_attr_rmtval_get (args); - } + goto out_release; } +if (retval != -EEXIST ) + goto out_release; + +/* +* Get the value, local or "remote" +*/ + blk = &state->path.blk[state->path.active -1 ]; + retval =xfs_attr3_leaf_getvalue (blk->bp, args); +if (retval)
fs/xfs/libxfs/xfs_attr.c xfs_attr_node_get(xfs_da_args_t *args) · line 1329
Quoted 10 lines
fs/xfs/libxfs/xfs_attr.c +6
-0+ goto out_release; +if (args->flags &ATTR_KERNOVAL ) + goto out_release; +if (args->rmtblkno >0 ) + retval =xfs_attr_rmtval_get (args);/* * If not in a transaction, we have to release all the buffers. */ +out_release:
No change at all here as far as I can tell, just using a goto to unwind error handling, and rewriting some code to be less dense/obsfucated.
Quoted 13 lines
fs/xfs/libxfs/xfs_attr_leaf.c +1
-1int xfs_attr_shortform_getvalue (xfs_da_args_t *args) {@@ -743,7 +746,7 @@ xfs_attr_shortform_getvalue(xfs_da_args_t *args)continue ;if (args->flags &ATTR_KERNOVAL ) { args->valuelen = sfe->valuelen; -return -EEXIST ; +return 0 ; }if (args->valuelen < sfe->valuelen) { args->valuelen = sfe->valuelen;
fs/xfs/libxfs/xfs_attr_leaf.c xfs_attr_shortform_getvalue(xfs_da_args_t *args) · line 758
Quoted 5 lines
fs/xfs/libxfs/xfs_attr_leaf.c +1
-1@@ -752,7 +755,7 @@ xfs_attr_shortform_getvalue(xfs_da_args_t *args) args->valuelen = sfe->valuelen;memcpy (args->value, &sfe->nameval[args->namelen], args->valuelen); -return -EEXIST ; +return 0 ;
And here we have the first change - xfs_attr_shortform_getvalue now returns 0 instead of -EEXIST when finding the xattr. The old calling conventions does indeed seem very odd. As xfs_attr_shortform_getvalue is directly called by the small xfs_attr_get_ilocked wrapper, which only has two calers that fix up -EEXIST to 0 this looks safe.
So overall this looks good, but I think having this split into two pure cleanups and one to actually switch this single case away from -EEXIST (and document the chain as I wrote above while following it) would have been more helpful.
So what:
PATCH 2/5 xfs: remove unnecessary indenting from xfs_attr3_leaf_getvalue
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
--- fs/xfs/libxfs/xfs_attr_leaf.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)
fs/xfs/libxfs/xfs_attr_leaf.c +17 -16
@@ -2391,24 +2391,25 @@ xfs_attr3_leaf_getvalue( } args->valuelen = valuelen;memcpy (args->value, &name_loc->nameval[args->namelen], valuelen); - }else { - name_rmt =xfs_attr3_leaf_name_remote (leaf, args->index); -ASSERT (name_rmt->namelen == args->namelen); -ASSERT (memcmp (args->name, name_rmt->name, args->namelen) ==0 ); - args->rmtvaluelen =be32_to_cpu (name_rmt->valuelen); - args->rmtblkno =be32_to_cpu (name_rmt->valueblk); - args->rmtblkcnt =xfs_attr3_rmt_blocks (args->dp->i_mount, - args->rmtvaluelen); -if (args->flags &ATTR_KERNOVAL ) { - args->valuelen = args->rmtvaluelen; -return 0 ; - } -if (args->valuelen < args->rmtvaluelen) { - args->valuelen = args->rmtvaluelen; -return -ERANGE ; - } +return 0 ; + } + + name_rmt =xfs_attr3_leaf_name_remote (leaf, args->index); +ASSERT (name_rmt->namelen == args->namelen); +ASSERT (memcmp (args->name, name_rmt->name, args->namelen) ==0 ); + args->rmtvaluelen =be32_to_cpu (name_rmt->valuelen); + args->rmtblkno =be32_to_cpu (name_rmt->valueblk); + args->rmtblkcnt =xfs_attr3_rmt_blocks (args->dp->i_mount, + args->rmtvaluelen); +if (args->flags &ATTR_KERNOVAL ) { + args->valuelen = args->rmtvaluelen; +return 0 ; + } +if (args->valuelen < args->rmtvaluelen) { args->valuelen = args->rmtvaluelen; +return -ERANGE ; } + args->valuelen = args->rmtvaluelen;return 0 ; }
Signature · 1 line
2.23.0.rc1
Re: [PATCH 2/5] xfs: remove unnecessary indenting from xfs_attr3_leaf_getvalue
Quoted 3 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Signed-off-by Dave Chinner <[email protected]>
Looks ok,
--D
Quoted 53 lines
--- fs/xfs/libxfs/xfs_attr_leaf.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)fs/xfs/libxfs/xfs_attr_leaf.c +17
-16@@ -2391,24 +2391,25 @@ xfs_attr3_leaf_getvalue( } args->valuelen = valuelen;memcpy (args->value, &name_loc->nameval[args->namelen], valuelen); - }else { - name_rmt =xfs_attr3_leaf_name_remote (leaf, args->index); -ASSERT (name_rmt->namelen == args->namelen); -ASSERT (memcmp (args->name, name_rmt->name, args->namelen) ==0 ); - args->rmtvaluelen =be32_to_cpu (name_rmt->valuelen); - args->rmtblkno =be32_to_cpu (name_rmt->valueblk); - args->rmtblkcnt =xfs_attr3_rmt_blocks (args->dp->i_mount, - args->rmtvaluelen); -if (args->flags &ATTR_KERNOVAL ) { - args->valuelen = args->rmtvaluelen; -return 0 ; - } -if (args->valuelen < args->rmtvaluelen) { - args->valuelen = args->rmtvaluelen; -return -ERANGE ; - } +return 0 ; + } + + name_rmt =xfs_attr3_leaf_name_remote (leaf, args->index); +ASSERT (name_rmt->namelen == args->namelen); +ASSERT (memcmp (args->name, name_rmt->name, args->namelen) ==0 ); + args->rmtvaluelen =be32_to_cpu (name_rmt->valuelen); + args->rmtblkno =be32_to_cpu (name_rmt->valueblk); + args->rmtblkcnt =xfs_attr3_rmt_blocks (args->dp->i_mount, + args->rmtvaluelen); +if (args->flags &ATTR_KERNOVAL ) { + args->valuelen = args->rmtvaluelen; +return 0 ; + } +if (args->valuelen < args->rmtvaluelen) { args->valuelen = args->rmtvaluelen; +return -ERANGE ; } + args->valuelen = args->rmtvaluelen;return 0 ; }Signature · 1 line
2.23.0.rc1
Re: [PATCH 2/5] xfs: remove unnecessary indenting from xfs_attr3_leaf_getvalue
In reply to Dave ChinnerQuoted 3 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Signed-off-by Dave Chinner <[email protected]>
Looks good,
PATCH 3/5 xfs: move remote attr retrieval into xfs_attr3_leaf_getvalue
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Because we repeat exactly the same code to get the remote attribute value after both calls to xfs_attr3_leaf_getvalue() if it's a remote attr. Just do it in xfs_attr3_leaf_getvalue() so the callers don't have to care about it.
--- fs/xfs/libxfs/xfs_attr.c | 16 +--------------- fs/xfs/libxfs/xfs_attr_leaf.c | 2 +- 2 files changed, 2 insertions(+), 16 deletions(-)
fs/xfs/libxfs/xfs_attr.c +1 -15
@@ -794,15 +794,7 @@ xfs_attr_leaf_get(xfs_da_args_t *args) } error =xfs_attr3_leaf_getvalue (bp, args);xfs_trans_brelse (args->trans, bp); -if (error) -return error; - -/* check if we have to retrieve a remote attribute to get the value */ -if (args->flags &ATTR_KERNOVAL ) -return 0 ; -if (!args->rmtblkno) -return 0 ; -return xfs_attr_rmtval_get (args); +return error; }/*========================================================================
@@ -1316,12 +1308,6 @@ xfs_attr_node_get(xfs_da_args_t *args)*/ blk = &state->path.blk[state->path.active -1 ]; retval =xfs_attr3_leaf_getvalue (blk->bp, args); -if (retval) - goto out_release; -if (args->flags &ATTR_KERNOVAL ) - goto out_release; -if (args->rmtblkno >0 ) - retval =xfs_attr_rmtval_get (args);/* * If not in a transaction, we have to release all the buffers.
fs/xfs/libxfs/xfs_attr_leaf.c +1 -1
@@ -2410,7 +2410,7 @@ xfs_attr3_leaf_getvalue(return -ERANGE ; } args->valuelen = args->rmtvaluelen; -return 0 ; +return xfs_attr_rmtval_get (args); }/*========================================================================
Signature · 1 line
2.23.0.rc1
Re: [PATCH 3/5] xfs: move remote attr retrieval into xfs_attr3_leaf_getvalue
Quoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Because we repeat exactly the same code to get the remote attribute value after both calls to
xfs_attr3_leaf_getvalue()if it's a remote attr. Just do it inxfs_attr3_leaf_getvalue()so the callers don't have to care about it.Signed-off-by Dave Chinner <[email protected]>
Looks ok,
--D
Quoted 55 lines
--- fs/xfs/libxfs/xfs_attr.c | 16 +--------------- fs/xfs/libxfs/xfs_attr_leaf.c | 2 +- 2 files changed, 2 insertions(+), 16 deletions(-)fs/xfs/libxfs/xfs_attr.c +1
-15@@ -794,15 +794,7 @@ xfs_attr_leaf_get(xfs_da_args_t *args) } error =xfs_attr3_leaf_getvalue (bp, args);xfs_trans_brelse (args->trans, bp); -if (error) -return error; - -/* check if we have to retrieve a remote attribute to get the value */ -if (args->flags &ATTR_KERNOVAL ) -return 0 ; -if (!args->rmtblkno) -return 0 ; -return xfs_attr_rmtval_get (args); +return error; }/*======================================================================== @@ -1316,12 +1308,6 @@ xfs_attr_node_get(xfs_da_args_t *args)*/ blk = &state->path.blk[state->path.active -1 ]; retval =xfs_attr3_leaf_getvalue (blk->bp, args); -if (retval) - goto out_release; -if (args->flags &ATTR_KERNOVAL ) - goto out_release; -if (args->rmtblkno >0 ) - retval =xfs_attr_rmtval_get (args);/* * If not in a transaction, we have to release all the buffers. fs/xfs/libxfs/xfs_attr_leaf.c +1
-1@@ -2410,7 +2410,7 @@ xfs_attr3_leaf_getvalue(return -ERANGE ; } args->valuelen = args->rmtvaluelen; -return 0 ; +return xfs_attr_rmtval_get (args); }/*======================================================================== Signature · 1 line
2.23.0.rc1
Re: [PATCH 3/5] xfs: move remote attr retrieval into xfs_attr3_leaf_getvalue
In reply to Dave ChinnerQuoted 8 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Because we repeat exactly the same code to get the remote attribute value after both calls to
xfs_attr3_leaf_getvalue()if it's a remote attr. Just do it inxfs_attr3_leaf_getvalue()so the callers don't have to care about it.Signed-off-by Dave Chinner <[email protected]>
Looks good,
PATCH 4/5 xfs: consolidate attribute value copying
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
The same code is used to copy do the attribute copying in three different places. Consolidate them into a single function in preparation from on-demand buffer allocation.
--- fs/xfs/libxfs/xfs_attr_leaf.c | 88 +++++++++++++++++++---------------- 1 file changed, 49 insertions(+), 39 deletions(-)
fs/xfs/libxfs/xfs_attr_leaf.c +49 -39
@@ -393,6 +393,44 @@ xfs_attr_namesp_match(int arg_flags, int ondisk_flags)return XFS_ATTR_NSP_ONDISK (ondisk_flags) ==XFS_ATTR_NSP_ARGS_TO_ONDISK (arg_flags); } +static int +xfs_attr_copy_value ( +struct xfs_da_args *args, +unsigned char *value, +int valuelen) +{ +/* +* No copy if all we have to do is get the length +*/ +if (args->flags &ATTR_KERNOVAL ) { + args->valuelen = valuelen; +return 0 ; + } + +/* +* No copy if the length of the existing buffer is too small +*/ +if (args->valuelen < valuelen) { + args->valuelen = valuelen; +return -ERANGE ; + } + args->valuelen = valuelen; + +/* remote block xattr requires IO for copy-in */ +if (args->rmtblkno) +return xfs_attr_rmtval_get (args); + +/* +* This is to prevent a GCC warning because the remote xattr case +* doesn't have a value to pass in. In that case, we never reach here, +* but GCC can't work that out and so throws a "passing NULL to +* memcpy" warning. +*/ +if (!value) +return -EINVAL ; +memcpy (args->value, value, valuelen); +return 0 ; +}/*======================================================================== * External routines when attribute fork size < XFS_LITINO(mp).
@@ -727,11 +765,12 @@ xfs_attr_shortform_lookup(xfs_da_args_t *args)* exist or we can't retrieve the value. */ int -xfs_attr_shortform_getvalue (xfs_da_args_t *args) +xfs_attr_shortform_getvalue ( +struct xfs_da_args *args) { -xfs_attr_shortform_t *sf; -xfs_attr_sf_entry_t *sfe; -int i; +struct xfs_attr_shortform *sf; +struct xfs_attr_sf_entry *sfe; +int i;ASSERT (args->dp->i_afp->if_flags ==XFS_IFINLINE ); sf = (xfs_attr_shortform_t *)args->dp->i_afp->if_u1.if_data;
@@ -744,18 +783,8 @@ xfs_attr_shortform_getvalue(xfs_da_args_t *args)continue ;if (!xfs_attr_namesp_match (args->flags, sfe->flags))continue ; -if (args->flags &ATTR_KERNOVAL ) { - args->valuelen = sfe->valuelen; -return 0 ; - } -if (args->valuelen < sfe->valuelen) { - args->valuelen = sfe->valuelen; -return -ERANGE ; - } - args->valuelen = sfe->valuelen; -memcpy (args->value, &sfe->nameval[args->namelen], - args->valuelen); -return 0 ; +return xfs_attr_copy_value (args, &sfe->nameval[args->namelen], + sfe->valuelen); }return -ENOATTR ; }
@@ -2368,7 +2397,6 @@ xfs_attr3_leaf_getvalue(struct xfs_attr_leaf_entry *entry;struct xfs_attr_leaf_name_local *name_loc;struct xfs_attr_leaf_name_remote *name_rmt; -int valuelen; leaf = bp->b_addr;xfs_attr3_leaf_hdr_from_disk (args->geo, &ichdr, leaf);
@@ -2380,18 +2408,9 @@ xfs_attr3_leaf_getvalue( name_loc =xfs_attr3_leaf_name_local (leaf, args->index);ASSERT (name_loc->namelen == args->namelen);ASSERT (memcmp (args->name, name_loc->nameval, args->namelen) ==0 ); - valuelen =be16_to_cpu (name_loc->valuelen); -if (args->flags &ATTR_KERNOVAL ) { - args->valuelen = valuelen; -return 0 ; - } -if (args->valuelen < valuelen) { - args->valuelen = valuelen; -return -ERANGE ; - } - args->valuelen = valuelen; -memcpy (args->value, &name_loc->nameval[args->namelen], valuelen); -return 0 ; +return xfs_attr_copy_value (args, + &name_loc->nameval[args->namelen], +be16_to_cpu (name_loc->valuelen)); } name_rmt =xfs_attr3_leaf_name_remote (leaf, args->index);
@@ -2401,16 +2420,7 @@ xfs_attr3_leaf_getvalue( args->rmtblkno =be32_to_cpu (name_rmt->valueblk); args->rmtblkcnt =xfs_attr3_rmt_blocks (args->dp->i_mount, args->rmtvaluelen); -if (args->flags &ATTR_KERNOVAL ) { - args->valuelen = args->rmtvaluelen; -return 0 ; - } -if (args->valuelen < args->rmtvaluelen) { - args->valuelen = args->rmtvaluelen; -return -ERANGE ; - } - args->valuelen = args->rmtvaluelen; -return xfs_attr_rmtval_get (args); +return xfs_attr_copy_value (args,NULL , args->rmtvaluelen); }/*========================================================================
Signature · 1 line
2.23.0.rc1
Re: [PATCH 4/5] xfs: consolidate attribute value copying
Quoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
The same code is used to copy do the attribute copying in three different places. Consolidate them into a single function in preparation from on-demand buffer allocation.
Signed-off-by Dave Chinner <[email protected]>
Looks ok,
--D
Quoted 142 lines
--- fs/xfs/libxfs/xfs_attr_leaf.c | 88 +++++++++++++++++++---------------- 1 file changed, 49 insertions(+), 39 deletions(-)fs/xfs/libxfs/xfs_attr_leaf.c +49
-39@@ -393,6 +393,44 @@ xfs_attr_namesp_match(int arg_flags, int ondisk_flags)return XFS_ATTR_NSP_ONDISK (ondisk_flags) ==XFS_ATTR_NSP_ARGS_TO_ONDISK (arg_flags); } +static int +xfs_attr_copy_value ( +struct xfs_da_args *args, +unsigned char *value, +int valuelen) +{ +/* +* No copy if all we have to do is get the length +*/ +if (args->flags &ATTR_KERNOVAL ) { + args->valuelen = valuelen; +return 0 ; + } + +/* +* No copy if the length of the existing buffer is too small +*/ +if (args->valuelen < valuelen) { + args->valuelen = valuelen; +return -ERANGE ; + } + args->valuelen = valuelen; + +/* remote block xattr requires IO for copy-in */ +if (args->rmtblkno) +return xfs_attr_rmtval_get (args); + +/* +* This is to prevent a GCC warning because the remote xattr case +* doesn't have a value to pass in. In that case, we never reach here, +* but GCC can't work that out and so throws a "passing NULL to +* memcpy" warning. +*/ +if (!value) +return -EINVAL ; +memcpy (args->value, value, valuelen); +return 0 ; +}/*======================================================================== * External routines when attribute fork size < XFS_LITINO(mp). @@ -727,11 +765,12 @@ xfs_attr_shortform_lookup(xfs_da_args_t *args)* exist or we can't retrieve the value. */ int -xfs_attr_shortform_getvalue (xfs_da_args_t *args) +xfs_attr_shortform_getvalue ( +struct xfs_da_args *args) { -xfs_attr_shortform_t *sf; -xfs_attr_sf_entry_t *sfe; -int i; +struct xfs_attr_shortform *sf; +struct xfs_attr_sf_entry *sfe; +int i;ASSERT (args->dp->i_afp->if_flags ==XFS_IFINLINE ); sf = (xfs_attr_shortform_t *)args->dp->i_afp->if_u1.if_data;@@ -744,18 +783,8 @@ xfs_attr_shortform_getvalue(xfs_da_args_t *args)continue ;if (!xfs_attr_namesp_match (args->flags, sfe->flags))continue ; -if (args->flags &ATTR_KERNOVAL ) { - args->valuelen = sfe->valuelen; -return 0 ; - } -if (args->valuelen < sfe->valuelen) { - args->valuelen = sfe->valuelen; -return -ERANGE ; - } - args->valuelen = sfe->valuelen; -memcpy (args->value, &sfe->nameval[args->namelen], - args->valuelen); -return 0 ; +return xfs_attr_copy_value (args, &sfe->nameval[args->namelen], + sfe->valuelen); }return -ENOATTR ; }@@ -2368,7 +2397,6 @@ xfs_attr3_leaf_getvalue(struct xfs_attr_leaf_entry *entry;struct xfs_attr_leaf_name_local *name_loc;struct xfs_attr_leaf_name_remote *name_rmt; -int valuelen; leaf = bp->b_addr;xfs_attr3_leaf_hdr_from_disk (args->geo, &ichdr, leaf);@@ -2380,18 +2408,9 @@ xfs_attr3_leaf_getvalue( name_loc =xfs_attr3_leaf_name_local (leaf, args->index);ASSERT (name_loc->namelen == args->namelen);ASSERT (memcmp (args->name, name_loc->nameval, args->namelen) ==0 ); - valuelen =be16_to_cpu (name_loc->valuelen); -if (args->flags &ATTR_KERNOVAL ) { - args->valuelen = valuelen; -return 0 ; - } -if (args->valuelen < valuelen) { - args->valuelen = valuelen; -return -ERANGE ; - } - args->valuelen = valuelen; -memcpy (args->value, &name_loc->nameval[args->namelen], valuelen); -return 0 ; +return xfs_attr_copy_value (args, + &name_loc->nameval[args->namelen], +be16_to_cpu (name_loc->valuelen)); } name_rmt =xfs_attr3_leaf_name_remote (leaf, args->index);@@ -2401,16 +2420,7 @@ xfs_attr3_leaf_getvalue( args->rmtblkno =be32_to_cpu (name_rmt->valueblk); args->rmtblkcnt =xfs_attr3_rmt_blocks (args->dp->i_mount, args->rmtvaluelen); -if (args->flags &ATTR_KERNOVAL ) { - args->valuelen = args->rmtvaluelen; -return 0 ; - } -if (args->valuelen < args->rmtvaluelen) { - args->valuelen = args->rmtvaluelen; -return -ERANGE ; - } - args->valuelen = args->rmtvaluelen; -return xfs_attr_rmtval_get (args); +return xfs_attr_copy_value (args,NULL , args->rmtvaluelen); }/*======================================================================== Signature · 1 line
2.23.0.rc1
Re: [PATCH 4/5] xfs: consolidate attribute value copying
In reply to Dave ChinnerQuoted 7 lines · Dave Chinner
From: Dave Chinner <[email protected]>
The same code is used to copy do the attribute copying in three different places. Consolidate them into a single function in preparation from on-demand buffer allocation.
Signed-off-by Dave Chinner <[email protected]>
Looks good,
PATCH 5/5 xfs: allocate xattr buffer on demand
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
When doing file lookups and checking for permissions, we end up in xfs_get_acl() to see if there are any ACLs on the inode. This requires and xattr lookup, and to do that we have to supply a buffer large enough to hold an maximum sized xattr.
On workloads were we are accessing a wide range of cache cold files under memory pressure (e.g. NFS fileservers) we end up spending a lot of time allocating the buffer. The buffer is 64k in length, so is a contiguous multi-page allocation, and if that then fails we fall back to vmalloc(). Hence the allocation here is /expensive/ when we are looking up hundreds of thousands of files a second.
Initial numbers from a bpf trace show average time in xfs_get_acl() is ~32us, with ~19us of that in the memory allocation. Note these are average times, so there are going to be affected by the worst case allocations more than the common fast case...
To avoid this, we could just do a "null" lookup to see if the ACL xattr exists and then only do the allocation if it exists. This, however, optimises the path for the "no ACL present" case at the expense of the "acl present" case. i.e. we can halve the time in xfs_get_acl() for the no acl case (i.e down to ~10-15us), but that then increases the ACL case by 30% (i.e. up to 40-45us).
To solve this and speed up both cases, drive the xattr buffer allocation into the attribute code once we know what the actual xattr length is. For the no-xattr case, we avoid the allocation completely, speeding up that case. For the common ACL case, we'll end up with a fast heap allocation (because it'll be smaller than a page), and only for the rarer "we have a remote xattr" will we have a multi-page allocation occur. Hence the common ACL case will be much faster, too.
--- fs/xfs/libxfs/xfs_attr.c | 42 ++++++++++++++++++++++++++++++----- fs/xfs/libxfs/xfs_attr.h | 6 +++-- fs/xfs/libxfs/xfs_attr_leaf.c | 6 +++++ fs/xfs/libxfs/xfs_da_btree.h | 4 +++- fs/xfs/xfs_acl.c | 12 ++++------ fs/xfs/xfs_ioctl.c | 2 +- fs/xfs/xfs_xattr.c | 2 +- 7 files changed, 55 insertions(+), 19 deletions(-)
fs/xfs/libxfs/xfs_attr.c +36 -6
@@ -118,12 +118,28 @@ xfs_attr_get_ilocked(return xfs_attr_node_get (args); } -/* Retrieve an extended attribute by name, and its value. */ +/* +* Retrieve an extended attribute by name, and its value if requested. +* +* If ATTR_KERNOVAL is set in @flags, then the caller does not want the value, +* just an indication whether the attribute exists and the size of the value if +* it exists. The size is returned in @valuelenp, +* +* If the attribute is found, but exceeds the size limit set by the caller in +* @valuelenp, return -ERANGE with the size of the attribute that was found in +* @valuelenp. +* +* If ATTR_ALLOC is set in @flags, allocate the buffer for the value after +* existence of the attribute has been determined. On success, return that +* buffer to the caller and leave them to free it. On failure, free any +* allocated buffer and ensure the buffer pointer returned to the caller is +* null. +*/ int xfs_attr_get (struct xfs_inode *ip,const unsigned char *name, -unsigned char *value, +unsigned char **value,int *valuelenp,int flags) {
@@ -131,6 +147,8 @@ xfs_attr_get(uint lock_mode;int error; +ASSERT ((flags & (ATTR_ALLOC |ATTR_KERNOVAL )) || *value); +XFS_STATS_INC (ip->i_mount, xs_attr_get);if (XFS_FORCED_SHUTDOWN (ip->i_mount))
@@ -140,17 +158,29 @@ xfs_attr_get(if (error)return error; - args.value = value; - args.valuelen = *valuelenp;/* Entirely possible to look up a name which doesn't exist */ args.op_flags =XFS_DA_OP_OKNOENT ; +if (flags &ATTR_ALLOC ) + args.op_flags |=XFS_DA_OP_ALLOCVAL ; +else + args.value = *value; + args.valuelen = *valuelenp; lock_mode =xfs_ilock_attr_map_shared (ip); error =xfs_attr_get_ilocked (ip, &args);xfs_iunlock (ip, lock_mode); - *valuelenp = args.valuelen; -return error; + +/* on error, we have to clean up allocated value buffers */ +if (error) { +if (flags &ATTR_ALLOC ) { +kmem_free (args.value); + *value =NULL ; + } +return error; + } + *value = args.value; +return 0 ; }/*
fs/xfs/libxfs/xfs_attr.h +4 -2
@@ -37,6 +37,7 @@ struct xfs_attr_list_context;#define ATTR_KERNOVAL 0x2000/* [kernel] get attr size only, not value */ #define ATTR_INCOMPLETE 0x4000/* [kernel] return INCOMPLETE attr keys */ +#define ATTR_ALLOC 0x8000/* allocate xattr buffer on demand */ #define XFS_ATTR_FLAGS \ { ATTR_DONTFOLLOW, "DONTFOLLOW" }, \
@@ -47,7 +48,8 @@ struct xfs_attr_list_context; { ATTR_REPLACE, "REPLACE" }, \ { ATTR_KERNOTIME, "KERNOTIME" }, \ { ATTR_KERNOVAL, "KERNOVAL" }, \ - { ATTR_INCOMPLETE, "INCOMPLETE" } + { ATTR_INCOMPLETE, "INCOMPLETE" }, \ + { ATTR_ALLOC, "ALLOC" }/* * The maximum size (into the kernel or returned from the kernel) of an
@@ -143,7 +145,7 @@ int xfs_attr_list_int(struct xfs_attr_list_context *);int xfs_inode_hasattr (struct xfs_inode *ip);int xfs_attr_get_ilocked (struct xfs_inode *ip,struct xfs_da_args *args);int xfs_attr_get (struct xfs_inode *ip,const unsigned char *name, -unsigned char *value,int *valuelenp,int flags); +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);
fs/xfs/libxfs/xfs_attr_leaf.c +6 -0
@@ -414,6 +414,12 @@ xfs_attr_copy_value( args->valuelen = valuelen;return -ERANGE ; } + +if (args->op_flags &XFS_DA_OP_ALLOCVAL ) { + args->value =kmem_alloc_large (valuelen,0 ); +if (!args->value) +return -ENOMEM ; + } args->valuelen = valuelen;/* remote block xattr requires IO for copy-in */
fs/xfs/libxfs/xfs_da_btree.h +3 -1
@@ -81,13 +81,15 @@ typedef struct xfs_da_args {#define XFS_DA_OP_ADDNAME 0x0004/* this is an add operation */ #define XFS_DA_OP_OKNOENT 0x0008/* lookup/add op, ENOENT ok, else die */ #define XFS_DA_OP_CILOOKUP 0x0010/* lookup to return CI name if found */ +#define XFS_DA_OP_ALLOCVAL 0x0020/* lookup to alloc buffer if found */ #define XFS_DA_OP_FLAGS \ { XFS_DA_OP_JUSTCHECK, "JUSTCHECK" }, \ { XFS_DA_OP_RENAME, "RENAME" }, \ { XFS_DA_OP_ADDNAME, "ADDNAME" }, \ { XFS_DA_OP_OKNOENT, "OKNOENT" }, \ - { XFS_DA_OP_CILOOKUP, "CILOOKUP" } + { XFS_DA_OP_CILOOKUP, "CILOOKUP" }, \ + { XFS_DA_OP_ALLOCVAL, "ALLOCVAL" }/* * Storage for holding state during Btree searches and split/join ops.
fs/xfs/xfs_acl.c +4 -8
@@ -112,7 +112,7 @@ xfs_get_acl(struct inode *inode, int type) {struct xfs_inode *ip =XFS_I (inode);struct posix_acl *acl =NULL ; -struct xfs_acl *xfs_acl; +struct xfs_acl *xfs_acl =NULL ;unsigned char *ea_name;int error;int len;
@@ -135,12 +135,8 @@ xfs_get_acl(struct inode *inode, int type)* go out to the disk. */ len =XFS_ACL_MAX_SIZE (ip->i_mount); - xfs_acl =kmem_zalloc_large (len,0 ); -if (!xfs_acl) -return ERR_PTR (-ENOMEM ); - - error =xfs_attr_get (ip, ea_name, (unsigned char *)xfs_acl, - &len,ATTR_ROOT ); + error =xfs_attr_get (ip, ea_name, (unsigned char **)&xfs_acl, &len, +ATTR_ALLOC |ATTR_ROOT );if (error) {/* * If the attribute doesn't exist make sure we have a negative
@@ -151,8 +147,8 @@ xfs_get_acl(struct inode *inode, int type) }else { acl =xfs_acl_from_disk (xfs_acl, len,XFS_ACL_MAX_ENTRIES (ip->i_mount)); +kmem_free (xfs_acl); } -kmem_free (xfs_acl);return acl; }
fs/xfs/xfs_ioctl.c +1 -1
@@ -438,7 +438,7 @@ xfs_attrmulti_attr_get(if (!kbuf)return -ENOMEM ; - error =xfs_attr_get (XFS_I (inode), name, kbuf, (int *)len, flags); + error =xfs_attr_get (XFS_I (inode), name, &kbuf, (int *)len, flags);if (error) goto out_kfree;
fs/xfs/xfs_xattr.c +1 -1
@@ -30,7 +30,7 @@ xfs_xattr_get(const struct xattr_handler *handler, struct dentry *unused, value =NULL ; } - error =xfs_attr_get (ip, (unsigned char *)name, value, &asize, xflags); + error =xfs_attr_get (ip, name, (unsigned char **)&value, &asize, xflags);if (error)return error;return asize;
Signature · 1 line
2.23.0.rc1
Quoted 35 lines · Dave Chinner
From: Dave Chinner <[email protected]>
When doing file lookups and checking for permissions, we end up in
xfs_get_acl()to see if there are any ACLs on the inode. This requires and xattr lookup, and to do that we have to supply a buffer large enough to hold an maximum sized xattr.On workloads were we are accessing a wide range of cache cold files under memory pressure (e.g. NFS fileservers) we end up spending a lot of time allocating the buffer. The buffer is 64k in length, so is a contiguous multi-page allocation, and if that then fails we fall back to
vmalloc(). Hence the allocation here is /expensive/ when we are looking up hundreds of thousands of files a second.Initial numbers from a bpf trace show average time in
xfs_get_acl()is ~32us, with ~19us of that in the memory allocation. Note these are average times, so there are going to be affected by the worst case allocations more than the common fast case...To avoid this, we could just do a "null" lookup to see if the ACL xattr exists and then only do the allocation if it exists. This, however, optimises the path for the "no ACL present" case at the expense of the "acl present" case. i.e. we can halve the time in
xfs_get_acl()for the no acl case (i.e down to ~10-15us), but that then increases the ACL case by 30% (i.e. up to 40-45us).To solve this and speed up both cases, drive the xattr buffer allocation into the attribute code once we know what the actual xattr length is. For the no-xattr case, we avoid the allocation completely, speeding up that case. For the common ACL case, we'll end up with a fast heap allocation (because it'll be smaller than a page), and only for the rarer "we have a remote xattr" will we have a multi-page allocation occur. Hence the common ACL case will be much faster, too.
Quoted 1 line
Signed-off-by Dave Chinner <[email protected]>
Looks ok, will throw this (and the dir speedup series) at the testing machine...
--D
Quoted 224 lines
--- fs/xfs/libxfs/xfs_attr.c | 42 ++++++++++++++++++++++++++++++----- fs/xfs/libxfs/xfs_attr.h | 6 +++-- fs/xfs/libxfs/xfs_attr_leaf.c | 6 +++++ fs/xfs/libxfs/xfs_da_btree.h | 4 +++- fs/xfs/xfs_acl.c | 12 ++++------ fs/xfs/xfs_ioctl.c | 2 +- fs/xfs/xfs_xattr.c | 2 +- 7 files changed, 55 insertions(+), 19 deletions(-)fs/xfs/libxfs/xfs_attr.c +36
-6@@ -118,12 +118,28 @@ xfs_attr_get_ilocked(return xfs_attr_node_get (args); } -/* Retrieve an extended attribute by name, and its value. */ +/* +* Retrieve an extended attribute by name, and its value if requested. +* +* If ATTR_KERNOVAL is set in @flags, then the caller does not want the value, +* just an indication whether the attribute exists and the size of the value if +* it exists. The size is returned in @valuelenp, +* +* If the attribute is found, but exceeds the size limit set by the caller in +* @valuelenp, return -ERANGE with the size of the attribute that was found in +* @valuelenp. +* +* If ATTR_ALLOC is set in @flags, allocate the buffer for the value after +* existence of the attribute has been determined. On success, return that +* buffer to the caller and leave them to free it. On failure, free any +* allocated buffer and ensure the buffer pointer returned to the caller is +* null. +*/ int xfs_attr_get (struct xfs_inode *ip,const unsigned char *name, -unsigned char *value, +unsigned char **value,int *valuelenp,int flags) {@@ -131,6 +147,8 @@ xfs_attr_get(uint lock_mode;int error; +ASSERT ((flags & (ATTR_ALLOC |ATTR_KERNOVAL )) || *value); +XFS_STATS_INC (ip->i_mount, xs_attr_get);if (XFS_FORCED_SHUTDOWN (ip->i_mount))@@ -140,17 +158,29 @@ xfs_attr_get(if (error)return error; - args.value = value; - args.valuelen = *valuelenp;/* Entirely possible to look up a name which doesn't exist */ args.op_flags =XFS_DA_OP_OKNOENT ; +if (flags &ATTR_ALLOC ) + args.op_flags |=XFS_DA_OP_ALLOCVAL ; +else + args.value = *value; + args.valuelen = *valuelenp; lock_mode =xfs_ilock_attr_map_shared (ip); error =xfs_attr_get_ilocked (ip, &args);xfs_iunlock (ip, lock_mode); - *valuelenp = args.valuelen; -return error; + +/* on error, we have to clean up allocated value buffers */ +if (error) { +if (flags &ATTR_ALLOC ) { +kmem_free (args.value); + *value =NULL ; + } +return error; + } + *value = args.value; +return 0 ; }/* fs/xfs/libxfs/xfs_attr.h +4
-2@@ -37,6 +37,7 @@ struct xfs_attr_list_context;#define ATTR_KERNOVAL 0x2000/* [kernel] get attr size only, not value */ #define ATTR_INCOMPLETE 0x4000/* [kernel] return INCOMPLETE attr keys */ +#define ATTR_ALLOC 0x8000/* allocate xattr buffer on demand */ #define XFS_ATTR_FLAGS \ { ATTR_DONTFOLLOW, "DONTFOLLOW" }, \@@ -47,7 +48,8 @@ struct xfs_attr_list_context; { ATTR_REPLACE, "REPLACE" }, \ { ATTR_KERNOTIME, "KERNOTIME" }, \ { ATTR_KERNOVAL, "KERNOVAL" }, \ - { ATTR_INCOMPLETE, "INCOMPLETE" } + { ATTR_INCOMPLETE, "INCOMPLETE" }, \ + { ATTR_ALLOC, "ALLOC" }/* * The maximum size (into the kernel or returned from the kernel) of an @@ -143,7 +145,7 @@ int xfs_attr_list_int(struct xfs_attr_list_context *);int xfs_inode_hasattr (struct xfs_inode *ip);int xfs_attr_get_ilocked (struct xfs_inode *ip,struct xfs_da_args *args);int xfs_attr_get (struct xfs_inode *ip,const unsigned char *name, -unsigned char *value,int *valuelenp,int flags); +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);fs/xfs/libxfs/xfs_attr_leaf.c +6
-0@@ -414,6 +414,12 @@ xfs_attr_copy_value( args->valuelen = valuelen;return -ERANGE ; } + +if (args->op_flags &XFS_DA_OP_ALLOCVAL ) { + args->value =kmem_alloc_large (valuelen,0 ); +if (!args->value) +return -ENOMEM ; + } args->valuelen = valuelen;/* remote block xattr requires IO for copy-in */ fs/xfs/libxfs/xfs_da_btree.h +3
-1@@ -81,13 +81,15 @@ typedef struct xfs_da_args {#define XFS_DA_OP_ADDNAME 0x0004/* this is an add operation */ #define XFS_DA_OP_OKNOENT 0x0008/* lookup/add op, ENOENT ok, else die */ #define XFS_DA_OP_CILOOKUP 0x0010/* lookup to return CI name if found */ +#define XFS_DA_OP_ALLOCVAL 0x0020/* lookup to alloc buffer if found */ #define XFS_DA_OP_FLAGS \ { XFS_DA_OP_JUSTCHECK, "JUSTCHECK" }, \ { XFS_DA_OP_RENAME, "RENAME" }, \ { XFS_DA_OP_ADDNAME, "ADDNAME" }, \ { XFS_DA_OP_OKNOENT, "OKNOENT" }, \ - { XFS_DA_OP_CILOOKUP, "CILOOKUP" } + { XFS_DA_OP_CILOOKUP, "CILOOKUP" }, \ + { XFS_DA_OP_ALLOCVAL, "ALLOCVAL" }/* * Storage for holding state during Btree searches and split/join ops. fs/xfs/xfs_acl.c +4
-8@@ -112,7 +112,7 @@ xfs_get_acl(struct inode *inode, int type) {struct xfs_inode *ip =XFS_I (inode);struct posix_acl *acl =NULL ; -struct xfs_acl *xfs_acl; +struct xfs_acl *xfs_acl =NULL ;unsigned char *ea_name;int error;int len;@@ -135,12 +135,8 @@ xfs_get_acl(struct inode *inode, int type)* go out to the disk. */ len =XFS_ACL_MAX_SIZE (ip->i_mount); - xfs_acl =kmem_zalloc_large (len,0 ); -if (!xfs_acl) -return ERR_PTR (-ENOMEM ); - - error =xfs_attr_get (ip, ea_name, (unsigned char *)xfs_acl, - &len,ATTR_ROOT ); + error =xfs_attr_get (ip, ea_name, (unsigned char **)&xfs_acl, &len, +ATTR_ALLOC |ATTR_ROOT );if (error) {/* * If the attribute doesn't exist make sure we have a negative @@ -151,8 +147,8 @@ xfs_get_acl(struct inode *inode, int type) }else { acl =xfs_acl_from_disk (xfs_acl, len,XFS_ACL_MAX_ENTRIES (ip->i_mount)); +kmem_free (xfs_acl); } -kmem_free (xfs_acl);return acl; }fs/xfs/xfs_ioctl.c +1
-1@@ -438,7 +438,7 @@ xfs_attrmulti_attr_get(if (!kbuf)return -ENOMEM ; - error =xfs_attr_get (XFS_I (inode), name, kbuf, (int *)len, flags); + error =xfs_attr_get (XFS_I (inode), name, &kbuf, (int *)len, flags);if (error) goto out_kfree;fs/xfs/xfs_xattr.c +1
-1@@ -30,7 +30,7 @@ xfs_xattr_get(const struct xattr_handler *handler, struct dentry *unused, value =NULL ; } - error =xfs_attr_get (ip, (unsigned char *)name, value, &asize, xflags); + error =xfs_attr_get (ip, name, (unsigned char **)&value, &asize, xflags);if (error)return error;return asize;Signature · 1 line
2.23.0.rc1
Quoted 35 lines · Dave Chinner
From: Dave Chinner <[email protected]>
When doing file lookups and checking for permissions, we end up in
xfs_get_acl()to see if there are any ACLs on the inode. This requires and xattr lookup, and to do that we have to supply a buffer large enough to hold an maximum sized xattr.On workloads were we are accessing a wide range of cache cold files under memory pressure (e.g. NFS fileservers) we end up spending a lot of time allocating the buffer. The buffer is 64k in length, so is a contiguous multi-page allocation, and if that then fails we fall back to
vmalloc(). Hence the allocation here is /expensive/ when we are looking up hundreds of thousands of files a second.Initial numbers from a bpf trace show average time in
xfs_get_acl()is ~32us, with ~19us of that in the memory allocation. Note these are average times, so there are going to be affected by the worst case allocations more than the common fast case...To avoid this, we could just do a "null" lookup to see if the ACL xattr exists and then only do the allocation if it exists. This, however, optimises the path for the "no ACL present" case at the expense of the "acl present" case. i.e. we can halve the time in
xfs_get_acl()for the no acl case (i.e down to ~10-15us), but that then increases the ACL case by 30% (i.e. up to 40-45us).To solve this and speed up both cases, drive the xattr buffer allocation into the attribute code once we know what the actual xattr length is. For the no-xattr case, we avoid the allocation completely, speeding up that case. For the common ACL case, we'll end up with a fast heap allocation (because it'll be smaller than a page), and only for the rarer "we have a remote xattr" will we have a multi-page allocation occur. Hence the common ACL case will be much faster, too.
Quoted 1 line
Signed-off-by Dave Chinner <[email protected]>
Looks good,