* [PATCH v2 1/6] xfs: convert noroom, okalloc in xfs_dialloc() to bool
2020-12-03 16:10 [PATCH v2 0/6] xfs: some xfs_dialloc() cleanup Gao Xiang
@ 2020-12-03 16:10 ` Gao Xiang
2020-12-03 20:32 ` Dave Chinner
2020-12-03 16:10 ` [PATCH v2 2/6] xfs: introduce xfs_dialloc_roll() Gao Xiang
` (4 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Gao Xiang @ 2020-12-03 16:10 UTC (permalink / raw)
To: linux-xfs; +Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, Gao Xiang
Boolean is preferred for such use.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.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] 16+ messages in thread* Re: [PATCH v2 1/6] xfs: convert noroom, okalloc in xfs_dialloc() to bool
2020-12-03 16:10 ` [PATCH v2 1/6] xfs: convert noroom, okalloc in xfs_dialloc() to bool Gao Xiang
@ 2020-12-03 20:32 ` Dave Chinner
0 siblings, 0 replies; 16+ messages in thread
From: Dave Chinner @ 2020-12-03 20:32 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-xfs, Darrick J. Wong, Christoph Hellwig
On Fri, Dec 04, 2020 at 12:10:23AM +0800, Gao Xiang wrote:
> Boolean is preferred for such use.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> fs/xfs/libxfs/xfs_ialloc.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
looks good,
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/6] xfs: introduce xfs_dialloc_roll()
2020-12-03 16:10 [PATCH v2 0/6] xfs: some xfs_dialloc() cleanup Gao Xiang
2020-12-03 16:10 ` [PATCH v2 1/6] xfs: convert noroom, okalloc in xfs_dialloc() to bool Gao Xiang
@ 2020-12-03 16:10 ` Gao Xiang
2020-12-03 19:20 ` Darrick J. Wong
2020-12-03 16:10 ` [PATCH v2 3/6] xfs: move on-disk inode allocation out of xfs_ialloc() Gao Xiang
` (3 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Gao Xiang @ 2020-12-03 16:10 UTC (permalink / raw)
To: linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, 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.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 45 ++++++++++++++++++++++++++++++++++++++
fs/xfs/libxfs/xfs_ialloc.h | 6 +++++
fs/xfs/xfs_inode.c | 39 +--------------------------------
3 files changed, 52 insertions(+), 38 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 45cf7e55f5ee..d5dc3167e2ff 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1682,6 +1682,51 @@ xfs_dialloc_ag(
return error;
}
+int
+xfs_dialloc_roll(
+ struct xfs_trans **tpp,
+ struct xfs_buf *agibp)
+{
+ struct xfs_trans *tp = *tpp;
+ void *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) {
+ xfs_buf_relse(agibp);
+ 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..a145e2a72530 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -32,6 +32,12 @@ xfs_make_iptr(struct xfs_mount *mp, struct xfs_buf *b, int o)
return xfs_buf_offset(b, o << (mp)->m_sb.sb_inodelog);
}
+/* XXX: will be removed in the following patch */
+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..4ebfb1a18f0f 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,11 @@ 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] 16+ messages in thread* Re: [PATCH v2 2/6] xfs: introduce xfs_dialloc_roll()
2020-12-03 16:10 ` [PATCH v2 2/6] xfs: introduce xfs_dialloc_roll() Gao Xiang
@ 2020-12-03 19:20 ` Darrick J. Wong
0 siblings, 0 replies; 16+ messages in thread
From: Darrick J. Wong @ 2020-12-03 19:20 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-xfs, Dave Chinner, Christoph Hellwig, Dave Chinner
On Fri, Dec 04, 2020 at 12:10:24AM +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.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> fs/xfs/libxfs/xfs_ialloc.c | 45 ++++++++++++++++++++++++++++++++++++++
> fs/xfs/libxfs/xfs_ialloc.h | 6 +++++
> fs/xfs/xfs_inode.c | 39 +--------------------------------
> 3 files changed, 52 insertions(+), 38 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 45cf7e55f5ee..d5dc3167e2ff 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -1682,6 +1682,51 @@ xfs_dialloc_ag(
> return error;
> }
>
> +int
> +xfs_dialloc_roll(
> + struct xfs_trans **tpp,
> + struct xfs_buf *agibp)
> +{
> + struct xfs_trans *tp = *tpp;
> + void *dqinfo = NULL;
struct xfs_dquot_acct instead of void?
> + 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;
Assuming Eric's ok with adding a dummy t_dqinfo to struct xfs_trans in
userspace, this seems fine to me.
--D
> + 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) {
> + xfs_buf_relse(agibp);
> + 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..a145e2a72530 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.h
> +++ b/fs/xfs/libxfs/xfs_ialloc.h
> @@ -32,6 +32,12 @@ xfs_make_iptr(struct xfs_mount *mp, struct xfs_buf *b, int o)
> return xfs_buf_offset(b, o << (mp)->m_sb.sb_inodelog);
> }
>
> +/* XXX: will be removed in the following patch */
> +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..4ebfb1a18f0f 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,11 @@ 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] 16+ messages in thread
* [PATCH v2 3/6] xfs: move on-disk inode allocation out of xfs_ialloc()
2020-12-03 16:10 [PATCH v2 0/6] xfs: some xfs_dialloc() cleanup Gao Xiang
2020-12-03 16:10 ` [PATCH v2 1/6] xfs: convert noroom, okalloc in xfs_dialloc() to bool Gao Xiang
2020-12-03 16:10 ` [PATCH v2 2/6] xfs: introduce xfs_dialloc_roll() Gao Xiang
@ 2020-12-03 16:10 ` Gao Xiang
2020-12-03 19:21 ` Darrick J. Wong
2020-12-03 20:31 ` Dave Chinner
2020-12-03 16:10 ` [PATCH v2 4/6] xfs: move xfs_dialloc_roll() into xfs_dialloc() Gao Xiang
` (2 subsequent siblings)
5 siblings, 2 replies; 16+ messages in thread
From: Gao Xiang @ 2020-12-03 16:10 UTC (permalink / raw)
To: linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, Dave Chinner,
Gao Xiang
From: Dave Chinner <dchinner@redhat.com>
So xfs_ialloc() will only address in-core inode allocation then.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
fs/xfs/xfs_inode.c | 200 +++++++++++++++------------------------------
1 file changed, 65 insertions(+), 135 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 4ebfb1a18f0f..34eca1624397 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
+static struct xfs_inode *
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)
+ 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,20 +881,19 @@ 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
xfs_dir_ialloc(
@@ -954,83 +908,59 @@ xfs_dir_ialloc(
xfs_inode_t **ipp) /* pointer to inode; it will be
locked. */
{
- 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 pino = 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);
+ *ipp = NULL;
/*
- * 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, pino, mode, ialloc_context, &ino);
+ if (error)
+ return 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_ialloc() then. It is guaranteed
* to succeed the second time.
*/
if (ialloc_context) {
- code = xfs_dialloc_roll(&tp, ialloc_context);
- if (code) {
- *ipp = NULL;
- return code;
- }
-
- /*
- * Call ialloc 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);
-
+ error = xfs_dialloc_roll(tpp, ialloc_context);
+ if (error)
+ return error;
/*
- * If we get an error at this point, return to the caller
- * so that the current transaction can be aborted.
+ * Call dialloc again. Since we've locked out all other
+ * allocations in this allocation group, this call should
+ * always succeed.
*/
- if (code) {
- *tpp = tp;
- *ipp = NULL;
- return code;
- }
- ASSERT(!ialloc_context && ip);
-
+ error = xfs_dialloc(*tpp, pino, mode, ialloc_context, &ino);
+ if (error)
+ return error;
+ ASSERT(!ialloc_context);
}
- *ipp = ip;
- *tpp = tp;
+ if (ino == NULLFSINO)
+ return -ENOSPC;
+ /* Initialise the newly allocated inode. */
+ ip = xfs_ialloc(*tpp, dp, ino, mode, nlink, rdev, prid);
+ if (IS_ERR(ip))
+ return PTR_ERR(ip);
+ *ipp = ip;
return 0;
}
--
2.18.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 3/6] xfs: move on-disk inode allocation out of xfs_ialloc()
2020-12-03 16:10 ` [PATCH v2 3/6] xfs: move on-disk inode allocation out of xfs_ialloc() Gao Xiang
@ 2020-12-03 19:21 ` Darrick J. Wong
2020-12-03 20:31 ` Dave Chinner
1 sibling, 0 replies; 16+ messages in thread
From: Darrick J. Wong @ 2020-12-03 19:21 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-xfs, Dave Chinner, Christoph Hellwig, Dave Chinner
On Fri, Dec 04, 2020 at 12:10:25AM +0800, Gao Xiang wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> So xfs_ialloc() will only address in-core inode allocation then.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
Looks good to me, I never liked ialloc_context anyway.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_inode.c | 200 +++++++++++++++------------------------------
> 1 file changed, 65 insertions(+), 135 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 4ebfb1a18f0f..34eca1624397 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
> +static struct xfs_inode *
> 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)
> + 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,20 +881,19 @@ 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
> xfs_dir_ialloc(
> @@ -954,83 +908,59 @@ xfs_dir_ialloc(
> xfs_inode_t **ipp) /* pointer to inode; it will be
> locked. */
> {
> - 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 pino = 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);
> + *ipp = NULL;
>
> /*
> - * 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, pino, mode, ialloc_context, &ino);
> + if (error)
> + return 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_ialloc() then. It is guaranteed
> * to succeed the second time.
> */
> if (ialloc_context) {
> - code = xfs_dialloc_roll(&tp, ialloc_context);
> - if (code) {
> - *ipp = NULL;
> - return code;
> - }
> -
> - /*
> - * Call ialloc 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);
> -
> + error = xfs_dialloc_roll(tpp, ialloc_context);
> + if (error)
> + return error;
> /*
> - * If we get an error at this point, return to the caller
> - * so that the current transaction can be aborted.
> + * Call dialloc again. Since we've locked out all other
> + * allocations in this allocation group, this call should
> + * always succeed.
> */
> - if (code) {
> - *tpp = tp;
> - *ipp = NULL;
> - return code;
> - }
> - ASSERT(!ialloc_context && ip);
> -
> + error = xfs_dialloc(*tpp, pino, mode, ialloc_context, &ino);
> + if (error)
> + return error;
> + ASSERT(!ialloc_context);
> }
>
> - *ipp = ip;
> - *tpp = tp;
> + if (ino == NULLFSINO)
> + return -ENOSPC;
>
> + /* Initialise the newly allocated inode. */
> + ip = xfs_ialloc(*tpp, dp, ino, mode, nlink, rdev, prid);
> + if (IS_ERR(ip))
> + return PTR_ERR(ip);
> + *ipp = ip;
> return 0;
> }
>
> --
> 2.18.4
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 3/6] xfs: move on-disk inode allocation out of xfs_ialloc()
2020-12-03 16:10 ` [PATCH v2 3/6] xfs: move on-disk inode allocation out of xfs_ialloc() Gao Xiang
2020-12-03 19:21 ` Darrick J. Wong
@ 2020-12-03 20:31 ` Dave Chinner
2020-12-04 0:58 ` Gao Xiang
1 sibling, 1 reply; 16+ messages in thread
From: Dave Chinner @ 2020-12-03 20:31 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-xfs, Darrick J. Wong, Christoph Hellwig, Dave Chinner
On Fri, Dec 04, 2020 at 12:10:25AM +0800, Gao Xiang wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> So xfs_ialloc() will only address in-core inode allocation then.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> fs/xfs/xfs_inode.c | 200 +++++++++++++++------------------------------
> 1 file changed, 65 insertions(+), 135 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 4ebfb1a18f0f..34eca1624397 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
> +static struct xfs_inode *
> xfs_ialloc(
Can we rename this xfs_dir_ialloc_init()?
That way we keep everything in xfs_inode.c under the same namespace
(xfs_dir_ialloc_*) and don't confuse it with functions in the
xfs_ialloc_* namespace in fs/xfs/libxfs/xfs_ialloc*.c...
Otherwise looks good.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/6] xfs: move on-disk inode allocation out of xfs_ialloc()
2020-12-03 20:31 ` Dave Chinner
@ 2020-12-04 0:58 ` Gao Xiang
0 siblings, 0 replies; 16+ messages in thread
From: Gao Xiang @ 2020-12-04 0:58 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, Darrick J. Wong, Christoph Hellwig, Dave Chinner
Hi Dave,
On Fri, Dec 04, 2020 at 07:31:30AM +1100, Dave Chinner wrote:
> On Fri, Dec 04, 2020 at 12:10:25AM +0800, Gao Xiang wrote:
...
> > - * 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
> > +static struct xfs_inode *
> > xfs_ialloc(
>
> Can we rename this xfs_dir_ialloc_init()?
>
> That way we keep everything in xfs_inode.c under the same namespace
> (xfs_dir_ialloc_*) and don't confuse it with functions in the
> xfs_ialloc_* namespace in fs/xfs/libxfs/xfs_ialloc*.c...
Ok, thanks for the suggestion! Let me revise it in the next version.
Thanks,
Gao Xiang
>
> Otherwise looks good.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 4/6] xfs: move xfs_dialloc_roll() into xfs_dialloc()
2020-12-03 16:10 [PATCH v2 0/6] xfs: some xfs_dialloc() cleanup Gao Xiang
` (2 preceding siblings ...)
2020-12-03 16:10 ` [PATCH v2 3/6] xfs: move on-disk inode allocation out of xfs_ialloc() Gao Xiang
@ 2020-12-03 16:10 ` Gao Xiang
2020-12-03 19:25 ` Darrick J. Wong
2020-12-03 16:10 ` [PATCH v2 5/6] xfs: spilt xfs_dialloc() into 2 functions Gao Xiang
2020-12-03 16:10 ` [PATCH v2 6/6] xfs: kill ialloced in xfs_dialloc() Gao Xiang
5 siblings, 1 reply; 16+ messages in thread
From: Gao Xiang @ 2020-12-03 16:10 UTC (permalink / raw)
To: linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, 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().
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 57 ++++++++++++--------------------------
fs/xfs/libxfs/xfs_ialloc.h | 22 +--------------
fs/xfs/xfs_inode.c | 24 +---------------
3 files changed, 20 insertions(+), 83 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index d5dc3167e2ff..d2d7378abf49 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)
@@ -1733,30 +1733,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;
@@ -1767,21 +1755,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;
@@ -1816,7 +1794,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;
}
@@ -1831,7 +1809,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;
@@ -1844,9 +1822,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;
@@ -1858,21 +1836,23 @@ 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 some inodes, roll the
+ * transaction so they can allocate one of the free
+ * inodes we just prepared for them.
*/
ASSERT(pag->pagi_freecount > 0);
xfs_perag_put(pag);
- *IO_agbp = agbp;
+ error = xfs_dialloc_roll(tpp, agbp);
+ if (error)
+ 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)
@@ -1884,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 a145e2a72530..13810ffe4af9 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -32,40 +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);
}
-/* XXX: will be removed in the following patch */
-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 34eca1624397..c039fc56b396 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -909,7 +909,6 @@ xfs_dir_ialloc(
locked. */
{
xfs_inode_t *ip;
- xfs_buf_t *ialloc_context = NULL;
xfs_ino_t pino = dp ? dp->i_ino : 0;
xfs_ino_t ino;
int error;
@@ -928,31 +927,10 @@ xfs_dir_ialloc(
* commit so that no other process can steal the inode(s) that we've
* just allocated.
*/
- error = xfs_dialloc(*tpp, pino, mode, ialloc_context, &ino);
+ error = xfs_dialloc(tpp, pino, mode, &ino);
if (error)
return 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() then. It is guaranteed
- * to succeed the second time.
- */
- if (ialloc_context) {
- error = xfs_dialloc_roll(tpp, ialloc_context);
- if (error)
- return 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, pino, mode, ialloc_context, &ino);
- if (error)
- return error;
- ASSERT(!ialloc_context);
- }
-
if (ino == NULLFSINO)
return -ENOSPC;
--
2.18.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 4/6] xfs: move xfs_dialloc_roll() into xfs_dialloc()
2020-12-03 16:10 ` [PATCH v2 4/6] xfs: move xfs_dialloc_roll() into xfs_dialloc() Gao Xiang
@ 2020-12-03 19:25 ` Darrick J. Wong
0 siblings, 0 replies; 16+ messages in thread
From: Darrick J. Wong @ 2020-12-03 19:25 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-xfs, Dave Chinner, Christoph Hellwig, Dave Chinner
On Fri, Dec 04, 2020 at 12:10:26AM +0800, Gao Xiang wrote:
> 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().
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
Heh. I sent my comments about to patch 4 as a reply to patch 3. :(
Well, at least the critical part is the same between both:
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_ialloc.c | 57 ++++++++++++--------------------------
> fs/xfs/libxfs/xfs_ialloc.h | 22 +--------------
> fs/xfs/xfs_inode.c | 24 +---------------
> 3 files changed, 20 insertions(+), 83 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index d5dc3167e2ff..d2d7378abf49 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)
> @@ -1733,30 +1733,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;
> @@ -1767,21 +1755,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;
> @@ -1816,7 +1794,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;
> }
> @@ -1831,7 +1809,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;
>
> @@ -1844,9 +1822,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;
> @@ -1858,21 +1836,23 @@ 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 some inodes, roll the
> + * transaction so they can allocate one of the free
> + * inodes we just prepared for them.
> */
> ASSERT(pag->pagi_freecount > 0);
> xfs_perag_put(pag);
>
> - *IO_agbp = agbp;
> + error = xfs_dialloc_roll(tpp, agbp);
> + if (error)
> + 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)
> @@ -1884,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 a145e2a72530..13810ffe4af9 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.h
> +++ b/fs/xfs/libxfs/xfs_ialloc.h
> @@ -32,40 +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);
> }
>
> -/* XXX: will be removed in the following patch */
> -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 34eca1624397..c039fc56b396 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -909,7 +909,6 @@ xfs_dir_ialloc(
> locked. */
> {
> xfs_inode_t *ip;
> - xfs_buf_t *ialloc_context = NULL;
> xfs_ino_t pino = dp ? dp->i_ino : 0;
> xfs_ino_t ino;
> int error;
> @@ -928,31 +927,10 @@ xfs_dir_ialloc(
> * commit so that no other process can steal the inode(s) that we've
> * just allocated.
> */
> - error = xfs_dialloc(*tpp, pino, mode, ialloc_context, &ino);
> + error = xfs_dialloc(tpp, pino, mode, &ino);
> if (error)
> return 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() then. It is guaranteed
> - * to succeed the second time.
> - */
> - if (ialloc_context) {
> - error = xfs_dialloc_roll(tpp, ialloc_context);
> - if (error)
> - return 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, pino, mode, ialloc_context, &ino);
> - if (error)
> - return error;
> - ASSERT(!ialloc_context);
> - }
> -
> if (ino == NULLFSINO)
> return -ENOSPC;
>
> --
> 2.18.4
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 5/6] xfs: spilt xfs_dialloc() into 2 functions
2020-12-03 16:10 [PATCH v2 0/6] xfs: some xfs_dialloc() cleanup Gao Xiang
` (3 preceding siblings ...)
2020-12-03 16:10 ` [PATCH v2 4/6] xfs: move xfs_dialloc_roll() into xfs_dialloc() Gao Xiang
@ 2020-12-03 16:10 ` Gao Xiang
2020-12-03 19:29 ` Darrick J. Wong
2020-12-03 16:10 ` [PATCH v2 6/6] xfs: kill ialloced in xfs_dialloc() Gao Xiang
5 siblings, 1 reply; 16+ messages in thread
From: Gao Xiang @ 2020-12-03 16:10 UTC (permalink / raw)
To: linux-xfs
Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, 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.
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 | 20 +++++++++----
fs/xfs/xfs_inode.c | 19 ++++++++----
3 files changed, 55 insertions(+), 43 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index d2d7378abf49..597629353d4d 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,
@@ -1728,21 +1728,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;
@@ -1755,15 +1756,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
@@ -1796,7 +1797,7 @@ xfs_dialloc(
if (!pag->pagi_init) {
error = xfs_ialloc_pagi_init(mp, *tpp, agno);
if (error)
- goto out_error;
+ break;
}
/*
@@ -1811,11 +1812,12 @@ 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;
+ *IO_agbp = agbp;
+ return 0;
}
if (!okalloc)
@@ -1826,19 +1828,17 @@ 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) {
/*
- * We successfully allocated some inodes, roll the
- * transaction so they can allocate one of the free
- * inodes we just prepared for them.
+ * We successfully allocated some inodes, so roll the
+ * transaction and return the locked AGI buffer to the
+ * caller so they can allocate one of the free inodes we
+ * just prepared for them.
*/
ASSERT(pag->pagi_freecount > 0);
xfs_perag_put(pag);
@@ -1847,8 +1847,8 @@ xfs_dialloc(
if (error)
return error;
- *inop = NULLFSINO;
- goto out_alloc;
+ *IO_agbp = agbp;
+ return 0;
}
nextag_relse_buffer:
@@ -1857,15 +1857,10 @@ 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;
}
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 c039fc56b396..d0ae0d6ee892 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -908,10 +908,11 @@ xfs_dir_ialloc(
xfs_inode_t **ipp) /* pointer to inode; it will be
locked. */
{
- xfs_inode_t *ip;
- xfs_ino_t pino = dp ? dp->i_ino : 0;
- xfs_ino_t ino;
- int error;
+ struct xfs_buf *agibp;
+ struct xfs_inode *ip;
+ xfs_ino_t pino = dp ? dp->i_ino : 0;
+ xfs_ino_t ino;
+ int error;
ASSERT((*tpp)->t_flags & XFS_TRANS_PERM_LOG_RES);
*ipp = NULL;
@@ -927,13 +928,19 @@ xfs_dir_ialloc(
* commit so that no other process can steal the inode(s) that we've
* just allocated.
*/
- error = xfs_dialloc(tpp, pino, mode, &ino);
+ error = xfs_dialloc_select_ag(tpp, pino, mode, &agibp);
if (error)
return error;
- if (ino == NULLFSINO)
+ if (!agibp)
return -ENOSPC;
+ /* Allocate an inode from the selected AG */
+ error = xfs_dialloc_ag(*tpp, agibp, pino, &ino);
+ if (error)
+ return error;
+ ASSERT(ino != NULLFSINO);
+
/* Initialise the newly allocated inode. */
ip = xfs_ialloc(*tpp, dp, ino, mode, nlink, rdev, prid);
if (IS_ERR(ip))
--
2.18.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 5/6] xfs: spilt xfs_dialloc() into 2 functions
2020-12-03 16:10 ` [PATCH v2 5/6] xfs: spilt xfs_dialloc() into 2 functions Gao Xiang
@ 2020-12-03 19:29 ` Darrick J. Wong
0 siblings, 0 replies; 16+ messages in thread
From: Darrick J. Wong @ 2020-12-03 19:29 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-xfs, Dave Chinner, Christoph Hellwig, Dave Chinner
On Fri, Dec 04, 2020 at 12:10:27AM +0800, Gao Xiang wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> This patch explicitly separates free inode chunk allocation and
> inode allocation into two individual high level operations.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
FWIW I thought about doing some similar things with the xfs_dir_ialloc
in the metadata directory tree patchset, so this makes sense to me (and
will probably simplify things) so:
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_ialloc.c | 59 +++++++++++++++++---------------------
> fs/xfs/libxfs/xfs_ialloc.h | 20 +++++++++----
> fs/xfs/xfs_inode.c | 19 ++++++++----
> 3 files changed, 55 insertions(+), 43 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index d2d7378abf49..597629353d4d 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,
> @@ -1728,21 +1728,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;
> @@ -1755,15 +1756,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
> @@ -1796,7 +1797,7 @@ xfs_dialloc(
> if (!pag->pagi_init) {
> error = xfs_ialloc_pagi_init(mp, *tpp, agno);
> if (error)
> - goto out_error;
> + break;
> }
>
> /*
> @@ -1811,11 +1812,12 @@ 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;
> + *IO_agbp = agbp;
> + return 0;
> }
>
> if (!okalloc)
> @@ -1826,19 +1828,17 @@ 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) {
> /*
> - * We successfully allocated some inodes, roll the
> - * transaction so they can allocate one of the free
> - * inodes we just prepared for them.
> + * We successfully allocated some inodes, so roll the
> + * transaction and return the locked AGI buffer to the
> + * caller so they can allocate one of the free inodes we
> + * just prepared for them.
> */
> ASSERT(pag->pagi_freecount > 0);
> xfs_perag_put(pag);
> @@ -1847,8 +1847,8 @@ xfs_dialloc(
> if (error)
> return error;
>
> - *inop = NULLFSINO;
> - goto out_alloc;
> + *IO_agbp = agbp;
> + return 0;
> }
>
> nextag_relse_buffer:
> @@ -1857,15 +1857,10 @@ 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;
> }
> 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 c039fc56b396..d0ae0d6ee892 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -908,10 +908,11 @@ xfs_dir_ialloc(
> xfs_inode_t **ipp) /* pointer to inode; it will be
> locked. */
> {
> - xfs_inode_t *ip;
> - xfs_ino_t pino = dp ? dp->i_ino : 0;
> - xfs_ino_t ino;
> - int error;
> + struct xfs_buf *agibp;
> + struct xfs_inode *ip;
> + xfs_ino_t pino = dp ? dp->i_ino : 0;
> + xfs_ino_t ino;
> + int error;
>
> ASSERT((*tpp)->t_flags & XFS_TRANS_PERM_LOG_RES);
> *ipp = NULL;
> @@ -927,13 +928,19 @@ xfs_dir_ialloc(
> * commit so that no other process can steal the inode(s) that we've
> * just allocated.
> */
> - error = xfs_dialloc(tpp, pino, mode, &ino);
> + error = xfs_dialloc_select_ag(tpp, pino, mode, &agibp);
> if (error)
> return error;
>
> - if (ino == NULLFSINO)
> + if (!agibp)
> return -ENOSPC;
>
> + /* Allocate an inode from the selected AG */
> + error = xfs_dialloc_ag(*tpp, agibp, pino, &ino);
> + if (error)
> + return error;
> + ASSERT(ino != NULLFSINO);
> +
> /* Initialise the newly allocated inode. */
> ip = xfs_ialloc(*tpp, dp, ino, mode, nlink, rdev, prid);
> if (IS_ERR(ip))
> --
> 2.18.4
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 6/6] xfs: kill ialloced in xfs_dialloc()
2020-12-03 16:10 [PATCH v2 0/6] xfs: some xfs_dialloc() cleanup Gao Xiang
` (4 preceding siblings ...)
2020-12-03 16:10 ` [PATCH v2 5/6] xfs: spilt xfs_dialloc() into 2 functions Gao Xiang
@ 2020-12-03 16:10 ` Gao Xiang
2020-12-03 19:08 ` Darrick J. Wong
2020-12-03 20:33 ` Dave Chinner
5 siblings, 2 replies; 16+ messages in thread
From: Gao Xiang @ 2020-12-03 16:10 UTC (permalink / raw)
To: linux-xfs; +Cc: Darrick J. Wong, Dave Chinner, Christoph Hellwig, Gao Xiang
It's enough to just use return code, and get rid of an argument.
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 597629353d4d..ec63afb59156 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;
}
@@ -1749,7 +1747,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;
@@ -1823,17 +1820,14 @@ 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)
error = 0;
break;
- }
-
- if (ialloced) {
+ } else if (error == 0) {
/*
* We successfully allocated some inodes, so roll the
* transaction and return the locked AGI buffer to the
--
2.18.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 6/6] xfs: kill ialloced in xfs_dialloc()
2020-12-03 16:10 ` [PATCH v2 6/6] xfs: kill ialloced in xfs_dialloc() Gao Xiang
@ 2020-12-03 19:08 ` Darrick J. Wong
2020-12-03 20:33 ` Dave Chinner
1 sibling, 0 replies; 16+ messages in thread
From: Darrick J. Wong @ 2020-12-03 19:08 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-xfs, Dave Chinner, Christoph Hellwig
On Fri, Dec 04, 2020 at 12:10:28AM +0800, Gao Xiang wrote:
> It's enough to just use return code, and get rid of an argument.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
LGTM,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_ialloc.c | 24 +++++++++---------------
> 1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 597629353d4d..ec63afb59156 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;
> }
>
> @@ -1749,7 +1747,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;
> @@ -1823,17 +1820,14 @@ 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)
> error = 0;
> break;
> - }
> -
> - if (ialloced) {
> + } else if (error == 0) {
> /*
> * We successfully allocated some inodes, so roll the
> * transaction and return the locked AGI buffer to the
> --
> 2.18.4
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 6/6] xfs: kill ialloced in xfs_dialloc()
2020-12-03 16:10 ` [PATCH v2 6/6] xfs: kill ialloced in xfs_dialloc() Gao Xiang
2020-12-03 19:08 ` Darrick J. Wong
@ 2020-12-03 20:33 ` Dave Chinner
1 sibling, 0 replies; 16+ messages in thread
From: Dave Chinner @ 2020-12-03 20:33 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-xfs, Darrick J. Wong, Christoph Hellwig
On Fri, Dec 04, 2020 at 12:10:28AM +0800, Gao Xiang wrote:
> It's enough to just use return code, and get rid of an argument.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> fs/xfs/libxfs/xfs_ialloc.c | 24 +++++++++---------------
> 1 file changed, 9 insertions(+), 15 deletions(-)
Look fine.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 16+ messages in thread