On Tue, Dec 06, 2011 at 04:58:20PM -0500, Christoph Hellwig wrote:
> No need to play games with the qlock now that the freelist lock nests inside
> it. Also clean up various outdated comments.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
Looks good, minor gripes notwithstanding.
Reviewed-by: Ben Myers <bpm@xxxxxxx>
> ---
> fs/xfs/xfs_qm.c | 64 ++++++++++++++------------------------------------------
> 1 file changed, 16 insertions(+), 48 deletions(-)
>
> Index: xfs/fs/xfs/xfs_qm.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_qm.c 2011-12-06 15:49:31.303693024 +0100
> +++ xfs/fs/xfs/xfs_qm.c 2011-12-06 15:51:39.467028734 +0100
> @@ -650,11 +650,7 @@ xfs_qm_dqattach_one(
>
> /*
> * Given a udquot and gdquot, attach a ptr to the group dquot in the
> - * udquot as a hint for future lookups. The idea sounds simple, but the
> - * execution isn't, because the udquot might have a group dquot attached
> - * already and getting rid of that gets us into lock ordering constraints.
> - * The process is complicated more by the fact that the dquots may or may not
> - * be locked on entry.
> + * udquot as a hint for future lookups.
> */
> STATIC void
> xfs_qm_dqattach_grouphint(
> @@ -665,45 +661,21 @@ xfs_qm_dqattach_grouphint(
>
> xfs_dqlock(udq);
>
> - if ((tmp = udq->q_gdquot)) {
> - if (tmp == gdq) {
> - xfs_dqunlock(udq);
> - return;
> - }
> + tmp = udq->q_gdquot;
> + if (tmp) {
> + if (tmp == gdq)
> + goto done;
>
> udq->q_gdquot = NULL;
> - /*
> - * We can't keep any dqlocks when calling dqrele,
> - * because the freelist lock comes before dqlocks.
> - */
> - xfs_dqunlock(udq);
> - /*
> - * we took a hard reference once upon a time in dqget,
> - * so give it back when the udquot no longer points at it
> - * dqput() does the unlocking of the dquot.
> - */
At least tell us where we got the ref we're going to dqrele. This
comment had some value.
> xfs_qm_dqrele(tmp);
> -
> - xfs_dqlock(udq);
> - xfs_dqlock(gdq);
> -
> - } else {
> - ASSERT(XFS_DQ_IS_LOCKED(udq));
> - xfs_dqlock(gdq);
> - }
> -
> - ASSERT(XFS_DQ_IS_LOCKED(udq));
> - ASSERT(XFS_DQ_IS_LOCKED(gdq));
> - /*
> - * Somebody could have attached a gdquot here,
> - * when we dropped the uqlock. If so, just do nothing.
> - */
> - if (udq->q_gdquot == NULL) {
> - XFS_DQHOLD(gdq);
> - udq->q_gdquot = gdq;
> }
>
> + xfs_dqlock(gdq);
> + XFS_DQHOLD(gdq);
> xfs_dqunlock(gdq);
> +
> + udq->q_gdquot = gdq;
> +done:
> xfs_dqunlock(udq);
> }
>
> @@ -770,17 +742,13 @@ xfs_qm_dqattach_locked(
> ASSERT(ip->i_gdquot);
>
> /*
> - * We may or may not have the i_udquot locked at this point,
> - * but this check is OK since we don't depend on the i_gdquot to
> - * be accurate 100% all the time. It is just a hint, and this
> - * will succeed in general.
> - */
> - if (ip->i_udquot->q_gdquot == ip->i_gdquot)
> - goto done;
> - /*
> - * Attach i_gdquot to the gdquot hint inside the i_udquot.
> + * We do not have i_udquot locked at this point, but this check
> + * is OK since we don't depend on the i_gdquot to be accurate
> + * 100% all the time. It is just a hint, and this will
> + * succeed in general.
> */
Tsk. Lets not be too smart for our own good.
> - xfs_qm_dqattach_grouphint(ip->i_udquot, ip->i_gdquot);
> + if (ip->i_udquot->q_gdquot != ip->i_gdquot)
> + xfs_qm_dqattach_grouphint(ip->i_udquot, ip->i_gdquot);
> }
>
> done:
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
