[01/18] xfs: clear XFS_DQ_FREEING if we can't lock the dquot buffer to flush
diff mbox series

Message ID 159353171640.2864738.7967439700762859129.stgit@magnolia
State Superseded
Headers show
Series
  • xfs: remove xfs_disk_quot from incore dquot
Related show

Commit Message

Darrick J. Wong June 30, 2020, 3:41 p.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

In commit 8d3d7e2b35ea, we changed xfs_qm_dqpurge to bail out if we
can't lock the dquot buf to flush the dquot.  This prevents the AIL from
blocking on the dquot, but it also forgets to clear the FREEING flag on
its way out.  A subsequent purge attempt will see the FREEING flag is
set and bail out, which leads to dqpurge_all failing to purge all the
dquots.  This causes unmounts and quotaoff operations to hang.

Fixes: 8d3d7e2b35ea ("xfs: trylock underlying buffer on dquot flush")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_qm.c |    1 +
 1 file changed, 1 insertion(+)

Comments

Allison Collins June 30, 2020, 9:35 p.m. UTC | #1
On 6/30/20 8:41 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In commit 8d3d7e2b35ea, we changed xfs_qm_dqpurge to bail out if we
> can't lock the dquot buf to flush the dquot.  This prevents the AIL from
> blocking on the dquot, but it also forgets to clear the FREEING flag on
> its way out.  A subsequent purge attempt will see the FREEING flag is
> set and bail out, which leads to dqpurge_all failing to purge all the
> dquots.  This causes unmounts and quotaoff operations to hang.
> 
> Fixes: 8d3d7e2b35ea ("xfs: trylock underlying buffer on dquot flush")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Ok
Reviewed-by: Allison Collins <allison.henderson@oracle.com>

> ---
>   fs/xfs/xfs_qm.c |    1 +
>   1 file changed, 1 insertion(+)
> 
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index d6cd83317344..938023dd8ce5 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -148,6 +148,7 @@ xfs_qm_dqpurge(
>   			error = xfs_bwrite(bp);
>   			xfs_buf_relse(bp);
>   		} else if (error == -EAGAIN) {
> +			dqp->dq_flags &= ~XFS_DQ_FREEING;
>   			goto out_unlock;
>   		}
>   		xfs_dqflock(dqp);
>
Chandan Babu R July 1, 2020, 8:32 a.m. UTC | #2
On Tuesday 30 June 2020 9:11:56 PM IST Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In commit 8d3d7e2b35ea, we changed xfs_qm_dqpurge to bail out if we
> can't lock the dquot buf to flush the dquot.  This prevents the AIL from
> blocking on the dquot, but it also forgets to clear the FREEING flag on
> its way out.  A subsequent purge attempt will see the FREEING flag is
> set and bail out, which leads to dqpurge_all failing to purge all the
> dquots.  This causes unmounts and quotaoff operations to hang.
>

xfs_qm_dqpurge() checks for the presence of XFS_DQ_FREEING. If it is set, it
indicates that another task is freeing this dquot.

A failed read operation could return -EAGAIN and hence xfs_qm_dqpurge()
returning without clearing XFS_DQ_FREEING would mean that future invocations
of this function would turn out to be a nop and hence the corresponding dquot
would linger around forever. Hence the fix is correct.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Fixes: 8d3d7e2b35ea ("xfs: trylock underlying buffer on dquot flush")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_qm.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index d6cd83317344..938023dd8ce5 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -148,6 +148,7 @@ xfs_qm_dqpurge(
>  			error = xfs_bwrite(bp);
>  			xfs_buf_relse(bp);
>  		} else if (error == -EAGAIN) {
> +			dqp->dq_flags &= ~XFS_DQ_FREEING;
>  			goto out_unlock;
>  		}
>  		xfs_dqflock(dqp);
> 
>
Christoph Hellwig July 1, 2020, 8:33 a.m. UTC | #3
On Tue, Jun 30, 2020 at 08:41:56AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In commit 8d3d7e2b35ea, we changed xfs_qm_dqpurge to bail out if we
> can't lock the dquot buf to flush the dquot.  This prevents the AIL from
> blocking on the dquot, but it also forgets to clear the FREEING flag on
> its way out.  A subsequent purge attempt will see the FREEING flag is
> set and bail out, which leads to dqpurge_all failing to purge all the
> dquots.  This causes unmounts and quotaoff operations to hang.
> 
> Fixes: 8d3d7e2b35ea ("xfs: trylock underlying buffer on dquot flush")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks like Dave submitted this as a separate fix just after you..

Reviewed-by: Christoph Hellwig <hch@lst.de>
Dave Chinner July 1, 2020, 10:42 p.m. UTC | #4
On Wed, Jul 01, 2020 at 09:33:58AM +0100, Christoph Hellwig wrote:
> On Tue, Jun 30, 2020 at 08:41:56AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In commit 8d3d7e2b35ea, we changed xfs_qm_dqpurge to bail out if we
> > can't lock the dquot buf to flush the dquot.  This prevents the AIL from
> > blocking on the dquot, but it also forgets to clear the FREEING flag on
> > its way out.  A subsequent purge attempt will see the FREEING flag is
> > set and bail out, which leads to dqpurge_all failing to purge all the
> > dquots.  This causes unmounts and quotaoff operations to hang.
> > 
> > Fixes: 8d3d7e2b35ea ("xfs: trylock underlying buffer on dquot flush")
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Looks like Dave submitted this as a separate fix just after you..

Ah, I didn't look at this patchset yesterday so didn't notice that
Darrick had already posted it. I'm happy for this version to be
merged...

Reviewed-by: Dave Chinner <dchinner@redhat.com>

Patch
diff mbox series

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index d6cd83317344..938023dd8ce5 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -148,6 +148,7 @@  xfs_qm_dqpurge(
 			error = xfs_bwrite(bp);
 			xfs_buf_relse(bp);
 		} else if (error == -EAGAIN) {
+			dqp->dq_flags &= ~XFS_DQ_FREEING;
 			goto out_unlock;
 		}
 		xfs_dqflock(dqp);