Re: [PATCH 10/16] xfs: nest qm_dqfrlist_lock insise the dquot qlock

Dave Chinner
2011-12-05T05:11:59Z

On Mon, Nov 28, 2011 at 03:27:32AM -0500, Christoph Hellwig wrote:
> Allow xfs_qm_dqput to work without trylock loops by nesting the freelist lock
> inside the dquot qlock. In turn that requires trylocks in the reclaim path
> instead, but given it's a classic tradeoff between fast and slow path, and
> we follow the model of the inode and dentry caches.

Funny how that keeps coming up ;)

> Document our new lock order now that it has settled.

Excellent.

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

Subject line has a type (insise).

>
> ---
> fs/xfs/xfs_dquot.c | 98 +++++++++++++++++++++--------------------------------
> fs/xfs/xfs_qm.c | 4 +-
> 2 files changed, 42 insertions(+), 60 deletions(-)
>
> Index: xfs/fs/xfs/xfs_dquot.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_dquot.c 2011-11-25 11:45:53.348630228 +0100
> +++ xfs/fs/xfs/xfs_dquot.c 2011-11-25 11:56:08.655296826 +0100
> @@ -39,20 +39,19 @@
> #include "xfs_qm.h"
> #include "xfs_trace.h"
>
> -
> /*
> - LOCK ORDER
> -
> - inode lock (ilock)
> - dquot hash-chain lock (hashlock)
> - xqm dquot freelist lock (freelistlock
> - mount's dquot list lock (mplistlock)
> - user dquot lock - lock ordering among dquots is based on the uid or gid
> - group dquot lock - similar to udquots. Between the two dquots, the udquot
> - has to be locked first.
> - pin lock - the dquot lock must be held to take this lock.
> - flush lock - ditto.
> -*/
> + * Lock order:
> + *
> + * ip->i_lock
> + * qh->qh_lock
> + * qi->qi_dqlist_lock
> + * dquot->q_qlock
> + * dquot->q_flush
> + * xfs_Gqm->qm_dqfrlist_lock
> + *
> + * If two dquots need to be locked the order is user before group/project,
> + * otherwise by the lowest id first, see xfs_dqlock2.

For the dquot->q_* locks, it might be worth adding the name of the
locking functions here so it is obvious when reading the code as
q_qlock and q_flush aren't used anywhere outside the locking
wrappers...

Otherwise looks sane.

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

--
Dave Chinner
david@fromorbit.com