[PATCH 0/2] xfs: quotaoff shutdown fixes

[PATCH 0/2] xfs: quotaoff shutdown fixes

[Brian Foster]

Hi all,

This fixes a couple issues I noticed with quotaoff when working on the
auto relog stuff. Patch 1 creates a helper and patch 2 uses it to
address a shutdown hang and memory leak. Thoughts, reviews, flames
appreciated.

Brian

Brian Foster (2):
  xfs: factor out quotaoff intent AIL removal and memory free
  xfs: fix unmount hang and memory leak on shutdown during quotaoff

 fs/xfs/xfs_dquot_item.c  | 44 ++++++++++++++++++++++++++++++++--------
 fs/xfs/xfs_dquot_item.h  |  1 +
 fs/xfs/xfs_qm_syscalls.c | 13 ++++++------
 3 files changed, 43 insertions(+), 15 deletions(-)

-- 
2.21.1

[PATCH 1/2] xfs: factor out quotaoff intent AIL removal and memory free

[Brian Foster]

AIL removal of the quotaoff start intent and free of both intents is
hardcoded to the ->iop_committed() handler of the end intent. Factor
out the start intent handling code so it can be used in a future
patch to properly handle quotaoff errors. Use xfs_trans_ail_remove()
instead of the _delete() variant to acquire the AIL lock and also
handle cases where an intent might not reside in the AIL at the
time of a failure.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_dquot_item.c | 29 ++++++++++++++++++++---------
 fs/xfs/xfs_dquot_item.h |  1 +
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index d60647d7197b..2b816e9b4465 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -307,18 +307,10 @@ xfs_qm_qoffend_logitem_committed(
 {
 	struct xfs_qoff_logitem	*qfe = QOFF_ITEM(lip);
 	struct xfs_qoff_logitem	*qfs = qfe->qql_start_lip;
-	struct xfs_ail		*ailp = qfs->qql_item.li_ailp;
 
-	/*
-	 * Delete the qoff-start logitem from the AIL.
-	 * xfs_trans_ail_delete() drops the AIL lock.
-	 */
-	spin_lock(&ailp->ail_lock);
-	xfs_trans_ail_delete(ailp, &qfs->qql_item, SHUTDOWN_LOG_IO_ERROR);
+	xfs_qm_qoff_logitem_relse(qfs);
 
-	kmem_free(qfs->qql_item.li_lv_shadow);
 	kmem_free(lip->li_lv_shadow);
-	kmem_free(qfs);
 	kmem_free(qfe);
 	return (xfs_lsn_t)-1;
 }
@@ -336,6 +328,25 @@ static const struct xfs_item_ops xfs_qm_qoff_logitem_ops = {
 	.iop_push	= xfs_qm_qoff_logitem_push,
 };
 
+/*
+ * Delete the quotaoff intent from the AIL and free it. On success,
+ * this should only be called for the start item. It can be used for
+ * either on shutdown or abort.
+ */
+void
+xfs_qm_qoff_logitem_relse(
+	struct xfs_qoff_logitem	*qoff)
+{
+	struct xfs_log_item	*lip = &qoff->qql_item;
+
+	ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags) ||
+	       test_bit(XFS_LI_ABORTED, &lip->li_flags) ||
+	       XFS_FORCED_SHUTDOWN(lip->li_mountp));
+	xfs_trans_ail_remove(lip, SHUTDOWN_LOG_IO_ERROR);
+	kmem_free(lip->li_lv_shadow);
+	kmem_free(qoff);
+}
+
 /*
  * Allocate and initialize an quotaoff item of the correct quota type(s).
  */
diff --git a/fs/xfs/xfs_dquot_item.h b/fs/xfs/xfs_dquot_item.h
index 3bb19e556ade..2b86a43d7ce2 100644
--- a/fs/xfs/xfs_dquot_item.h
+++ b/fs/xfs/xfs_dquot_item.h
@@ -28,6 +28,7 @@ void xfs_qm_dquot_logitem_init(struct xfs_dquot *dqp);
 struct xfs_qoff_logitem	*xfs_qm_qoff_logitem_init(struct xfs_mount *mp,
 		struct xfs_qoff_logitem *start,
 		uint flags);
+void xfs_qm_qoff_logitem_relse(struct xfs_qoff_logitem *);
 struct xfs_qoff_logitem	*xfs_trans_get_qoff_item(struct xfs_trans *tp,
 		struct xfs_qoff_logitem *startqoff,
 		uint flags);
-- 
2.21.1

[PATCH 2/2] xfs: fix unmount hang and memory leak on shutdown during quotaoff

[Brian Foster]

AIL removal of the quotaoff start intent and free of both quotaoff
intents is currently limited to the ->iop_committed() handler of the
end intent. This executes when the end intent is committed to the
on-disk log and marks the completion of the operation. The problem
with this is it assumes the success of the operation. If a shutdown
or other error occurs during the quotaoff, it's possible for the
quotaoff task to exit without removing the start intent from the
AIL. This results in an unmount hang as the AIL cannot be emptied.
Further, no other codepath frees the intents and so this is also a
memory leak vector.

First, update the high level quotaoff error path to directly remove
and free the quotaoff start intent if it still exists in the AIL at
the time of the error. Next, update both of the start and end
quotaoff intents with an ->iop_release() callback to properly handle
transaction abort.

This means that If the quotaoff start transaction aborts, it frees
the start intent in the transaction commit path. If the filesystem
shuts down before the end transaction allocates, the quotaoff
sequence removes and frees the start intent. If the end transaction
aborts, it removes the start intent and frees both. This ensures
that a shutdown does not result in a hung unmount and that memory is
not leaked regardless of when a quotaoff error occurs.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_dquot_item.c  | 15 +++++++++++++++
 fs/xfs/xfs_qm_syscalls.c | 13 +++++++------
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
index 2b816e9b4465..cf65e2e43c6e 100644
--- a/fs/xfs/xfs_dquot_item.c
+++ b/fs/xfs/xfs_dquot_item.c
@@ -315,17 +315,32 @@ xfs_qm_qoffend_logitem_committed(
 	return (xfs_lsn_t)-1;
 }
 
+STATIC void
+xfs_qm_qoff_logitem_release(
+	struct xfs_log_item	*lip)
+{
+	struct xfs_qoff_logitem	*qoff = QOFF_ITEM(lip);
+
+	if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
+		if (qoff->qql_start_lip)
+			xfs_qm_qoff_logitem_relse(qoff->qql_start_lip);
+		xfs_qm_qoff_logitem_relse(qoff);
+	}
+}
+
 static const struct xfs_item_ops xfs_qm_qoffend_logitem_ops = {
 	.iop_size	= xfs_qm_qoff_logitem_size,
 	.iop_format	= xfs_qm_qoff_logitem_format,
 	.iop_committed	= xfs_qm_qoffend_logitem_committed,
 	.iop_push	= xfs_qm_qoff_logitem_push,
+	.iop_release	= xfs_qm_qoff_logitem_release,
 };
 
 static const struct xfs_item_ops xfs_qm_qoff_logitem_ops = {
 	.iop_size	= xfs_qm_qoff_logitem_size,
 	.iop_format	= xfs_qm_qoff_logitem_format,
 	.iop_push	= xfs_qm_qoff_logitem_push,
+	.iop_release	= xfs_qm_qoff_logitem_release,
 };
 
 /*
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 1ea82764bf89..5d5ac65aa1cc 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -29,8 +29,6 @@ xfs_qm_log_quotaoff(
 	int			error;
 	struct xfs_qoff_logitem	*qoffi;
 
-	*qoffstartp = NULL;
-
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_quotaoff, 0, 0, 0, &tp);
 	if (error)
 		goto out;
@@ -62,7 +60,7 @@ xfs_qm_log_quotaoff(
 STATIC int
 xfs_qm_log_quotaoff_end(
 	struct xfs_mount	*mp,
-	struct xfs_qoff_logitem	*startqoff,
+	struct xfs_qoff_logitem	**startqoff,
 	uint			flags)
 {
 	struct xfs_trans	*tp;
@@ -73,9 +71,10 @@ xfs_qm_log_quotaoff_end(
 	if (error)
 		return error;
 
-	qoffi = xfs_trans_get_qoff_item(tp, startqoff,
+	qoffi = xfs_trans_get_qoff_item(tp, *startqoff,
 					flags & XFS_ALL_QUOTA_ACCT);
 	xfs_trans_log_quotaoff_item(tp, qoffi);
+	*startqoff = NULL;
 
 	/*
 	 * We have to make sure that the transaction is secure on disk before we
@@ -103,7 +102,7 @@ xfs_qm_scall_quotaoff(
 	uint			dqtype;
 	int			error;
 	uint			inactivate_flags;
-	struct xfs_qoff_logitem	*qoffstart;
+	struct xfs_qoff_logitem	*qoffstart = NULL;
 
 	/*
 	 * No file system can have quotas enabled on disk but not in core.
@@ -228,7 +227,7 @@ xfs_qm_scall_quotaoff(
 	 * So, we have QUOTAOFF start and end logitems; the start
 	 * logitem won't get overwritten until the end logitem appears...
 	 */
-	error = xfs_qm_log_quotaoff_end(mp, qoffstart, flags);
+	error = xfs_qm_log_quotaoff_end(mp, &qoffstart, flags);
 	if (error) {
 		/* We're screwed now. Shutdown is the only option. */
 		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
@@ -261,6 +260,8 @@ xfs_qm_scall_quotaoff(
 	}
 
 out_unlock:
+	if (error && qoffstart)
+		xfs_qm_qoff_logitem_relse(qoffstart);
 	mutex_unlock(&q->qi_quotaofflock);
 	return error;
 }
-- 
2.21.1

Re: [PATCH 1/2] xfs: factor out quotaoff intent AIL removal and memory free

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 01:00:31PM -0400, Brian Foster wrote:
> AIL removal of the quotaoff start intent and free of both intents is
> hardcoded to the ->iop_committed() handler of the end intent. Factor
> out the start intent handling code so it can be used in a future
> patch to properly handle quotaoff errors. Use xfs_trans_ail_remove()
> instead of the _delete() variant to acquire the AIL lock and also
> handle cases where an intent might not reside in the AIL at the
> time of a failure.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks like a straightforward hoist...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_dquot_item.c | 29 ++++++++++++++++++++---------
>  fs/xfs/xfs_dquot_item.h |  1 +
>  2 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> index d60647d7197b..2b816e9b4465 100644
> --- a/fs/xfs/xfs_dquot_item.c
> +++ b/fs/xfs/xfs_dquot_item.c
> @@ -307,18 +307,10 @@ xfs_qm_qoffend_logitem_committed(
>  {
>  	struct xfs_qoff_logitem	*qfe = QOFF_ITEM(lip);
>  	struct xfs_qoff_logitem	*qfs = qfe->qql_start_lip;
> -	struct xfs_ail		*ailp = qfs->qql_item.li_ailp;
>  
> -	/*
> -	 * Delete the qoff-start logitem from the AIL.
> -	 * xfs_trans_ail_delete() drops the AIL lock.
> -	 */
> -	spin_lock(&ailp->ail_lock);
> -	xfs_trans_ail_delete(ailp, &qfs->qql_item, SHUTDOWN_LOG_IO_ERROR);
> +	xfs_qm_qoff_logitem_relse(qfs);
>  
> -	kmem_free(qfs->qql_item.li_lv_shadow);
>  	kmem_free(lip->li_lv_shadow);
> -	kmem_free(qfs);
>  	kmem_free(qfe);
>  	return (xfs_lsn_t)-1;
>  }
> @@ -336,6 +328,25 @@ static const struct xfs_item_ops xfs_qm_qoff_logitem_ops = {
>  	.iop_push	= xfs_qm_qoff_logitem_push,
>  };
>  
> +/*
> + * Delete the quotaoff intent from the AIL and free it. On success,
> + * this should only be called for the start item. It can be used for
> + * either on shutdown or abort.
> + */
> +void
> +xfs_qm_qoff_logitem_relse(
> +	struct xfs_qoff_logitem	*qoff)
> +{
> +	struct xfs_log_item	*lip = &qoff->qql_item;
> +
> +	ASSERT(test_bit(XFS_LI_IN_AIL, &lip->li_flags) ||
> +	       test_bit(XFS_LI_ABORTED, &lip->li_flags) ||
> +	       XFS_FORCED_SHUTDOWN(lip->li_mountp));
> +	xfs_trans_ail_remove(lip, SHUTDOWN_LOG_IO_ERROR);
> +	kmem_free(lip->li_lv_shadow);
> +	kmem_free(qoff);
> +}
> +
>  /*
>   * Allocate and initialize an quotaoff item of the correct quota type(s).
>   */
> diff --git a/fs/xfs/xfs_dquot_item.h b/fs/xfs/xfs_dquot_item.h
> index 3bb19e556ade..2b86a43d7ce2 100644
> --- a/fs/xfs/xfs_dquot_item.h
> +++ b/fs/xfs/xfs_dquot_item.h
> @@ -28,6 +28,7 @@ void xfs_qm_dquot_logitem_init(struct xfs_dquot *dqp);
>  struct xfs_qoff_logitem	*xfs_qm_qoff_logitem_init(struct xfs_mount *mp,
>  		struct xfs_qoff_logitem *start,
>  		uint flags);
> +void xfs_qm_qoff_logitem_relse(struct xfs_qoff_logitem *);
>  struct xfs_qoff_logitem	*xfs_trans_get_qoff_item(struct xfs_trans *tp,
>  		struct xfs_qoff_logitem *startqoff,
>  		uint flags);
> -- 
> 2.21.1
> 

Re: [PATCH 2/2] xfs: fix unmount hang and memory leak on shutdown during quotaoff

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 01:00:32PM -0400, Brian Foster wrote:
> AIL removal of the quotaoff start intent and free of both quotaoff
> intents is currently limited to the ->iop_committed() handler of the
> end intent. This executes when the end intent is committed to the
> on-disk log and marks the completion of the operation. The problem
> with this is it assumes the success of the operation. If a shutdown
> or other error occurs during the quotaoff, it's possible for the
> quotaoff task to exit without removing the start intent from the
> AIL. This results in an unmount hang as the AIL cannot be emptied.
> Further, no other codepath frees the intents and so this is also a
> memory leak vector.

And I'm guessing that you'd rather we taught the quota items to be
self-releasing under error rather than making the quotaoff code be smart
enough to free the quotaoff-start item?

> First, update the high level quotaoff error path to directly remove
> and free the quotaoff start intent if it still exists in the AIL at
> the time of the error. Next, update both of the start and end
> quotaoff intents with an ->iop_release() callback to properly handle
> transaction abort.

I wonder, does this mean that we can drop the if (->io_release) check in
xfs_trans_free_items?  ISTR we were wondering at one point if there ever
was a real use case for items that don't have a release function.

> This means that If the quotaoff start transaction aborts, it frees
> the start intent in the transaction commit path. If the filesystem
> shuts down before the end transaction allocates, the quotaoff
> sequence removes and frees the start intent. If the end transaction
> aborts, it removes the start intent and frees both. This ensures
> that a shutdown does not result in a hung unmount and that memory is
> not leaked regardless of when a quotaoff error occurs.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

FWIW, the code looks reasonable.

--D

> ---
>  fs/xfs/xfs_dquot_item.c  | 15 +++++++++++++++
>  fs/xfs/xfs_qm_syscalls.c | 13 +++++++------
>  2 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> index 2b816e9b4465..cf65e2e43c6e 100644
> --- a/fs/xfs/xfs_dquot_item.c
> +++ b/fs/xfs/xfs_dquot_item.c
> @@ -315,17 +315,32 @@ xfs_qm_qoffend_logitem_committed(
>  	return (xfs_lsn_t)-1;
>  }
>  
> +STATIC void
> +xfs_qm_qoff_logitem_release(
> +	struct xfs_log_item	*lip)
> +{
> +	struct xfs_qoff_logitem	*qoff = QOFF_ITEM(lip);
> +
> +	if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
> +		if (qoff->qql_start_lip)
> +			xfs_qm_qoff_logitem_relse(qoff->qql_start_lip);
> +		xfs_qm_qoff_logitem_relse(qoff);
> +	}
> +}
> +
>  static const struct xfs_item_ops xfs_qm_qoffend_logitem_ops = {
>  	.iop_size	= xfs_qm_qoff_logitem_size,
>  	.iop_format	= xfs_qm_qoff_logitem_format,
>  	.iop_committed	= xfs_qm_qoffend_logitem_committed,
>  	.iop_push	= xfs_qm_qoff_logitem_push,
> +	.iop_release	= xfs_qm_qoff_logitem_release,
>  };
>  
>  static const struct xfs_item_ops xfs_qm_qoff_logitem_ops = {
>  	.iop_size	= xfs_qm_qoff_logitem_size,
>  	.iop_format	= xfs_qm_qoff_logitem_format,
>  	.iop_push	= xfs_qm_qoff_logitem_push,
> +	.iop_release	= xfs_qm_qoff_logitem_release,
>  };
>  
>  /*
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index 1ea82764bf89..5d5ac65aa1cc 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -29,8 +29,6 @@ xfs_qm_log_quotaoff(
>  	int			error;
>  	struct xfs_qoff_logitem	*qoffi;
>  
> -	*qoffstartp = NULL;
> -
>  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_quotaoff, 0, 0, 0, &tp);
>  	if (error)
>  		goto out;
> @@ -62,7 +60,7 @@ xfs_qm_log_quotaoff(
>  STATIC int
>  xfs_qm_log_quotaoff_end(
>  	struct xfs_mount	*mp,
> -	struct xfs_qoff_logitem	*startqoff,
> +	struct xfs_qoff_logitem	**startqoff,
>  	uint			flags)
>  {
>  	struct xfs_trans	*tp;
> @@ -73,9 +71,10 @@ xfs_qm_log_quotaoff_end(
>  	if (error)
>  		return error;
>  
> -	qoffi = xfs_trans_get_qoff_item(tp, startqoff,
> +	qoffi = xfs_trans_get_qoff_item(tp, *startqoff,
>  					flags & XFS_ALL_QUOTA_ACCT);
>  	xfs_trans_log_quotaoff_item(tp, qoffi);
> +	*startqoff = NULL;
>  
>  	/*
>  	 * We have to make sure that the transaction is secure on disk before we
> @@ -103,7 +102,7 @@ xfs_qm_scall_quotaoff(
>  	uint			dqtype;
>  	int			error;
>  	uint			inactivate_flags;
> -	struct xfs_qoff_logitem	*qoffstart;
> +	struct xfs_qoff_logitem	*qoffstart = NULL;
>  
>  	/*
>  	 * No file system can have quotas enabled on disk but not in core.
> @@ -228,7 +227,7 @@ xfs_qm_scall_quotaoff(
>  	 * So, we have QUOTAOFF start and end logitems; the start
>  	 * logitem won't get overwritten until the end logitem appears...
>  	 */
> -	error = xfs_qm_log_quotaoff_end(mp, qoffstart, flags);
> +	error = xfs_qm_log_quotaoff_end(mp, &qoffstart, flags);
>  	if (error) {
>  		/* We're screwed now. Shutdown is the only option. */
>  		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> @@ -261,6 +260,8 @@ xfs_qm_scall_quotaoff(
>  	}
>  
>  out_unlock:
> +	if (error && qoffstart)
> +		xfs_qm_qoff_logitem_relse(qoffstart);
>  	mutex_unlock(&q->qi_quotaofflock);
>  	return error;
>  }
> -- 
> 2.21.1
> 

Re: [PATCH 2/2] xfs: fix unmount hang and memory leak on shutdown during quotaoff

[Brian Foster]

On Mon, Mar 16, 2020 at 02:32:23PM -0700, Darrick J. Wong wrote:
> On Mon, Mar 16, 2020 at 01:00:32PM -0400, Brian Foster wrote:
> > AIL removal of the quotaoff start intent and free of both quotaoff
> > intents is currently limited to the ->iop_committed() handler of the
> > end intent. This executes when the end intent is committed to the
> > on-disk log and marks the completion of the operation. The problem
> > with this is it assumes the success of the operation. If a shutdown
> > or other error occurs during the quotaoff, it's possible for the
> > quotaoff task to exit without removing the start intent from the
> > AIL. This results in an unmount hang as the AIL cannot be emptied.
> > Further, no other codepath frees the intents and so this is also a
> > memory leak vector.
> 
> And I'm guessing that you'd rather we taught the quota items to be
> self-releasing under error rather than making the quotaoff code be smart
> enough to free the quotaoff-start item?
> 

It's a combination of both because they are separate transactions... If
the item is "owned" by a transaction and the transaction aborts, we
"release" all attached items as we would for any other transaction/item.
Once the start transaction commits, the start item is AIL resident and
there's no guarantee we'll ever get to the end transaction (i.e. a
shutdown could cause the transaction allocation to fail). The quotaoff
code handles cleaning up the start item in that scenario. This is
similar to the whole EFD holding a reference to the EFI model, except
quotaoff is a bit more hacky..

From a development perspective, my approach was to fix up the intent
handling such that adheres to common practice wrt to transactions and
then address the gap(s) in the quotaoff code (i.e. the separate
start/end transactions being an implementation detail of quotaoff).

> > First, update the high level quotaoff error path to directly remove
> > and free the quotaoff start intent if it still exists in the AIL at
> > the time of the error. Next, update both of the start and end
> > quotaoff intents with an ->iop_release() callback to properly handle
> > transaction abort.
> 
> I wonder, does this mean that we can drop the if (->io_release) check in
> xfs_trans_free_items?  ISTR we were wondering at one point if there ever
> was a real use case for items that don't have a release function.
> 

Hmm.. this was reworked fairly recently IIRC to condense some of the
callbacks. This used to be an ->iop_unlock() call. It was renamed to
->iop_release() to primarily handle the abort scenario (yet can also be
called via log I/O completion based on a magic flag) and the non-abort
->iop_unlock() call was folded into ->iop_committing(). Clear as mud? :)
In any event, I'm not aware of any further effort to remove
->iop_release() from xfs_trans_free_items() as that is the typical abort
path that this patch relies on..

Brian

> > This means that If the quotaoff start transaction aborts, it frees
> > the start intent in the transaction commit path. If the filesystem
> > shuts down before the end transaction allocates, the quotaoff
> > sequence removes and frees the start intent. If the end transaction
> > aborts, it removes the start intent and frees both. This ensures
> > that a shutdown does not result in a hung unmount and that memory is
> > not leaked regardless of when a quotaoff error occurs.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> 
> FWIW, the code looks reasonable.
> 
> --D
> 
> > ---
> >  fs/xfs/xfs_dquot_item.c  | 15 +++++++++++++++
> >  fs/xfs/xfs_qm_syscalls.c | 13 +++++++------
> >  2 files changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> > index 2b816e9b4465..cf65e2e43c6e 100644
> > --- a/fs/xfs/xfs_dquot_item.c
> > +++ b/fs/xfs/xfs_dquot_item.c
> > @@ -315,17 +315,32 @@ xfs_qm_qoffend_logitem_committed(
> >  	return (xfs_lsn_t)-1;
> >  }
> >  
> > +STATIC void
> > +xfs_qm_qoff_logitem_release(
> > +	struct xfs_log_item	*lip)
> > +{
> > +	struct xfs_qoff_logitem	*qoff = QOFF_ITEM(lip);
> > +
> > +	if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
> > +		if (qoff->qql_start_lip)
> > +			xfs_qm_qoff_logitem_relse(qoff->qql_start_lip);
> > +		xfs_qm_qoff_logitem_relse(qoff);
> > +	}
> > +}
> > +
> >  static const struct xfs_item_ops xfs_qm_qoffend_logitem_ops = {
> >  	.iop_size	= xfs_qm_qoff_logitem_size,
> >  	.iop_format	= xfs_qm_qoff_logitem_format,
> >  	.iop_committed	= xfs_qm_qoffend_logitem_committed,
> >  	.iop_push	= xfs_qm_qoff_logitem_push,
> > +	.iop_release	= xfs_qm_qoff_logitem_release,
> >  };
> >  
> >  static const struct xfs_item_ops xfs_qm_qoff_logitem_ops = {
> >  	.iop_size	= xfs_qm_qoff_logitem_size,
> >  	.iop_format	= xfs_qm_qoff_logitem_format,
> >  	.iop_push	= xfs_qm_qoff_logitem_push,
> > +	.iop_release	= xfs_qm_qoff_logitem_release,
> >  };
> >  
> >  /*
> > diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> > index 1ea82764bf89..5d5ac65aa1cc 100644
> > --- a/fs/xfs/xfs_qm_syscalls.c
> > +++ b/fs/xfs/xfs_qm_syscalls.c
> > @@ -29,8 +29,6 @@ xfs_qm_log_quotaoff(
> >  	int			error;
> >  	struct xfs_qoff_logitem	*qoffi;
> >  
> > -	*qoffstartp = NULL;
> > -
> >  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_quotaoff, 0, 0, 0, &tp);
> >  	if (error)
> >  		goto out;
> > @@ -62,7 +60,7 @@ xfs_qm_log_quotaoff(
> >  STATIC int
> >  xfs_qm_log_quotaoff_end(
> >  	struct xfs_mount	*mp,
> > -	struct xfs_qoff_logitem	*startqoff,
> > +	struct xfs_qoff_logitem	**startqoff,
> >  	uint			flags)
> >  {
> >  	struct xfs_trans	*tp;
> > @@ -73,9 +71,10 @@ xfs_qm_log_quotaoff_end(
> >  	if (error)
> >  		return error;
> >  
> > -	qoffi = xfs_trans_get_qoff_item(tp, startqoff,
> > +	qoffi = xfs_trans_get_qoff_item(tp, *startqoff,
> >  					flags & XFS_ALL_QUOTA_ACCT);
> >  	xfs_trans_log_quotaoff_item(tp, qoffi);
> > +	*startqoff = NULL;
> >  
> >  	/*
> >  	 * We have to make sure that the transaction is secure on disk before we
> > @@ -103,7 +102,7 @@ xfs_qm_scall_quotaoff(
> >  	uint			dqtype;
> >  	int			error;
> >  	uint			inactivate_flags;
> > -	struct xfs_qoff_logitem	*qoffstart;
> > +	struct xfs_qoff_logitem	*qoffstart = NULL;
> >  
> >  	/*
> >  	 * No file system can have quotas enabled on disk but not in core.
> > @@ -228,7 +227,7 @@ xfs_qm_scall_quotaoff(
> >  	 * So, we have QUOTAOFF start and end logitems; the start
> >  	 * logitem won't get overwritten until the end logitem appears...
> >  	 */
> > -	error = xfs_qm_log_quotaoff_end(mp, qoffstart, flags);
> > +	error = xfs_qm_log_quotaoff_end(mp, &qoffstart, flags);
> >  	if (error) {
> >  		/* We're screwed now. Shutdown is the only option. */
> >  		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> > @@ -261,6 +260,8 @@ xfs_qm_scall_quotaoff(
> >  	}
> >  
> >  out_unlock:
> > +	if (error && qoffstart)
> > +		xfs_qm_qoff_logitem_relse(qoffstart);
> >  	mutex_unlock(&q->qi_quotaofflock);
> >  	return error;
> >  }
> > -- 
> > 2.21.1
> > 
> 

Re: [PATCH 2/2] xfs: fix unmount hang and memory leak on shutdown during quotaoff

[Darrick J. Wong]

On Tue, Mar 17, 2020 at 07:40:11AM -0400, Brian Foster wrote:
> On Mon, Mar 16, 2020 at 02:32:23PM -0700, Darrick J. Wong wrote:
> > On Mon, Mar 16, 2020 at 01:00:32PM -0400, Brian Foster wrote:
> > > AIL removal of the quotaoff start intent and free of both quotaoff
> > > intents is currently limited to the ->iop_committed() handler of the
> > > end intent. This executes when the end intent is committed to the
> > > on-disk log and marks the completion of the operation. The problem
> > > with this is it assumes the success of the operation. If a shutdown
> > > or other error occurs during the quotaoff, it's possible for the
> > > quotaoff task to exit without removing the start intent from the
> > > AIL. This results in an unmount hang as the AIL cannot be emptied.
> > > Further, no other codepath frees the intents and so this is also a
> > > memory leak vector.
> > 
> > And I'm guessing that you'd rather we taught the quota items to be
> > self-releasing under error rather than making the quotaoff code be smart
> > enough to free the quotaoff-start item?
> > 
> 
> It's a combination of both because they are separate transactions... If
> the item is "owned" by a transaction and the transaction aborts, we
> "release" all attached items as we would for any other transaction/item.
> Once the start transaction commits, the start item is AIL resident and
> there's no guarantee we'll ever get to the end transaction (i.e. a
> shutdown could cause the transaction allocation to fail). The quotaoff
> code handles cleaning up the start item in that scenario. This is
> similar to the whole EFD holding a reference to the EFI model, except
> quotaoff is a bit more hacky..
> 
> From a development perspective, my approach was to fix up the intent
> handling such that adheres to common practice wrt to transactions and
> then address the gap(s) in the quotaoff code (i.e. the separate
> start/end transactions being an implementation detail of quotaoff).

<nod> Sounds like a reasonable approach for a pretty odd duck.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> > > First, update the high level quotaoff error path to directly remove
> > > and free the quotaoff start intent if it still exists in the AIL at
> > > the time of the error. Next, update both of the start and end
> > > quotaoff intents with an ->iop_release() callback to properly handle
> > > transaction abort.
> > 
> > I wonder, does this mean that we can drop the if (->io_release) check in
> > xfs_trans_free_items?  ISTR we were wondering at one point if there ever
> > was a real use case for items that don't have a release function.
> > 
> 
> Hmm.. this was reworked fairly recently IIRC to condense some of the
> callbacks. This used to be an ->iop_unlock() call. It was renamed to
> ->iop_release() to primarily handle the abort scenario (yet can also be
> called via log I/O completion based on a magic flag) and the non-abort
> ->iop_unlock() call was folded into ->iop_committing(). Clear as mud? :)
> In any event, I'm not aware of any further effort to remove
> ->iop_release() from xfs_trans_free_items() as that is the typical abort
> path that this patch relies on..

Oh, I didn't mean dropping ->iop_release completely, I just wondered
about removing the conditional, e.g.

	if (->iop_release)
		->iop_release(...);

becomes:

	->iop_release(...);

Now that every log item type (I think?) actually has a release method.

--D

> Brian
> 
> > > This means that If the quotaoff start transaction aborts, it frees
> > > the start intent in the transaction commit path. If the filesystem
> > > shuts down before the end transaction allocates, the quotaoff
> > > sequence removes and frees the start intent. If the end transaction
> > > aborts, it removes the start intent and frees both. This ensures
> > > that a shutdown does not result in a hung unmount and that memory is
> > > not leaked regardless of when a quotaoff error occurs.
> > > 
> > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > 
> > FWIW, the code looks reasonable.
> > 
> > --D
> > 
> > > ---
> > >  fs/xfs/xfs_dquot_item.c  | 15 +++++++++++++++
> > >  fs/xfs/xfs_qm_syscalls.c | 13 +++++++------
> > >  2 files changed, 22 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> > > index 2b816e9b4465..cf65e2e43c6e 100644
> > > --- a/fs/xfs/xfs_dquot_item.c
> > > +++ b/fs/xfs/xfs_dquot_item.c
> > > @@ -315,17 +315,32 @@ xfs_qm_qoffend_logitem_committed(
> > >  	return (xfs_lsn_t)-1;
> > >  }
> > >  
> > > +STATIC void
> > > +xfs_qm_qoff_logitem_release(
> > > +	struct xfs_log_item	*lip)
> > > +{
> > > +	struct xfs_qoff_logitem	*qoff = QOFF_ITEM(lip);
> > > +
> > > +	if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
> > > +		if (qoff->qql_start_lip)
> > > +			xfs_qm_qoff_logitem_relse(qoff->qql_start_lip);
> > > +		xfs_qm_qoff_logitem_relse(qoff);
> > > +	}
> > > +}
> > > +
> > >  static const struct xfs_item_ops xfs_qm_qoffend_logitem_ops = {
> > >  	.iop_size	= xfs_qm_qoff_logitem_size,
> > >  	.iop_format	= xfs_qm_qoff_logitem_format,
> > >  	.iop_committed	= xfs_qm_qoffend_logitem_committed,
> > >  	.iop_push	= xfs_qm_qoff_logitem_push,
> > > +	.iop_release	= xfs_qm_qoff_logitem_release,
> > >  };
> > >  
> > >  static const struct xfs_item_ops xfs_qm_qoff_logitem_ops = {
> > >  	.iop_size	= xfs_qm_qoff_logitem_size,
> > >  	.iop_format	= xfs_qm_qoff_logitem_format,
> > >  	.iop_push	= xfs_qm_qoff_logitem_push,
> > > +	.iop_release	= xfs_qm_qoff_logitem_release,
> > >  };
> > >  
> > >  /*
> > > diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> > > index 1ea82764bf89..5d5ac65aa1cc 100644
> > > --- a/fs/xfs/xfs_qm_syscalls.c
> > > +++ b/fs/xfs/xfs_qm_syscalls.c
> > > @@ -29,8 +29,6 @@ xfs_qm_log_quotaoff(
> > >  	int			error;
> > >  	struct xfs_qoff_logitem	*qoffi;
> > >  
> > > -	*qoffstartp = NULL;
> > > -
> > >  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_quotaoff, 0, 0, 0, &tp);
> > >  	if (error)
> > >  		goto out;
> > > @@ -62,7 +60,7 @@ xfs_qm_log_quotaoff(
> > >  STATIC int
> > >  xfs_qm_log_quotaoff_end(
> > >  	struct xfs_mount	*mp,
> > > -	struct xfs_qoff_logitem	*startqoff,
> > > +	struct xfs_qoff_logitem	**startqoff,
> > >  	uint			flags)
> > >  {
> > >  	struct xfs_trans	*tp;
> > > @@ -73,9 +71,10 @@ xfs_qm_log_quotaoff_end(
> > >  	if (error)
> > >  		return error;
> > >  
> > > -	qoffi = xfs_trans_get_qoff_item(tp, startqoff,
> > > +	qoffi = xfs_trans_get_qoff_item(tp, *startqoff,
> > >  					flags & XFS_ALL_QUOTA_ACCT);
> > >  	xfs_trans_log_quotaoff_item(tp, qoffi);
> > > +	*startqoff = NULL;
> > >  
> > >  	/*
> > >  	 * We have to make sure that the transaction is secure on disk before we
> > > @@ -103,7 +102,7 @@ xfs_qm_scall_quotaoff(
> > >  	uint			dqtype;
> > >  	int			error;
> > >  	uint			inactivate_flags;
> > > -	struct xfs_qoff_logitem	*qoffstart;
> > > +	struct xfs_qoff_logitem	*qoffstart = NULL;
> > >  
> > >  	/*
> > >  	 * No file system can have quotas enabled on disk but not in core.
> > > @@ -228,7 +227,7 @@ xfs_qm_scall_quotaoff(
> > >  	 * So, we have QUOTAOFF start and end logitems; the start
> > >  	 * logitem won't get overwritten until the end logitem appears...
> > >  	 */
> > > -	error = xfs_qm_log_quotaoff_end(mp, qoffstart, flags);
> > > +	error = xfs_qm_log_quotaoff_end(mp, &qoffstart, flags);
> > >  	if (error) {
> > >  		/* We're screwed now. Shutdown is the only option. */
> > >  		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> > > @@ -261,6 +260,8 @@ xfs_qm_scall_quotaoff(
> > >  	}
> > >  
> > >  out_unlock:
> > > +	if (error && qoffstart)
> > > +		xfs_qm_qoff_logitem_relse(qoffstart);
> > >  	mutex_unlock(&q->qi_quotaofflock);
> > >  	return error;
> > >  }
> > > -- 
> > > 2.21.1
> > > 
> > 
> 

Re: [PATCH 2/2] xfs: fix unmount hang and memory leak on shutdown during quotaoff

[Brian Foster]

On Tue, Mar 17, 2020 at 07:46:07AM -0700, Darrick J. Wong wrote:
> On Tue, Mar 17, 2020 at 07:40:11AM -0400, Brian Foster wrote:
> > On Mon, Mar 16, 2020 at 02:32:23PM -0700, Darrick J. Wong wrote:
> > > On Mon, Mar 16, 2020 at 01:00:32PM -0400, Brian Foster wrote:
> > > > AIL removal of the quotaoff start intent and free of both quotaoff
> > > > intents is currently limited to the ->iop_committed() handler of the
> > > > end intent. This executes when the end intent is committed to the
> > > > on-disk log and marks the completion of the operation. The problem
> > > > with this is it assumes the success of the operation. If a shutdown
> > > > or other error occurs during the quotaoff, it's possible for the
> > > > quotaoff task to exit without removing the start intent from the
> > > > AIL. This results in an unmount hang as the AIL cannot be emptied.
> > > > Further, no other codepath frees the intents and so this is also a
> > > > memory leak vector.
> > > 
> > > And I'm guessing that you'd rather we taught the quota items to be
> > > self-releasing under error rather than making the quotaoff code be smart
> > > enough to free the quotaoff-start item?
> > > 
> > 
> > It's a combination of both because they are separate transactions... If
> > the item is "owned" by a transaction and the transaction aborts, we
> > "release" all attached items as we would for any other transaction/item.
> > Once the start transaction commits, the start item is AIL resident and
> > there's no guarantee we'll ever get to the end transaction (i.e. a
> > shutdown could cause the transaction allocation to fail). The quotaoff
> > code handles cleaning up the start item in that scenario. This is
> > similar to the whole EFD holding a reference to the EFI model, except
> > quotaoff is a bit more hacky..
> > 
> > From a development perspective, my approach was to fix up the intent
> > handling such that adheres to common practice wrt to transactions and
> > then address the gap(s) in the quotaoff code (i.e. the separate
> > start/end transactions being an implementation detail of quotaoff).
> 
> <nod> Sounds like a reasonable approach for a pretty odd duck.
> 
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> > > > First, update the high level quotaoff error path to directly remove
> > > > and free the quotaoff start intent if it still exists in the AIL at
> > > > the time of the error. Next, update both of the start and end
> > > > quotaoff intents with an ->iop_release() callback to properly handle
> > > > transaction abort.
> > > 
> > > I wonder, does this mean that we can drop the if (->io_release) check in
> > > xfs_trans_free_items?  ISTR we were wondering at one point if there ever
> > > was a real use case for items that don't have a release function.
> > > 
> > 
> > Hmm.. this was reworked fairly recently IIRC to condense some of the
> > callbacks. This used to be an ->iop_unlock() call. It was renamed to
> > ->iop_release() to primarily handle the abort scenario (yet can also be
> > called via log I/O completion based on a magic flag) and the non-abort
> > ->iop_unlock() call was folded into ->iop_committing(). Clear as mud? :)
> > In any event, I'm not aware of any further effort to remove
> > ->iop_release() from xfs_trans_free_items() as that is the typical abort
> > path that this patch relies on..
> 
> Oh, I didn't mean dropping ->iop_release completely, I just wondered
> about removing the conditional, e.g.
> 
> 	if (->iop_release)
> 		->iop_release(...);
> 
> becomes:
> 
> 	->iop_release(...);
> 
> Now that every log item type (I think?) actually has a release method.
> 

Ah, I misread. -ENOCOFFEE I guess. :P That seems reasonable to me, but
tbh I'm not sure it's worth saving 1 LOC either. *shrug*

Brian

> --D
> 
> > Brian
> > 
> > > > This means that If the quotaoff start transaction aborts, it frees
> > > > the start intent in the transaction commit path. If the filesystem
> > > > shuts down before the end transaction allocates, the quotaoff
> > > > sequence removes and frees the start intent. If the end transaction
> > > > aborts, it removes the start intent and frees both. This ensures
> > > > that a shutdown does not result in a hung unmount and that memory is
> > > > not leaked regardless of when a quotaoff error occurs.
> > > > 
> > > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > 
> > > FWIW, the code looks reasonable.
> > > 
> > > --D
> > > 
> > > > ---
> > > >  fs/xfs/xfs_dquot_item.c  | 15 +++++++++++++++
> > > >  fs/xfs/xfs_qm_syscalls.c | 13 +++++++------
> > > >  2 files changed, 22 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> > > > index 2b816e9b4465..cf65e2e43c6e 100644
> > > > --- a/fs/xfs/xfs_dquot_item.c
> > > > +++ b/fs/xfs/xfs_dquot_item.c
> > > > @@ -315,17 +315,32 @@ xfs_qm_qoffend_logitem_committed(
> > > >  	return (xfs_lsn_t)-1;
> > > >  }
> > > >  
> > > > +STATIC void
> > > > +xfs_qm_qoff_logitem_release(
> > > > +	struct xfs_log_item	*lip)
> > > > +{
> > > > +	struct xfs_qoff_logitem	*qoff = QOFF_ITEM(lip);
> > > > +
> > > > +	if (test_bit(XFS_LI_ABORTED, &lip->li_flags)) {
> > > > +		if (qoff->qql_start_lip)
> > > > +			xfs_qm_qoff_logitem_relse(qoff->qql_start_lip);
> > > > +		xfs_qm_qoff_logitem_relse(qoff);
> > > > +	}
> > > > +}
> > > > +
> > > >  static const struct xfs_item_ops xfs_qm_qoffend_logitem_ops = {
> > > >  	.iop_size	= xfs_qm_qoff_logitem_size,
> > > >  	.iop_format	= xfs_qm_qoff_logitem_format,
> > > >  	.iop_committed	= xfs_qm_qoffend_logitem_committed,
> > > >  	.iop_push	= xfs_qm_qoff_logitem_push,
> > > > +	.iop_release	= xfs_qm_qoff_logitem_release,
> > > >  };
> > > >  
> > > >  static const struct xfs_item_ops xfs_qm_qoff_logitem_ops = {
> > > >  	.iop_size	= xfs_qm_qoff_logitem_size,
> > > >  	.iop_format	= xfs_qm_qoff_logitem_format,
> > > >  	.iop_push	= xfs_qm_qoff_logitem_push,
> > > > +	.iop_release	= xfs_qm_qoff_logitem_release,
> > > >  };
> > > >  
> > > >  /*
> > > > diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> > > > index 1ea82764bf89..5d5ac65aa1cc 100644
> > > > --- a/fs/xfs/xfs_qm_syscalls.c
> > > > +++ b/fs/xfs/xfs_qm_syscalls.c
> > > > @@ -29,8 +29,6 @@ xfs_qm_log_quotaoff(
> > > >  	int			error;
> > > >  	struct xfs_qoff_logitem	*qoffi;
> > > >  
> > > > -	*qoffstartp = NULL;
> > > > -
> > > >  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_qm_quotaoff, 0, 0, 0, &tp);
> > > >  	if (error)
> > > >  		goto out;
> > > > @@ -62,7 +60,7 @@ xfs_qm_log_quotaoff(
> > > >  STATIC int
> > > >  xfs_qm_log_quotaoff_end(
> > > >  	struct xfs_mount	*mp,
> > > > -	struct xfs_qoff_logitem	*startqoff,
> > > > +	struct xfs_qoff_logitem	**startqoff,
> > > >  	uint			flags)
> > > >  {
> > > >  	struct xfs_trans	*tp;
> > > > @@ -73,9 +71,10 @@ xfs_qm_log_quotaoff_end(
> > > >  	if (error)
> > > >  		return error;
> > > >  
> > > > -	qoffi = xfs_trans_get_qoff_item(tp, startqoff,
> > > > +	qoffi = xfs_trans_get_qoff_item(tp, *startqoff,
> > > >  					flags & XFS_ALL_QUOTA_ACCT);
> > > >  	xfs_trans_log_quotaoff_item(tp, qoffi);
> > > > +	*startqoff = NULL;
> > > >  
> > > >  	/*
> > > >  	 * We have to make sure that the transaction is secure on disk before we
> > > > @@ -103,7 +102,7 @@ xfs_qm_scall_quotaoff(
> > > >  	uint			dqtype;
> > > >  	int			error;
> > > >  	uint			inactivate_flags;
> > > > -	struct xfs_qoff_logitem	*qoffstart;
> > > > +	struct xfs_qoff_logitem	*qoffstart = NULL;
> > > >  
> > > >  	/*
> > > >  	 * No file system can have quotas enabled on disk but not in core.
> > > > @@ -228,7 +227,7 @@ xfs_qm_scall_quotaoff(
> > > >  	 * So, we have QUOTAOFF start and end logitems; the start
> > > >  	 * logitem won't get overwritten until the end logitem appears...
> > > >  	 */
> > > > -	error = xfs_qm_log_quotaoff_end(mp, qoffstart, flags);
> > > > +	error = xfs_qm_log_quotaoff_end(mp, &qoffstart, flags);
> > > >  	if (error) {
> > > >  		/* We're screwed now. Shutdown is the only option. */
> > > >  		xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> > > > @@ -261,6 +260,8 @@ xfs_qm_scall_quotaoff(
> > > >  	}
> > > >  
> > > >  out_unlock:
> > > > +	if (error && qoffstart)
> > > > +		xfs_qm_qoff_logitem_relse(qoffstart);
> > > >  	mutex_unlock(&q->qi_quotaofflock);
> > > >  	return error;
> > > >  }
> > > > -- 
> > > > 2.21.1
> > > > 
> > > 
> > 
> 

Re: [PATCH 1/2] xfs: factor out quotaoff intent AIL removal and memory free

[Christoph Hellwig]

On Mon, Mar 16, 2020 at 01:00:31PM -0400, Brian Foster wrote:
> AIL removal of the quotaoff start intent and free of both intents is
> hardcoded to the ->iop_committed() handler of the end intent. Factor
> out the start intent handling code so it can be used in a future
> patch to properly handle quotaoff errors. Use xfs_trans_ail_remove()
> instead of the _delete() variant to acquire the AIL lock and also
> handle cases where an intent might not reside in the AIL at the
> time of a failure.

xfs_trans_ail_delete alsmost seems nicer here rather than duplicating
the most be in ail except for abort/shutdown.  But either way it looks
correct, so if you prefer it this way:

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

Re: [PATCH 2/2] xfs: fix unmount hang and memory leak on shutdown during quotaoff

[Christoph Hellwig]

Looks good,

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