[PATCH 0/3 v2] xfs: allocate xattr buffer on demand

[PATCH 0/3 v2] xfs: allocate xattr buffer on demand

[Dave Chinner]

Ok, this one works on v4 format filesystems with 256 byte inodes.

Three patches now, the first cleans up the error returns from
the shortform, leaf and node for attritbute value retrieval. The
existing code conflates the lookup function return values with the
return values needed for retreiving the attribute values.

That is, shortform returns -EEXIST to indicate that the attribute
exists and was retrieved, and returns -ENOATTR is it didn't exist.

Leaf and node form return zero if it was retrieved, -ENOATTR is it
didn't exist, and some other negative error if something else went
wrong. And some of the return values are hidden several layers deep
in the remote attribute read code.

SO the first patch cleans this all up and makes it consistent, and
cleans up some of the error checking and remote attribute retrieval
for leaf/node format code.

The second patch folds the remote attr retreival into the
xfs_attr_leaf_getvalue() function, rather than doing it on return
and having to check all over again whether the attr was a remote
attr.

The thrid patch what remains of the original single patch - it
factors out the copying of the attribute value from the shortform
and leaf code, making them all use the same code for buffer
allocation and remote value retrieval. The high level code is
modified to allow the retrieval code to allocate the buffer on
demand.

The previous patch failed because ACLs in leaf format attributes are
only covered by the xfstests "attr" group when using 256 byte
inodes.  generic/318 creates an acl that doesn't fit in line in a
256 byte inode, so it was tripping over the return value
inconsistency that the first patch in this series fixes up.

Cheers,

Dave.

[PATCH 1/3] xfs: make attr lookup returns consistent

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

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.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_attr.c        | 57 +++++++++++++++++++++------------
 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, 49 insertions(+), 27 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index d48fcf11cc35..776343c4f22b 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -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 int
 xfs_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.
+ * Retreive 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 int
 xfs_attr_node_get(xfs_da_args_t *args)
@@ -1289,29 +1301,32 @@ xfs_attr_node_get(xfs_da_args_t *args)
 	state->mp = args->dp->i_mount;
 
 	/*
-	 * Search to see if name exists, and get back a pointer to it.
+	  Search to see if name exists, and get back a pointer to it.
 	 */
 	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;
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 70eb941d02e4..d056767b5c53 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -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(
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 4eb30d357045..3e39b7d40f25 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -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(
diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
index 1afc58bf71dd..e9248ad4f842 100644
--- a/fs/xfs/scrub/attr.c
+++ b/fs/xfs/scrub/attr.c
@@ -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;
-- 
2.23.0.rc1

[PATCH 2/3] xfs: move remote attr retrieval into xfs_attr3_leaf_getvalue

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

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.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_attr.c      | 16 +---------------
 fs/xfs/libxfs/xfs_attr_leaf.c | 35 ++++++++++++++++++-----------------
 2 files changed, 19 insertions(+), 32 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 776343c4f22b..5e6b6846e607 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -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.
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index d056767b5c53..e325cdbc9818 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -2391,25 +2391,26 @@ 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;
 	}
-	return 0;
+	if (args->valuelen < args->rmtvaluelen) {
+		args->valuelen = args->rmtvaluelen;
+		return -ERANGE;
+	}
+	args->valuelen = args->rmtvaluelen;
+	return xfs_attr_rmtval_get(args);
 }
 
 /*========================================================================
-- 
2.23.0.rc1

[PATCH 3/3] xfs: allocate xattr buffer on demand

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

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.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_attr.c      | 42 +++++++++++++---
 fs/xfs/libxfs/xfs_attr.h      |  6 ++-
 fs/xfs/libxfs/xfs_attr_leaf.c | 95 +++++++++++++++++++++--------------
 fs/xfs/libxfs/xfs_da_btree.h  |  4 +-
 fs/xfs/xfs_acl.c              | 16 ++----
 fs/xfs/xfs_ioctl.c            |  2 +-
 fs/xfs/xfs_xattr.c            |  2 +-
 7 files changed, 105 insertions(+), 62 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 5e6b6846e607..99f9e0cf05a6 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -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;
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index ff28ebf3b635..94badfa1743e 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -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);
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index e325cdbc9818..fe3c164d5cfe 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -393,6 +393,51 @@ 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;
+	}
+
+	if (args->op_flags & XFS_DA_OP_ALLOCVAL) {
+		args->value = kmem_alloc_large(valuelen, KM_SLEEP);
+		if (!args->value)
+			return -ENOMEM;
+	}
+	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 +772,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 +790,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 +2404,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 +2415,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 +2427,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);
 }
 
 /*========================================================================
diff --git a/fs/xfs/libxfs/xfs_da_btree.h b/fs/xfs/libxfs/xfs_da_btree.h
index 84dd865b6c3d..ae0bbd20d9ca 100644
--- a/fs/xfs/libxfs/xfs_da_btree.h
+++ b/fs/xfs/libxfs/xfs_da_btree.h
@@ -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.
diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index cbda40d40326..78c5e590b771 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -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;
@@ -130,17 +130,9 @@ xfs_get_acl(struct inode *inode, int type)
 		BUG();
 	}
 
-	/*
-	 * If we have a cached ACLs value just return it, not need to
-	 * go out to the disk.
-	 */
 	len = XFS_ACL_MAX_SIZE(ip->i_mount);
-	xfs_acl = kmem_zalloc_large(len, KM_SLEEP);
-	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 +143,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;
 }
 
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 6f7848cd5527..5f73feb40384 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -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;
 
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index 3123b5aaad2a..cb895b1df5e4 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -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;
-- 
2.23.0.rc1

Re: [PATCH 3/3] xfs: allocate xattr buffer on demand

[Darrick J. Wong]

On Wed, Aug 28, 2019 at 02:23:50PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_attr.c      | 42 +++++++++++++---
>  fs/xfs/libxfs/xfs_attr.h      |  6 ++-
>  fs/xfs/libxfs/xfs_attr_leaf.c | 95 +++++++++++++++++++++--------------
>  fs/xfs/libxfs/xfs_da_btree.h  |  4 +-
>  fs/xfs/xfs_acl.c              | 16 ++----
>  fs/xfs/xfs_ioctl.c            |  2 +-
>  fs/xfs/xfs_xattr.c            |  2 +-
>  7 files changed, 105 insertions(+), 62 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 5e6b6846e607..99f9e0cf05a6 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -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;
>  }
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index ff28ebf3b635..94badfa1743e 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -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);
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index e325cdbc9818..fe3c164d5cfe 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -393,6 +393,51 @@ 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;
> +	}
> +
> +	if (args->op_flags & XFS_DA_OP_ALLOCVAL) {
> +		args->value = kmem_alloc_large(valuelen, KM_SLEEP);
> +		if (!args->value)
> +			return -ENOMEM;
> +	}
> +	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 +772,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 +790,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 +2404,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 +2415,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 +2427,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);
>  }
>  
>  /*========================================================================
> diff --git a/fs/xfs/libxfs/xfs_da_btree.h b/fs/xfs/libxfs/xfs_da_btree.h
> index 84dd865b6c3d..ae0bbd20d9ca 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.h
> +++ b/fs/xfs/libxfs/xfs_da_btree.h
> @@ -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.
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index cbda40d40326..78c5e590b771 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
> @@ -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;
> @@ -130,17 +130,9 @@ xfs_get_acl(struct inode *inode, int type)
>  		BUG();
>  	}
>  
> -	/*
> -	 * If we have a cached ACLs value just return it, not need to
> -	 * go out to the disk.
> -	 */
>  	len = XFS_ACL_MAX_SIZE(ip->i_mount);
> -	xfs_acl = kmem_zalloc_large(len, KM_SLEEP);
> -	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 +143,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;
>  }
>  
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 6f7848cd5527..5f73feb40384 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -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;
>  
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index 3123b5aaad2a..cb895b1df5e4 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -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;
> -- 
> 2.23.0.rc1
> 

Re: [PATCH 2/3] xfs: move remote attr retrieval into xfs_attr3_leaf_getvalue

[Darrick J. Wong]

On Wed, Aug 28, 2019 at 02:23:49PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_attr.c      | 16 +---------------
>  fs/xfs/libxfs/xfs_attr_leaf.c | 35 ++++++++++++++++++-----------------
>  2 files changed, 19 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 776343c4f22b..5e6b6846e607 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -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.
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index d056767b5c53..e325cdbc9818 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -2391,25 +2391,26 @@ 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;
>  	}
> -	return 0;
> +	if (args->valuelen < args->rmtvaluelen) {
> +		args->valuelen = args->rmtvaluelen;
> +		return -ERANGE;
> +	}
> +	args->valuelen = args->rmtvaluelen;
> +	return xfs_attr_rmtval_get(args);
>  }
>  
>  /*========================================================================
> -- 
> 2.23.0.rc1
> 

Re: [PATCH 1/3] xfs: make attr lookup returns consistent

[Darrick J. Wong]

On Wed, Aug 28, 2019 at 02:23:48PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c        | 57 +++++++++++++++++++++------------
>  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, 49 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index d48fcf11cc35..776343c4f22b 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -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 int
>  xfs_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.
> + * Retreive the attribute data from a node attribute list.

"Retrieve"

>   *
>   * 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 int
>  xfs_attr_node_get(xfs_da_args_t *args)
> @@ -1289,29 +1301,32 @@ xfs_attr_node_get(xfs_da_args_t *args)
>  	state->mp = args->dp->i_mount;
>  
>  	/*
> -	 * Search to see if name exists, and get back a pointer to it.
> +	  Search to see if name exists, and get back a pointer to it.

Comment damage here?

Meh whatever will just fix it.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  	 */
>  	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;
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 70eb941d02e4..d056767b5c53 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -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(
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
> index 4eb30d357045..3e39b7d40f25 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -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(
> diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> index 1afc58bf71dd..e9248ad4f842 100644
> --- a/fs/xfs/scrub/attr.c
> +++ b/fs/xfs/scrub/attr.c
> @@ -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;
> -- 
> 2.23.0.rc1
> 

Re: [PATCH 1/3] xfs: make attr lookup returns consistent

[Christoph Hellwig]

On Wed, Aug 28, 2019 at 02:23:48PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c        | 57 +++++++++++++++++++++------------
>  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, 49 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index d48fcf11cc35..776343c4f22b 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -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 int
>  xfs_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.
> + * Retreive 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 int
>  xfs_attr_node_get(xfs_da_args_t *args)
> @@ -1289,29 +1301,32 @@ xfs_attr_node_get(xfs_da_args_t *args)
>  	state->mp = args->dp->i_mount;
>  
>  	/*
> -	 * Search to see if name exists, and get back a pointer to it.
> +	  Search to see if name exists, and get back a pointer to it.
>  	 */
>  	error = xfs_da3_node_lookup_int(state, &retval);
>  	if (error) {
>  		retval = error;

Given that you are cleaning up this mess, can you check if there
is any point in the weird xfs_da3_node_lookup_int calling conventions?
It looks like it can return errnos in both the return value and
*revtval, and from a quick check it seems like all callers treat them
more or less the same.

Re: [PATCH 2/3] xfs: move remote attr retrieval into xfs_attr3_leaf_getvalue

[Christoph Hellwig]

On Wed, Aug 28, 2019 at 02:23:49PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> 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.

It also refactors xfs_attr3_leaf_getvalue to be more readable first.
Which confused the heck out of me when reading the code.  I'd prefer
that to be split into a prep patch, but on its own both changes look
good to me, so:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/3] xfs: allocate xattr buffer on demand

[Christoph Hellwig]

On Wed, Aug 28, 2019 at 02:23:50PM +1000, Dave Chinner wrote:
> + * 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.

Given that all three callers pass ATTR_ALLOC, do we even need a flag
and keep the old behavior around, at least at the xfs_attr_get level?
For the lower level we still have scrub, but that fills out the args
structure directly.

> +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;
> +	}
> +
> +	if (args->op_flags & XFS_DA_OP_ALLOCVAL) {
> +		args->value = kmem_alloc_large(valuelen, KM_SLEEP);
> +		if (!args->value)
> +			return -ENOMEM;
> +	}
> +	args->valuelen = valuelen;

Can't we just move the setting of ->valuelen up to common code shared
between all the branches?  That means it would also be set on an
allocation error, but that should be harmless.

> +	/* 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;
> +}

Can you split creating this helper into a separate prep patch?  While
not strictly required it would make reviewing what is consolidation
vs what is new code for the on-demand buffer allocation a little easier.

> +	error = xfs_attr_get(ip, ea_name, (unsigned char **)&xfs_acl, &len,
> +				ATTR_ALLOC|ATTR_ROOT);

Please keep space between the symbols and | on each side.

Re: [PATCH 1/3] xfs: make attr lookup returns consistent

[Dave Chinner]

On Thu, Aug 29, 2019 at 12:41:39AM -0700, Christoph Hellwig wrote:
> On Wed, Aug 28, 2019 at 02:23:48PM +1000, Dave Chinner wrote:
> > @@ -1289,29 +1301,32 @@ xfs_attr_node_get(xfs_da_args_t *args)
> >  	state->mp = args->dp->i_mount;
> >  
> >  	/*
> > -	 * Search to see if name exists, and get back a pointer to it.
> > +	  Search to see if name exists, and get back a pointer to it.
> >  	 */
> >  	error = xfs_da3_node_lookup_int(state, &retval);
> >  	if (error) {
> >  		retval = error;
> 
> Given that you are cleaning up this mess, can you check if there
> is any point in the weird xfs_da3_node_lookup_int calling conventions?

retval propagates down into child functions like
xfs_da3_path_shift(), xfs_dir2_leafn_lookup_int(), etc, so
untangling that mess is non-trivial.

> It looks like it can return errnos in both the return value and
> *revtval, and from a quick check it seems like all callers treat them
> more or less the same.

Maybe so, but I don't have the time to do a deep dive into both the
directory and the attribute code to determine what such a cleanup
might look like. I think it's way out of scope for the problem being
solved by this patchset...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] xfs: allocate xattr buffer on demand

[Dave Chinner]

On Thu, Aug 29, 2019 at 12:55:59AM -0700, Christoph Hellwig wrote:
> On Wed, Aug 28, 2019 at 02:23:50PM +1000, Dave Chinner wrote:
> > + * 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.
> 
> Given that all three callers pass ATTR_ALLOC, do we even need a flag

Only one caller passes ATTR_ALLOC - the ACL code. The other two
have their own buffers that are supplied....

> and keep the old behavior around, at least at the xfs_attr_get level?
> For the lower level we still have scrub, but that fills out the args
> structure directly.
> 
> > +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;
> > +	}
> > +
> > +	if (args->op_flags & XFS_DA_OP_ALLOCVAL) {
> > +		args->value = kmem_alloc_large(valuelen, KM_SLEEP);
> > +		if (!args->value)
> > +			return -ENOMEM;
> > +	}
> > +	args->valuelen = valuelen;
> 
> Can't we just move the setting of ->valuelen up to common code shared
> between all the branches?  That means it would also be set on an
> allocation error, but that should be harmless.

Can't overwrite args->valuelen until we've done the ERANGE check.
Sure, I could put it in a local variable, but that doesn't reduce
the amount of code, or make it obvious that we intentionally return
the attribute size when the supplied buffer it too small...

> > +	/* 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;
> > +}
> 
> Can you split creating this helper into a separate prep patch?  While
> not strictly required it would make reviewing what is consolidation
> vs what is new code for the on-demand buffer allocation a little easier.

ok.

> > +	error = xfs_attr_get(ip, ea_name, (unsigned char **)&xfs_acl, &len,
> > +				ATTR_ALLOC|ATTR_ROOT);
> 
> Please keep space between the symbols and | on each side.

Fixed.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] xfs: allocate xattr buffer on demand

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 08:45:16PM +1000, Dave Chinner wrote:
> > Given that all three callers pass ATTR_ALLOC, do we even need a flag
> 
> Only one caller passes ATTR_ALLOC - the ACL code. The other two
> have their own buffers that are supplied....

Oops, I misread the patch as all three callers changed, but not
actually to pass the flag but just for the different buffer passing.

That being said - xfs_attrmulti_attr_get can trivially use this
scheme.  And for the VFS call it would also make sense, but it
would be a huge change, so maybe some other time.

> Can't overwrite args->valuelen until we've done the ERANGE check.
> Sure, I could put it in a local variable, but that doesn't reduce
> the amount of code, or make it obvious that we intentionally return
> the attribute size when the supplied buffer it too small...

Ok.