[PATCH v2 0/4] xfs: consolidate metadir creation code

[PATCH v2 0/4] xfs: consolidate metadir creation code

[Johannes Thumshirn]

The XFS metadata directory tree has grown several nearly identical
open-coded copies of the "create a metadata inode" sequence: allocate the
creation transaction, create and link the inode, do a bit of type
specific inode setup, commit, and finish or tear down the inode on both
the success and error paths.

Factor that boilerplate into a single helper, xfs_metadir_create_file(),
which takes the file mode and an optional callback to finish initializing
the new inode inside the creation transaction, and convert the existing
users - xfs_metadir_mkdir(), the quota inodes and the rtgroup inodes -
over to it.

Having a single helper also makes it straightforward to add new metadata
inode types without copying the sequence yet again.

No functional change intended otherwise.

Changes to v1:
- Re-based onto xfs/for-next HEAD: f88caa121eac ("xfs: use xfs_csn_t for xlog_cil_push_now() push_seq parameter")
- Added patch 4/4
- Added Christoph's review

Johannes Thumshirn (4):
  xfs: add xfs_metadir_create_file helper
  xfs: create quota metadir inodes using xfs_metadir_create_file
  xfs: create rtgroup metadir inodes using xfs_metadir_create_file
  xfs: mark internal metadir file creation helpers static

 fs/xfs/libxfs/xfs_dquot_buf.c | 39 ++++++---------------
 fs/xfs/libxfs/xfs_metadir.c   | 64 ++++++++++++++++++++++-------------
 fs/xfs/libxfs/xfs_metadir.h   |  8 +++--
 fs/xfs/libxfs/xfs_rtgroup.c   | 58 ++++++++++++++-----------------
 4 files changed, 82 insertions(+), 87 deletions(-)

-- 
2.54.0

[PATCH v2 1/4] xfs: add xfs_metadir_create_file helper

[Johannes Thumshirn]

Factor the metadata inode create/commit/cleanup lifecycle out of
xfs_metadir_mkdir into a reusable helper that takes an optional callback
to initialize the new inode, and convert xfs_metadir_mkdir to it.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/xfs/libxfs/xfs_metadir.c | 58 +++++++++++++++++++++++--------------
 fs/xfs/libxfs/xfs_metadir.h |  6 ++++
 2 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_metadir.c b/fs/xfs/libxfs/xfs_metadir.c
index 74c4596ee4cf..0d6a153bc9e9 100644
--- a/fs/xfs/libxfs/xfs_metadir.c
+++ b/fs/xfs/libxfs/xfs_metadir.c
@@ -438,48 +438,64 @@ xfs_metadir_cancel(
 	xfs_metadir_teardown(upd, error);
 }
 
-/* Create a metadata for the last component of the path. */
 int
-xfs_metadir_mkdir(
-	struct xfs_inode		*dp,
-	const char			*path,
+xfs_metadir_create_file(
+	struct xfs_metadir_update	*upd,
+	umode_t				mode,
+	xfs_metadir_createfn		create,
+	void				*priv,
 	struct xfs_inode		**ipp)
 {
-	struct xfs_metadir_update	upd = {
-		.dp			= dp,
-		.path			= path,
-		.metafile_type		= XFS_METAFILE_DIR,
-	};
 	int				error;
 
-	if (xfs_is_shutdown(dp->i_mount))
+	if (xfs_is_shutdown(upd->dp->i_mount))
 		return -EIO;
 
-	/* Allocate a transaction to create the last directory. */
-	error = xfs_metadir_start_create(&upd);
+	error = xfs_metadir_start_create(upd);
 	if (error)
 		return error;
 
-	/* Create the subdirectory and take our reference. */
-	error = xfs_metadir_create(&upd, S_IFDIR);
+	error = xfs_metadir_create(upd, mode);
 	if (error)
 		goto out_cancel;
 
-	error = xfs_metadir_commit(&upd);
+	if (create) {
+		error = create(upd, priv);
+		if (error)
+			goto out_cancel;
+	}
+
+	error = xfs_metadir_commit(upd);
 	if (error)
 		goto out_irele;
 
-	xfs_finish_inode_setup(upd.ip);
-	*ipp = upd.ip;
+	xfs_finish_inode_setup(upd->ip);
+	*ipp = upd->ip;
 	return 0;
 
 out_cancel:
-	xfs_metadir_cancel(&upd, error);
+	xfs_metadir_cancel(upd, error);
 out_irele:
 	/* Have to finish setting up the inode to ensure it's deleted. */
-	if (upd.ip) {
-		xfs_finish_inode_setup(upd.ip);
-		xfs_irele(upd.ip);
+	if (upd->ip) {
+		xfs_finish_inode_setup(upd->ip);
+		xfs_irele(upd->ip);
 	}
 	return error;
 }
+
+/* Create a metadata for the last component of the path. */
+int
+xfs_metadir_mkdir(
+	struct xfs_inode		*dp,
+	const char			*path,
+	struct xfs_inode		**ipp)
+{
+	struct xfs_metadir_update	upd = {
+		.dp			= dp,
+		.path			= path,
+		.metafile_type		= XFS_METAFILE_DIR,
+	};
+
+	return xfs_metadir_create_file(&upd, S_IFDIR, NULL, NULL, ipp);
+}
diff --git a/fs/xfs/libxfs/xfs_metadir.h b/fs/xfs/libxfs/xfs_metadir.h
index bfecac7d3d14..a795a2d0e3fe 100644
--- a/fs/xfs/libxfs/xfs_metadir.h
+++ b/fs/xfs/libxfs/xfs_metadir.h
@@ -35,6 +35,12 @@ int xfs_metadir_load(struct xfs_trans *tp, struct xfs_inode *dp,
 int xfs_metadir_start_create(struct xfs_metadir_update *upd);
 int xfs_metadir_create(struct xfs_metadir_update *upd, umode_t mode);
 
+typedef int (*xfs_metadir_createfn)(struct xfs_metadir_update *upd, void *priv);
+
+int xfs_metadir_create_file(struct xfs_metadir_update *upd, umode_t mode,
+		xfs_metadir_createfn create, void *priv,
+		struct xfs_inode **ipp);
+
 int xfs_metadir_start_link(struct xfs_metadir_update *upd);
 int xfs_metadir_link(struct xfs_metadir_update *upd);
 
-- 
2.54.0

[PATCH v2 2/4] xfs: create quota metadir inodes using xfs_metadir_create_file

[Johannes Thumshirn]

Now that we have xfs_metadir_create_file() use it in
xfs_dqinode_metadir_create().

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/xfs/libxfs/xfs_dquot_buf.c | 39 ++++++++++-------------------------
 1 file changed, 11 insertions(+), 28 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
index bbada0d3cc08..f960474bed3d 100644
--- a/fs/xfs/libxfs/xfs_dquot_buf.c
+++ b/fs/xfs/libxfs/xfs_dquot_buf.c
@@ -416,6 +416,15 @@ xfs_dqinode_load(
 	return 0;
 }
 
+static int
+xfs_dqinode_init(
+	struct xfs_metadir_update	*upd,
+	void				*priv)
+{
+	xfs_trans_log_inode(upd->tp, upd->ip, XFS_ILOG_CORE);
+	return 0;
+}
+
 /* Create a metadata directory quota inode. */
 int
 xfs_dqinode_metadir_create(
@@ -428,35 +437,9 @@ xfs_dqinode_metadir_create(
 		.metafile_type		= xfs_dqinode_metafile_type(type),
 		.path			= xfs_dqinode_path(type),
 	};
-	int				error;
 
-	error = xfs_metadir_start_create(&upd);
-	if (error)
-		return error;
-
-	error = xfs_metadir_create(&upd, S_IFREG);
-	if (error)
-		goto out_cancel;
-
-	xfs_trans_log_inode(upd.tp, upd.ip, XFS_ILOG_CORE);
-
-	error = xfs_metadir_commit(&upd);
-	if (error)
-		goto out_irele;
-
-	xfs_finish_inode_setup(upd.ip);
-	*ipp = upd.ip;
-	return 0;
-
-out_cancel:
-	xfs_metadir_cancel(&upd, error);
-out_irele:
-	/* Have to finish setting up the inode to ensure it's deleted. */
-	if (upd.ip) {
-		xfs_finish_inode_setup(upd.ip);
-		xfs_irele(upd.ip);
-	}
-	return error;
+	return xfs_metadir_create_file(&upd, S_IFREG, xfs_dqinode_init, NULL,
+			ipp);
 }
 
 #ifndef __KERNEL__
-- 
2.54.0

[PATCH v2 3/4] xfs: create rtgroup metadir inodes using xfs_metadir_create_file

[Johannes Thumshirn]

Now that we have xfs_metadir_create_file() use it in
xfs_rtginode_create().

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/xfs/libxfs/xfs_rtgroup.c | 58 +++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 32 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_rtgroup.c b/fs/xfs/libxfs/xfs_rtgroup.c
index c85d50953218..fe7222bbe449 100644
--- a/fs/xfs/libxfs/xfs_rtgroup.c
+++ b/fs/xfs/libxfs/xfs_rtgroup.c
@@ -517,6 +517,25 @@ xfs_rtginode_irele(
 	*ipp = NULL;
 }
 
+struct xfs_rtginode_create {
+	struct xfs_rtgroup		*rtg;
+	enum xfs_rtg_inodes		type;
+	bool				init;
+};
+
+static int
+xfs_rtginode_init(
+	struct xfs_metadir_update	*upd,
+	void				*priv)
+{
+	struct xfs_rtginode_create	*rc = priv;
+	const struct xfs_rtginode_ops	*ops = &xfs_rtginode_ops[rc->type];
+
+	xfs_rtginode_lockdep_setup(upd->ip, rtg_rgno(rc->rtg), rc->type);
+	upd->ip->i_projid = rtg_rgno(rc->rtg);
+	return ops->create(rc->rtg, upd->ip, upd->tp, rc->init);
+}
+
 /* Add a metadata inode for a realtime rmap btree. */
 int
 xfs_rtginode_create(
@@ -526,6 +545,11 @@ xfs_rtginode_create(
 {
 	const struct xfs_rtginode_ops	*ops = &xfs_rtginode_ops[type];
 	struct xfs_mount		*mp = rtg_mount(rtg);
+	struct xfs_rtginode_create	rc = {
+		.rtg			= rtg,
+		.type			= type,
+		.init			= init,
+	};
 	struct xfs_metadir_update	upd = {
 		.dp			= mp->m_rtdirip,
 		.metafile_type		= ops->metafile_type,
@@ -544,38 +568,8 @@ xfs_rtginode_create(
 	if (!upd.path)
 		return -ENOMEM;
 
-	error = xfs_metadir_start_create(&upd);
-	if (error)
-		goto out_path;
-
-	error = xfs_metadir_create(&upd, S_IFREG);
-	if (error)
-		goto out_cancel;
-
-	xfs_rtginode_lockdep_setup(upd.ip, rtg_rgno(rtg), type);
-
-	upd.ip->i_projid = rtg_rgno(rtg);
-	error = ops->create(rtg, upd.ip, upd.tp, init);
-	if (error)
-		goto out_cancel;
-
-	error = xfs_metadir_commit(&upd);
-	if (error)
-		goto out_path;
-
-	kfree(upd.path);
-	xfs_finish_inode_setup(upd.ip);
-	rtg->rtg_inodes[type] = upd.ip;
-	return 0;
-
-out_cancel:
-	xfs_metadir_cancel(&upd, error);
-	/* Have to finish setting up the inode to ensure it's deleted. */
-	if (upd.ip) {
-		xfs_finish_inode_setup(upd.ip);
-		xfs_irele(upd.ip);
-	}
-out_path:
+	error = xfs_metadir_create_file(&upd, S_IFREG, xfs_rtginode_init, &rc,
+			&rtg->rtg_inodes[type]);
 	kfree(upd.path);
 	return error;
 }
-- 
2.54.0

[PATCH v2 4/4] xfs: mark internal metadir file creation helpers static

[Johannes Thumshirn]

Now that there is xfs_metadir_create_file() mark
xfs_metadir_start_create(), xfs_metadir_create() and xfs_metadir_cancel()
as static and remove them from xfs_metadir.h.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/xfs/libxfs/xfs_metadir.c | 6 +++---
 fs/xfs/libxfs/xfs_metadir.h | 4 ----
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_metadir.c b/fs/xfs/libxfs/xfs_metadir.c
index 0d6a153bc9e9..7c6b086b73db 100644
--- a/fs/xfs/libxfs/xfs_metadir.c
+++ b/fs/xfs/libxfs/xfs_metadir.c
@@ -182,7 +182,7 @@ xfs_metadir_teardown(
  * Begin the process of creating a metadata file by allocating transactions
  * and taking whatever resources we're going to need.
  */
-int
+static int
 xfs_metadir_start_create(
 	struct xfs_metadir_update	*upd)
 {
@@ -236,7 +236,7 @@ xfs_metadir_start_create(
  * a negative error code.  If an inode is passed back, the caller must finish
  * setting up the inode before releasing it.
  */
-int
+static int
 xfs_metadir_create(
 	struct xfs_metadir_update	*upd,
 	umode_t				mode)
@@ -425,7 +425,7 @@ xfs_metadir_commit(
 }
 
 /* Cancel a metadir update and unlock/drop all resources. */
-void
+static void
 xfs_metadir_cancel(
 	struct xfs_metadir_update	*upd,
 	int				error)
diff --git a/fs/xfs/libxfs/xfs_metadir.h b/fs/xfs/libxfs/xfs_metadir.h
index a795a2d0e3fe..e434b9d1c932 100644
--- a/fs/xfs/libxfs/xfs_metadir.h
+++ b/fs/xfs/libxfs/xfs_metadir.h
@@ -32,9 +32,6 @@ int xfs_metadir_load(struct xfs_trans *tp, struct xfs_inode *dp,
 		const char *path, enum xfs_metafile_type metafile_type,
 		struct xfs_inode **ipp);
 
-int xfs_metadir_start_create(struct xfs_metadir_update *upd);
-int xfs_metadir_create(struct xfs_metadir_update *upd, umode_t mode);
-
 typedef int (*xfs_metadir_createfn)(struct xfs_metadir_update *upd, void *priv);
 
 int xfs_metadir_create_file(struct xfs_metadir_update *upd, umode_t mode,
@@ -45,7 +42,6 @@ int xfs_metadir_start_link(struct xfs_metadir_update *upd);
 int xfs_metadir_link(struct xfs_metadir_update *upd);
 
 int xfs_metadir_commit(struct xfs_metadir_update *upd);
-void xfs_metadir_cancel(struct xfs_metadir_update *upd, int error);
 
 int xfs_metadir_mkdir(struct xfs_inode *dp, const char *path,
 		struct xfs_inode **ipp);
-- 
2.54.0

Re: [PATCH v2 1/4] xfs: add xfs_metadir_create_file helper

[Christoph Hellwig]

Looks good:

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

Re: [PATCH v2 4/4] xfs: mark internal metadir file creation helpers static

[Christoph Hellwig]

Looks good:

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