Re: [PATCH 0/6 v2] xfs: xfs_iflush_cluster vs xfs_reclaim_inode

[Dave Chinner <david@fromorbit.com>]

On Mon, Apr 11, 2016 at 09:37:18AM -0400, Brian Foster wrote:
> On Sat, Apr 09, 2016 at 08:17:06AM +1000, Dave Chinner wrote:
> > On Fri, Apr 08, 2016 at 01:18:44PM -0400, Brian Foster wrote:
> > > On Fri, Apr 08, 2016 at 09:37:45AM +1000, Dave Chinner wrote:
> > > > Hi folks,
> > > > 
> > > > This is the second version of this patch set, first posted and
> > > > described here:
> > > > 
> > > > http://oss.sgi.com/archives/xfs/2016-04/msg00069.html
> > > > 
> > > > The only change from the first version is splitting up the first
> > > > patch into two as Christoph requested - one for the bug fix, the
> > > > other for the variable renaming.
> > > > 
> > > 
> > > Did your xfstests testing for this series include generic/233? I'm
> > > seeing a consistently reproducible test hang. The test is hanging on a
> > > "xfs_quota -x -c off -ug /mnt/scratch" command. The stack is as follows:
> > > 
> > > [<ffffffffa0772306>] xfs_qm_dquot_walk.isra.8+0x196/0x1b0 [xfs]
> > > [<ffffffffa0774a98>] xfs_qm_dqpurge_all+0x78/0x80 [xfs]
> > > [<ffffffffa07713e8>] xfs_qm_scall_quotaoff+0x148/0x640 [xfs]
> > > [<ffffffffa077733d>] xfs_quota_disable+0x3d/0x50 [xfs]
> > > [<ffffffff812c27e3>] SyS_quotactl+0x3b3/0x8c0
> > > [<ffffffff81003e17>] do_syscall_64+0x67/0x190
> > > [<ffffffff81763f7f>] return_from_SYSCALL_64+0x0/0x7a
> > > [<ffffffffffffffff>] 0xffffffffffffffff
....
> > IOWs, this looks like xfs_qm_dquot_walk() is skipping dquots because
> > xfs_qm_dqpurge is hitting this:
> > 
> >         xfs_dqlock(dqp);
> >         if ((dqp->dq_flags & XFS_DQ_FREEING) || dqp->q_nrefs != 0) {
> >                 xfs_dqunlock(dqp);
> >                 return -EAGAIN;
> >         }
> > 
> > So that means we've got an inode that probably hasn't been
> > reclaimed, because the last thing that happens during reclaim is the
> > dquots are detatched from the inode and hence the reference counts
> > are dropped.
> > 
> > > FWIW, this only occurs with patch 6 applied. The test and scratch
> > > devices are both 10GB lvm volumes formatted with mkfs defaults (v5).
> > 
> > I can't see how patch 6 would prevent an inode from being reclaimed,
> > as all the changes occur *after* the reclaim decision has been made.
> > More investigation needed, I guess...
> > 
> 
> The attached diff addresses the problem for me. Feel free to fold it
> into the original patch.
....
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index a60db43..749689c 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -818,14 +818,15 @@ xfs_inode_set_reclaim_tag(
>  STATIC void
>  __xfs_inode_clear_reclaim(
>  	xfs_perag_t	*pag,
> -	xfs_inode_t	*ip)
> +	xfs_inode_t	*ip,
> +	xfs_ino_t	ino)
>  {
>  	pag->pag_ici_reclaimable--;
>  	if (!pag->pag_ici_reclaimable) {
>  		/* clear the reclaim tag from the perag radix tree */
>  		spin_lock(&ip->i_mount->m_perag_lock);
>  		radix_tree_tag_clear(&ip->i_mount->m_perag_tree,
> -				XFS_INO_TO_AGNO(ip->i_mount, ip->i_ino),
> +				XFS_INO_TO_AGNO(ip->i_mount, ino),
>  				XFS_ICI_RECLAIM_TAG);
>  		spin_unlock(&ip->i_mount->m_perag_lock);
>  		trace_xfs_perag_clear_reclaim(ip->i_mount, pag->pag_agno,

Yeah, that'll do it. Though I think the fix should be something
different - why do we need ip->i_ino to find the agno or the
xfs_mount when we've already got pag->pag_agno and pag->pag_mount?

I'll clean up all these per-ag tag functions to only take xfs_perag
and an xfs_ino_t where needed and repost once i've tested it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs