[PATCHSET 0/6] xfs: cleanups for logged xattr updates

[PATCHSET 0/6] xfs: cleanups for logged xattr updates

[Darrick J. Wong]

Hi all,

Here are a bunch of cleanups to the logged xattr code to use slab caches
for better memory efficiency, reduce the size of the attr intent
tracking structure, fix some wonkinessin the attri/attrd cache
constructors, and give things more appropriat enames.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D
---
 fs/xfs/libxfs/xfs_attr.c        |  165 +++++++++++++++++----------------------
 fs/xfs/libxfs/xfs_attr.h        |   54 +++++--------
 fs/xfs/libxfs/xfs_attr_remote.c |    6 +
 fs/xfs/libxfs/xfs_attr_remote.h |    6 +
 fs/xfs/libxfs/xfs_da_btree.c    |   11 +++
 fs/xfs/libxfs/xfs_da_btree.h    |    1 
 fs/xfs/libxfs/xfs_defer.c       |    8 --
 fs/xfs/libxfs/xfs_log_format.h  |    8 +-
 fs/xfs/xfs_attr_item.c          |   56 +++++++------
 fs/xfs/xfs_attr_item.h          |    9 +-
 fs/xfs/xfs_super.c              |   19 ++++
 11 files changed, 176 insertions(+), 167 deletions(-)

[PATCH 1/6] xfs: clean up xfs_attr_node_hasname

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

The calling conventions of this function are a mess -- callers /can/
provide a pointer to a pointer to a state structure, but it's not
required, and as evidenced by the last two patches, the callers that do
weren't be careful enough about how to deal with an existing da state.

Push the allocation and freeing responsibilty to the callers, which
means that callers from the xattr node state machine steps now have the
visibility to allocate or free the da state structure as they please.
As a bonus, the node remove/add paths for larp-mode replaces can reset
the da state structure instead of freeing and immediately reallocating
it.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c     |   63 +++++++++++++++++++++---------------------
 fs/xfs/libxfs/xfs_da_btree.c |   11 +++++++
 fs/xfs/libxfs/xfs_da_btree.h |    1 +
 3 files changed, 44 insertions(+), 31 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 499a15480b57..381b8d46529a 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -61,8 +61,8 @@ STATIC void xfs_attr_restore_rmt_blk(struct xfs_da_args *args);
 static int xfs_attr_node_try_addname(struct xfs_attr_item *attr);
 STATIC int xfs_attr_node_addname_find_attr(struct xfs_attr_item *attr);
 STATIC int xfs_attr_node_remove_attr(struct xfs_attr_item *attr);
-STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
-				 struct xfs_da_state **state);
+STATIC int xfs_attr_node_lookup(struct xfs_da_args *args,
+		struct xfs_da_state *state);
 
 int
 xfs_inode_hasattr(
@@ -594,6 +594,19 @@ xfs_attr_leaf_mark_incomplete(
 	return xfs_attr3_leaf_setflag(args);
 }
 
+/* Ensure the da state of an xattr deferred work item is ready to go. */
+static inline void
+xfs_attr_item_ensure_da_state(
+	struct xfs_attr_item	*attr)
+{
+	struct xfs_da_args	*args = attr->xattri_da_args;
+
+	if (!attr->xattri_da_state)
+		attr->xattri_da_state = xfs_da_state_alloc(args);
+	else
+		xfs_da_state_reset(attr->xattri_da_state, args);
+}
+
 /*
  * Initial setup for xfs_attr_node_removename.  Make sure the attr is there and
  * the blocks are valid.  Attr keys with remote blocks will be marked
@@ -607,7 +620,8 @@ int xfs_attr_node_removename_setup(
 	struct xfs_da_state		*state;
 	int				error;
 
-	error = xfs_attr_node_hasname(args, &attr->xattri_da_state);
+	xfs_attr_item_ensure_da_state(attr);
+	error = xfs_attr_node_lookup(args, attr->xattri_da_state);
 	if (error != -EEXIST)
 		goto out;
 	error = 0;
@@ -855,6 +869,7 @@ xfs_attr_lookup(
 {
 	struct xfs_inode	*dp = args->dp;
 	struct xfs_buf		*bp = NULL;
+	struct xfs_da_state	*state;
 	int			error;
 
 	if (!xfs_inode_hasattr(dp))
@@ -872,7 +887,10 @@ xfs_attr_lookup(
 		return error;
 	}
 
-	return xfs_attr_node_hasname(args, NULL);
+	state = xfs_da_state_alloc(args);
+	error = xfs_attr_node_lookup(args, state);
+	xfs_da_state_free(state);
+	return error;
 }
 
 static int
@@ -1387,34 +1405,20 @@ xfs_attr_leaf_get(xfs_da_args_t *args)
 	return error;
 }
 
-/*
- * Return EEXIST if attr is found, or ENOATTR if not
- * statep: If not null is set to point at the found state.  Caller will
- *         be responsible for freeing the state in this case.
- */
+/* Return EEXIST if attr is found, or ENOATTR if not. */
 STATIC int
-xfs_attr_node_hasname(
+xfs_attr_node_lookup(
 	struct xfs_da_args	*args,
-	struct xfs_da_state	**statep)
+	struct xfs_da_state	*state)
 {
-	struct xfs_da_state	*state;
 	int			retval, error;
 
-	state = xfs_da_state_alloc(args);
-	if (statep != NULL) {
-		ASSERT(*statep == NULL);
-		*statep = state;
-	}
-
 	/*
 	 * 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;
-
-	if (!statep)
-		xfs_da_state_free(state);
+		return error;
 
 	return retval;
 }
@@ -1430,15 +1434,12 @@ xfs_attr_node_addname_find_attr(
 	struct xfs_da_args	*args = attr->xattri_da_args;
 	int			error;
 
-	if (attr->xattri_da_state)
-		xfs_da_state_free(attr->xattri_da_state);
-	attr->xattri_da_state = NULL;
-
 	/*
 	 * Search to see if name already exists, and get back a pointer
 	 * to where it should go.
 	 */
-	error = xfs_attr_node_hasname(args, &attr->xattri_da_state);
+	xfs_attr_item_ensure_da_state(attr);
+	error = xfs_attr_node_lookup(args, attr->xattri_da_state);
 	switch (error) {
 	case -ENOATTR:
 		if (args->op_flags & XFS_DA_OP_REPLACE)
@@ -1599,7 +1600,7 @@ STATIC int
 xfs_attr_node_get(
 	struct xfs_da_args	*args)
 {
-	struct xfs_da_state	*state = NULL;
+	struct xfs_da_state	*state;
 	struct xfs_da_state_blk	*blk;
 	int			i;
 	int			error;
@@ -1609,7 +1610,8 @@ xfs_attr_node_get(
 	/*
 	 * Search to see if name exists, and get back a pointer to it.
 	 */
-	error = xfs_attr_node_hasname(args, &state);
+	state = xfs_da_state_alloc(args);
+	error = xfs_attr_node_lookup(args, state);
 	if (error != -EEXIST)
 		goto out_release;
 
@@ -1628,8 +1630,7 @@ xfs_attr_node_get(
 		state->path.blk[i].bp = NULL;
 	}
 
-	if (state)
-		xfs_da_state_free(state);
+	xfs_da_state_free(state);
 	return error;
 }
 
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index aa74f3fdb571..e7201dc68f43 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -117,6 +117,17 @@ xfs_da_state_free(xfs_da_state_t *state)
 	kmem_cache_free(xfs_da_state_cache, state);
 }
 
+void
+xfs_da_state_reset(
+	struct xfs_da_state	*state,
+	struct xfs_da_args	*args)
+{
+	xfs_da_state_kill_altpath(state);
+	memset(state, 0, sizeof(struct xfs_da_state));
+	state->args = args;
+	state->mp = state->args->dp->i_mount;
+}
+
 static inline int xfs_dabuf_nfsb(struct xfs_mount *mp, int whichfork)
 {
 	if (whichfork == XFS_DATA_FORK)
diff --git a/fs/xfs/libxfs/xfs_da_btree.h b/fs/xfs/libxfs/xfs_da_btree.h
index ed2303e4d46a..d33b7686a0b3 100644
--- a/fs/xfs/libxfs/xfs_da_btree.h
+++ b/fs/xfs/libxfs/xfs_da_btree.h
@@ -225,6 +225,7 @@ enum xfs_dacmp xfs_da_compname(struct xfs_da_args *args,
 
 struct xfs_da_state *xfs_da_state_alloc(struct xfs_da_args *args);
 void xfs_da_state_free(xfs_da_state_t *state);
+void xfs_da_state_reset(struct xfs_da_state *state, struct xfs_da_args *args);
 
 void	xfs_da3_node_hdr_from_disk(struct xfs_mount *mp,
 		struct xfs_da3_icnode_hdr *to, struct xfs_da_intnode *from);

[PATCH 2/6] xfs: put the xattr intent item op flags in their own namespace

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

The flags that are stored in the extended attr intent log item really
should have a separate namespace from the rest of the XFS_ATTR_* flags.
Give them one to make it a little more obvious that they're intent item
flags.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c       |    6 +++---
 fs/xfs/libxfs/xfs_attr.h       |    2 +-
 fs/xfs/libxfs/xfs_log_format.h |    8 ++++----
 fs/xfs/xfs_attr_item.c         |   20 ++++++++++----------
 4 files changed, 18 insertions(+), 18 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 381b8d46529a..0f88f6e17101 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -918,7 +918,7 @@ xfs_attr_defer_add(
 	struct xfs_attr_item	*new;
 	int			error = 0;
 
-	error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET, &new);
+	error = xfs_attr_item_init(args, XFS_ATTRI_OP_FLAGS_SET, &new);
 	if (error)
 		return error;
 
@@ -937,7 +937,7 @@ xfs_attr_defer_replace(
 	struct xfs_attr_item	*new;
 	int			error = 0;
 
-	error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REPLACE, &new);
+	error = xfs_attr_item_init(args, XFS_ATTRI_OP_FLAGS_REPLACE, &new);
 	if (error)
 		return error;
 
@@ -957,7 +957,7 @@ xfs_attr_defer_remove(
 	struct xfs_attr_item	*new;
 	int			error;
 
-	error  = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REMOVE, &new);
+	error  = xfs_attr_item_init(args, XFS_ATTRI_OP_FLAGS_REMOVE, &new);
 	if (error)
 		return error;
 
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 1af7abe29eef..c739caa11a4b 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -530,7 +530,7 @@ struct xfs_attr_item {
 	enum xfs_delattr_state		xattri_dela_state;
 
 	/*
-	 * Attr operation being performed - XFS_ATTR_OP_FLAGS_*
+	 * Attr operation being performed - XFS_ATTRI_OP_FLAGS_*
 	 */
 	unsigned int			xattri_op_flags;
 
diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index 5017500bfd8b..377b230aecfd 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -906,10 +906,10 @@ struct xfs_icreate_log {
  * Flags for deferred attribute operations.
  * Upper bits are flags, lower byte is type code
  */
-#define XFS_ATTR_OP_FLAGS_SET		1	/* Set the attribute */
-#define XFS_ATTR_OP_FLAGS_REMOVE	2	/* Remove the attribute */
-#define XFS_ATTR_OP_FLAGS_REPLACE	3	/* Replace the attribute */
-#define XFS_ATTR_OP_FLAGS_TYPE_MASK	0xFF	/* Flags type mask */
+#define XFS_ATTRI_OP_FLAGS_SET		1	/* Set the attribute */
+#define XFS_ATTRI_OP_FLAGS_REMOVE	2	/* Remove the attribute */
+#define XFS_ATTRI_OP_FLAGS_REPLACE	3	/* Replace the attribute */
+#define XFS_ATTRI_OP_FLAGS_TYPE_MASK	0xFF	/* Flags type mask */
 
 #define XFS_ATTRI_FILTER_ROOT		(1u << XFS_ATTR_ROOT_BIT)
 #define XFS_ATTRI_FILTER_SECURE		(1u << XFS_ATTR_SECURE_BIT)
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 7cbb640d7856..930366055013 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -350,7 +350,7 @@ xfs_attr_log_item(
 	attrp = &attrip->attri_format;
 	attrp->alfi_ino = attr->xattri_da_args->dp->i_ino;
 	attrp->alfi_op_flags = attr->xattri_op_flags &
-						XFS_ATTR_OP_FLAGS_TYPE_MASK;
+						XFS_ATTRI_OP_FLAGS_TYPE_MASK;
 	attrp->alfi_value_len = attr->xattri_da_args->valuelen;
 	attrp->alfi_name_len = attr->xattri_da_args->namelen;
 	attrp->alfi_attr_filter = attr->xattri_da_args->attr_filter &
@@ -493,12 +493,12 @@ xfs_attri_validate(
 	struct xfs_attri_log_format	*attrp)
 {
 	unsigned int			op = attrp->alfi_op_flags &
-					     XFS_ATTR_OP_FLAGS_TYPE_MASK;
+					     XFS_ATTRI_OP_FLAGS_TYPE_MASK;
 
 	if (attrp->__pad != 0)
 		return false;
 
-	if (attrp->alfi_op_flags & ~XFS_ATTR_OP_FLAGS_TYPE_MASK)
+	if (attrp->alfi_op_flags & ~XFS_ATTRI_OP_FLAGS_TYPE_MASK)
 		return false;
 
 	if (attrp->alfi_attr_filter & ~XFS_ATTRI_FILTER_MASK)
@@ -506,9 +506,9 @@ xfs_attri_validate(
 
 	/* alfi_op_flags should be either a set or remove */
 	switch (op) {
-	case XFS_ATTR_OP_FLAGS_SET:
-	case XFS_ATTR_OP_FLAGS_REPLACE:
-	case XFS_ATTR_OP_FLAGS_REMOVE:
+	case XFS_ATTRI_OP_FLAGS_SET:
+	case XFS_ATTRI_OP_FLAGS_REPLACE:
+	case XFS_ATTRI_OP_FLAGS_REMOVE:
 		break;
 	default:
 		return false;
@@ -565,7 +565,7 @@ xfs_attri_item_recover(
 
 	attr->xattri_da_args = args;
 	attr->xattri_op_flags = attrp->alfi_op_flags &
-						XFS_ATTR_OP_FLAGS_TYPE_MASK;
+						XFS_ATTRI_OP_FLAGS_TYPE_MASK;
 
 	args->dp = ip;
 	args->geo = mp->m_attr_geo;
@@ -577,8 +577,8 @@ xfs_attri_item_recover(
 	args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT;
 
 	switch (attr->xattri_op_flags) {
-	case XFS_ATTR_OP_FLAGS_SET:
-	case XFS_ATTR_OP_FLAGS_REPLACE:
+	case XFS_ATTRI_OP_FLAGS_SET:
+	case XFS_ATTRI_OP_FLAGS_REPLACE:
 		args->value = attrip->attri_value;
 		args->valuelen = attrp->alfi_value_len;
 		args->total = xfs_attr_calc_size(args, &local);
@@ -587,7 +587,7 @@ xfs_attri_item_recover(
 		else
 			attr->xattri_dela_state = xfs_attr_init_add_state(args);
 		break;
-	case XFS_ATTR_OP_FLAGS_REMOVE:
+	case XFS_ATTRI_OP_FLAGS_REMOVE:
 		if (!xfs_inode_hasattr(args->dp))
 			goto out;
 		attr->xattri_dela_state = xfs_attr_init_remove_state(args);

[PATCH 3/6] xfs: use a separate slab cache for deferred xattr work state

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Create a separate slab cache for struct xfs_attr_item objects, since we
can pack the (96-byte) intent items more tightly than we can with the
general slab cache objects.  On x86, this means 42 intents per memory
page instead of 32.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c  |   20 +++++++++++++++++++-
 fs/xfs/libxfs/xfs_attr.h  |    4 ++++
 fs/xfs/libxfs/xfs_defer.c |    4 ++++
 fs/xfs/xfs_attr_item.c    |    5 ++++-
 4 files changed, 31 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 0f88f6e17101..687e1b0c49f9 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -29,6 +29,7 @@
 
 struct kmem_cache		*xfs_attri_cache;
 struct kmem_cache		*xfs_attrd_cache;
+struct kmem_cache		*xfs_attr_intent_cache;
 
 /*
  * xfs_attr.c
@@ -902,7 +903,7 @@ xfs_attr_item_init(
 
 	struct xfs_attr_item	*new;
 
-	new = kmem_zalloc(sizeof(struct xfs_attr_item), KM_NOFS);
+	new = kmem_cache_zalloc(xfs_attr_intent_cache, GFP_NOFS | __GFP_NOFAIL);
 	new->xattri_op_flags = op_flags;
 	new->xattri_da_args = args;
 
@@ -1650,3 +1651,20 @@ xfs_attr_namecheck(
 	/* There shouldn't be any nulls here */
 	return !memchr(name, 0, length);
 }
+
+int __init
+xfs_attr_intent_init_cache(void)
+{
+	xfs_attr_intent_cache = kmem_cache_create("xfs_attr_item",
+			sizeof(struct xfs_attr_item),
+			0, 0, NULL);
+
+	return xfs_attr_intent_cache != NULL ? 0 : -ENOMEM;
+}
+
+void
+xfs_attr_intent_destroy_cache(void)
+{
+	kmem_cache_destroy(xfs_attr_intent_cache);
+	xfs_attr_intent_cache = NULL;
+}
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index c739caa11a4b..cb3b3d270569 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -634,4 +634,8 @@ xfs_attr_init_replace_state(struct xfs_da_args *args)
 	return xfs_attr_init_add_state(args);
 }
 
+extern struct kmem_cache *xfs_attr_intent_cache;
+int __init xfs_attr_intent_init_cache(void);
+void xfs_attr_intent_destroy_cache(void);
+
 #endif	/* __XFS_ATTR_H__ */
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index ceb222b4f261..ed65f7e5a9c7 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -877,6 +877,9 @@ xfs_defer_init_item_caches(void)
 	if (error)
 		goto err;
 	error = xfs_attrd_init_cache();
+	if (error)
+		goto err;
+	error = xfs_attr_intent_init_cache();
 	if (error)
 		goto err;
 	return 0;
@@ -889,6 +892,7 @@ xfs_defer_init_item_caches(void)
 void
 xfs_defer_destroy_item_caches(void)
 {
+	xfs_attr_intent_destroy_cache();
 	xfs_attri_destroy_cache();
 	xfs_attrd_destroy_cache();
 	xfs_extfree_intent_destroy_cache();
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 930366055013..89cabd792b7d 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -404,7 +404,10 @@ xfs_attr_free_item(
 {
 	if (attr->xattri_da_state)
 		xfs_da_state_free(attr->xattri_da_state);
-	kmem_free(attr);
+	if (attr->xattri_da_args->op_flags & XFS_DA_OP_RECOVERY)
+		kmem_free(attr);
+	else
+		kmem_cache_free(xfs_attr_intent_cache, attr);
 }
 
 /* Process an attr. */

[PATCH 4/6] xfs: remove struct xfs_attr_item.xattri_flags

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Nobody uses this field, so get rid of it and the unused flag definition.
Rearrange the structure layout to reduce its size from 96 to 88 bytes.
This gets us from 42 to 46 objects per page.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.h |   32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index cb3b3d270569..f0b93515c1e8 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -501,15 +501,19 @@ enum xfs_delattr_state {
 	{ XFS_DAS_NODE_REMOVE_ATTR,	"XFS_DAS_NODE_REMOVE_ATTR" }, \
 	{ XFS_DAS_DONE,			"XFS_DAS_DONE" }
 
-/*
- * Defines for xfs_attr_item.xattri_flags
- */
-#define XFS_DAC_LEAF_ADDNAME_INIT	0x01 /* xfs_attr_leaf_addname init*/
-
 /*
  * Context used for keeping track of delayed attribute operations
  */
 struct xfs_attr_item {
+	/*
+	 * used to log this item to an intent containing a list of attrs to
+	 * commit later
+	 */
+	struct list_head		xattri_list;
+
+	/* Used in xfs_attr_node_removename to roll through removing blocks */
+	struct xfs_da_state		*xattri_da_state;
+
 	struct xfs_da_args		*xattri_da_args;
 
 	/*
@@ -517,16 +521,7 @@ struct xfs_attr_item {
 	 */
 	struct xfs_buf			*xattri_leaf_bp;
 
-	/* Used in xfs_attr_rmtval_set_blk to roll through allocating blocks */
-	struct xfs_bmbt_irec		xattri_map;
-	xfs_dablk_t			xattri_lblkno;
-	int				xattri_blkcnt;
-
-	/* Used in xfs_attr_node_removename to roll through removing blocks */
-	struct xfs_da_state		*xattri_da_state;
-
 	/* Used to keep track of current state of delayed operation */
-	unsigned int			xattri_flags;
 	enum xfs_delattr_state		xattri_dela_state;
 
 	/*
@@ -534,11 +529,10 @@ struct xfs_attr_item {
 	 */
 	unsigned int			xattri_op_flags;
 
-	/*
-	 * used to log this item to an intent containing a list of attrs to
-	 * commit later
-	 */
-	struct list_head		xattri_list;
+	/* Used in xfs_attr_rmtval_set_blk to roll through allocating blocks */
+	xfs_dablk_t			xattri_lblkno;
+	int				xattri_blkcnt;
+	struct xfs_bmbt_irec		xattri_map;
 };
 
 

[PATCH 5/6] xfs: put attr[id] log item cache init with the others

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Initialize and destroy the xattr log item caches in the same places that
we do all the other log item caches.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c  |   36 ------------------------------------
 fs/xfs/libxfs/xfs_attr.h  |    8 --------
 fs/xfs/libxfs/xfs_defer.c |    8 --------
 fs/xfs/xfs_attr_item.c    |    3 +++
 fs/xfs/xfs_attr_item.h    |    3 +++
 fs/xfs/xfs_super.c        |   19 +++++++++++++++++++
 6 files changed, 25 insertions(+), 52 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 687e1b0c49f9..4056edf9f06e 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -27,8 +27,6 @@
 #include "xfs_attr_item.h"
 #include "xfs_log.h"
 
-struct kmem_cache		*xfs_attri_cache;
-struct kmem_cache		*xfs_attrd_cache;
 struct kmem_cache		*xfs_attr_intent_cache;
 
 /*
@@ -1113,40 +1111,6 @@ xfs_attr_set(
 	goto out_unlock;
 }
 
-int __init
-xfs_attri_init_cache(void)
-{
-	xfs_attri_cache = kmem_cache_create("xfs_attri",
-					    sizeof(struct xfs_attri_log_item),
-					    0, 0, NULL);
-
-	return xfs_attri_cache != NULL ? 0 : -ENOMEM;
-}
-
-void
-xfs_attri_destroy_cache(void)
-{
-	kmem_cache_destroy(xfs_attri_cache);
-	xfs_attri_cache = NULL;
-}
-
-int __init
-xfs_attrd_init_cache(void)
-{
-	xfs_attrd_cache = kmem_cache_create("xfs_attrd",
-					    sizeof(struct xfs_attrd_log_item),
-					    0, 0, NULL);
-
-	return xfs_attrd_cache != NULL ? 0 : -ENOMEM;
-}
-
-void
-xfs_attrd_destroy_cache(void)
-{
-	kmem_cache_destroy(xfs_attrd_cache);
-	xfs_attrd_cache = NULL;
-}
-
 /*========================================================================
  * External routines when attribute list is inside the inode
  *========================================================================*/
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index f0b93515c1e8..22a2f288c1c0 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -558,14 +558,6 @@ int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
 void xfs_init_attr_trans(struct xfs_da_args *args, struct xfs_trans_res *tres,
 			 unsigned int *total);
 
-extern struct kmem_cache	*xfs_attri_cache;
-extern struct kmem_cache	*xfs_attrd_cache;
-
-int __init xfs_attri_init_cache(void);
-void xfs_attri_destroy_cache(void);
-int __init xfs_attrd_init_cache(void);
-void xfs_attrd_destroy_cache(void);
-
 /*
  * Check to see if the attr should be upgraded from non-existent or shortform to
  * single-leaf-block attribute list.
diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
index ed65f7e5a9c7..ace229c1d251 100644
--- a/fs/xfs/libxfs/xfs_defer.c
+++ b/fs/xfs/libxfs/xfs_defer.c
@@ -871,12 +871,6 @@ xfs_defer_init_item_caches(void)
 	if (error)
 		goto err;
 	error = xfs_extfree_intent_init_cache();
-	if (error)
-		goto err;
-	error = xfs_attri_init_cache();
-	if (error)
-		goto err;
-	error = xfs_attrd_init_cache();
 	if (error)
 		goto err;
 	error = xfs_attr_intent_init_cache();
@@ -893,8 +887,6 @@ void
 xfs_defer_destroy_item_caches(void)
 {
 	xfs_attr_intent_destroy_cache();
-	xfs_attri_destroy_cache();
-	xfs_attrd_destroy_cache();
 	xfs_extfree_intent_destroy_cache();
 	xfs_bmap_intent_destroy_cache();
 	xfs_refcount_intent_destroy_cache();
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 89cabd792b7d..1747127434b8 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -29,6 +29,9 @@
 #include "xfs_log_priv.h"
 #include "xfs_log_recover.h"
 
+struct kmem_cache		*xfs_attri_cache;
+struct kmem_cache		*xfs_attrd_cache;
+
 static const struct xfs_item_ops xfs_attri_item_ops;
 static const struct xfs_item_ops xfs_attrd_item_ops;
 static struct xfs_attrd_log_item *xfs_trans_get_attrd(struct xfs_trans *tp,
diff --git a/fs/xfs/xfs_attr_item.h b/fs/xfs/xfs_attr_item.h
index c3b779f82adb..cc2fbc9d58a7 100644
--- a/fs/xfs/xfs_attr_item.h
+++ b/fs/xfs/xfs_attr_item.h
@@ -43,4 +43,7 @@ struct xfs_attrd_log_item {
 	struct xfs_attrd_log_format	attrd_format;
 };
 
+extern struct kmem_cache	*xfs_attri_cache;
+extern struct kmem_cache	*xfs_attrd_cache;
+
 #endif	/* __XFS_ATTR_ITEM_H__ */
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 93e43e1a2863..51ce127a0cc6 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -38,6 +38,7 @@
 #include "xfs_pwork.h"
 #include "xfs_ag.h"
 #include "xfs_defer.h"
+#include "xfs_attr_item.h"
 
 #include <linux/magic.h>
 #include <linux/fs_context.h>
@@ -2083,8 +2084,24 @@ xfs_init_caches(void)
 	if (!xfs_bui_cache)
 		goto out_destroy_bud_cache;
 
+	xfs_attrd_cache = kmem_cache_create("xfs_attrd_item",
+					    sizeof(struct xfs_attrd_log_item),
+					    0, 0, NULL);
+	if (!xfs_attrd_cache)
+		goto out_destroy_bui_cache;
+
+	xfs_attri_cache = kmem_cache_create("xfs_attri_item",
+					    sizeof(struct xfs_attri_log_item),
+					    0, 0, NULL);
+	if (!xfs_attri_cache)
+		goto out_destroy_attrd_cache;
+
 	return 0;
 
+ out_destroy_attrd_cache:
+	kmem_cache_destroy(xfs_attrd_cache);
+ out_destroy_bui_cache:
+	kmem_cache_destroy(xfs_bui_cache);
  out_destroy_bud_cache:
 	kmem_cache_destroy(xfs_bud_cache);
  out_destroy_cui_cache:
@@ -2131,6 +2148,8 @@ xfs_destroy_caches(void)
 	 * destroy caches.
 	 */
 	rcu_barrier();
+	kmem_cache_destroy(xfs_attri_cache);
+	kmem_cache_destroy(xfs_attrd_cache);
 	kmem_cache_destroy(xfs_bui_cache);
 	kmem_cache_destroy(xfs_bud_cache);
 	kmem_cache_destroy(xfs_cui_cache);

[PATCH 6/6] xfs: rename struct xfs_attr_item to xfs_attr_intent

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Everywhere else in XFS, structures that capture the state of an ongoing
deferred work item all have names that end with "_intent".  The new
extended attribute deferred work items are not named as such, so fix it
to follow the naming convention used elsewhere.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c        |   52 ++++++++++++++++++++-------------------
 fs/xfs/libxfs/xfs_attr.h        |    8 +++---
 fs/xfs/libxfs/xfs_attr_remote.c |    6 ++---
 fs/xfs/libxfs/xfs_attr_remote.h |    6 ++---
 fs/xfs/xfs_attr_item.c          |   28 +++++++++++----------
 fs/xfs/xfs_attr_item.h          |    6 ++---
 6 files changed, 53 insertions(+), 53 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 4056edf9f06e..427cc07d412e 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -57,9 +57,9 @@ STATIC int xfs_attr_leaf_try_add(struct xfs_da_args *args, struct xfs_buf *bp);
  */
 STATIC int xfs_attr_node_get(xfs_da_args_t *args);
 STATIC void xfs_attr_restore_rmt_blk(struct xfs_da_args *args);
-static int xfs_attr_node_try_addname(struct xfs_attr_item *attr);
-STATIC int xfs_attr_node_addname_find_attr(struct xfs_attr_item *attr);
-STATIC int xfs_attr_node_remove_attr(struct xfs_attr_item *attr);
+static int xfs_attr_node_try_addname(struct xfs_attr_intent *attr);
+STATIC int xfs_attr_node_addname_find_attr(struct xfs_attr_intent *attr);
+STATIC int xfs_attr_node_remove_attr(struct xfs_attr_intent *attr);
 STATIC int xfs_attr_node_lookup(struct xfs_da_args *args,
 		struct xfs_da_state *state);
 
@@ -376,7 +376,7 @@ xfs_attr_try_sf_addname(
 
 static int
 xfs_attr_sf_addname(
-	struct xfs_attr_item		*attr)
+	struct xfs_attr_intent		*attr)
 {
 	struct xfs_da_args		*args = attr->xattri_da_args;
 	struct xfs_inode		*dp = args->dp;
@@ -422,7 +422,7 @@ xfs_attr_sf_addname(
  */
 static enum xfs_delattr_state
 xfs_attr_complete_op(
-	struct xfs_attr_item	*attr,
+	struct xfs_attr_intent	*attr,
 	enum xfs_delattr_state	replace_state)
 {
 	struct xfs_da_args	*args = attr->xattri_da_args;
@@ -438,7 +438,7 @@ xfs_attr_complete_op(
 
 static int
 xfs_attr_leaf_addname(
-	struct xfs_attr_item	*attr)
+	struct xfs_attr_intent	*attr)
 {
 	struct xfs_da_args	*args = attr->xattri_da_args;
 	int			error;
@@ -492,7 +492,7 @@ xfs_attr_leaf_addname(
  */
 static int
 xfs_attr_node_addname(
-	struct xfs_attr_item	*attr)
+	struct xfs_attr_intent	*attr)
 {
 	struct xfs_da_args	*args = attr->xattri_da_args;
 	int			error;
@@ -529,7 +529,7 @@ xfs_attr_node_addname(
 
 static int
 xfs_attr_rmtval_alloc(
-	struct xfs_attr_item		*attr)
+	struct xfs_attr_intent		*attr)
 {
 	struct xfs_da_args              *args = attr->xattri_da_args;
 	int				error = 0;
@@ -596,7 +596,7 @@ xfs_attr_leaf_mark_incomplete(
 /* Ensure the da state of an xattr deferred work item is ready to go. */
 static inline void
 xfs_attr_item_ensure_da_state(
-	struct xfs_attr_item	*attr)
+	struct xfs_attr_intent	*attr)
 {
 	struct xfs_da_args	*args = attr->xattri_da_args;
 
@@ -613,7 +613,7 @@ xfs_attr_item_ensure_da_state(
  */
 static
 int xfs_attr_node_removename_setup(
-	struct xfs_attr_item		*attr)
+	struct xfs_attr_intent		*attr)
 {
 	struct xfs_da_args		*args = attr->xattri_da_args;
 	struct xfs_da_state		*state;
@@ -651,7 +651,7 @@ int xfs_attr_node_removename_setup(
  */
 static int
 xfs_attr_leaf_remove_attr(
-	struct xfs_attr_item		*attr)
+	struct xfs_attr_intent		*attr)
 {
 	struct xfs_da_args              *args = attr->xattri_da_args;
 	struct xfs_inode		*dp = args->dp;
@@ -716,7 +716,7 @@ xfs_attr_leaf_shrink(
  */
 int
 xfs_attr_set_iter(
-	struct xfs_attr_item		*attr)
+	struct xfs_attr_intent		*attr)
 {
 	struct xfs_da_args              *args = attr->xattri_da_args;
 	int				error = 0;
@@ -893,13 +893,13 @@ xfs_attr_lookup(
 }
 
 static int
-xfs_attr_item_init(
+xfs_attr_intent_init(
 	struct xfs_da_args	*args,
 	unsigned int		op_flags,	/* op flag (set or remove) */
-	struct xfs_attr_item	**attr)		/* new xfs_attr_item */
+	struct xfs_attr_intent	**attr)		/* new xfs_attr_intent */
 {
 
-	struct xfs_attr_item	*new;
+	struct xfs_attr_intent	*new;
 
 	new = kmem_cache_zalloc(xfs_attr_intent_cache, GFP_NOFS | __GFP_NOFAIL);
 	new->xattri_op_flags = op_flags;
@@ -914,10 +914,10 @@ static int
 xfs_attr_defer_add(
 	struct xfs_da_args	*args)
 {
-	struct xfs_attr_item	*new;
+	struct xfs_attr_intent	*new;
 	int			error = 0;
 
-	error = xfs_attr_item_init(args, XFS_ATTRI_OP_FLAGS_SET, &new);
+	error = xfs_attr_intent_init(args, XFS_ATTRI_OP_FLAGS_SET, &new);
 	if (error)
 		return error;
 
@@ -933,10 +933,10 @@ static int
 xfs_attr_defer_replace(
 	struct xfs_da_args	*args)
 {
-	struct xfs_attr_item	*new;
+	struct xfs_attr_intent	*new;
 	int			error = 0;
 
-	error = xfs_attr_item_init(args, XFS_ATTRI_OP_FLAGS_REPLACE, &new);
+	error = xfs_attr_intent_init(args, XFS_ATTRI_OP_FLAGS_REPLACE, &new);
 	if (error)
 		return error;
 
@@ -953,10 +953,10 @@ xfs_attr_defer_remove(
 	struct xfs_da_args	*args)
 {
 
-	struct xfs_attr_item	*new;
+	struct xfs_attr_intent	*new;
 	int			error;
 
-	error  = xfs_attr_item_init(args, XFS_ATTRI_OP_FLAGS_REMOVE, &new);
+	error  = xfs_attr_intent_init(args, XFS_ATTRI_OP_FLAGS_REMOVE, &new);
 	if (error)
 		return error;
 
@@ -1394,7 +1394,7 @@ xfs_attr_node_lookup(
 
 STATIC int
 xfs_attr_node_addname_find_attr(
-	 struct xfs_attr_item	*attr)
+	 struct xfs_attr_intent	*attr)
 {
 	struct xfs_da_args	*args = attr->xattri_da_args;
 	int			error;
@@ -1447,7 +1447,7 @@ xfs_attr_node_addname_find_attr(
  */
 static int
 xfs_attr_node_try_addname(
-	struct xfs_attr_item		*attr)
+	struct xfs_attr_intent		*attr)
 {
 	struct xfs_da_args		*args = attr->xattri_da_args;
 	struct xfs_da_state		*state = attr->xattri_da_state;
@@ -1513,7 +1513,7 @@ xfs_attr_node_removename(
 
 static int
 xfs_attr_node_remove_attr(
-	struct xfs_attr_item		*attr)
+	struct xfs_attr_intent		*attr)
 {
 	struct xfs_da_args		*args = attr->xattri_da_args;
 	struct xfs_da_state		*state = NULL;
@@ -1619,8 +1619,8 @@ xfs_attr_namecheck(
 int __init
 xfs_attr_intent_init_cache(void)
 {
-	xfs_attr_intent_cache = kmem_cache_create("xfs_attr_item",
-			sizeof(struct xfs_attr_item),
+	xfs_attr_intent_cache = kmem_cache_create("xfs_attr_intent",
+			sizeof(struct xfs_attr_intent),
 			0, 0, NULL);
 
 	return xfs_attr_intent_cache != NULL ? 0 : -ENOMEM;
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 22a2f288c1c0..b88b6d74e4fc 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -434,7 +434,7 @@ struct xfs_attr_list_context {
  */
 
 /*
- * Enum values for xfs_attr_item.xattri_da_state
+ * Enum values for xfs_attr_intent.xattri_da_state
  *
  * These values are used by delayed attribute operations to keep track  of where
  * they were before they returned -EAGAIN.  A return code of -EAGAIN signals the
@@ -504,7 +504,7 @@ enum xfs_delattr_state {
 /*
  * Context used for keeping track of delayed attribute operations
  */
-struct xfs_attr_item {
+struct xfs_attr_intent {
 	/*
 	 * used to log this item to an intent containing a list of attrs to
 	 * commit later
@@ -551,8 +551,8 @@ bool xfs_attr_is_leaf(struct xfs_inode *ip);
 int xfs_attr_get_ilocked(struct xfs_da_args *args);
 int xfs_attr_get(struct xfs_da_args *args);
 int xfs_attr_set(struct xfs_da_args *args);
-int xfs_attr_set_iter(struct xfs_attr_item *attr);
-int xfs_attr_remove_iter(struct xfs_attr_item *attr);
+int xfs_attr_set_iter(struct xfs_attr_intent *attr);
+int xfs_attr_remove_iter(struct xfs_attr_intent *attr);
 bool xfs_attr_namecheck(const void *name, size_t length);
 int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
 void xfs_init_attr_trans(struct xfs_da_args *args, struct xfs_trans_res *tres,
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 4250159ecced..7298c148f848 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -568,7 +568,7 @@ xfs_attr_rmtval_stale(
  */
 int
 xfs_attr_rmtval_find_space(
-	struct xfs_attr_item		*attr)
+	struct xfs_attr_intent		*attr)
 {
 	struct xfs_da_args		*args = attr->xattri_da_args;
 	struct xfs_bmbt_irec		*map = &attr->xattri_map;
@@ -598,7 +598,7 @@ xfs_attr_rmtval_find_space(
  */
 int
 xfs_attr_rmtval_set_blk(
-	struct xfs_attr_item		*attr)
+	struct xfs_attr_intent		*attr)
 {
 	struct xfs_da_args		*args = attr->xattri_da_args;
 	struct xfs_inode		*dp = args->dp;
@@ -674,7 +674,7 @@ xfs_attr_rmtval_invalidate(
  */
 int
 xfs_attr_rmtval_remove(
-	struct xfs_attr_item		*attr)
+	struct xfs_attr_intent		*attr)
 {
 	struct xfs_da_args		*args = attr->xattri_da_args;
 	int				error, done;
diff --git a/fs/xfs/libxfs/xfs_attr_remote.h b/fs/xfs/libxfs/xfs_attr_remote.h
index 62b398edec3f..d097ec6c4dc3 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.h
+++ b/fs/xfs/libxfs/xfs_attr_remote.h
@@ -12,9 +12,9 @@ int xfs_attr_rmtval_get(struct xfs_da_args *args);
 int xfs_attr_rmtval_stale(struct xfs_inode *ip, struct xfs_bmbt_irec *map,
 		xfs_buf_flags_t incore_flags);
 int xfs_attr_rmtval_invalidate(struct xfs_da_args *args);
-int xfs_attr_rmtval_remove(struct xfs_attr_item *attr);
+int xfs_attr_rmtval_remove(struct xfs_attr_intent *attr);
 int xfs_attr_rmt_find_hole(struct xfs_da_args *args);
 int xfs_attr_rmtval_set_value(struct xfs_da_args *args);
-int xfs_attr_rmtval_set_blk(struct xfs_attr_item *attr);
-int xfs_attr_rmtval_find_space(struct xfs_attr_item *attr);
+int xfs_attr_rmtval_set_blk(struct xfs_attr_intent *attr);
+int xfs_attr_rmtval_find_space(struct xfs_attr_intent *attr);
 #endif /* __XFS_ATTR_REMOTE_H__ */
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 1747127434b8..fb84f71388c4 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -300,7 +300,7 @@ xfs_attrd_item_intent(
  */
 STATIC int
 xfs_xattri_finish_update(
-	struct xfs_attr_item		*attr,
+	struct xfs_attr_intent		*attr,
 	struct xfs_attrd_log_item	*attrdp)
 {
 	struct xfs_da_args		*args = attr->xattri_da_args;
@@ -338,7 +338,7 @@ STATIC void
 xfs_attr_log_item(
 	struct xfs_trans		*tp,
 	struct xfs_attri_log_item	*attrip,
-	struct xfs_attr_item		*attr)
+	struct xfs_attr_intent		*attr)
 {
 	struct xfs_attri_log_format	*attrp;
 
@@ -346,9 +346,9 @@ xfs_attr_log_item(
 	set_bit(XFS_LI_DIRTY, &attrip->attri_item.li_flags);
 
 	/*
-	 * At this point the xfs_attr_item has been constructed, and we've
+	 * At this point the xfs_attr_intent has been constructed, and we've
 	 * created the log intent. Fill in the attri log item and log format
-	 * structure with fields from this xfs_attr_item
+	 * structure with fields from this xfs_attr_intent
 	 */
 	attrp = &attrip->attri_format;
 	attrp->alfi_ino = attr->xattri_da_args->dp->i_ino;
@@ -377,7 +377,7 @@ xfs_attr_create_intent(
 {
 	struct xfs_mount		*mp = tp->t_mountp;
 	struct xfs_attri_log_item	*attrip;
-	struct xfs_attr_item		*attr;
+	struct xfs_attr_intent		*attr;
 
 	ASSERT(count == 1);
 
@@ -403,7 +403,7 @@ xfs_attr_create_intent(
 
 static inline void
 xfs_attr_free_item(
-	struct xfs_attr_item		*attr)
+	struct xfs_attr_intent		*attr)
 {
 	if (attr->xattri_da_state)
 		xfs_da_state_free(attr->xattri_da_state);
@@ -421,11 +421,11 @@ xfs_attr_finish_item(
 	struct list_head		*item,
 	struct xfs_btree_cur		**state)
 {
-	struct xfs_attr_item		*attr;
+	struct xfs_attr_intent		*attr;
 	struct xfs_attrd_log_item	*done_item = NULL;
 	int				error;
 
-	attr = container_of(item, struct xfs_attr_item, xattri_list);
+	attr = container_of(item, struct xfs_attr_intent, xattri_list);
 	if (done)
 		done_item = ATTRD_ITEM(done);
 
@@ -455,9 +455,9 @@ STATIC void
 xfs_attr_cancel_item(
 	struct list_head		*item)
 {
-	struct xfs_attr_item		*attr;
+	struct xfs_attr_intent		*attr;
 
-	attr = container_of(item, struct xfs_attr_item, xattri_list);
+	attr = container_of(item, struct xfs_attr_intent, xattri_list);
 	xfs_attr_free_item(attr);
 }
 
@@ -469,10 +469,10 @@ xfs_attri_item_committed(
 	struct xfs_attri_log_item	*attrip = ATTRI_ITEM(lip);
 
 	/*
-	 * The attrip refers to xfs_attr_item memory to log the name and value
+	 * The attrip refers to xfs_attr_intent memory to log the name and value
 	 * with the intent item. This already occurred when the intent was
 	 * committed so these fields are no longer accessed. Clear them out of
-	 * caution since we're about to free the xfs_attr_item.
+	 * caution since we're about to free the xfs_attr_intent.
 	 */
 	attrip->attri_name = NULL;
 	attrip->attri_value = NULL;
@@ -540,7 +540,7 @@ xfs_attri_item_recover(
 	struct list_head		*capture_list)
 {
 	struct xfs_attri_log_item	*attrip = ATTRI_ITEM(lip);
-	struct xfs_attr_item		*attr;
+	struct xfs_attr_intent		*attr;
 	struct xfs_mount		*mp = lip->li_log->l_mp;
 	struct xfs_inode		*ip;
 	struct xfs_da_args		*args;
@@ -565,7 +565,7 @@ xfs_attri_item_recover(
 	if (error)
 		return error;
 
-	attr = kmem_zalloc(sizeof(struct xfs_attr_item) +
+	attr = kmem_zalloc(sizeof(struct xfs_attr_intent) +
 			   sizeof(struct xfs_da_args), KM_NOFS);
 	args = (struct xfs_da_args *)(attr + 1);
 
diff --git a/fs/xfs/xfs_attr_item.h b/fs/xfs/xfs_attr_item.h
index cc2fbc9d58a7..a40e702e0215 100644
--- a/fs/xfs/xfs_attr_item.h
+++ b/fs/xfs/xfs_attr_item.h
@@ -15,13 +15,13 @@ struct kmem_zone;
  * This is the "attr intention" log item.  It is used to log the fact that some
  * extended attribute operations need to be processed.  An operation is
  * currently either a set or remove.  Set or remove operations are described by
- * the xfs_attr_item which may be logged to this intent.
+ * the xfs_attr_intent which may be logged to this intent.
  *
  * During a normal attr operation, name and value point to the name and value
  * fields of the caller's xfs_da_args structure.  During a recovery, the name
  * and value buffers are copied from the log, and stored in a trailing buffer
- * attached to the xfs_attr_item until they are committed.  They are freed when
- * the xfs_attr_item itself is freed when the work is done.
+ * attached to the xfs_attr_intent until they are committed.  They are freed
+ * when the xfs_attr_intent itself is freed when the work is done.
  */
 struct xfs_attri_log_item {
 	struct xfs_log_item		attri_item;

Re: [PATCH 1/6] xfs: clean up xfs_attr_node_hasname

[Alli]

On Sun, 2022-05-15 at 20:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The calling conventions of this function are a mess -- callers /can/
> provide a pointer to a pointer to a state structure, but it's not
> required, and as evidenced by the last two patches, the callers that
> do
> weren't be careful enough about how to deal with an existing da
> state.
> 
> Push the allocation and freeing responsibilty to the callers, which
> means that callers from the xattr node state machine steps now have
> the
> visibility to allocate or free the da state structure as they please.
> As a bonus, the node remove/add paths for larp-mode replaces can
> reset
> the da state structure instead of freeing and immediately
> reallocating
> it.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Ok, I think it looks ok
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c     |   63 +++++++++++++++++++++-----------
> ----------
>  fs/xfs/libxfs/xfs_da_btree.c |   11 +++++++
>  fs/xfs/libxfs/xfs_da_btree.h |    1 +
>  3 files changed, 44 insertions(+), 31 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 499a15480b57..381b8d46529a 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -61,8 +61,8 @@ STATIC void xfs_attr_restore_rmt_blk(struct
> xfs_da_args *args);
>  static int xfs_attr_node_try_addname(struct xfs_attr_item *attr);
>  STATIC int xfs_attr_node_addname_find_attr(struct xfs_attr_item
> *attr);
>  STATIC int xfs_attr_node_remove_attr(struct xfs_attr_item *attr);
> -STATIC int xfs_attr_node_hasname(xfs_da_args_t *args,
> -				 struct xfs_da_state **state);
> +STATIC int xfs_attr_node_lookup(struct xfs_da_args *args,
> +		struct xfs_da_state *state);
>  
>  int
>  xfs_inode_hasattr(
> @@ -594,6 +594,19 @@ xfs_attr_leaf_mark_incomplete(
>  	return xfs_attr3_leaf_setflag(args);
>  }
>  
> +/* Ensure the da state of an xattr deferred work item is ready to
> go. */
> +static inline void
> +xfs_attr_item_ensure_da_state(
> +	struct xfs_attr_item	*attr)
> +{
> +	struct xfs_da_args	*args = attr->xattri_da_args;
> +
> +	if (!attr->xattri_da_state)
> +		attr->xattri_da_state = xfs_da_state_alloc(args);
> +	else
> +		xfs_da_state_reset(attr->xattri_da_state, args);
> +}
> +
>  /*
>   * Initial setup for xfs_attr_node_removename.  Make sure the attr
> is there and
>   * the blocks are valid.  Attr keys with remote blocks will be
> marked
> @@ -607,7 +620,8 @@ int xfs_attr_node_removename_setup(
>  	struct xfs_da_state		*state;
>  	int				error;
>  
> -	error = xfs_attr_node_hasname(args, &attr->xattri_da_state);
> +	xfs_attr_item_ensure_da_state(attr);
> +	error = xfs_attr_node_lookup(args, attr->xattri_da_state);
>  	if (error != -EEXIST)
>  		goto out;
>  	error = 0;
> @@ -855,6 +869,7 @@ xfs_attr_lookup(
>  {
>  	struct xfs_inode	*dp = args->dp;
>  	struct xfs_buf		*bp = NULL;
> +	struct xfs_da_state	*state;
>  	int			error;
>  
>  	if (!xfs_inode_hasattr(dp))
> @@ -872,7 +887,10 @@ xfs_attr_lookup(
>  		return error;
>  	}
>  
> -	return xfs_attr_node_hasname(args, NULL);
> +	state = xfs_da_state_alloc(args);
> +	error = xfs_attr_node_lookup(args, state);
> +	xfs_da_state_free(state);
> +	return error;
>  }
>  
>  static int
> @@ -1387,34 +1405,20 @@ xfs_attr_leaf_get(xfs_da_args_t *args)
>  	return error;
>  }
>  
> -/*
> - * Return EEXIST if attr is found, or ENOATTR if not
> - * statep: If not null is set to point at the found state.  Caller
> will
> - *         be responsible for freeing the state in this case.
> - */
> +/* Return EEXIST if attr is found, or ENOATTR if not. */
>  STATIC int
> -xfs_attr_node_hasname(
> +xfs_attr_node_lookup(
>  	struct xfs_da_args	*args,
> -	struct xfs_da_state	**statep)
> +	struct xfs_da_state	*state)
>  {
> -	struct xfs_da_state	*state;
>  	int			retval, error;
>  
> -	state = xfs_da_state_alloc(args);
> -	if (statep != NULL) {
> -		ASSERT(*statep == NULL);
> -		*statep = state;
> -	}
> -
>  	/*
>  	 * 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;
> -
> -	if (!statep)
> -		xfs_da_state_free(state);
> +		return error;
>  
>  	return retval;
>  }
> @@ -1430,15 +1434,12 @@ xfs_attr_node_addname_find_attr(
>  	struct xfs_da_args	*args = attr->xattri_da_args;
>  	int			error;
>  
> -	if (attr->xattri_da_state)
> -		xfs_da_state_free(attr->xattri_da_state);
> -	attr->xattri_da_state = NULL;
> -
>  	/*
>  	 * Search to see if name already exists, and get back a pointer
>  	 * to where it should go.
>  	 */
> -	error = xfs_attr_node_hasname(args, &attr->xattri_da_state);
> +	xfs_attr_item_ensure_da_state(attr);
> +	error = xfs_attr_node_lookup(args, attr->xattri_da_state);
>  	switch (error) {
>  	case -ENOATTR:
>  		if (args->op_flags & XFS_DA_OP_REPLACE)
> @@ -1599,7 +1600,7 @@ STATIC int
>  xfs_attr_node_get(
>  	struct xfs_da_args	*args)
>  {
> -	struct xfs_da_state	*state = NULL;
> +	struct xfs_da_state	*state;
>  	struct xfs_da_state_blk	*blk;
>  	int			i;
>  	int			error;
> @@ -1609,7 +1610,8 @@ xfs_attr_node_get(
>  	/*
>  	 * Search to see if name exists, and get back a pointer to it.
>  	 */
> -	error = xfs_attr_node_hasname(args, &state);
> +	state = xfs_da_state_alloc(args);
> +	error = xfs_attr_node_lookup(args, state);
>  	if (error != -EEXIST)
>  		goto out_release;
>  
> @@ -1628,8 +1630,7 @@ xfs_attr_node_get(
>  		state->path.blk[i].bp = NULL;
>  	}
>  
> -	if (state)
> -		xfs_da_state_free(state);
> +	xfs_da_state_free(state);
>  	return error;
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c
> b/fs/xfs/libxfs/xfs_da_btree.c
> index aa74f3fdb571..e7201dc68f43 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -117,6 +117,17 @@ xfs_da_state_free(xfs_da_state_t *state)
>  	kmem_cache_free(xfs_da_state_cache, state);
>  }
>  
> +void
> +xfs_da_state_reset(
> +	struct xfs_da_state	*state,
> +	struct xfs_da_args	*args)
> +{
> +	xfs_da_state_kill_altpath(state);
> +	memset(state, 0, sizeof(struct xfs_da_state));
> +	state->args = args;
> +	state->mp = state->args->dp->i_mount;
> +}
> +
>  static inline int xfs_dabuf_nfsb(struct xfs_mount *mp, int
> whichfork)
>  {
>  	if (whichfork == XFS_DATA_FORK)
> diff --git a/fs/xfs/libxfs/xfs_da_btree.h
> b/fs/xfs/libxfs/xfs_da_btree.h
> index ed2303e4d46a..d33b7686a0b3 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.h
> +++ b/fs/xfs/libxfs/xfs_da_btree.h
> @@ -225,6 +225,7 @@ enum xfs_dacmp xfs_da_compname(struct xfs_da_args
> *args,
>  
>  struct xfs_da_state *xfs_da_state_alloc(struct xfs_da_args *args);
>  void xfs_da_state_free(xfs_da_state_t *state);
> +void xfs_da_state_reset(struct xfs_da_state *state, struct
> xfs_da_args *args);
>  
>  void	xfs_da3_node_hdr_from_disk(struct xfs_mount *mp,
>  		struct xfs_da3_icnode_hdr *to, struct xfs_da_intnode
> *from);
> 

Re: [PATCH 2/6] xfs: put the xattr intent item op flags in their own namespace

[Alli]

On Sun, 2022-05-15 at 20:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> The flags that are stored in the extended attr intent log item really
> should have a separate namespace from the rest of the XFS_ATTR_*
> flags.
> Give them one to make it a little more obvious that they're intent
> item
> flags.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Looks fine
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c       |    6 +++---
>  fs/xfs/libxfs/xfs_attr.h       |    2 +-
>  fs/xfs/libxfs/xfs_log_format.h |    8 ++++----
>  fs/xfs/xfs_attr_item.c         |   20 ++++++++++----------
>  4 files changed, 18 insertions(+), 18 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 381b8d46529a..0f88f6e17101 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -918,7 +918,7 @@ xfs_attr_defer_add(
>  	struct xfs_attr_item	*new;
>  	int			error = 0;
>  
> -	error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET, &new);
> +	error = xfs_attr_item_init(args, XFS_ATTRI_OP_FLAGS_SET, &new);
>  	if (error)
>  		return error;
>  
> @@ -937,7 +937,7 @@ xfs_attr_defer_replace(
>  	struct xfs_attr_item	*new;
>  	int			error = 0;
>  
> -	error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REPLACE,
> &new);
> +	error = xfs_attr_item_init(args, XFS_ATTRI_OP_FLAGS_REPLACE,
> &new);
>  	if (error)
>  		return error;
>  
> @@ -957,7 +957,7 @@ xfs_attr_defer_remove(
>  	struct xfs_attr_item	*new;
>  	int			error;
>  
> -	error  = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REMOVE,
> &new);
> +	error  = xfs_attr_item_init(args, XFS_ATTRI_OP_FLAGS_REMOVE,
> &new);
>  	if (error)
>  		return error;
>  
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 1af7abe29eef..c739caa11a4b 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -530,7 +530,7 @@ struct xfs_attr_item {
>  	enum xfs_delattr_state		xattri_dela_state;
>  
>  	/*
> -	 * Attr operation being performed - XFS_ATTR_OP_FLAGS_*
> +	 * Attr operation being performed - XFS_ATTRI_OP_FLAGS_*
>  	 */
>  	unsigned int			xattri_op_flags;
>  
> diff --git a/fs/xfs/libxfs/xfs_log_format.h
> b/fs/xfs/libxfs/xfs_log_format.h
> index 5017500bfd8b..377b230aecfd 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -906,10 +906,10 @@ struct xfs_icreate_log {
>   * Flags for deferred attribute operations.
>   * Upper bits are flags, lower byte is type code
>   */
> -#define XFS_ATTR_OP_FLAGS_SET		1	/* Set the attribute
> */
> -#define XFS_ATTR_OP_FLAGS_REMOVE	2	/* Remove the attribute */
> -#define XFS_ATTR_OP_FLAGS_REPLACE	3	/* Replace the attribute */
> -#define XFS_ATTR_OP_FLAGS_TYPE_MASK	0xFF	/* Flags type mask */
> +#define XFS_ATTRI_OP_FLAGS_SET		1	/* Set the attribute
> */
> +#define XFS_ATTRI_OP_FLAGS_REMOVE	2	/* Remove the attribute */
> +#define XFS_ATTRI_OP_FLAGS_REPLACE	3	/* Replace the attribute */
> +#define XFS_ATTRI_OP_FLAGS_TYPE_MASK	0xFF	/* Flags type mask */
>  
>  #define XFS_ATTRI_FILTER_ROOT		(1u <<
> XFS_ATTR_ROOT_BIT)
>  #define XFS_ATTRI_FILTER_SECURE		(1u <<
> XFS_ATTR_SECURE_BIT)
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 7cbb640d7856..930366055013 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -350,7 +350,7 @@ xfs_attr_log_item(
>  	attrp = &attrip->attri_format;
>  	attrp->alfi_ino = attr->xattri_da_args->dp->i_ino;
>  	attrp->alfi_op_flags = attr->xattri_op_flags &
> -						XFS_ATTR_OP_FLAGS_TYPE_
> MASK;
> +						XFS_ATTRI_OP_FLAGS_TYPE
> _MASK;
>  	attrp->alfi_value_len = attr->xattri_da_args->valuelen;
>  	attrp->alfi_name_len = attr->xattri_da_args->namelen;
>  	attrp->alfi_attr_filter = attr->xattri_da_args->attr_filter &
> @@ -493,12 +493,12 @@ xfs_attri_validate(
>  	struct xfs_attri_log_format	*attrp)
>  {
>  	unsigned int			op = attrp->alfi_op_flags &
> -					     XFS_ATTR_OP_FLAGS_TYPE_MAS
> K;
> +					     XFS_ATTRI_OP_FLAGS_TYPE_MA
> SK;
>  
>  	if (attrp->__pad != 0)
>  		return false;
>  
> -	if (attrp->alfi_op_flags & ~XFS_ATTR_OP_FLAGS_TYPE_MASK)
> +	if (attrp->alfi_op_flags & ~XFS_ATTRI_OP_FLAGS_TYPE_MASK)
>  		return false;
>  
>  	if (attrp->alfi_attr_filter & ~XFS_ATTRI_FILTER_MASK)
> @@ -506,9 +506,9 @@ xfs_attri_validate(
>  
>  	/* alfi_op_flags should be either a set or remove */
>  	switch (op) {
> -	case XFS_ATTR_OP_FLAGS_SET:
> -	case XFS_ATTR_OP_FLAGS_REPLACE:
> -	case XFS_ATTR_OP_FLAGS_REMOVE:
> +	case XFS_ATTRI_OP_FLAGS_SET:
> +	case XFS_ATTRI_OP_FLAGS_REPLACE:
> +	case XFS_ATTRI_OP_FLAGS_REMOVE:
>  		break;
>  	default:
>  		return false;
> @@ -565,7 +565,7 @@ xfs_attri_item_recover(
>  
>  	attr->xattri_da_args = args;
>  	attr->xattri_op_flags = attrp->alfi_op_flags &
> -						XFS_ATTR_OP_FLAGS_TYPE_
> MASK;
> +						XFS_ATTRI_OP_FLAGS_TYPE
> _MASK;
>  
>  	args->dp = ip;
>  	args->geo = mp->m_attr_geo;
> @@ -577,8 +577,8 @@ xfs_attri_item_recover(
>  	args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT;
>  
>  	switch (attr->xattri_op_flags) {
> -	case XFS_ATTR_OP_FLAGS_SET:
> -	case XFS_ATTR_OP_FLAGS_REPLACE:
> +	case XFS_ATTRI_OP_FLAGS_SET:
> +	case XFS_ATTRI_OP_FLAGS_REPLACE:
>  		args->value = attrip->attri_value;
>  		args->valuelen = attrp->alfi_value_len;
>  		args->total = xfs_attr_calc_size(args, &local);
> @@ -587,7 +587,7 @@ xfs_attri_item_recover(
>  		else
>  			attr->xattri_dela_state =
> xfs_attr_init_add_state(args);
>  		break;
> -	case XFS_ATTR_OP_FLAGS_REMOVE:
> +	case XFS_ATTRI_OP_FLAGS_REMOVE:
>  		if (!xfs_inode_hasattr(args->dp))
>  			goto out;
>  		attr->xattri_dela_state =
> xfs_attr_init_remove_state(args);
> 

Re: [PATCH 3/6] xfs: use a separate slab cache for deferred xattr work state

[Alli]

On Sun, 2022-05-15 at 20:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Create a separate slab cache for struct xfs_attr_item objects, since
> we
> can pack the (96-byte) intent items more tightly than we can with the
> general slab cache objects.  On x86, this means 42 intents per memory
> page instead of 32.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_attr.c  |   20 +++++++++++++++++++-
>  fs/xfs/libxfs/xfs_attr.h  |    4 ++++
>  fs/xfs/libxfs/xfs_defer.c |    4 ++++
>  fs/xfs/xfs_attr_item.c    |    5 ++++-
>  4 files changed, 31 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 0f88f6e17101..687e1b0c49f9 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -29,6 +29,7 @@
>  
>  struct kmem_cache		*xfs_attri_cache;
>  struct kmem_cache		*xfs_attrd_cache;
> +struct kmem_cache		*xfs_attr_intent_cache;
Functionally this looks ok.  It does make me think that at some point
we may want to look at improving the log item naming scheme in general.
It was my observation that most items have a xfs_*i_cache and an xfs_*d
_cache which I think stand for "intent" and "done" caches respectively
(?).   And now were adding another xfs_attr_intent_cache, but I feel
like if I were new to the code, it wouldnt be immediately clear why
there is a xfs_attri_cache and a xfs_attr_intent_cache.

Initially I had modeled attrs from the extent free code which called it
an "xfs_extent_free_item", hence the name "xfs_attr_item".  So i
suppose in that scheme we're logging items to intent items. But it
looks like rmap and bmap call them intents (xfs_rmap_intent and
xfs_bmap_intent).  Which I guess would suggest we log intents to intent
items?  So now this leaves extent free the weird one. :-)

In any case, I do think having the extra cache is an improvement so:
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>> 

But it does make me think that xfs_*i/d_cache could use some clarity
perhaps as a separate cleanup effort.  Maybe xfs_*i/d_item_cache or
something like that.

>  
>  /*
>   * xfs_attr.c
> @@ -902,7 +903,7 @@ xfs_attr_item_init(
>  
>  	struct xfs_attr_item	*new;
>  
> -	new = kmem_zalloc(sizeof(struct xfs_attr_item), KM_NOFS);
> +	new = kmem_cache_zalloc(xfs_attr_intent_cache, GFP_NOFS |
> __GFP_NOFAIL);
>  	new->xattri_op_flags = op_flags;
>  	new->xattri_da_args = args;
>  
> @@ -1650,3 +1651,20 @@ xfs_attr_namecheck(
>  	/* There shouldn't be any nulls here */
>  	return !memchr(name, 0, length);
>  }
> +
> +int __init
> +xfs_attr_intent_init_cache(void)
> +{
> +	xfs_attr_intent_cache = kmem_cache_create("xfs_attr_item",
> +			sizeof(struct xfs_attr_item),
> +			0, 0, NULL);
> +
> +	return xfs_attr_intent_cache != NULL ? 0 : -ENOMEM;
> +}
> +
> +void
> +xfs_attr_intent_destroy_cache(void)
> +{
> +	kmem_cache_destroy(xfs_attr_intent_cache);
> +	xfs_attr_intent_cache = NULL;
> +}
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index c739caa11a4b..cb3b3d270569 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -634,4 +634,8 @@ xfs_attr_init_replace_state(struct xfs_da_args
> *args)
>  	return xfs_attr_init_add_state(args);
>  }
>  
> +extern struct kmem_cache *xfs_attr_intent_cache;
> +int __init xfs_attr_intent_init_cache(void);
> +void xfs_attr_intent_destroy_cache(void);
> +
>  #endif	/* __XFS_ATTR_H__ */
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index ceb222b4f261..ed65f7e5a9c7 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -877,6 +877,9 @@ xfs_defer_init_item_caches(void)
>  	if (error)
>  		goto err;
>  	error = xfs_attrd_init_cache();
> +	if (error)
> +		goto err;
> +	error = xfs_attr_intent_init_cache();
>  	if (error)
>  		goto err;
>  	return 0;
> @@ -889,6 +892,7 @@ xfs_defer_init_item_caches(void)
>  void
>  xfs_defer_destroy_item_caches(void)
>  {
> +	xfs_attr_intent_destroy_cache();
>  	xfs_attri_destroy_cache();
>  	xfs_attrd_destroy_cache();
>  	xfs_extfree_intent_destroy_cache();
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 930366055013..89cabd792b7d 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -404,7 +404,10 @@ xfs_attr_free_item(
>  {
>  	if (attr->xattri_da_state)
>  		xfs_da_state_free(attr->xattri_da_state);
> -	kmem_free(attr);
> +	if (attr->xattri_da_args->op_flags & XFS_DA_OP_RECOVERY)
> +		kmem_free(attr);
> +	else
> +		kmem_cache_free(xfs_attr_intent_cache, attr);
>  }
>  
>  /* Process an attr. */
> 

Re: [PATCH 4/6] xfs: remove struct xfs_attr_item.xattri_flags

[Alli]

On Sun, 2022-05-15 at 20:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Nobody uses this field, so get rid of it and the unused flag
> definition.
> Rearrange the structure layout to reduce its size from 96 to 88
> bytes.
> This gets us from 42 to 46 objects per page.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Looks fine
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.h |   32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index cb3b3d270569..f0b93515c1e8 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -501,15 +501,19 @@ enum xfs_delattr_state {
>  	{ XFS_DAS_NODE_REMOVE_ATTR,	"XFS_DAS_NODE_REMOVE_ATTR" },
> \
>  	{ XFS_DAS_DONE,			"XFS_DAS_DONE" }
>  
> -/*
> - * Defines for xfs_attr_item.xattri_flags
> - */
> -#define XFS_DAC_LEAF_ADDNAME_INIT	0x01 /* xfs_attr_leaf_addname
> init*/
> -
>  /*
>   * Context used for keeping track of delayed attribute operations
>   */
>  struct xfs_attr_item {
> +	/*
> +	 * used to log this item to an intent containing a list of
> attrs to
> +	 * commit later
> +	 */
> +	struct list_head		xattri_list;
> +
> +	/* Used in xfs_attr_node_removename to roll through removing
> blocks */
> +	struct xfs_da_state		*xattri_da_state;
> +
>  	struct xfs_da_args		*xattri_da_args;
>  
>  	/*
> @@ -517,16 +521,7 @@ struct xfs_attr_item {
>  	 */
>  	struct xfs_buf			*xattri_leaf_bp;
>  
> -	/* Used in xfs_attr_rmtval_set_blk to roll through allocating
> blocks */
> -	struct xfs_bmbt_irec		xattri_map;
> -	xfs_dablk_t			xattri_lblkno;
> -	int				xattri_blkcnt;
> -
> -	/* Used in xfs_attr_node_removename to roll through removing
> blocks */
> -	struct xfs_da_state		*xattri_da_state;
> -
>  	/* Used to keep track of current state of delayed operation */
> -	unsigned int			xattri_flags;
>  	enum xfs_delattr_state		xattri_dela_state;
>  
>  	/*
> @@ -534,11 +529,10 @@ struct xfs_attr_item {
>  	 */
>  	unsigned int			xattri_op_flags;
>  
> -	/*
> -	 * used to log this item to an intent containing a list of
> attrs to
> -	 * commit later
> -	 */
> -	struct list_head		xattri_list;
> +	/* Used in xfs_attr_rmtval_set_blk to roll through allocating
> blocks */
> +	xfs_dablk_t			xattri_lblkno;
> +	int				xattri_blkcnt;
> +	struct xfs_bmbt_irec		xattri_map;
>  };
>  
>  
> 

Re: [PATCH 5/6] xfs: put attr[id] log item cache init with the others

[Alli]

On Sun, 2022-05-15 at 20:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Initialize and destroy the xattr log item caches in the same places
> that
> we do all the other log item caches.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Ok, looks good
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c  |   36 ---------------------------------
> ---
>  fs/xfs/libxfs/xfs_attr.h  |    8 --------
>  fs/xfs/libxfs/xfs_defer.c |    8 --------
>  fs/xfs/xfs_attr_item.c    |    3 +++
>  fs/xfs/xfs_attr_item.h    |    3 +++
>  fs/xfs/xfs_super.c        |   19 +++++++++++++++++++
>  6 files changed, 25 insertions(+), 52 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 687e1b0c49f9..4056edf9f06e 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -27,8 +27,6 @@
>  #include "xfs_attr_item.h"
>  #include "xfs_log.h"
>  
> -struct kmem_cache		*xfs_attri_cache;
> -struct kmem_cache		*xfs_attrd_cache;
>  struct kmem_cache		*xfs_attr_intent_cache;
>  
>  /*
> @@ -1113,40 +1111,6 @@ xfs_attr_set(
>  	goto out_unlock;
>  }
>  
> -int __init
> -xfs_attri_init_cache(void)
> -{
> -	xfs_attri_cache = kmem_cache_create("xfs_attri",
> -					    sizeof(struct
> xfs_attri_log_item),
> -					    0, 0, NULL);
> -
> -	return xfs_attri_cache != NULL ? 0 : -ENOMEM;
> -}
> -
> -void
> -xfs_attri_destroy_cache(void)
> -{
> -	kmem_cache_destroy(xfs_attri_cache);
> -	xfs_attri_cache = NULL;
> -}
> -
> -int __init
> -xfs_attrd_init_cache(void)
> -{
> -	xfs_attrd_cache = kmem_cache_create("xfs_attrd",
> -					    sizeof(struct
> xfs_attrd_log_item),
> -					    0, 0, NULL);
> -
> -	return xfs_attrd_cache != NULL ? 0 : -ENOMEM;
> -}
> -
> -void
> -xfs_attrd_destroy_cache(void)
> -{
> -	kmem_cache_destroy(xfs_attrd_cache);
> -	xfs_attrd_cache = NULL;
> -}
> -
>  /*==================================================================
> ======
>   * External routines when attribute list is inside the inode
>  
> *====================================================================
> ====*/
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index f0b93515c1e8..22a2f288c1c0 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -558,14 +558,6 @@ int xfs_attr_calc_size(struct xfs_da_args *args,
> int *local);
>  void xfs_init_attr_trans(struct xfs_da_args *args, struct
> xfs_trans_res *tres,
>  			 unsigned int *total);
>  
> -extern struct kmem_cache	*xfs_attri_cache;
> -extern struct kmem_cache	*xfs_attrd_cache;
> -
> -int __init xfs_attri_init_cache(void);
> -void xfs_attri_destroy_cache(void);
> -int __init xfs_attrd_init_cache(void);
> -void xfs_attrd_destroy_cache(void);
> -
>  /*
>   * Check to see if the attr should be upgraded from non-existent or
> shortform to
>   * single-leaf-block attribute list.
> diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> index ed65f7e5a9c7..ace229c1d251 100644
> --- a/fs/xfs/libxfs/xfs_defer.c
> +++ b/fs/xfs/libxfs/xfs_defer.c
> @@ -871,12 +871,6 @@ xfs_defer_init_item_caches(void)
>  	if (error)
>  		goto err;
>  	error = xfs_extfree_intent_init_cache();
> -	if (error)
> -		goto err;
> -	error = xfs_attri_init_cache();
> -	if (error)
> -		goto err;
> -	error = xfs_attrd_init_cache();
>  	if (error)
>  		goto err;
>  	error = xfs_attr_intent_init_cache();
> @@ -893,8 +887,6 @@ void
>  xfs_defer_destroy_item_caches(void)
>  {
>  	xfs_attr_intent_destroy_cache();
> -	xfs_attri_destroy_cache();
> -	xfs_attrd_destroy_cache();
>  	xfs_extfree_intent_destroy_cache();
>  	xfs_bmap_intent_destroy_cache();
>  	xfs_refcount_intent_destroy_cache();
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 89cabd792b7d..1747127434b8 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -29,6 +29,9 @@
>  #include "xfs_log_priv.h"
>  #include "xfs_log_recover.h"
>  
> +struct kmem_cache		*xfs_attri_cache;
> +struct kmem_cache		*xfs_attrd_cache;
> +
>  static const struct xfs_item_ops xfs_attri_item_ops;
>  static const struct xfs_item_ops xfs_attrd_item_ops;
>  static struct xfs_attrd_log_item *xfs_trans_get_attrd(struct
> xfs_trans *tp,
> diff --git a/fs/xfs/xfs_attr_item.h b/fs/xfs/xfs_attr_item.h
> index c3b779f82adb..cc2fbc9d58a7 100644
> --- a/fs/xfs/xfs_attr_item.h
> +++ b/fs/xfs/xfs_attr_item.h
> @@ -43,4 +43,7 @@ struct xfs_attrd_log_item {
>  	struct xfs_attrd_log_format	attrd_format;
>  };
>  
> +extern struct kmem_cache	*xfs_attri_cache;
> +extern struct kmem_cache	*xfs_attrd_cache;
> +
>  #endif	/* __XFS_ATTR_ITEM_H__ */
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 93e43e1a2863..51ce127a0cc6 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -38,6 +38,7 @@
>  #include "xfs_pwork.h"
>  #include "xfs_ag.h"
>  #include "xfs_defer.h"
> +#include "xfs_attr_item.h"
>  
>  #include <linux/magic.h>
>  #include <linux/fs_context.h>
> @@ -2083,8 +2084,24 @@ xfs_init_caches(void)
>  	if (!xfs_bui_cache)
>  		goto out_destroy_bud_cache;
>  
> +	xfs_attrd_cache = kmem_cache_create("xfs_attrd_item",
> +					    sizeof(struct
> xfs_attrd_log_item),
> +					    0, 0, NULL);
> +	if (!xfs_attrd_cache)
> +		goto out_destroy_bui_cache;
> +
> +	xfs_attri_cache = kmem_cache_create("xfs_attri_item",
> +					    sizeof(struct
> xfs_attri_log_item),
> +					    0, 0, NULL);
> +	if (!xfs_attri_cache)
> +		goto out_destroy_attrd_cache;
> +
>  	return 0;
>  
> + out_destroy_attrd_cache:
> +	kmem_cache_destroy(xfs_attrd_cache);
> + out_destroy_bui_cache:
> +	kmem_cache_destroy(xfs_bui_cache);
>   out_destroy_bud_cache:
>  	kmem_cache_destroy(xfs_bud_cache);
>   out_destroy_cui_cache:
> @@ -2131,6 +2148,8 @@ xfs_destroy_caches(void)
>  	 * destroy caches.
>  	 */
>  	rcu_barrier();
> +	kmem_cache_destroy(xfs_attri_cache);
> +	kmem_cache_destroy(xfs_attrd_cache);
>  	kmem_cache_destroy(xfs_bui_cache);
>  	kmem_cache_destroy(xfs_bud_cache);
>  	kmem_cache_destroy(xfs_cui_cache);
> 

Re: [PATCH 6/6] xfs: rename struct xfs_attr_item to xfs_attr_intent

[Alli]

On Sun, 2022-05-15 at 20:32 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Everywhere else in XFS, structures that capture the state of an
> ongoing
> deferred work item all have names that end with "_intent".  The new
> extended attribute deferred work items are not named as such, so fix
> it
> to follow the naming convention used elsewhere.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Ok, looks fine
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>

> ---
>  fs/xfs/libxfs/xfs_attr.c        |   52 ++++++++++++++++++++---------
> ----------
>  fs/xfs/libxfs/xfs_attr.h        |    8 +++---
>  fs/xfs/libxfs/xfs_attr_remote.c |    6 ++---
>  fs/xfs/libxfs/xfs_attr_remote.h |    6 ++---
>  fs/xfs/xfs_attr_item.c          |   28 +++++++++++----------
>  fs/xfs/xfs_attr_item.h          |    6 ++---
>  6 files changed, 53 insertions(+), 53 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 4056edf9f06e..427cc07d412e 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -57,9 +57,9 @@ STATIC int xfs_attr_leaf_try_add(struct xfs_da_args
> *args, struct xfs_buf *bp);
>   */
>  STATIC int xfs_attr_node_get(xfs_da_args_t *args);
>  STATIC void xfs_attr_restore_rmt_blk(struct xfs_da_args *args);
> -static int xfs_attr_node_try_addname(struct xfs_attr_item *attr);
> -STATIC int xfs_attr_node_addname_find_attr(struct xfs_attr_item
> *attr);
> -STATIC int xfs_attr_node_remove_attr(struct xfs_attr_item *attr);
> +static int xfs_attr_node_try_addname(struct xfs_attr_intent *attr);
> +STATIC int xfs_attr_node_addname_find_attr(struct xfs_attr_intent
> *attr);
> +STATIC int xfs_attr_node_remove_attr(struct xfs_attr_intent *attr);
>  STATIC int xfs_attr_node_lookup(struct xfs_da_args *args,
>  		struct xfs_da_state *state);
>  
> @@ -376,7 +376,7 @@ xfs_attr_try_sf_addname(
>  
>  static int
>  xfs_attr_sf_addname(
> -	struct xfs_attr_item		*attr)
> +	struct xfs_attr_intent		*attr)
>  {
>  	struct xfs_da_args		*args = attr->xattri_da_args;
>  	struct xfs_inode		*dp = args->dp;
> @@ -422,7 +422,7 @@ xfs_attr_sf_addname(
>   */
>  static enum xfs_delattr_state
>  xfs_attr_complete_op(
> -	struct xfs_attr_item	*attr,
> +	struct xfs_attr_intent	*attr,
>  	enum xfs_delattr_state	replace_state)
>  {
>  	struct xfs_da_args	*args = attr->xattri_da_args;
> @@ -438,7 +438,7 @@ xfs_attr_complete_op(
>  
>  static int
>  xfs_attr_leaf_addname(
> -	struct xfs_attr_item	*attr)
> +	struct xfs_attr_intent	*attr)
>  {
>  	struct xfs_da_args	*args = attr->xattri_da_args;
>  	int			error;
> @@ -492,7 +492,7 @@ xfs_attr_leaf_addname(
>   */
>  static int
>  xfs_attr_node_addname(
> -	struct xfs_attr_item	*attr)
> +	struct xfs_attr_intent	*attr)
>  {
>  	struct xfs_da_args	*args = attr->xattri_da_args;
>  	int			error;
> @@ -529,7 +529,7 @@ xfs_attr_node_addname(
>  
>  static int
>  xfs_attr_rmtval_alloc(
> -	struct xfs_attr_item		*attr)
> +	struct xfs_attr_intent		*attr)
>  {
>  	struct xfs_da_args              *args = attr->xattri_da_args;
>  	int				error = 0;
> @@ -596,7 +596,7 @@ xfs_attr_leaf_mark_incomplete(
>  /* Ensure the da state of an xattr deferred work item is ready to
> go. */
>  static inline void
>  xfs_attr_item_ensure_da_state(
> -	struct xfs_attr_item	*attr)
> +	struct xfs_attr_intent	*attr)
>  {
>  	struct xfs_da_args	*args = attr->xattri_da_args;
>  
> @@ -613,7 +613,7 @@ xfs_attr_item_ensure_da_state(
>   */
>  static
>  int xfs_attr_node_removename_setup(
> -	struct xfs_attr_item		*attr)
> +	struct xfs_attr_intent		*attr)
>  {
>  	struct xfs_da_args		*args = attr->xattri_da_args;
>  	struct xfs_da_state		*state;
> @@ -651,7 +651,7 @@ int xfs_attr_node_removename_setup(
>   */
>  static int
>  xfs_attr_leaf_remove_attr(
> -	struct xfs_attr_item		*attr)
> +	struct xfs_attr_intent		*attr)
>  {
>  	struct xfs_da_args              *args = attr->xattri_da_args;
>  	struct xfs_inode		*dp = args->dp;
> @@ -716,7 +716,7 @@ xfs_attr_leaf_shrink(
>   */
>  int
>  xfs_attr_set_iter(
> -	struct xfs_attr_item		*attr)
> +	struct xfs_attr_intent		*attr)
>  {
>  	struct xfs_da_args              *args = attr->xattri_da_args;
>  	int				error = 0;
> @@ -893,13 +893,13 @@ xfs_attr_lookup(
>  }
>  
>  static int
> -xfs_attr_item_init(
> +xfs_attr_intent_init(
>  	struct xfs_da_args	*args,
>  	unsigned int		op_flags,	/* op flag (set or
> remove) */
> -	struct xfs_attr_item	**attr)		/* new xfs_attr_item
> */
> +	struct xfs_attr_intent	**attr)		/* new
> xfs_attr_intent */
>  {
>  
> -	struct xfs_attr_item	*new;
> +	struct xfs_attr_intent	*new;
>  
>  	new = kmem_cache_zalloc(xfs_attr_intent_cache, GFP_NOFS |
> __GFP_NOFAIL);
>  	new->xattri_op_flags = op_flags;
> @@ -914,10 +914,10 @@ static int
>  xfs_attr_defer_add(
>  	struct xfs_da_args	*args)
>  {
> -	struct xfs_attr_item	*new;
> +	struct xfs_attr_intent	*new;
>  	int			error = 0;
>  
> -	error = xfs_attr_item_init(args, XFS_ATTRI_OP_FLAGS_SET, &new);
> +	error = xfs_attr_intent_init(args, XFS_ATTRI_OP_FLAGS_SET,
> &new);
>  	if (error)
>  		return error;
>  
> @@ -933,10 +933,10 @@ static int
>  xfs_attr_defer_replace(
>  	struct xfs_da_args	*args)
>  {
> -	struct xfs_attr_item	*new;
> +	struct xfs_attr_intent	*new;
>  	int			error = 0;
>  
> -	error = xfs_attr_item_init(args, XFS_ATTRI_OP_FLAGS_REPLACE,
> &new);
> +	error = xfs_attr_intent_init(args, XFS_ATTRI_OP_FLAGS_REPLACE,
> &new);
>  	if (error)
>  		return error;
>  
> @@ -953,10 +953,10 @@ xfs_attr_defer_remove(
>  	struct xfs_da_args	*args)
>  {
>  
> -	struct xfs_attr_item	*new;
> +	struct xfs_attr_intent	*new;
>  	int			error;
>  
> -	error  = xfs_attr_item_init(args, XFS_ATTRI_OP_FLAGS_REMOVE,
> &new);
> +	error  = xfs_attr_intent_init(args, XFS_ATTRI_OP_FLAGS_REMOVE,
> &new);
>  	if (error)
>  		return error;
>  
> @@ -1394,7 +1394,7 @@ xfs_attr_node_lookup(
>  
>  STATIC int
>  xfs_attr_node_addname_find_attr(
> -	 struct xfs_attr_item	*attr)
> +	 struct xfs_attr_intent	*attr)
>  {
>  	struct xfs_da_args	*args = attr->xattri_da_args;
>  	int			error;
> @@ -1447,7 +1447,7 @@ xfs_attr_node_addname_find_attr(
>   */
>  static int
>  xfs_attr_node_try_addname(
> -	struct xfs_attr_item		*attr)
> +	struct xfs_attr_intent		*attr)
>  {
>  	struct xfs_da_args		*args = attr->xattri_da_args;
>  	struct xfs_da_state		*state = attr-
> >xattri_da_state;
> @@ -1513,7 +1513,7 @@ xfs_attr_node_removename(
>  
>  static int
>  xfs_attr_node_remove_attr(
> -	struct xfs_attr_item		*attr)
> +	struct xfs_attr_intent		*attr)
>  {
>  	struct xfs_da_args		*args = attr->xattri_da_args;
>  	struct xfs_da_state		*state = NULL;
> @@ -1619,8 +1619,8 @@ xfs_attr_namecheck(
>  int __init
>  xfs_attr_intent_init_cache(void)
>  {
> -	xfs_attr_intent_cache = kmem_cache_create("xfs_attr_item",
> -			sizeof(struct xfs_attr_item),
> +	xfs_attr_intent_cache = kmem_cache_create("xfs_attr_intent",
> +			sizeof(struct xfs_attr_intent),
>  			0, 0, NULL);
>  
>  	return xfs_attr_intent_cache != NULL ? 0 : -ENOMEM;
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 22a2f288c1c0..b88b6d74e4fc 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -434,7 +434,7 @@ struct xfs_attr_list_context {
>   */
>  
>  /*
> - * Enum values for xfs_attr_item.xattri_da_state
> + * Enum values for xfs_attr_intent.xattri_da_state
>   *
>   * These values are used by delayed attribute operations to keep
> track  of where
>   * they were before they returned -EAGAIN.  A return code of -EAGAIN
> signals the
> @@ -504,7 +504,7 @@ enum xfs_delattr_state {
>  /*
>   * Context used for keeping track of delayed attribute operations
>   */
> -struct xfs_attr_item {
> +struct xfs_attr_intent {
>  	/*
>  	 * used to log this item to an intent containing a list of
> attrs to
>  	 * commit later
> @@ -551,8 +551,8 @@ bool xfs_attr_is_leaf(struct xfs_inode *ip);
>  int xfs_attr_get_ilocked(struct xfs_da_args *args);
>  int xfs_attr_get(struct xfs_da_args *args);
>  int xfs_attr_set(struct xfs_da_args *args);
> -int xfs_attr_set_iter(struct xfs_attr_item *attr);
> -int xfs_attr_remove_iter(struct xfs_attr_item *attr);
> +int xfs_attr_set_iter(struct xfs_attr_intent *attr);
> +int xfs_attr_remove_iter(struct xfs_attr_intent *attr);
>  bool xfs_attr_namecheck(const void *name, size_t length);
>  int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
>  void xfs_init_attr_trans(struct xfs_da_args *args, struct
> xfs_trans_res *tres,
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.c
> b/fs/xfs/libxfs/xfs_attr_remote.c
> index 4250159ecced..7298c148f848 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.c
> +++ b/fs/xfs/libxfs/xfs_attr_remote.c
> @@ -568,7 +568,7 @@ xfs_attr_rmtval_stale(
>   */
>  int
>  xfs_attr_rmtval_find_space(
> -	struct xfs_attr_item		*attr)
> +	struct xfs_attr_intent		*attr)
>  {
>  	struct xfs_da_args		*args = attr->xattri_da_args;
>  	struct xfs_bmbt_irec		*map = &attr->xattri_map;
> @@ -598,7 +598,7 @@ xfs_attr_rmtval_find_space(
>   */
>  int
>  xfs_attr_rmtval_set_blk(
> -	struct xfs_attr_item		*attr)
> +	struct xfs_attr_intent		*attr)
>  {
>  	struct xfs_da_args		*args = attr->xattri_da_args;
>  	struct xfs_inode		*dp = args->dp;
> @@ -674,7 +674,7 @@ xfs_attr_rmtval_invalidate(
>   */
>  int
>  xfs_attr_rmtval_remove(
> -	struct xfs_attr_item		*attr)
> +	struct xfs_attr_intent		*attr)
>  {
>  	struct xfs_da_args		*args = attr->xattri_da_args;
>  	int				error, done;
> diff --git a/fs/xfs/libxfs/xfs_attr_remote.h
> b/fs/xfs/libxfs/xfs_attr_remote.h
> index 62b398edec3f..d097ec6c4dc3 100644
> --- a/fs/xfs/libxfs/xfs_attr_remote.h
> +++ b/fs/xfs/libxfs/xfs_attr_remote.h
> @@ -12,9 +12,9 @@ int xfs_attr_rmtval_get(struct xfs_da_args *args);
>  int xfs_attr_rmtval_stale(struct xfs_inode *ip, struct xfs_bmbt_irec
> *map,
>  		xfs_buf_flags_t incore_flags);
>  int xfs_attr_rmtval_invalidate(struct xfs_da_args *args);
> -int xfs_attr_rmtval_remove(struct xfs_attr_item *attr);
> +int xfs_attr_rmtval_remove(struct xfs_attr_intent *attr);
>  int xfs_attr_rmt_find_hole(struct xfs_da_args *args);
>  int xfs_attr_rmtval_set_value(struct xfs_da_args *args);
> -int xfs_attr_rmtval_set_blk(struct xfs_attr_item *attr);
> -int xfs_attr_rmtval_find_space(struct xfs_attr_item *attr);
> +int xfs_attr_rmtval_set_blk(struct xfs_attr_intent *attr);
> +int xfs_attr_rmtval_find_space(struct xfs_attr_intent *attr);
>  #endif /* __XFS_ATTR_REMOTE_H__ */
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 1747127434b8..fb84f71388c4 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -300,7 +300,7 @@ xfs_attrd_item_intent(
>   */
>  STATIC int
>  xfs_xattri_finish_update(
> -	struct xfs_attr_item		*attr,
> +	struct xfs_attr_intent		*attr,
>  	struct xfs_attrd_log_item	*attrdp)
>  {
>  	struct xfs_da_args		*args = attr->xattri_da_args;
> @@ -338,7 +338,7 @@ STATIC void
>  xfs_attr_log_item(
>  	struct xfs_trans		*tp,
>  	struct xfs_attri_log_item	*attrip,
> -	struct xfs_attr_item		*attr)
> +	struct xfs_attr_intent		*attr)
>  {
>  	struct xfs_attri_log_format	*attrp;
>  
> @@ -346,9 +346,9 @@ xfs_attr_log_item(
>  	set_bit(XFS_LI_DIRTY, &attrip->attri_item.li_flags);
>  
>  	/*
> -	 * At this point the xfs_attr_item has been constructed, and
> we've
> +	 * At this point the xfs_attr_intent has been constructed, and
> we've
>  	 * created the log intent. Fill in the attri log item and log
> format
> -	 * structure with fields from this xfs_attr_item
> +	 * structure with fields from this xfs_attr_intent
>  	 */
>  	attrp = &attrip->attri_format;
>  	attrp->alfi_ino = attr->xattri_da_args->dp->i_ino;
> @@ -377,7 +377,7 @@ xfs_attr_create_intent(
>  {
>  	struct xfs_mount		*mp = tp->t_mountp;
>  	struct xfs_attri_log_item	*attrip;
> -	struct xfs_attr_item		*attr;
> +	struct xfs_attr_intent		*attr;
>  
>  	ASSERT(count == 1);
>  
> @@ -403,7 +403,7 @@ xfs_attr_create_intent(
>  
>  static inline void
>  xfs_attr_free_item(
> -	struct xfs_attr_item		*attr)
> +	struct xfs_attr_intent		*attr)
>  {
>  	if (attr->xattri_da_state)
>  		xfs_da_state_free(attr->xattri_da_state);
> @@ -421,11 +421,11 @@ xfs_attr_finish_item(
>  	struct list_head		*item,
>  	struct xfs_btree_cur		**state)
>  {
> -	struct xfs_attr_item		*attr;
> +	struct xfs_attr_intent		*attr;
>  	struct xfs_attrd_log_item	*done_item = NULL;
>  	int				error;
>  
> -	attr = container_of(item, struct xfs_attr_item, xattri_list);
> +	attr = container_of(item, struct xfs_attr_intent, xattri_list);
>  	if (done)
>  		done_item = ATTRD_ITEM(done);
>  
> @@ -455,9 +455,9 @@ STATIC void
>  xfs_attr_cancel_item(
>  	struct list_head		*item)
>  {
> -	struct xfs_attr_item		*attr;
> +	struct xfs_attr_intent		*attr;
>  
> -	attr = container_of(item, struct xfs_attr_item, xattri_list);
> +	attr = container_of(item, struct xfs_attr_intent, xattri_list);
>  	xfs_attr_free_item(attr);
>  }
>  
> @@ -469,10 +469,10 @@ xfs_attri_item_committed(
>  	struct xfs_attri_log_item	*attrip = ATTRI_ITEM(lip);
>  
>  	/*
> -	 * The attrip refers to xfs_attr_item memory to log the name
> and value
> +	 * The attrip refers to xfs_attr_intent memory to log the name
> and value
>  	 * with the intent item. This already occurred when the intent
> was
>  	 * committed so these fields are no longer accessed. Clear them
> out of
> -	 * caution since we're about to free the xfs_attr_item.
> +	 * caution since we're about to free the xfs_attr_intent.
>  	 */
>  	attrip->attri_name = NULL;
>  	attrip->attri_value = NULL;
> @@ -540,7 +540,7 @@ xfs_attri_item_recover(
>  	struct list_head		*capture_list)
>  {
>  	struct xfs_attri_log_item	*attrip = ATTRI_ITEM(lip);
> -	struct xfs_attr_item		*attr;
> +	struct xfs_attr_intent		*attr;
>  	struct xfs_mount		*mp = lip->li_log->l_mp;
>  	struct xfs_inode		*ip;
>  	struct xfs_da_args		*args;
> @@ -565,7 +565,7 @@ xfs_attri_item_recover(
>  	if (error)
>  		return error;
>  
> -	attr = kmem_zalloc(sizeof(struct xfs_attr_item) +
> +	attr = kmem_zalloc(sizeof(struct xfs_attr_intent) +
>  			   sizeof(struct xfs_da_args), KM_NOFS);
>  	args = (struct xfs_da_args *)(attr + 1);
>  
> diff --git a/fs/xfs/xfs_attr_item.h b/fs/xfs/xfs_attr_item.h
> index cc2fbc9d58a7..a40e702e0215 100644
> --- a/fs/xfs/xfs_attr_item.h
> +++ b/fs/xfs/xfs_attr_item.h
> @@ -15,13 +15,13 @@ struct kmem_zone;
>   * This is the "attr intention" log item.  It is used to log the
> fact that some
>   * extended attribute operations need to be processed.  An operation
> is
>   * currently either a set or remove.  Set or remove operations are
> described by
> - * the xfs_attr_item which may be logged to this intent.
> + * the xfs_attr_intent which may be logged to this intent.
>   *
>   * During a normal attr operation, name and value point to the name
> and value
>   * fields of the caller's xfs_da_args structure.  During a recovery,
> the name
>   * and value buffers are copied from the log, and stored in a
> trailing buffer
> - * attached to the xfs_attr_item until they are committed.  They are
> freed when
> - * the xfs_attr_item itself is freed when the work is done.
> + * attached to the xfs_attr_intent until they are committed.  They
> are freed
> + * when the xfs_attr_intent itself is freed when the work is done.
>   */
>  struct xfs_attri_log_item {
>  	struct xfs_log_item		attri_item;
> 

Re: [PATCH 3/6] xfs: use a separate slab cache for deferred xattr work state

[Darrick J. Wong]

On Tue, May 17, 2022 at 11:20:46AM -0700, Alli wrote:
> On Sun, 2022-05-15 at 20:32 -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Create a separate slab cache for struct xfs_attr_item objects, since
> > we
> > can pack the (96-byte) intent items more tightly than we can with the
> > general slab cache objects.  On x86, this means 42 intents per memory
> > page instead of 32.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_attr.c  |   20 +++++++++++++++++++-
> >  fs/xfs/libxfs/xfs_attr.h  |    4 ++++
> >  fs/xfs/libxfs/xfs_defer.c |    4 ++++
> >  fs/xfs/xfs_attr_item.c    |    5 ++++-
> >  4 files changed, 31 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 0f88f6e17101..687e1b0c49f9 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -29,6 +29,7 @@
> >  
> >  struct kmem_cache		*xfs_attri_cache;
> >  struct kmem_cache		*xfs_attrd_cache;
> > +struct kmem_cache		*xfs_attr_intent_cache;
> Functionally this looks ok.  It does make me think that at some point
> we may want to look at improving the log item naming scheme in general.

<nod>

> It was my observation that most items have a xfs_*i_cache and an xfs_*d
> _cache which I think stand for "intent" and "done" caches respectively
> (?).   And now were adding another xfs_attr_intent_cache, but I feel
> like if I were new to the code, it wouldnt be immediately clear why
> there is a xfs_attri_cache and a xfs_attr_intent_cache.

The distinction between the three (XXXi, XXXd, XXX_intent) wasn't
entirely clear to me either, years ago.  TBH, it didn't fully come into
focus for me until 2020 when I wrote the log-assisted extent swapping
code, since it was the first piece of code I touched where deferred work
could happen without log items.

Here's my current understanding of how the three pieces fit together.
You probably already know this, but I'll post it all here for everyone
else following along at home:

The deferred work item (aka that huge state machine coded up in
xfs_attr_set_iter) is a high level filesystem operation that does a
bunch of complex work using a series of smaller transactions.  The big
operation needs to store a bunch of in-memory state as it progresses
through the operation; this is what's stored in xfs_XXX_intent.

(If I had to do it all over again, I'd probably have named this
differently, such as xfs_rmap_deferred instead of xfs_rmap_intent; and
the slab caches xfs_rmap_deferred_cache instead of
xfs_rmap_intent_cache.)

If a deferred work item wants to support restarting an operation after a
crash, it needs to log a "log intent item" to the first transaction
where it commits to doing an operation.  This is what log recovery picks
up in those _commit_pass2 functions, so the log intent item must contain
whatever breadcrumbs are needed to figure out where in the complex
operation was the filesystem when it crashed.

Each time the deferred work item makes some progress, it should log a
"log intent done item" to the same transaction in which it made
progress, which serves as a tombstone for the recovered intent item.  If
there's more work to be done, the deferred work item must also log a new
"log intent item" reflecting the updated internal deferred work state.

The log intent items are usually named xfs_XXXi_log_item, and the log
intent done items are usually named xfs_XXXd_log_item.  The caches are
named xfs_XXXi_cache and xfs_XXXd_cache.

Note: Deferred work items are /not/ required to use the log to track
progress.  This can happen if the user-visible effects of the operation
are idempotent (defragmenting with fsr) or if the operations are staged
in such an order that inconsistency is not possible (xattr INCOMPLETE
flags).

> Initially I had modeled attrs from the extent free code which called it
> an "xfs_extent_free_item", hence the name "xfs_attr_item".  So i
> suppose in that scheme we're logging items to intent items. But it
> looks like rmap and bmap call them intents (xfs_rmap_intent and
> xfs_bmap_intent).  Which I guess would suggest we log intents to intent
> items?  So now this leaves extent free the weird one. :-)

Yeah.  The naming is weird.  Maybe some day I'll do a treewide change
and fix all this stuff, since "item" is a bit vague to me and confusable
with log items... and an "item/intent" doesn't even necessarily have an
associated log item!  The thing is, renaming like that makes rebasing
developer trees that much harder, which is why I haven't done that.

> In any case, I do think having the extra cache is an improvement so:
> Reviewed-by: Allison Henderson <allison.henderson@oracle.com>> 

Thanks!

> But it does make me think that xfs_*i/d_cache could use some clarity
> perhaps as a separate cleanup effort.  Maybe xfs_*i/d_item_cache or
> something like that.

Hmm.  I'll ponder that. :)

--D

> >  
> >  /*
> >   * xfs_attr.c
> > @@ -902,7 +903,7 @@ xfs_attr_item_init(
> >  
> >  	struct xfs_attr_item	*new;
> >  
> > -	new = kmem_zalloc(sizeof(struct xfs_attr_item), KM_NOFS);
> > +	new = kmem_cache_zalloc(xfs_attr_intent_cache, GFP_NOFS |
> > __GFP_NOFAIL);
> >  	new->xattri_op_flags = op_flags;
> >  	new->xattri_da_args = args;
> >  
> > @@ -1650,3 +1651,20 @@ xfs_attr_namecheck(
> >  	/* There shouldn't be any nulls here */
> >  	return !memchr(name, 0, length);
> >  }
> > +
> > +int __init
> > +xfs_attr_intent_init_cache(void)
> > +{
> > +	xfs_attr_intent_cache = kmem_cache_create("xfs_attr_item",
> > +			sizeof(struct xfs_attr_item),
> > +			0, 0, NULL);
> > +
> > +	return xfs_attr_intent_cache != NULL ? 0 : -ENOMEM;
> > +}
> > +
> > +void
> > +xfs_attr_intent_destroy_cache(void)
> > +{
> > +	kmem_cache_destroy(xfs_attr_intent_cache);
> > +	xfs_attr_intent_cache = NULL;
> > +}
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index c739caa11a4b..cb3b3d270569 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -634,4 +634,8 @@ xfs_attr_init_replace_state(struct xfs_da_args
> > *args)
> >  	return xfs_attr_init_add_state(args);
> >  }
> >  
> > +extern struct kmem_cache *xfs_attr_intent_cache;
> > +int __init xfs_attr_intent_init_cache(void);
> > +void xfs_attr_intent_destroy_cache(void);
> > +
> >  #endif	/* __XFS_ATTR_H__ */
> > diff --git a/fs/xfs/libxfs/xfs_defer.c b/fs/xfs/libxfs/xfs_defer.c
> > index ceb222b4f261..ed65f7e5a9c7 100644
> > --- a/fs/xfs/libxfs/xfs_defer.c
> > +++ b/fs/xfs/libxfs/xfs_defer.c
> > @@ -877,6 +877,9 @@ xfs_defer_init_item_caches(void)
> >  	if (error)
> >  		goto err;
> >  	error = xfs_attrd_init_cache();
> > +	if (error)
> > +		goto err;
> > +	error = xfs_attr_intent_init_cache();
> >  	if (error)
> >  		goto err;
> >  	return 0;
> > @@ -889,6 +892,7 @@ xfs_defer_init_item_caches(void)
> >  void
> >  xfs_defer_destroy_item_caches(void)
> >  {
> > +	xfs_attr_intent_destroy_cache();
> >  	xfs_attri_destroy_cache();
> >  	xfs_attrd_destroy_cache();
> >  	xfs_extfree_intent_destroy_cache();
> > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> > index 930366055013..89cabd792b7d 100644
> > --- a/fs/xfs/xfs_attr_item.c
> > +++ b/fs/xfs/xfs_attr_item.c
> > @@ -404,7 +404,10 @@ xfs_attr_free_item(
> >  {
> >  	if (attr->xattri_da_state)
> >  		xfs_da_state_free(attr->xattri_da_state);
> > -	kmem_free(attr);
> > +	if (attr->xattri_da_args->op_flags & XFS_DA_OP_RECOVERY)
> > +		kmem_free(attr);
> > +	else
> > +		kmem_cache_free(xfs_attr_intent_cache, attr);
> >  }
> >  
> >  /* Process an attr. */
> > 
>