* [PATCH v4 1/6] xfs: convert noroom, okalloc in xfs_dialloc() to bool
2020-12-08 12:19 [PATCH v4 0/6] xfs: some xfs_dialloc() cleanup Gao Xiang
@ 2020-12-08 12:19 ` Gao Xiang
2020-12-08 12:19 ` [PATCH v4 2/6] xfs: introduce xfs_dialloc_roll() Gao Xiang
` (4 subsequent siblings)
5 siblings, 0 replies; 15+ messages in thread
From: Gao Xiang @ 2020-12-08 12:19 UTC (permalink / raw)
To: linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, Eric Sandeen,
Gao Xiang
Boolean is preferred for such use.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 974e71bc4a3a..45cf7e55f5ee 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1716,11 +1716,11 @@ xfs_dialloc(
xfs_agnumber_t agno;
int error;
int ialloced;
- int noroom = 0;
+ bool noroom = false;
xfs_agnumber_t start_agno;
struct xfs_perag *pag;
struct xfs_ino_geometry *igeo = M_IGEO(mp);
- int okalloc = 1;
+ bool okalloc = true;
if (*IO_agbp) {
/*
@@ -1753,8 +1753,8 @@ xfs_dialloc(
if (igeo->maxicount &&
percpu_counter_read_positive(&mp->m_icount) + igeo->ialloc_inos
> igeo->maxicount) {
- noroom = 1;
- okalloc = 0;
+ noroom = true;
+ okalloc = false;
}
/*
--
2.18.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v4 2/6] xfs: introduce xfs_dialloc_roll()
2020-12-08 12:19 [PATCH v4 0/6] xfs: some xfs_dialloc() cleanup Gao Xiang
2020-12-08 12:19 ` [PATCH v4 1/6] xfs: convert noroom, okalloc in xfs_dialloc() to bool Gao Xiang
@ 2020-12-08 12:19 ` Gao Xiang
2020-12-08 23:09 ` Darrick J. Wong
2020-12-08 12:20 ` [PATCH v4 3/6] xfs: move on-disk inode allocation out of xfs_ialloc() Gao Xiang
` (3 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Gao Xiang @ 2020-12-08 12:19 UTC (permalink / raw)
To: linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, Eric Sandeen,
Dave Chinner, Gao Xiang
From: Dave Chinner <dchinner@redhat.com>
Introduce a helper to make the on-disk inode allocation rolling
logic clearer in preparation of the following cleanup.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 43 ++++++++++++++++++++++++++++++++++++++
fs/xfs/libxfs/xfs_ialloc.h | 5 +++++
fs/xfs/xfs_inode.c | 37 +-------------------------------
3 files changed, 49 insertions(+), 36 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 45cf7e55f5ee..23e94d43acb2 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1682,6 +1682,49 @@ xfs_dialloc_ag(
return error;
}
+int
+xfs_dialloc_roll(
+ struct xfs_trans **tpp,
+ struct xfs_buf *agibp)
+{
+ struct xfs_trans *tp = *tpp;
+ struct xfs_dquot_acct *dqinfo = NULL;
+ unsigned int tflags = 0;
+ int error;
+
+ /*
+ * Hold to on to the agibp across the commit so no other allocation can
+ * come in and take the free inodes we just allocated for our caller.
+ */
+ xfs_trans_bhold(tp, agibp);
+
+ /*
+ * 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 (tp->t_dqinfo) {
+ dqinfo = tp->t_dqinfo;
+ tp->t_dqinfo = NULL;
+ tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
+ tp->t_flags &= ~XFS_TRANS_DQ_DIRTY;
+ }
+
+ error = 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;
+ }
+
+ *tpp = tp;
+ if (error)
+ return error;
+ xfs_trans_bjoin(tp, agibp);
+ return 0;
+}
+
/*
* Allocate an inode on disk.
*
diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
index 72b3468b97b1..bd6e0db9e23c 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -32,6 +32,11 @@ xfs_make_iptr(struct xfs_mount *mp, struct xfs_buf *b, int o)
return xfs_buf_offset(b, o << (mp)->m_sb.sb_inodelog);
}
+int
+xfs_dialloc_roll(
+ struct xfs_trans **tpp,
+ struct xfs_buf *agibp);
+
/*
* Allocate an inode on disk.
* Mode is used to tell whether the new inode will need space, and whether
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 2bfbcf28b1bd..76282da7a05c 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -958,8 +958,6 @@ xfs_dir_ialloc(
xfs_inode_t *ip;
xfs_buf_t *ialloc_context = NULL;
int code;
- void *dqinfo;
- uint tflags;
tp = *tpp;
ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
@@ -1003,46 +1001,13 @@ xfs_dir_ialloc(
* to succeed the second time.
*/
if (ialloc_context) {
- /*
- * Normally, xfs_trans_commit releases all the locks.
- * We call bhold to hang on to the ialloc_context across
- * the commit. Holding this buffer prevents any other
- * processes from doing any allocations in this
- * allocation group.
- */
- 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 = xfs_dialloc_roll(&tp, ialloc_context);
if (code) {
xfs_buf_relse(ialloc_context);
*tpp = tp;
*ipp = NULL;
return code;
}
- xfs_trans_bjoin(tp, ialloc_context);
/*
* Call ialloc again. Since we've locked out all
--
2.18.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/6] xfs: introduce xfs_dialloc_roll()
2020-12-08 12:19 ` [PATCH v4 2/6] xfs: introduce xfs_dialloc_roll() Gao Xiang
@ 2020-12-08 23:09 ` Darrick J. Wong
2020-12-08 23:36 ` Gao Xiang
0 siblings, 1 reply; 15+ messages in thread
From: Darrick J. Wong @ 2020-12-08 23:09 UTC (permalink / raw)
To: Gao Xiang
Cc: linux-xfs, Dave Chinner, Christoph Hellwig, Eric Sandeen, Dave Chinner
On Tue, Dec 08, 2020 at 08:19:59PM +0800, Gao Xiang wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Introduce a helper to make the on-disk inode allocation rolling
> logic clearer in preparation of the following cleanup.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> fs/xfs/libxfs/xfs_ialloc.c | 43 ++++++++++++++++++++++++++++++++++++++
> fs/xfs/libxfs/xfs_ialloc.h | 5 +++++
> fs/xfs/xfs_inode.c | 37 +-------------------------------
> 3 files changed, 49 insertions(+), 36 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 45cf7e55f5ee..23e94d43acb2 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -1682,6 +1682,49 @@ xfs_dialloc_ag(
> return error;
> }
>
> +int
> +xfs_dialloc_roll(
> + struct xfs_trans **tpp,
> + struct xfs_buf *agibp)
> +{
> + struct xfs_trans *tp = *tpp;
> + struct xfs_dquot_acct *dqinfo = NULL;
> + unsigned int tflags = 0;
> + int error;
> +
> + /*
> + * Hold to on to the agibp across the commit so no other allocation can
> + * come in and take the free inodes we just allocated for our caller.
> + */
> + xfs_trans_bhold(tp, agibp);
> +
> + /*
> + * 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 (tp->t_dqinfo) {
> + dqinfo = tp->t_dqinfo;
> + tp->t_dqinfo = NULL;
> + tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
> + tp->t_flags &= ~XFS_TRANS_DQ_DIRTY;
FWIW, one of xiakaixu's cleanup patches removes XFS_TRANS_DQ_DIRTY on
the grounds that there seemed to be a 1:1 correlation between the dqinfo
being set and the flag being set. That creates a minor merge conflict
that I can fix at commit time. The rest looks fine, so
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> + }
> +
> + error = 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;
> + }
> +
> + *tpp = tp;
> + if (error)
> + return error;
> + xfs_trans_bjoin(tp, agibp);
> + return 0;
> +}
> +
> /*
> * Allocate an inode on disk.
> *
> diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
> index 72b3468b97b1..bd6e0db9e23c 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.h
> +++ b/fs/xfs/libxfs/xfs_ialloc.h
> @@ -32,6 +32,11 @@ xfs_make_iptr(struct xfs_mount *mp, struct xfs_buf *b, int o)
> return xfs_buf_offset(b, o << (mp)->m_sb.sb_inodelog);
> }
>
> +int
> +xfs_dialloc_roll(
> + struct xfs_trans **tpp,
> + struct xfs_buf *agibp);
> +
> /*
> * Allocate an inode on disk.
> * Mode is used to tell whether the new inode will need space, and whether
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 2bfbcf28b1bd..76282da7a05c 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -958,8 +958,6 @@ xfs_dir_ialloc(
> xfs_inode_t *ip;
> xfs_buf_t *ialloc_context = NULL;
> int code;
> - void *dqinfo;
> - uint tflags;
>
> tp = *tpp;
> ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> @@ -1003,46 +1001,13 @@ xfs_dir_ialloc(
> * to succeed the second time.
> */
> if (ialloc_context) {
> - /*
> - * Normally, xfs_trans_commit releases all the locks.
> - * We call bhold to hang on to the ialloc_context across
> - * the commit. Holding this buffer prevents any other
> - * processes from doing any allocations in this
> - * allocation group.
> - */
> - 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 = xfs_dialloc_roll(&tp, ialloc_context);
> if (code) {
> xfs_buf_relse(ialloc_context);
> *tpp = tp;
> *ipp = NULL;
> return code;
> }
> - xfs_trans_bjoin(tp, ialloc_context);
>
> /*
> * Call ialloc again. Since we've locked out all
> --
> 2.18.4
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/6] xfs: introduce xfs_dialloc_roll()
2020-12-08 23:09 ` Darrick J. Wong
@ 2020-12-08 23:36 ` Gao Xiang
0 siblings, 0 replies; 15+ messages in thread
From: Gao Xiang @ 2020-12-08 23:36 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-xfs, Dave Chinner, Christoph Hellwig, Eric Sandeen, Dave Chinner
On Tue, Dec 08, 2020 at 03:09:13PM -0800, Darrick J. Wong wrote:
> On Tue, Dec 08, 2020 at 08:19:59PM +0800, Gao Xiang wrote:
...
> >
> > +int
> > +xfs_dialloc_roll(
> > + struct xfs_trans **tpp,
> > + struct xfs_buf *agibp)
> > +{
> > + struct xfs_trans *tp = *tpp;
> > + struct xfs_dquot_acct *dqinfo = NULL;
> > + unsigned int tflags = 0;
> > + int error;
> > +
> > + /*
> > + * Hold to on to the agibp across the commit so no other allocation can
> > + * come in and take the free inodes we just allocated for our caller.
> > + */
> > + xfs_trans_bhold(tp, agibp);
> > +
> > + /*
> > + * 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 (tp->t_dqinfo) {
> > + dqinfo = tp->t_dqinfo;
> > + tp->t_dqinfo = NULL;
> > + tflags = tp->t_flags & XFS_TRANS_DQ_DIRTY;
> > + tp->t_flags &= ~XFS_TRANS_DQ_DIRTY;
>
> FWIW, one of xiakaixu's cleanup patches removes XFS_TRANS_DQ_DIRTY on
> the grounds that there seemed to be a 1:1 correlation between the dqinfo
> being set and the flag being set. That creates a minor merge conflict
> that I can fix at commit time. The rest looks fine, so
Yeah, it sounds good to me, thanks for reminder & help!
Thanks,
Gao Xiang
>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>
> --D
>
>
> > + }
> > +
> > + error = 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;
> > + }
> > +
> > + *tpp = tp;
> > + if (error)
> > + return error;
> > + xfs_trans_bjoin(tp, agibp);
> > + return 0;
> > +}
> > +
> > /*
> > * Allocate an inode on disk.
> > *
> > diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
> > index 72b3468b97b1..bd6e0db9e23c 100644
> > --- a/fs/xfs/libxfs/xfs_ialloc.h
> > +++ b/fs/xfs/libxfs/xfs_ialloc.h
> > @@ -32,6 +32,11 @@ xfs_make_iptr(struct xfs_mount *mp, struct xfs_buf *b, int o)
> > return xfs_buf_offset(b, o << (mp)->m_sb.sb_inodelog);
> > }
> >
> > +int
> > +xfs_dialloc_roll(
> > + struct xfs_trans **tpp,
> > + struct xfs_buf *agibp);
> > +
> > /*
> > * Allocate an inode on disk.
> > * Mode is used to tell whether the new inode will need space, and whether
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 2bfbcf28b1bd..76282da7a05c 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -958,8 +958,6 @@ xfs_dir_ialloc(
> > xfs_inode_t *ip;
> > xfs_buf_t *ialloc_context = NULL;
> > int code;
> > - void *dqinfo;
> > - uint tflags;
> >
> > tp = *tpp;
> > ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> > @@ -1003,46 +1001,13 @@ xfs_dir_ialloc(
> > * to succeed the second time.
> > */
> > if (ialloc_context) {
> > - /*
> > - * Normally, xfs_trans_commit releases all the locks.
> > - * We call bhold to hang on to the ialloc_context across
> > - * the commit. Holding this buffer prevents any other
> > - * processes from doing any allocations in this
> > - * allocation group.
> > - */
> > - 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 = xfs_dialloc_roll(&tp, ialloc_context);
> > if (code) {
> > xfs_buf_relse(ialloc_context);
> > *tpp = tp;
> > *ipp = NULL;
> > return code;
> > }
> > - xfs_trans_bjoin(tp, ialloc_context);
> >
> > /*
> > * Call ialloc again. Since we've locked out all
> > --
> > 2.18.4
> >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 3/6] xfs: move on-disk inode allocation out of xfs_ialloc()
2020-12-08 12:19 [PATCH v4 0/6] xfs: some xfs_dialloc() cleanup Gao Xiang
2020-12-08 12:19 ` [PATCH v4 1/6] xfs: convert noroom, okalloc in xfs_dialloc() to bool Gao Xiang
2020-12-08 12:19 ` [PATCH v4 2/6] xfs: introduce xfs_dialloc_roll() Gao Xiang
@ 2020-12-08 12:20 ` Gao Xiang
2020-12-08 23:09 ` Darrick J. Wong
2020-12-09 7:52 ` Christoph Hellwig
2020-12-08 12:20 ` [PATCH v4 4/6] xfs: move xfs_dialloc_roll() into xfs_dialloc() Gao Xiang
` (2 subsequent siblings)
5 siblings, 2 replies; 15+ messages in thread
From: Gao Xiang @ 2020-12-08 12:20 UTC (permalink / raw)
To: linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, Eric Sandeen,
Dave Chinner, Gao Xiang
From: Dave Chinner <dchinner@redhat.com>
So xfs_ialloc() will only address in-core inode allocation then,
Also, rename xfs_ialloc() to xfs_dir_ialloc_init() in order to
keep everything in xfs_inode.c under the same namespace.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
fs/xfs/xfs_inode.c | 220 +++++++++++++++----------------------------
fs/xfs/xfs_inode.h | 6 +-
fs/xfs/xfs_qm.c | 27 +++---
fs/xfs/xfs_symlink.c | 8 +-
4 files changed, 98 insertions(+), 163 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 76282da7a05c..ae6c83d46aaa 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -761,68 +761,25 @@ xfs_inode_inherit_flags2(
}
/*
- * Allocate an inode on disk and return a copy of its in-core version.
- * The in-core inode is locked exclusively. Set mode, nlink, and rdev
- * appropriately within the inode. The uid and gid for the inode are
- * set according to the contents of the given cred structure.
- *
- * Use xfs_dialloc() to allocate the on-disk inode. If xfs_dialloc()
- * has a free inode available, call xfs_iget() to obtain the in-core
- * version of the allocated inode. Finally, fill in the inode and
- * log its initial contents. In this case, ialloc_context would be
- * set to NULL.
- *
- * If xfs_dialloc() does not have an available inode, it will replenish
- * its supply by doing an allocation. Since we can only do one
- * allocation within a transaction without deadlocks, we must commit
- * the current transaction before returning the inode itself.
- * In this case, therefore, we will set ialloc_context and return.
- * The caller should then commit the current transaction, start a new
- * transaction, and call xfs_ialloc() again to actually get the inode.
- *
- * To ensure that some other process does not grab the inode that
- * was allocated during the first call to xfs_ialloc(), this routine
- * also returns the [locked] bp pointing to the head of the freelist
- * as ialloc_context. The caller should hold this buffer across
- * the commit and pass it back into this routine on the second call.
- *
- * If we are allocating quota inodes, we do not have a parent inode
- * to attach to or associate with (i.e. pip == NULL) because they
- * are not linked into the directory structure - they are attached
- * directly to the superblock - and so have no parent.
+ * Initialise a newly allocated inode and return the in-core inode to the
+ * caller locked exclusively.
*/
-static int
-xfs_ialloc(
- xfs_trans_t *tp,
- xfs_inode_t *pip,
- umode_t mode,
- xfs_nlink_t nlink,
- dev_t rdev,
- prid_t prid,
- xfs_buf_t **ialloc_context,
- xfs_inode_t **ipp)
+static struct xfs_inode *
+xfs_init_new_inode(
+ struct xfs_trans *tp,
+ struct xfs_inode *pip,
+ xfs_ino_t ino,
+ umode_t mode,
+ xfs_nlink_t nlink,
+ dev_t rdev,
+ prid_t prid)
{
- struct xfs_mount *mp = tp->t_mountp;
- xfs_ino_t ino;
- xfs_inode_t *ip;
- uint flags;
- int error;
- struct timespec64 tv;
- struct inode *inode;
-
- /*
- * Call the space management code to pick
- * the on-disk inode to be allocated.
- */
- error = xfs_dialloc(tp, pip ? pip->i_ino : 0, mode,
- ialloc_context, &ino);
- if (error)
- return error;
- if (*ialloc_context || ino == NULLFSINO) {
- *ipp = NULL;
- return 0;
- }
- ASSERT(*ialloc_context == NULL);
+ struct xfs_mount *mp = tp->t_mountp;
+ struct xfs_inode *ip;
+ unsigned int flags;
+ int error;
+ struct timespec64 tv;
+ struct inode *inode;
/*
* Protect against obviously corrupt allocation btree records. Later
@@ -833,18 +790,16 @@ xfs_ialloc(
*/
if ((pip && ino == pip->i_ino) || !xfs_verify_dir_ino(mp, ino)) {
xfs_alert(mp, "Allocated a known in-use inode 0x%llx!", ino);
- return -EFSCORRUPTED;
+ return ERR_PTR(-EFSCORRUPTED);
}
/*
- * Get the in-core inode with the lock held exclusively.
- * This is because we're setting fields here we need
- * to prevent others from looking at until we're done.
+ * Get the in-core inode with the lock held exclusively to prevent
+ * others from looking at until we're done.
*/
- error = xfs_iget(mp, tp, ino, XFS_IGET_CREATE,
- XFS_ILOCK_EXCL, &ip);
+ error = xfs_iget(mp, tp, ino, XFS_IGET_CREATE, XFS_ILOCK_EXCL, &ip);
if (error)
- return error;
+ return ERR_PTR(error);
ASSERT(ip != NULL);
inode = VFS_I(ip);
inode->i_mode = mode;
@@ -926,22 +881,21 @@ xfs_ialloc(
/* now that we have an i_mode we can setup the inode structure */
xfs_setup_inode(ip);
-
- *ipp = ip;
- return 0;
+ return ip;
}
/*
- * Allocates a new inode from disk and return a pointer to the
- * incore copy. This routine will internally commit the current
- * transaction and allocate a new one if the Space Manager needed
- * to do an allocation to replenish the inode free-list.
- *
- * This routine is designed to be called from xfs_create and
- * xfs_create_dir.
+ * Allocates a new inode from disk and return a pointer to the incore copy. This
+ * routine will internally commit the current transaction and allocate a new one
+ * if we needed to allocate more on-disk free inodes to perform the requested
+ * operation.
*
+ * If we are allocating quota inodes, we do not have a parent inode to attach to
+ * or associate with (i.e. dp == NULL) because they are not linked into the
+ * directory structure - they are attached directly to the superblock - and so
+ * have no parent.
*/
-int
+struct xfs_inode *
xfs_dir_ialloc(
xfs_trans_t **tpp, /* input: current transaction;
output: may be a new transaction. */
@@ -950,90 +904,60 @@ xfs_dir_ialloc(
umode_t mode,
xfs_nlink_t nlink,
dev_t rdev,
- prid_t prid, /* project id */
- xfs_inode_t **ipp) /* pointer to inode; it will be
- locked. */
+ prid_t prid) /* project id */
{
- xfs_trans_t *tp;
xfs_inode_t *ip;
xfs_buf_t *ialloc_context = NULL;
- int code;
-
- tp = *tpp;
- ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
+ xfs_ino_t parent_ino = dp ? dp->i_ino : 0;
+ xfs_ino_t ino;
+ int error;
- /*
- * xfs_ialloc will return a pointer to an incore inode if
- * the Space Manager has an available inode on the free
- * list. Otherwise, it will do an allocation and replenish
- * the freelist. Since we can only do one allocation per
- * transaction without deadlocks, we will need to commit the
- * current transaction and start a new one. We will then
- * need to call xfs_ialloc again to get the inode.
- *
- * If xfs_ialloc did an allocation to replenish the freelist,
- * it returns the bp containing the head of the freelist as
- * ialloc_context. We will hold a lock on it across the
- * transaction commit so that no other process can steal
- * the inode(s) that we've just allocated.
- */
- code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid, &ialloc_context,
- &ip);
+ ASSERT((*tpp)->t_flags & XFS_TRANS_PERM_LOG_RES);
/*
- * Return an error if we were unable to allocate a new inode.
- * This should only happen if we run out of space on disk or
- * encounter a disk error.
+ * Call the space management code to pick the on-disk inode to be
+ * allocated and replenish the freelist. Since we can only do one
+ * allocation per transaction without deadlocks, we will need to
+ * commit the current transaction and start a new one.
+ * If xfs_dialloc did an allocation to replenish the freelist, it
+ * returns the bp containing the head of the freelist as
+ * ialloc_context. We will hold a lock on it across the transaction
+ * commit so that no other process can steal the inode(s) that we've
+ * just allocated.
*/
- if (code) {
- *ipp = NULL;
- return code;
- }
- if (!ialloc_context && !ip) {
- *ipp = NULL;
- return -ENOSPC;
- }
+ error = xfs_dialloc(*tpp, parent_ino, mode, &ialloc_context, &ino);
+ if (error)
+ return ERR_PTR(error);
/*
* If the AGI buffer is non-NULL, then we were unable to get an
* inode in one operation. We need to commit the current
- * transaction and call xfs_ialloc() again. It is guaranteed
+ * transaction and call xfs_dialloc() again. It is guaranteed
* to succeed the second time.
*/
if (ialloc_context) {
- code = xfs_dialloc_roll(&tp, ialloc_context);
- if (code) {
+ error = xfs_dialloc_roll(tpp, ialloc_context);
+ if (error) {
xfs_buf_relse(ialloc_context);
- *tpp = tp;
- *ipp = NULL;
- return code;
+ return ERR_PTR(error);
}
-
/*
- * Call ialloc again. Since we've locked out all
- * other allocations in this allocation group,
- * this call should always succeed.
+ * Call dialloc again. Since we've locked out all other
+ * allocations in this allocation group, this call should
+ * always succeed.
*/
- code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid,
- &ialloc_context, &ip);
-
- /*
- * If we get an error at this point, return to the caller
- * so that the current transaction can be aborted.
- */
- if (code) {
- *tpp = tp;
- *ipp = NULL;
- return code;
- }
- ASSERT(!ialloc_context && ip);
-
+ error = xfs_dialloc(*tpp, parent_ino, mode,
+ &ialloc_context, &ino);
+ if (error)
+ return ERR_PTR(error);
+ ASSERT(!ialloc_context);
}
- *ipp = ip;
- *tpp = tp;
+ if (ino == NULLFSINO)
+ return ERR_PTR(-ENOSPC);
- return 0;
+ /* Initialise the newly allocated inode. */
+ return xfs_init_new_inode(*tpp, dp, ino, mode, nlink, rdev, prid);
}
/*
@@ -1147,9 +1071,12 @@ xfs_create(
* entry pointing to them, but a directory also the "." entry
* pointing to itself.
*/
- error = xfs_dir_ialloc(&tp, dp, mode, is_dir ? 2 : 1, rdev, prid, &ip);
- if (error)
+ ip = xfs_dir_ialloc(&tp, dp, mode, is_dir ? 2 : 1, rdev, prid);
+ if (IS_ERR(ip)) {
+ error = PTR_ERR(ip);
+ ip = NULL;
goto out_trans_cancel;
+ }
/*
* Now we join the directory inode to the transaction. We do not do it
@@ -1269,9 +1196,12 @@ xfs_create_tmpfile(
if (error)
goto out_trans_cancel;
- error = xfs_dir_ialloc(&tp, dp, mode, 0, 0, prid, &ip);
- if (error)
+ ip = xfs_dir_ialloc(&tp, dp, mode, 0, 0, prid);
+ if (IS_ERR(ip)) {
+ error = PTR_ERR(ip);
+ ip = NULL;
goto out_trans_cancel;
+ }
if (mp->m_flags & XFS_MOUNT_WSYNC)
xfs_trans_set_sync(tp);
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 751a3d1d7d84..95b4ae35e6df 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -407,9 +407,9 @@ void xfs_lock_two_inodes(struct xfs_inode *ip0, uint ip0_mode,
xfs_extlen_t xfs_get_extsz_hint(struct xfs_inode *ip);
xfs_extlen_t xfs_get_cowextsz_hint(struct xfs_inode *ip);
-int xfs_dir_ialloc(struct xfs_trans **, struct xfs_inode *, umode_t,
- xfs_nlink_t, dev_t, prid_t,
- struct xfs_inode **);
+struct xfs_inode *
+xfs_dir_ialloc(struct xfs_trans **, struct xfs_inode *, umode_t, xfs_nlink_t,
+ dev_t, prid_t);
static inline int
xfs_itruncate_extents(
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index b2a9abee8b2b..bfdf71d87777 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -737,15 +737,15 @@ xfs_qm_destroy_quotainfo(
*/
STATIC int
xfs_qm_qino_alloc(
- xfs_mount_t *mp,
- xfs_inode_t **ip,
- uint flags)
+ struct xfs_mount *mp,
+ struct xfs_inode **ipp,
+ unsigned int flags)
{
xfs_trans_t *tp;
int error;
bool need_alloc = true;
- *ip = NULL;
+ *ipp = NULL;
/*
* With superblock that doesn't have separate pquotino, we
* share an inode between gquota and pquota. If the on-disk
@@ -771,7 +771,7 @@ xfs_qm_qino_alloc(
return -EFSCORRUPTED;
}
if (ino != NULLFSINO) {
- error = xfs_iget(mp, NULL, ino, 0, 0, ip);
+ error = xfs_iget(mp, NULL, ino, 0, 0, ipp);
if (error)
return error;
mp->m_sb.sb_gquotino = NULLFSINO;
@@ -787,11 +787,14 @@ xfs_qm_qino_alloc(
return error;
if (need_alloc) {
- error = xfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0, 0, ip);
- if (error) {
+ struct xfs_inode *ip;
+
+ ip = xfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0, 0);
+ if (IS_ERR(ip)) {
xfs_trans_cancel(tp);
- return error;
+ return PTR_ERR(ip);
}
+ *ipp = ip;
}
/*
@@ -812,11 +815,11 @@ xfs_qm_qino_alloc(
mp->m_sb.sb_qflags = mp->m_qflags & XFS_ALL_QUOTA_ACCT;
}
if (flags & XFS_QMOPT_UQUOTA)
- mp->m_sb.sb_uquotino = (*ip)->i_ino;
+ mp->m_sb.sb_uquotino = (*ipp)->i_ino;
else if (flags & XFS_QMOPT_GQUOTA)
- mp->m_sb.sb_gquotino = (*ip)->i_ino;
+ mp->m_sb.sb_gquotino = (*ipp)->i_ino;
else
- mp->m_sb.sb_pquotino = (*ip)->i_ino;
+ mp->m_sb.sb_pquotino = (*ipp)->i_ino;
spin_unlock(&mp->m_sb_lock);
xfs_log_sb(tp);
@@ -826,7 +829,7 @@ xfs_qm_qino_alloc(
xfs_alert(mp, "%s failed (error %d)!", __func__, error);
}
if (need_alloc)
- xfs_finish_inode_setup(*ip);
+ xfs_finish_inode_setup(*ipp);
return error;
}
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index 8e88a7ca387e..988fc771f089 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -223,10 +223,12 @@ xfs_symlink(
/*
* Allocate an inode for the symlink.
*/
- error = xfs_dir_ialloc(&tp, dp, S_IFLNK | (mode & ~S_IFMT), 1, 0,
- prid, &ip);
- if (error)
+ ip = xfs_dir_ialloc(&tp, dp, S_IFLNK | (mode & ~S_IFMT), 1, 0, prid);
+ if (IS_ERR(ip)) {
+ error = PTR_ERR(ip);
+ ip = NULL;
goto out_trans_cancel;
+ }
/*
* Now we join the directory inode to the transaction. We do not do it
--
2.18.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/6] xfs: move on-disk inode allocation out of xfs_ialloc()
2020-12-08 12:20 ` [PATCH v4 3/6] xfs: move on-disk inode allocation out of xfs_ialloc() Gao Xiang
@ 2020-12-08 23:09 ` Darrick J. Wong
2020-12-09 7:52 ` Christoph Hellwig
1 sibling, 0 replies; 15+ messages in thread
From: Darrick J. Wong @ 2020-12-08 23:09 UTC (permalink / raw)
To: Gao Xiang
Cc: linux-xfs, Dave Chinner, Christoph Hellwig, Eric Sandeen, Dave Chinner
On Tue, Dec 08, 2020 at 08:20:00PM +0800, Gao Xiang wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> So xfs_ialloc() will only address in-core inode allocation then,
> Also, rename xfs_ialloc() to xfs_dir_ialloc_init() in order to
> keep everything in xfs_inode.c under the same namespace.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
Looks fine to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_inode.c | 220 +++++++++++++++----------------------------
> fs/xfs/xfs_inode.h | 6 +-
> fs/xfs/xfs_qm.c | 27 +++---
> fs/xfs/xfs_symlink.c | 8 +-
> 4 files changed, 98 insertions(+), 163 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 76282da7a05c..ae6c83d46aaa 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -761,68 +761,25 @@ xfs_inode_inherit_flags2(
> }
>
> /*
> - * Allocate an inode on disk and return a copy of its in-core version.
> - * The in-core inode is locked exclusively. Set mode, nlink, and rdev
> - * appropriately within the inode. The uid and gid for the inode are
> - * set according to the contents of the given cred structure.
> - *
> - * Use xfs_dialloc() to allocate the on-disk inode. If xfs_dialloc()
> - * has a free inode available, call xfs_iget() to obtain the in-core
> - * version of the allocated inode. Finally, fill in the inode and
> - * log its initial contents. In this case, ialloc_context would be
> - * set to NULL.
> - *
> - * If xfs_dialloc() does not have an available inode, it will replenish
> - * its supply by doing an allocation. Since we can only do one
> - * allocation within a transaction without deadlocks, we must commit
> - * the current transaction before returning the inode itself.
> - * In this case, therefore, we will set ialloc_context and return.
> - * The caller should then commit the current transaction, start a new
> - * transaction, and call xfs_ialloc() again to actually get the inode.
> - *
> - * To ensure that some other process does not grab the inode that
> - * was allocated during the first call to xfs_ialloc(), this routine
> - * also returns the [locked] bp pointing to the head of the freelist
> - * as ialloc_context. The caller should hold this buffer across
> - * the commit and pass it back into this routine on the second call.
> - *
> - * If we are allocating quota inodes, we do not have a parent inode
> - * to attach to or associate with (i.e. pip == NULL) because they
> - * are not linked into the directory structure - they are attached
> - * directly to the superblock - and so have no parent.
> + * Initialise a newly allocated inode and return the in-core inode to the
> + * caller locked exclusively.
> */
> -static int
> -xfs_ialloc(
> - xfs_trans_t *tp,
> - xfs_inode_t *pip,
> - umode_t mode,
> - xfs_nlink_t nlink,
> - dev_t rdev,
> - prid_t prid,
> - xfs_buf_t **ialloc_context,
> - xfs_inode_t **ipp)
> +static struct xfs_inode *
> +xfs_init_new_inode(
> + struct xfs_trans *tp,
> + struct xfs_inode *pip,
> + xfs_ino_t ino,
> + umode_t mode,
> + xfs_nlink_t nlink,
> + dev_t rdev,
> + prid_t prid)
> {
> - struct xfs_mount *mp = tp->t_mountp;
> - xfs_ino_t ino;
> - xfs_inode_t *ip;
> - uint flags;
> - int error;
> - struct timespec64 tv;
> - struct inode *inode;
> -
> - /*
> - * Call the space management code to pick
> - * the on-disk inode to be allocated.
> - */
> - error = xfs_dialloc(tp, pip ? pip->i_ino : 0, mode,
> - ialloc_context, &ino);
> - if (error)
> - return error;
> - if (*ialloc_context || ino == NULLFSINO) {
> - *ipp = NULL;
> - return 0;
> - }
> - ASSERT(*ialloc_context == NULL);
> + struct xfs_mount *mp = tp->t_mountp;
> + struct xfs_inode *ip;
> + unsigned int flags;
> + int error;
> + struct timespec64 tv;
> + struct inode *inode;
>
> /*
> * Protect against obviously corrupt allocation btree records. Later
> @@ -833,18 +790,16 @@ xfs_ialloc(
> */
> if ((pip && ino == pip->i_ino) || !xfs_verify_dir_ino(mp, ino)) {
> xfs_alert(mp, "Allocated a known in-use inode 0x%llx!", ino);
> - return -EFSCORRUPTED;
> + return ERR_PTR(-EFSCORRUPTED);
> }
>
> /*
> - * Get the in-core inode with the lock held exclusively.
> - * This is because we're setting fields here we need
> - * to prevent others from looking at until we're done.
> + * Get the in-core inode with the lock held exclusively to prevent
> + * others from looking at until we're done.
> */
> - error = xfs_iget(mp, tp, ino, XFS_IGET_CREATE,
> - XFS_ILOCK_EXCL, &ip);
> + error = xfs_iget(mp, tp, ino, XFS_IGET_CREATE, XFS_ILOCK_EXCL, &ip);
> if (error)
> - return error;
> + return ERR_PTR(error);
> ASSERT(ip != NULL);
> inode = VFS_I(ip);
> inode->i_mode = mode;
> @@ -926,22 +881,21 @@ xfs_ialloc(
>
> /* now that we have an i_mode we can setup the inode structure */
> xfs_setup_inode(ip);
> -
> - *ipp = ip;
> - return 0;
> + return ip;
> }
>
> /*
> - * Allocates a new inode from disk and return a pointer to the
> - * incore copy. This routine will internally commit the current
> - * transaction and allocate a new one if the Space Manager needed
> - * to do an allocation to replenish the inode free-list.
> - *
> - * This routine is designed to be called from xfs_create and
> - * xfs_create_dir.
> + * Allocates a new inode from disk and return a pointer to the incore copy. This
> + * routine will internally commit the current transaction and allocate a new one
> + * if we needed to allocate more on-disk free inodes to perform the requested
> + * operation.
> *
> + * If we are allocating quota inodes, we do not have a parent inode to attach to
> + * or associate with (i.e. dp == NULL) because they are not linked into the
> + * directory structure - they are attached directly to the superblock - and so
> + * have no parent.
> */
> -int
> +struct xfs_inode *
> xfs_dir_ialloc(
> xfs_trans_t **tpp, /* input: current transaction;
> output: may be a new transaction. */
> @@ -950,90 +904,60 @@ xfs_dir_ialloc(
> umode_t mode,
> xfs_nlink_t nlink,
> dev_t rdev,
> - prid_t prid, /* project id */
> - xfs_inode_t **ipp) /* pointer to inode; it will be
> - locked. */
> + prid_t prid) /* project id */
> {
> - xfs_trans_t *tp;
> xfs_inode_t *ip;
> xfs_buf_t *ialloc_context = NULL;
> - int code;
> -
> - tp = *tpp;
> - ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
> + xfs_ino_t parent_ino = dp ? dp->i_ino : 0;
> + xfs_ino_t ino;
> + int error;
>
> - /*
> - * xfs_ialloc will return a pointer to an incore inode if
> - * the Space Manager has an available inode on the free
> - * list. Otherwise, it will do an allocation and replenish
> - * the freelist. Since we can only do one allocation per
> - * transaction without deadlocks, we will need to commit the
> - * current transaction and start a new one. We will then
> - * need to call xfs_ialloc again to get the inode.
> - *
> - * If xfs_ialloc did an allocation to replenish the freelist,
> - * it returns the bp containing the head of the freelist as
> - * ialloc_context. We will hold a lock on it across the
> - * transaction commit so that no other process can steal
> - * the inode(s) that we've just allocated.
> - */
> - code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid, &ialloc_context,
> - &ip);
> + ASSERT((*tpp)->t_flags & XFS_TRANS_PERM_LOG_RES);
>
> /*
> - * Return an error if we were unable to allocate a new inode.
> - * This should only happen if we run out of space on disk or
> - * encounter a disk error.
> + * Call the space management code to pick the on-disk inode to be
> + * allocated and replenish the freelist. Since we can only do one
> + * allocation per transaction without deadlocks, we will need to
> + * commit the current transaction and start a new one.
> + * If xfs_dialloc did an allocation to replenish the freelist, it
> + * returns the bp containing the head of the freelist as
> + * ialloc_context. We will hold a lock on it across the transaction
> + * commit so that no other process can steal the inode(s) that we've
> + * just allocated.
> */
> - if (code) {
> - *ipp = NULL;
> - return code;
> - }
> - if (!ialloc_context && !ip) {
> - *ipp = NULL;
> - return -ENOSPC;
> - }
> + error = xfs_dialloc(*tpp, parent_ino, mode, &ialloc_context, &ino);
> + if (error)
> + return ERR_PTR(error);
>
> /*
> * If the AGI buffer is non-NULL, then we were unable to get an
> * inode in one operation. We need to commit the current
> - * transaction and call xfs_ialloc() again. It is guaranteed
> + * transaction and call xfs_dialloc() again. It is guaranteed
> * to succeed the second time.
> */
> if (ialloc_context) {
> - code = xfs_dialloc_roll(&tp, ialloc_context);
> - if (code) {
> + error = xfs_dialloc_roll(tpp, ialloc_context);
> + if (error) {
> xfs_buf_relse(ialloc_context);
> - *tpp = tp;
> - *ipp = NULL;
> - return code;
> + return ERR_PTR(error);
> }
> -
> /*
> - * Call ialloc again. Since we've locked out all
> - * other allocations in this allocation group,
> - * this call should always succeed.
> + * Call dialloc again. Since we've locked out all other
> + * allocations in this allocation group, this call should
> + * always succeed.
> */
> - code = xfs_ialloc(tp, dp, mode, nlink, rdev, prid,
> - &ialloc_context, &ip);
> -
> - /*
> - * If we get an error at this point, return to the caller
> - * so that the current transaction can be aborted.
> - */
> - if (code) {
> - *tpp = tp;
> - *ipp = NULL;
> - return code;
> - }
> - ASSERT(!ialloc_context && ip);
> -
> + error = xfs_dialloc(*tpp, parent_ino, mode,
> + &ialloc_context, &ino);
> + if (error)
> + return ERR_PTR(error);
> + ASSERT(!ialloc_context);
> }
>
> - *ipp = ip;
> - *tpp = tp;
> + if (ino == NULLFSINO)
> + return ERR_PTR(-ENOSPC);
>
> - return 0;
> + /* Initialise the newly allocated inode. */
> + return xfs_init_new_inode(*tpp, dp, ino, mode, nlink, rdev, prid);
> }
>
> /*
> @@ -1147,9 +1071,12 @@ xfs_create(
> * entry pointing to them, but a directory also the "." entry
> * pointing to itself.
> */
> - error = xfs_dir_ialloc(&tp, dp, mode, is_dir ? 2 : 1, rdev, prid, &ip);
> - if (error)
> + ip = xfs_dir_ialloc(&tp, dp, mode, is_dir ? 2 : 1, rdev, prid);
> + if (IS_ERR(ip)) {
> + error = PTR_ERR(ip);
> + ip = NULL;
> goto out_trans_cancel;
> + }
>
> /*
> * Now we join the directory inode to the transaction. We do not do it
> @@ -1269,9 +1196,12 @@ xfs_create_tmpfile(
> if (error)
> goto out_trans_cancel;
>
> - error = xfs_dir_ialloc(&tp, dp, mode, 0, 0, prid, &ip);
> - if (error)
> + ip = xfs_dir_ialloc(&tp, dp, mode, 0, 0, prid);
> + if (IS_ERR(ip)) {
> + error = PTR_ERR(ip);
> + ip = NULL;
> goto out_trans_cancel;
> + }
>
> if (mp->m_flags & XFS_MOUNT_WSYNC)
> xfs_trans_set_sync(tp);
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 751a3d1d7d84..95b4ae35e6df 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -407,9 +407,9 @@ void xfs_lock_two_inodes(struct xfs_inode *ip0, uint ip0_mode,
> xfs_extlen_t xfs_get_extsz_hint(struct xfs_inode *ip);
> xfs_extlen_t xfs_get_cowextsz_hint(struct xfs_inode *ip);
>
> -int xfs_dir_ialloc(struct xfs_trans **, struct xfs_inode *, umode_t,
> - xfs_nlink_t, dev_t, prid_t,
> - struct xfs_inode **);
> +struct xfs_inode *
> +xfs_dir_ialloc(struct xfs_trans **, struct xfs_inode *, umode_t, xfs_nlink_t,
> + dev_t, prid_t);
>
> static inline int
> xfs_itruncate_extents(
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index b2a9abee8b2b..bfdf71d87777 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -737,15 +737,15 @@ xfs_qm_destroy_quotainfo(
> */
> STATIC int
> xfs_qm_qino_alloc(
> - xfs_mount_t *mp,
> - xfs_inode_t **ip,
> - uint flags)
> + struct xfs_mount *mp,
> + struct xfs_inode **ipp,
> + unsigned int flags)
> {
> xfs_trans_t *tp;
> int error;
> bool need_alloc = true;
>
> - *ip = NULL;
> + *ipp = NULL;
> /*
> * With superblock that doesn't have separate pquotino, we
> * share an inode between gquota and pquota. If the on-disk
> @@ -771,7 +771,7 @@ xfs_qm_qino_alloc(
> return -EFSCORRUPTED;
> }
> if (ino != NULLFSINO) {
> - error = xfs_iget(mp, NULL, ino, 0, 0, ip);
> + error = xfs_iget(mp, NULL, ino, 0, 0, ipp);
> if (error)
> return error;
> mp->m_sb.sb_gquotino = NULLFSINO;
> @@ -787,11 +787,14 @@ xfs_qm_qino_alloc(
> return error;
>
> if (need_alloc) {
> - error = xfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0, 0, ip);
> - if (error) {
> + struct xfs_inode *ip;
> +
> + ip = xfs_dir_ialloc(&tp, NULL, S_IFREG, 1, 0, 0);
> + if (IS_ERR(ip)) {
> xfs_trans_cancel(tp);
> - return error;
> + return PTR_ERR(ip);
> }
> + *ipp = ip;
> }
>
> /*
> @@ -812,11 +815,11 @@ xfs_qm_qino_alloc(
> mp->m_sb.sb_qflags = mp->m_qflags & XFS_ALL_QUOTA_ACCT;
> }
> if (flags & XFS_QMOPT_UQUOTA)
> - mp->m_sb.sb_uquotino = (*ip)->i_ino;
> + mp->m_sb.sb_uquotino = (*ipp)->i_ino;
> else if (flags & XFS_QMOPT_GQUOTA)
> - mp->m_sb.sb_gquotino = (*ip)->i_ino;
> + mp->m_sb.sb_gquotino = (*ipp)->i_ino;
> else
> - mp->m_sb.sb_pquotino = (*ip)->i_ino;
> + mp->m_sb.sb_pquotino = (*ipp)->i_ino;
> spin_unlock(&mp->m_sb_lock);
> xfs_log_sb(tp);
>
> @@ -826,7 +829,7 @@ xfs_qm_qino_alloc(
> xfs_alert(mp, "%s failed (error %d)!", __func__, error);
> }
> if (need_alloc)
> - xfs_finish_inode_setup(*ip);
> + xfs_finish_inode_setup(*ipp);
> return error;
> }
>
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 8e88a7ca387e..988fc771f089 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -223,10 +223,12 @@ xfs_symlink(
> /*
> * Allocate an inode for the symlink.
> */
> - error = xfs_dir_ialloc(&tp, dp, S_IFLNK | (mode & ~S_IFMT), 1, 0,
> - prid, &ip);
> - if (error)
> + ip = xfs_dir_ialloc(&tp, dp, S_IFLNK | (mode & ~S_IFMT), 1, 0, prid);
> + if (IS_ERR(ip)) {
> + error = PTR_ERR(ip);
> + ip = NULL;
> goto out_trans_cancel;
> + }
>
> /*
> * Now we join the directory inode to the transaction. We do not do it
> --
> 2.18.4
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/6] xfs: move on-disk inode allocation out of xfs_ialloc()
2020-12-08 12:20 ` [PATCH v4 3/6] xfs: move on-disk inode allocation out of xfs_ialloc() Gao Xiang
2020-12-08 23:09 ` Darrick J. Wong
@ 2020-12-09 7:52 ` Christoph Hellwig
2020-12-09 8:43 ` Gao Xiang
1 sibling, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2020-12-09 7:52 UTC (permalink / raw)
To: Gao Xiang
Cc: linux-xfs, Darrick J. Wong, Dave Chinner, Christoph Hellwig,
Eric Sandeen, Dave Chinner
> + /* Initialise the newly allocated inode. */
> + return xfs_init_new_inode(*tpp, dp, ino, mode, nlink, rdev, prid);
IMHO this comment is not overly helpful..
> + if (IS_ERR(ip)) {
> + error = PTR_ERR(ip);
> + ip = NULL;
> goto out_trans_cancel;
> + }
And the calling convention with the ERR_PTR return does not seem to
fit the call chain to well. But those are minor details, so:
> STATIC int
> xfs_qm_qino_alloc(
> - xfs_mount_t *mp,
> - xfs_inode_t **ip,
> - uint flags)
> + struct xfs_mount *mp,
> + struct xfs_inode **ipp,
> + unsigned int flags)
> {
> xfs_trans_t *tp;
> int error;
> bool need_alloc = true;
Why do you reindent and de-typdefify the arguments, but not the local
variables?
All the stuff below also seems to deal with the fact that the old return
ip by reference calling convention seems to actually work better with
the code base..
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/6] xfs: move on-disk inode allocation out of xfs_ialloc()
2020-12-09 7:52 ` Christoph Hellwig
@ 2020-12-09 8:43 ` Gao Xiang
2020-12-09 10:22 ` Christoph Hellwig
0 siblings, 1 reply; 15+ messages in thread
From: Gao Xiang @ 2020-12-09 8:43 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-xfs, Darrick J. Wong, Dave Chinner, Eric Sandeen, Dave Chinner
Hi Christoph,
On Wed, Dec 09, 2020 at 08:52:46AM +0100, Christoph Hellwig wrote:
> > + /* Initialise the newly allocated inode. */
> > + return xfs_init_new_inode(*tpp, dp, ino, mode, nlink, rdev, prid);
>
> IMHO this comment is not overly helpful..
That was inherited from old version, I could get rid of in the next version....
>
> > + if (IS_ERR(ip)) {
> > + error = PTR_ERR(ip);
> > + ip = NULL;
> > goto out_trans_cancel;
> > + }
>
> And the calling convention with the ERR_PTR return does not seem to
> fit the call chain to well. But those are minor details, so:
Yeah, I also think so, since I found many error exit paths
rely on ip == NULL.
>
> > STATIC int
> > xfs_qm_qino_alloc(
> > - xfs_mount_t *mp,
> > - xfs_inode_t **ip,
> > - uint flags)
> > + struct xfs_mount *mp,
> > + struct xfs_inode **ipp,
> > + unsigned int flags)
> > {
> > xfs_trans_t *tp;
> > int error;
> > bool need_alloc = true;
>
> Why do you reindent and de-typdefify the arguments, but not the local
> variables?
since I renamed *ip to *ipp (it seems that should be *ipp here), so I need to
modify this line
- xfs_inode_t **ip,
so I fixed the typedef, but it introduced an intent issue, so I fixed the
whole input argument block for better coding style.
That is related to the argument only, so I didn't fix the local argument
though (since I didn't touch them).
>
> All the stuff below also seems to deal with the fact that the old return
> ip by reference calling convention seems to actually work better with
> the code base..
Yeah, so maybe I should revert back to the old code? not sure... Anyway,
I think codebase could be changed over time from a single change. Anyway,
I'm fine with either way. So I may hear your perference about this and send
out the next version (I think such cleanup can be fited in 5.11, so I can
base on this and do more work....)
Thanks,
Gao Xiang
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/6] xfs: move on-disk inode allocation out of xfs_ialloc()
2020-12-09 8:43 ` Gao Xiang
@ 2020-12-09 10:22 ` Christoph Hellwig
2020-12-09 11:13 ` Gao Xiang
0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2020-12-09 10:22 UTC (permalink / raw)
To: Gao Xiang
Cc: Christoph Hellwig, linux-xfs, Darrick J. Wong, Dave Chinner,
Eric Sandeen, Dave Chinner
On Wed, Dec 09, 2020 at 04:43:42PM +0800, Gao Xiang wrote:
> Yeah, so maybe I should revert back to the old code? not sure... Anyway,
> I think codebase could be changed over time from a single change. Anyway,
> I'm fine with either way. So I may hear your perference about this and send
> out the next version (I think such cleanup can be fited in 5.11, so I can
> base on this and do more work....)
Personally I'd prefer to just use the errno return and ipp by reference
calling convention for the newly added helper as well. But I'm ok with
all variants, so maybe I should add my:
Reviewed-by: Christoph Hellwig <hch@lst.de>
here in case Darrick wants to pick this up.
Looking at idmapped mounts series it would really help to get this in
ASAP to avoid conflicts.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 3/6] xfs: move on-disk inode allocation out of xfs_ialloc()
2020-12-09 10:22 ` Christoph Hellwig
@ 2020-12-09 11:13 ` Gao Xiang
0 siblings, 0 replies; 15+ messages in thread
From: Gao Xiang @ 2020-12-09 11:13 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-xfs, Darrick J. Wong, Dave Chinner, Eric Sandeen, Dave Chinner
On Wed, Dec 09, 2020 at 11:22:37AM +0100, Christoph Hellwig wrote:
> On Wed, Dec 09, 2020 at 04:43:42PM +0800, Gao Xiang wrote:
> > Yeah, so maybe I should revert back to the old code? not sure... Anyway,
> > I think codebase could be changed over time from a single change. Anyway,
> > I'm fine with either way. So I may hear your perference about this and send
> > out the next version (I think such cleanup can be fited in 5.11, so I can
> > base on this and do more work....)
>
> Personally I'd prefer to just use the errno return and ipp by reference
> calling convention for the newly added helper as well. But I'm ok with
> all variants, so maybe I should add my:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> here in case Darrick wants to pick this up.
>
> Looking at idmapped mounts series it would really help to get this in
> ASAP to avoid conflicts.
Ok, let me send out a quick next version to get rid of that inlined
comment mentioned earlier.
Thanks,
Gao Xiang
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 4/6] xfs: move xfs_dialloc_roll() into xfs_dialloc()
2020-12-08 12:19 [PATCH v4 0/6] xfs: some xfs_dialloc() cleanup Gao Xiang
` (2 preceding siblings ...)
2020-12-08 12:20 ` [PATCH v4 3/6] xfs: move on-disk inode allocation out of xfs_ialloc() Gao Xiang
@ 2020-12-08 12:20 ` Gao Xiang
2020-12-08 12:20 ` [PATCH v4 5/6] xfs: spilt xfs_dialloc() into 2 functions Gao Xiang
2020-12-08 12:20 ` [PATCH v4 6/6] xfs: kill ialloced in xfs_dialloc() Gao Xiang
5 siblings, 0 replies; 15+ messages in thread
From: Gao Xiang @ 2020-12-08 12:20 UTC (permalink / raw)
To: linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, Eric Sandeen,
Dave Chinner, Gao Xiang
From: Dave Chinner <dchinner@redhat.com>
Get rid of the confusing ialloc_context and failure handling around
xfs_dialloc() by moving xfs_dialloc_roll() into xfs_dialloc().
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 59 +++++++++++++-------------------------
fs/xfs/libxfs/xfs_ialloc.h | 21 +-------------
fs/xfs/xfs_inode.c | 38 ++----------------------
3 files changed, 24 insertions(+), 94 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 23e94d43acb2..074c2d83de77 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1682,7 +1682,7 @@ xfs_dialloc_ag(
return error;
}
-int
+static int
xfs_dialloc_roll(
struct xfs_trans **tpp,
struct xfs_buf *agibp)
@@ -1731,30 +1731,18 @@ xfs_dialloc_roll(
* Mode is used to tell whether the new inode will need space, and whether it
* is a directory.
*
- * This function is designed to be called twice if it has to do an allocation
- * to make more free inodes. On the first call, *IO_agbp should be set to NULL.
- * If an inode is available without having to performn an allocation, an inode
- * number is returned. In this case, *IO_agbp is set to NULL. If an allocation
- * needs to be done, xfs_dialloc returns the current AGI buffer in *IO_agbp.
- * The caller should then commit the current transaction, allocate a
- * new transaction, and call xfs_dialloc() again, passing in the previous value
- * of *IO_agbp. IO_agbp should be held across the transactions. Since the AGI
- * buffer is locked across the two calls, the second call is guaranteed to have
- * a free inode available.
- *
* Once we successfully pick an inode its number is returned and the on-disk
* data structures are updated. The inode itself is not read in, since doing so
* would break ordering constraints with xfs_reclaim.
*/
int
xfs_dialloc(
- struct xfs_trans *tp,
+ struct xfs_trans **tpp,
xfs_ino_t parent,
umode_t mode,
- struct xfs_buf **IO_agbp,
xfs_ino_t *inop)
{
- struct xfs_mount *mp = tp->t_mountp;
+ struct xfs_mount *mp = (*tpp)->t_mountp;
struct xfs_buf *agbp;
xfs_agnumber_t agno;
int error;
@@ -1765,21 +1753,11 @@ xfs_dialloc(
struct xfs_ino_geometry *igeo = M_IGEO(mp);
bool okalloc = true;
- if (*IO_agbp) {
- /*
- * If the caller passes in a pointer to the AGI buffer,
- * continue where we left off before. In this case, we
- * know that the allocation group has free inodes.
- */
- agbp = *IO_agbp;
- goto out_alloc;
- }
-
/*
* We do not have an agbp, so select an initial allocation
* group for inode allocation.
*/
- start_agno = xfs_ialloc_ag_select(tp, parent, mode);
+ start_agno = xfs_ialloc_ag_select(*tpp, parent, mode);
if (start_agno == NULLAGNUMBER) {
*inop = NULLFSINO;
return 0;
@@ -1814,7 +1792,7 @@ xfs_dialloc(
}
if (!pag->pagi_init) {
- error = xfs_ialloc_pagi_init(mp, tp, agno);
+ error = xfs_ialloc_pagi_init(mp, *tpp, agno);
if (error)
goto out_error;
}
@@ -1829,7 +1807,7 @@ xfs_dialloc(
* Then read in the AGI buffer and recheck with the AGI buffer
* lock held.
*/
- error = xfs_ialloc_read_agi(mp, tp, agno, &agbp);
+ error = xfs_ialloc_read_agi(mp, *tpp, agno, &agbp);
if (error)
goto out_error;
@@ -1842,9 +1820,9 @@ xfs_dialloc(
goto nextag_relse_buffer;
- error = xfs_ialloc_ag_alloc(tp, agbp, &ialloced);
+ error = xfs_ialloc_ag_alloc(*tpp, agbp, &ialloced);
if (error) {
- xfs_trans_brelse(tp, agbp);
+ xfs_trans_brelse(*tpp, agbp);
if (error != -ENOSPC)
goto out_error;
@@ -1856,21 +1834,25 @@ xfs_dialloc(
if (ialloced) {
/*
- * We successfully allocated some inodes, return
- * the current context to the caller so that it
- * can commit the current transaction and call
- * us again where we left off.
+ * We successfully allocated space for an inode cluster
+ * in this AG. Roll the transaction so that we can
+ * allocate one of the new inodes.
*/
ASSERT(pag->pagi_freecount > 0);
xfs_perag_put(pag);
- *IO_agbp = agbp;
+ error = xfs_dialloc_roll(tpp, agbp);
+ if (error) {
+ xfs_buf_relse(agbp);
+ return error;
+ }
+
*inop = NULLFSINO;
- return 0;
+ goto out_alloc;
}
nextag_relse_buffer:
- xfs_trans_brelse(tp, agbp);
+ xfs_trans_brelse(*tpp, agbp);
nextag:
xfs_perag_put(pag);
if (++agno == mp->m_sb.sb_agcount)
@@ -1882,8 +1864,7 @@ xfs_dialloc(
}
out_alloc:
- *IO_agbp = NULL;
- return xfs_dialloc_ag(tp, agbp, parent, inop);
+ return xfs_dialloc_ag(*tpp, agbp, parent, inop);
out_error:
xfs_perag_put(pag);
return error;
diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
index bd6e0db9e23c..13810ffe4af9 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -32,39 +32,20 @@ xfs_make_iptr(struct xfs_mount *mp, struct xfs_buf *b, int o)
return xfs_buf_offset(b, o << (mp)->m_sb.sb_inodelog);
}
-int
-xfs_dialloc_roll(
- struct xfs_trans **tpp,
- struct xfs_buf *agibp);
-
/*
* Allocate an inode on disk.
* Mode is used to tell whether the new inode will need space, and whether
* it is a directory.
*
- * To work within the constraint of one allocation per transaction,
- * xfs_dialloc() is designed to be called twice if it has to do an
- * allocation to make more free inodes. If an inode is
- * available without an allocation, agbp would be set to the current
- * agbp and alloc_done set to false.
- * If an allocation needed to be done, agbp would be set to the
- * inode header of the allocation group and alloc_done set to true.
- * The caller should then commit the current transaction and allocate a new
- * transaction. xfs_dialloc() should then be called again with
- * the agbp value returned from the previous call.
- *
* Once we successfully pick an inode its number is returned and the
* on-disk data structures are updated. The inode itself is not read
* in, since doing so would break ordering constraints with xfs_reclaim.
- *
- * *agbp should be set to NULL on the first call, *alloc_done set to FALSE.
*/
int /* error */
xfs_dialloc(
- struct xfs_trans *tp, /* transaction pointer */
+ struct xfs_trans **tpp, /* double pointer of transaction */
xfs_ino_t parent, /* parent inode (directory) */
umode_t mode, /* mode bits for new inode */
- struct xfs_buf **agbp, /* buf for a.g. inode header */
xfs_ino_t *inop); /* inode number allocated */
/*
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index ae6c83d46aaa..ec8433ea6985 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -907,7 +907,6 @@ xfs_dir_ialloc(
prid_t prid) /* project id */
{
xfs_inode_t *ip;
- xfs_buf_t *ialloc_context = NULL;
xfs_ino_t parent_ino = dp ? dp->i_ino : 0;
xfs_ino_t ino;
int error;
@@ -916,43 +915,12 @@ xfs_dir_ialloc(
/*
* Call the space management code to pick the on-disk inode to be
- * allocated and replenish the freelist. Since we can only do one
- * allocation per transaction without deadlocks, we will need to
- * commit the current transaction and start a new one.
- * If xfs_dialloc did an allocation to replenish the freelist, it
- * returns the bp containing the head of the freelist as
- * ialloc_context. We will hold a lock on it across the transaction
- * commit so that no other process can steal the inode(s) that we've
- * just allocated.
- */
- error = xfs_dialloc(*tpp, parent_ino, mode, &ialloc_context, &ino);
+ * allocated.
+ */
+ error = xfs_dialloc(tpp, parent_ino, mode, &ino);
if (error)
return ERR_PTR(error);
- /*
- * If the AGI buffer is non-NULL, then we were unable to get an
- * inode in one operation. We need to commit the current
- * transaction and call xfs_dialloc() again. It is guaranteed
- * to succeed the second time.
- */
- if (ialloc_context) {
- error = xfs_dialloc_roll(tpp, ialloc_context);
- if (error) {
- xfs_buf_relse(ialloc_context);
- return ERR_PTR(error);
- }
- /*
- * Call dialloc again. Since we've locked out all other
- * allocations in this allocation group, this call should
- * always succeed.
- */
- error = xfs_dialloc(*tpp, parent_ino, mode,
- &ialloc_context, &ino);
- if (error)
- return ERR_PTR(error);
- ASSERT(!ialloc_context);
- }
-
if (ino == NULLFSINO)
return ERR_PTR(-ENOSPC);
--
2.18.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v4 5/6] xfs: spilt xfs_dialloc() into 2 functions
2020-12-08 12:19 [PATCH v4 0/6] xfs: some xfs_dialloc() cleanup Gao Xiang
` (3 preceding siblings ...)
2020-12-08 12:20 ` [PATCH v4 4/6] xfs: move xfs_dialloc_roll() into xfs_dialloc() Gao Xiang
@ 2020-12-08 12:20 ` Gao Xiang
2020-12-09 7:54 ` Christoph Hellwig
2020-12-08 12:20 ` [PATCH v4 6/6] xfs: kill ialloced in xfs_dialloc() Gao Xiang
5 siblings, 1 reply; 15+ messages in thread
From: Gao Xiang @ 2020-12-08 12:20 UTC (permalink / raw)
To: linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, Eric Sandeen,
Dave Chinner, Gao Xiang
From: Dave Chinner <dchinner@redhat.com>
This patch explicitly separates free inode chunk allocation and
inode allocation into two individual high level operations.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 54 +++++++++++++++++---------------------
fs/xfs/libxfs/xfs_ialloc.h | 20 ++++++++++----
fs/xfs/xfs_inode.c | 18 ++++++++-----
3 files changed, 51 insertions(+), 41 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 074c2d83de77..dcb076d5c390 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1570,7 +1570,7 @@ xfs_dialloc_ag_update_inobt(
* The caller selected an AG for us, and made sure that free inodes are
* available.
*/
-STATIC int
+int
xfs_dialloc_ag(
struct xfs_trans *tp,
struct xfs_buf *agbp,
@@ -1726,21 +1726,22 @@ xfs_dialloc_roll(
}
/*
- * Allocate an inode on disk.
+ * Select and prepare an AG for inode allocation.
*
- * Mode is used to tell whether the new inode will need space, and whether it
- * is a directory.
+ * Mode is used to tell whether the new inode is a directory and hence where to
+ * locate it.
*
- * Once we successfully pick an inode its number is returned and the on-disk
- * data structures are updated. The inode itself is not read in, since doing so
- * would break ordering constraints with xfs_reclaim.
+ * This function will ensure that the selected AG has free inodes available to
+ * allocate from. The selected AGI will be returned locked to the caller, and it
+ * will allocate more free inodes if required. If no free inodes are found or
+ * can be allocated, no AGI will be returned.
*/
int
-xfs_dialloc(
+xfs_dialloc_select_ag(
struct xfs_trans **tpp,
xfs_ino_t parent,
umode_t mode,
- xfs_ino_t *inop)
+ struct xfs_buf **IO_agbp)
{
struct xfs_mount *mp = (*tpp)->t_mountp;
struct xfs_buf *agbp;
@@ -1753,15 +1754,15 @@ xfs_dialloc(
struct xfs_ino_geometry *igeo = M_IGEO(mp);
bool okalloc = true;
+ *IO_agbp = NULL;
+
/*
* We do not have an agbp, so select an initial allocation
* group for inode allocation.
*/
start_agno = xfs_ialloc_ag_select(*tpp, parent, mode);
- if (start_agno == NULLAGNUMBER) {
- *inop = NULLFSINO;
+ if (start_agno == NULLAGNUMBER)
return 0;
- }
/*
* If we have already hit the ceiling of inode blocks then clear
@@ -1794,7 +1795,7 @@ xfs_dialloc(
if (!pag->pagi_init) {
error = xfs_ialloc_pagi_init(mp, *tpp, agno);
if (error)
- goto out_error;
+ break;
}
/*
@@ -1809,11 +1810,11 @@ xfs_dialloc(
*/
error = xfs_ialloc_read_agi(mp, *tpp, agno, &agbp);
if (error)
- goto out_error;
+ break;
if (pag->pagi_freecount) {
xfs_perag_put(pag);
- goto out_alloc;
+ goto found_ag;
}
if (!okalloc)
@@ -1824,12 +1825,9 @@ xfs_dialloc(
if (error) {
xfs_trans_brelse(*tpp, agbp);
- if (error != -ENOSPC)
- goto out_error;
-
- xfs_perag_put(pag);
- *inop = NULLFSINO;
- return 0;
+ if (error == -ENOSPC)
+ error = 0;
+ break;
}
if (ialloced) {
@@ -1846,9 +1844,7 @@ xfs_dialloc(
xfs_buf_relse(agbp);
return error;
}
-
- *inop = NULLFSINO;
- goto out_alloc;
+ goto found_ag;
}
nextag_relse_buffer:
@@ -1857,17 +1853,15 @@ xfs_dialloc(
xfs_perag_put(pag);
if (++agno == mp->m_sb.sb_agcount)
agno = 0;
- if (agno == start_agno) {
- *inop = NULLFSINO;
+ if (agno == start_agno)
return noroom ? -ENOSPC : 0;
- }
}
-out_alloc:
- return xfs_dialloc_ag(*tpp, agbp, parent, inop);
-out_error:
xfs_perag_put(pag);
return error;
+found_ag:
+ *IO_agbp = agbp;
+ return 0;
}
/*
diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
index 13810ffe4af9..3511086a7ae1 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -37,16 +37,26 @@ xfs_make_iptr(struct xfs_mount *mp, struct xfs_buf *b, int o)
* Mode is used to tell whether the new inode will need space, and whether
* it is a directory.
*
- * Once we successfully pick an inode its number is returned and the
- * on-disk data structures are updated. The inode itself is not read
- * in, since doing so would break ordering constraints with xfs_reclaim.
+ * There are two phases to inode allocation: selecting an AG and ensuring
+ * that it contains free inodes, followed by allocating one of the free
+ * inodes. xfs_dialloc_select_ag() does the former and returns a locked AGI
+ * to the caller, ensuring that followup call to xfs_dialloc_ag() will
+ * have free inodes to allocate from. xfs_dialloc_ag() will return the inode
+ * number of the free inode we allocated.
*/
int /* error */
-xfs_dialloc(
+xfs_dialloc_select_ag(
struct xfs_trans **tpp, /* double pointer of transaction */
xfs_ino_t parent, /* parent inode (directory) */
umode_t mode, /* mode bits for new inode */
- xfs_ino_t *inop); /* inode number allocated */
+ struct xfs_buf **IO_agbp);
+
+int
+xfs_dialloc_ag(
+ struct xfs_trans *tp,
+ struct xfs_buf *agbp,
+ xfs_ino_t parent,
+ xfs_ino_t *inop);
/*
* Free disk inode. Carefully avoids touching the incore inode, all
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index ec8433ea6985..09e934b8332d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -906,10 +906,10 @@ xfs_dir_ialloc(
dev_t rdev,
prid_t prid) /* project id */
{
- xfs_inode_t *ip;
- xfs_ino_t parent_ino = dp ? dp->i_ino : 0;
- xfs_ino_t ino;
- int error;
+ struct xfs_buf *agibp;
+ xfs_ino_t parent_ino = dp ? dp->i_ino : 0;
+ xfs_ino_t ino;
+ int error;
ASSERT((*tpp)->t_flags & XFS_TRANS_PERM_LOG_RES);
@@ -917,13 +917,19 @@ xfs_dir_ialloc(
* Call the space management code to pick the on-disk inode to be
* allocated.
*/
- error = xfs_dialloc(tpp, parent_ino, mode, &ino);
+ error = xfs_dialloc_select_ag(tpp, parent_ino, mode, &agibp);
if (error)
return ERR_PTR(error);
- if (ino == NULLFSINO)
+ if (!agibp)
return ERR_PTR(-ENOSPC);
+ /* Allocate an inode from the selected AG */
+ error = xfs_dialloc_ag(*tpp, agibp, parent_ino, &ino);
+ if (error)
+ return ERR_PTR(error);
+ ASSERT(ino != NULLFSINO);
+
/* Initialise the newly allocated inode. */
return xfs_init_new_inode(*tpp, dp, ino, mode, nlink, rdev, prid);
}
--
2.18.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v4 6/6] xfs: kill ialloced in xfs_dialloc()
2020-12-08 12:19 [PATCH v4 0/6] xfs: some xfs_dialloc() cleanup Gao Xiang
` (4 preceding siblings ...)
2020-12-08 12:20 ` [PATCH v4 5/6] xfs: spilt xfs_dialloc() into 2 functions Gao Xiang
@ 2020-12-08 12:20 ` Gao Xiang
5 siblings, 0 replies; 15+ messages in thread
From: Gao Xiang @ 2020-12-08 12:20 UTC (permalink / raw)
To: linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, Eric Sandeen,
Gao Xiang
It's enough to just use return code, and get rid of an argument.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index dcb076d5c390..063a1a543890 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -607,13 +607,13 @@ xfs_inobt_insert_sprec(
/*
* Allocate new inodes in the allocation group specified by agbp.
- * Return 0 for success, else error code.
+ * Returns 0 if inodes were allocated in this AG; 1 if there was no space
+ * in this AG; or the usual negative error code.
*/
STATIC int
xfs_ialloc_ag_alloc(
struct xfs_trans *tp,
- struct xfs_buf *agbp,
- int *alloc)
+ struct xfs_buf *agbp)
{
struct xfs_agi *agi;
struct xfs_alloc_arg args;
@@ -795,10 +795,9 @@ xfs_ialloc_ag_alloc(
allocmask = (1 << (newlen / XFS_INODES_PER_HOLEMASK_BIT)) - 1;
}
- if (args.fsbno == NULLFSBLOCK) {
- *alloc = 0;
- return 0;
- }
+ if (args.fsbno == NULLFSBLOCK)
+ return 1;
+
ASSERT(args.len == args.minlen);
/*
@@ -903,7 +902,6 @@ xfs_ialloc_ag_alloc(
*/
xfs_trans_mod_sb(tp, XFS_TRANS_SB_ICOUNT, (long)newlen);
xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, (long)newlen);
- *alloc = 1;
return 0;
}
@@ -1747,7 +1745,6 @@ xfs_dialloc_select_ag(
struct xfs_buf *agbp;
xfs_agnumber_t agno;
int error;
- int ialloced;
bool noroom = false;
xfs_agnumber_t start_agno;
struct xfs_perag *pag;
@@ -1820,9 +1817,8 @@ xfs_dialloc_select_ag(
if (!okalloc)
goto nextag_relse_buffer;
-
- error = xfs_ialloc_ag_alloc(*tpp, agbp, &ialloced);
- if (error) {
+ error = xfs_ialloc_ag_alloc(*tpp, agbp);
+ if (error < 0) {
xfs_trans_brelse(*tpp, agbp);
if (error == -ENOSPC)
@@ -1830,7 +1826,7 @@ xfs_dialloc_select_ag(
break;
}
- if (ialloced) {
+ if (error == 0) {
/*
* We successfully allocated space for an inode cluster
* in this AG. Roll the transaction so that we can
--
2.18.4
^ permalink raw reply related [flat|nested] 15+ messages in thread