diff mbox series

[11/22] xfs: refactor special inode roll out of xfs_dir_ialloc

Message ID 154630921076.18437.14193766050551378677.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: hoist inode operations to libxfs | expand

Commit Message

Darrick J. Wong Jan. 1, 2019, 2:20 a.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

In xfs_dir_ialloc, we roll the transaction if we had to allocate a new
inode chunk and before we actually initialize the inode.  In the kernel
this requires us to detach the transaction's quota charge information
from the ichunk allocation transaction and to attach it the ialloc
transaction because we don't charge quota for inode chunks.  This
doesn't exist in the userspace side of things, so pop it out into a
separately called function.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_inode_util.h |    6 ++++
 fs/xfs/xfs_inode.c             |   66 ++++++++++++++++++++++++----------------
 2 files changed, 46 insertions(+), 26 deletions(-)

Comments

Christoph Hellwig Jan. 17, 2019, 2:24 p.m. UTC | #1 
> diff --git a/fs/xfs/libxfs/xfs_inode_util.h b/fs/xfs/libxfs/xfs_inode_util.h
> index 5a1d98d1546d..ee274d74b8d4 100644
> --- a/fs/xfs/libxfs/xfs_inode_util.h
> +++ b/fs/xfs/libxfs/xfs_inode_util.h
> @@ -82,6 +82,12 @@ struct xfs_ialloc_ops {
>  
>  	/* Do any final setup needed before we return the inode. */
>  	void (*setup)(struct xfs_inode *ip);
> +
> +	/*
> +	 * Roll the transaction between allocating a new ichunk and
> +	 * initializing a new inode core.
> +	 */
> +	int (*ichunk_roll)(struct xfs_trans **tpp);

Sorry, but this whole idea to add gracious indirect calls is just
backwards.  They do have a non-trivial cost, and we should rather
get rid of pointless indirect calls instead of adding more.
Darrick J. Wong Jan. 17, 2019, 8:04 p.m. UTC | #2 
On Thu, Jan 17, 2019 at 06:24:27AM -0800, Christoph Hellwig wrote:
> > diff --git a/fs/xfs/libxfs/xfs_inode_util.h b/fs/xfs/libxfs/xfs_inode_util.h
> > index 5a1d98d1546d..ee274d74b8d4 100644
> > --- a/fs/xfs/libxfs/xfs_inode_util.h
> > +++ b/fs/xfs/libxfs/xfs_inode_util.h
> > @@ -82,6 +82,12 @@ struct xfs_ialloc_ops {
> >  
> >  	/* Do any final setup needed before we return the inode. */
> >  	void (*setup)(struct xfs_inode *ip);
> > +
> > +	/*
> > +	 * Roll the transaction between allocating a new ichunk and
> > +	 * initializing a new inode core.
> > +	 */
> > +	int (*ichunk_roll)(struct xfs_trans **tpp);
> 
> Sorry, but this whole idea to add gracious indirect calls is just
> backwards.  They do have a non-trivial cost, and we should rather
> get rid of pointless indirect calls instead of adding more.

I built all this indirect call stuff so that mkfs protofile code would
be able to set parameters ... but seeing as Eric said he'll deprecate
all that, I think it's no longer necessary.

--D
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_inode_util.h b/fs/xfs/libxfs/xfs_inode_util.h
index 5a1d98d1546d..ee274d74b8d4 100644
--- a/fs/xfs/libxfs/xfs_inode_util.h
+++ b/fs/xfs/libxfs/xfs_inode_util.h
@@ -82,6 +82,12 @@  struct xfs_ialloc_ops {
 
 	/* Do any final setup needed before we return the inode. */
 	void (*setup)(struct xfs_inode *ip);
+
+	/*
+	 * Roll the transaction between allocating a new ichunk and
+	 * initializing a new inode core.
+	 */
+	int (*ichunk_roll)(struct xfs_trans **tpp);
 };
 
 /* The libxfs client must provide this symbol. */
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index e6fda8777fe1..82de92bc84b9 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -647,10 +647,49 @@  xfs_ialloc_platform_init(
 	VFS_I(ip)->i_ctime = tv;
 }
 
+/*
+ * Roll the transaction after allocating an inode chunk and before allocating
+ * the actual inode, moving the quota charge information to the second
+ * transaction.
+ */
+static int
+xfs_dir_ialloc_roll(
+	struct xfs_trans	**tpp)
+{
+	struct xfs_dquot_acct	*dqinfo = NULL;
+	unsigned int		tflags = 0;
+	int			error;
+
+	/*
+	 * We want the quota changes to be associated with the next
+	 * transaction, NOT this one. So, detach the dqinfo from this
+	 * and attach it to the next transaction.
+	 */
+	if ((*tpp)->t_dqinfo) {
+		dqinfo = (*tpp)->t_dqinfo;
+		(*tpp)->t_dqinfo = NULL;
+		tflags = (*tpp)->t_flags & XFS_TRANS_DQ_DIRTY;
+		(*tpp)->t_flags &= ~(XFS_TRANS_DQ_DIRTY);
+	}
+
+	error = xfs_trans_roll(tpp);
+
+	/*
+	 * Re-attach the quota info that we detached from prev trx.
+	 */
+	if (dqinfo) {
+		(*tpp)->t_dqinfo = dqinfo;
+		(*tpp)->t_flags |= tflags;
+	}
+
+	return error;
+}
+
 const struct xfs_ialloc_ops xfs_default_ialloc_ops = {
 	.iget		= xfs_ialloc_iget,
 	.platform_init	= xfs_ialloc_platform_init,
 	.setup		= xfs_setup_inode,
+	.ichunk_roll	= xfs_dir_ialloc_roll,
 };
 
 /*
@@ -672,8 +711,6 @@  xfs_dir_ialloc(
 	struct xfs_trans		*tp;
 	struct xfs_inode		*ip;
 	struct xfs_buf			*ialloc_context = NULL;
-	void				*dqinfo;
-	uint				tflags;
 	int				code;
 
 	tp = *tpp;
@@ -726,30 +763,7 @@  xfs_dir_ialloc(
 		 */
 		xfs_trans_bhold(tp, ialloc_context);
 
-		/*
-		 * We want the quota changes to be associated with the next
-		 * transaction, NOT this one. So, detach the dqinfo from this
-		 * and attach it to the next transaction.
-		 */
-		dqinfo = NULL;
-		tflags = 0;
-		if (tp->t_dqinfo) {
-			dqinfo = (void *)tp->t_dqinfo;
-			tp->t_dqinfo = NULL;
-			tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
-			tp->t_flags &= ~(XFS_TRANS_DQ_DIRTY);
-		}
-
-		code = xfs_trans_roll(&tp);
-
-		/*
-		 * Re-attach the quota info that we detached from prev trx.
-		 */
-		if (dqinfo) {
-			tp->t_dqinfo = dqinfo;
-			tp->t_flags |= tflags;
-		}
-
+		code = args->ops->ichunk_roll(&tp);
 		if (code) {
 			xfs_buf_relse(ialloc_context);
 			*tpp = tp;