Date: Mon, 12 Jan 2009 10:06:37 +1100
From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 4/7] xfs: lockdep annotations for xfs_dqlock2
Message-ID: <20090111230637.GE8071@disturbed>
Mail-Followup-To: Christoph Hellwig <hch@infradead.org>,
	xfs@oss.sgi.com
References: <20090109221104.237540000@bombadil.infradead.org> <20090109221300.520949000@bombadil.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20090109221300.520949000@bombadil.infradead.org>
User-Agent: Mutt/1.5.18 (2008-05-17)

On Fri, Jan 09, 2009 at 05:11:08PM -0500, Christoph Hellwig wrote:
> xfs_dqlock2 locks two xfs_dquots, which is fine as it always locks the
> dquot with the lower id first.  Use mutex_lock_nested to tell lockdep 
> about this fact.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/quota/xfs_dquot.c
> ===================================================================
> --- xfs.orig/fs/xfs/quota/xfs_dquot.c	2008-12-29 11:28:54.459613573 +0100
> +++ xfs/fs/xfs/quota/xfs_dquot.c	2008-12-29 11:29:31.037738842 +0100
> @@ -1383,6 +1383,12 @@ xfs_dqunlock_nonotify(
>  	mutex_unlock(&(dqp->q_qlock));
>  }
>  
> +/*
> + * Lock two xfs_dquot structures.
> + *
> + * To avoid deadlocks we always lock the quota structure with
> + * the lowerd id first.
> + */
>  void
>  xfs_dqlock2(
>  	xfs_dquot_t	*d1,
> @@ -1393,10 +1399,10 @@ xfs_dqlock2(
>  		if (be32_to_cpu(d1->q_core.d_id) >
>  		    be32_to_cpu(d2->q_core.d_id)) {
>  			xfs_dqlock(d2);
> -			xfs_dqlock(d1);
> +			mutex_lock_nested(&d1->q_qlock, XFS_QLOCK_2ND);
>  		} else {
>  			xfs_dqlock(d1);
> -			xfs_dqlock(d2);
> +			mutex_lock_nested(&d2->q_qlock, XFS_QLOCK_2ND);

This looks a bit wierd.

Yes, xfs_dqlock() is just a wrapper around mutex_lock, but we should
be consistent here. Can you add a xfs_dqlock_nested() wrapper to do
this?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com