* [PATCH 01/16] xfs: untange SYNC_WAIT and SYNC_TRYLOCK meanings for xfs_qm_dqflush
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 3:57 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 02/16] xfs: make sure to really flush all dquots in xfs_qm_quotacheck Christoph Hellwig
` (14 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-dquota-cleanup-SYNC_-flags --]
[-- Type: text/plain, Size: 2044 bytes --]
Only skip pinned dquots if SYNC_TRYLOCK is specified, and adjust the callers
to keep the behaviour unchanged.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_dquot.c | 2 +-
fs/xfs/xfs_dquot_item.c | 2 +-
fs/xfs/xfs_qm.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
Index: xfs/fs/xfs/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.c 2011-11-05 08:54:01.729993938 +0100
+++ xfs/fs/xfs/xfs_dquot.c 2011-11-25 11:45:32.668742260 +0100
@@ -1169,7 +1169,7 @@ xfs_qm_dqflush(
* If not dirty, or it's pinned and we are not supposed to block, nada.
*/
if (!XFS_DQ_IS_DIRTY(dqp) ||
- (!(flags & SYNC_WAIT) && atomic_read(&dqp->q_pincount) > 0)) {
+ ((flags & SYNC_TRYLOCK) && atomic_read(&dqp->q_pincount) > 0)) {
xfs_dqfunlock(dqp);
return 0;
}
Index: xfs/fs/xfs/xfs_dquot_item.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot_item.c 2011-11-25 11:43:25.269432441 +0100
+++ xfs/fs/xfs/xfs_dquot_item.c 2011-11-25 11:45:32.668742260 +0100
@@ -133,7 +133,7 @@ xfs_qm_dquot_logitem_push(
* lock without sleeping, then there must not have been
* anyone in the process of flushing the dquot.
*/
- error = xfs_qm_dqflush(dqp, 0);
+ error = xfs_qm_dqflush(dqp, SYNC_TRYLOCK);
if (error)
xfs_warn(dqp->q_mount, "%s: push error %d on dqp %p",
__func__, error, dqp);
Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c 2011-11-19 20:14:00.400421363 +0100
+++ xfs/fs/xfs/xfs_qm.c 2011-11-25 11:45:32.672075575 +0100
@@ -1661,7 +1661,7 @@ xfs_qm_quotacheck(
* successfully.
*/
if (!error)
- error = xfs_qm_dqflush_all(mp, 0);
+ error = xfs_qm_dqflush_all(mp, SYNC_TRYLOCK);
/*
* We can get this error if we couldn't do a dquot allocation inside
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* [PATCH 02/16] xfs: make sure to really flush all dquots in xfs_qm_quotacheck
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
2011-11-28 8:27 ` [PATCH 01/16] xfs: untange SYNC_WAIT and SYNC_TRYLOCK meanings for xfs_qm_dqflush Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 3:59 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 03/16] xfs: remove xfs_qm_sync Christoph Hellwig
` (13 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-quotacheck-dont-trylock --]
[-- Type: text/plain, Size: 939 bytes --]
Make sure we do not skip any dquots when flushing them out after a
quotacheck to make sure that we will never have any dirty dquots on a life
filesystem. At this point no dquot should be pinnable, but lets be anal
about it.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_qm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c 2011-11-25 11:45:32.672075575 +0100
+++ xfs/fs/xfs/xfs_qm.c 2011-11-25 11:45:35.875391556 +0100
@@ -1661,7 +1661,7 @@ xfs_qm_quotacheck(
* successfully.
*/
if (!error)
- error = xfs_qm_dqflush_all(mp, SYNC_TRYLOCK);
+ error = xfs_qm_dqflush_all(mp, 0);
/*
* We can get this error if we couldn't do a dquot allocation inside
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 02/16] xfs: make sure to really flush all dquots in xfs_qm_quotacheck
2011-11-28 8:27 ` [PATCH 02/16] xfs: make sure to really flush all dquots in xfs_qm_quotacheck Christoph Hellwig
@ 2011-12-05 3:59 ` Dave Chinner
0 siblings, 0 replies; 41+ messages in thread
From: Dave Chinner @ 2011-12-05 3:59 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Nov 28, 2011 at 03:27:24AM -0500, Christoph Hellwig wrote:
> Make sure we do not skip any dquots when flushing them out after a
> quotacheck to make sure that we will never have any dirty dquots on a life
live
> filesystem. At this point no dquot should be pinnable, but lets be anal
pedantic
> about it.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> ---
> fs/xfs/xfs_qm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: xfs/fs/xfs/xfs_qm.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_qm.c 2011-11-25 11:45:32.672075575 +0100
> +++ xfs/fs/xfs/xfs_qm.c 2011-11-25 11:45:35.875391556 +0100
> @@ -1661,7 +1661,7 @@ xfs_qm_quotacheck(
> * successfully.
> */
> if (!error)
> - error = xfs_qm_dqflush_all(mp, SYNC_TRYLOCK);
> + error = xfs_qm_dqflush_all(mp, 0);
>
> /*
> * We can get this error if we couldn't do a dquot allocation inside
We definitely should flush everything here, so looks good.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 03/16] xfs: remove xfs_qm_sync
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
2011-11-28 8:27 ` [PATCH 01/16] xfs: untange SYNC_WAIT and SYNC_TRYLOCK meanings for xfs_qm_dqflush Christoph Hellwig
2011-11-28 8:27 ` [PATCH 02/16] xfs: make sure to really flush all dquots in xfs_qm_quotacheck Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 4:09 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 04/16] xfs: remove the sync_mode argument to xfs_qm_dqflush_all Christoph Hellwig
` (12 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-remove-xfs_qm_sync --]
[-- Type: text/plain, Size: 6539 bytes --]
Now that we can't have any dirty dquots around that aren't in the AIL we
can get rid of the explicit dquot syncing from xfssyncd and xfs_fs_sync_fs.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_qm.c | 94 -----------------------------------------------------
fs/xfs/xfs_qm.h | 6 ---
fs/xfs/xfs_quota.h | 5 --
fs/xfs/xfs_super.c | 11 +-----
fs/xfs/xfs_sync.c | 6 ---
5 files changed, 3 insertions(+), 119 deletions(-)
Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c 2011-11-25 11:45:35.875391556 +0100
+++ xfs/fs/xfs/xfs_qm.c 2011-11-25 11:45:37.468716258 +0100
@@ -879,100 +879,6 @@ xfs_qm_dqdetach(
}
}
-int
-xfs_qm_sync(
- struct xfs_mount *mp,
- int flags)
-{
- struct xfs_quotainfo *q = mp->m_quotainfo;
- int recl, restarts;
- struct xfs_dquot *dqp;
- int error;
-
- if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
- return 0;
-
- restarts = 0;
-
- again:
- mutex_lock(&q->qi_dqlist_lock);
- /*
- * dqpurge_all() also takes the mplist lock and iterate thru all dquots
- * in quotaoff. However, if the QUOTA_ACTIVE bits are not cleared
- * when we have the mplist lock, we know that dquots will be consistent
- * as long as we have it locked.
- */
- if (!XFS_IS_QUOTA_ON(mp)) {
- mutex_unlock(&q->qi_dqlist_lock);
- return 0;
- }
- ASSERT(mutex_is_locked(&q->qi_dqlist_lock));
- list_for_each_entry(dqp, &q->qi_dqlist, q_mplist) {
- /*
- * If this is vfs_sync calling, then skip the dquots that
- * don't 'seem' to be dirty. ie. don't acquire dqlock.
- * This is very similar to what xfs_sync does with inodes.
- */
- if (flags & SYNC_TRYLOCK) {
- if (!XFS_DQ_IS_DIRTY(dqp))
- continue;
- if (!xfs_qm_dqlock_nowait(dqp))
- continue;
- } else {
- xfs_dqlock(dqp);
- }
-
- /*
- * Now, find out for sure if this dquot is dirty or not.
- */
- if (! XFS_DQ_IS_DIRTY(dqp)) {
- xfs_dqunlock(dqp);
- continue;
- }
-
- /* XXX a sentinel would be better */
- recl = q->qi_dqreclaims;
- if (!xfs_dqflock_nowait(dqp)) {
- if (flags & SYNC_TRYLOCK) {
- xfs_dqunlock(dqp);
- continue;
- }
- /*
- * If we can't grab the flush lock then if the caller
- * really wanted us to give this our best shot, so
- * see if we can give a push to the buffer before we wait
- * on the flush lock. At this point, we know that
- * even though the dquot is being flushed,
- * it has (new) dirty data.
- */
- xfs_qm_dqflock_pushbuf_wait(dqp);
- }
- /*
- * Let go of the mplist lock. We don't want to hold it
- * across a disk write
- */
- mutex_unlock(&q->qi_dqlist_lock);
- error = xfs_qm_dqflush(dqp, flags);
- xfs_dqunlock(dqp);
- if (error && XFS_FORCED_SHUTDOWN(mp))
- return 0; /* Need to prevent umount failure */
- else if (error)
- return error;
-
- mutex_lock(&q->qi_dqlist_lock);
- if (recl != q->qi_dqreclaims) {
- if (++restarts >= XFS_QM_SYNC_MAX_RESTARTS)
- break;
-
- mutex_unlock(&q->qi_dqlist_lock);
- goto again;
- }
- }
-
- mutex_unlock(&q->qi_dqlist_lock);
- return 0;
-}
-
/*
* The hash chains and the mplist use the same xfs_dqhash structure as
* their list head, but we can take the mplist qh_lock and one of the
Index: xfs/fs/xfs/xfs_qm.h
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.h 2011-11-05 08:54:00.740993267 +0100
+++ xfs/fs/xfs/xfs_qm.h 2011-11-25 11:45:37.468716258 +0100
@@ -33,12 +33,6 @@ extern kmem_zone_t *qm_dqzone;
extern kmem_zone_t *qm_dqtrxzone;
/*
- * Used in xfs_qm_sync called by xfs_sync to count the max times that it can
- * iterate over the mountpt's dquot list in one call.
- */
-#define XFS_QM_SYNC_MAX_RESTARTS 7
-
-/*
* Ditto, for xfs_qm_dqreclaim_one.
*/
#define XFS_QM_RECLAIM_MAX_RESTARTS 4
Index: xfs/fs/xfs/xfs_quota.h
===================================================================
--- xfs.orig/fs/xfs/xfs_quota.h 2011-11-05 08:54:00.748995021 +0100
+++ xfs/fs/xfs/xfs_quota.h 2011-11-25 11:45:37.468716258 +0100
@@ -326,7 +326,6 @@ extern int xfs_qm_dqattach_locked(struct
extern void xfs_qm_dqdetach(struct xfs_inode *);
extern void xfs_qm_dqrele(struct xfs_dquot *);
extern void xfs_qm_statvfs(struct xfs_inode *, struct kstatfs *);
-extern int xfs_qm_sync(struct xfs_mount *, int);
extern int xfs_qm_newmount(struct xfs_mount *, uint *, uint *);
extern void xfs_qm_mount_quotas(struct xfs_mount *);
extern void xfs_qm_unmount(struct xfs_mount *);
@@ -366,10 +365,6 @@ static inline int xfs_trans_reserve_quot
#define xfs_qm_dqdetach(ip)
#define xfs_qm_dqrele(d)
#define xfs_qm_statvfs(ip, s)
-static inline int xfs_qm_sync(struct xfs_mount *mp, int flags)
-{
- return 0;
-}
#define xfs_qm_newmount(mp, a, b) (0)
#define xfs_qm_mount_quotas(mp)
#define xfs_qm_unmount(mp)
Index: xfs/fs/xfs/xfs_super.c
===================================================================
--- xfs.orig/fs/xfs/xfs_super.c 2011-11-25 11:43:20.222793115 +0100
+++ xfs/fs/xfs/xfs_super.c 2011-11-25 11:45:37.472049573 +0100
@@ -1025,17 +1025,10 @@ xfs_fs_sync_fs(
int error;
/*
- * Not much we can do for the first async pass. Writing out the
- * superblock would be counter-productive as we are going to redirty
- * when writing out other data and metadata (and writing out a single
- * block is quite fast anyway).
- *
- * Try to asynchronously kick off quota syncing at least.
+ * Doing anything during the async pass would be counterproductive.
*/
- if (!wait) {
- xfs_qm_sync(mp, SYNC_TRYLOCK);
+ if (!wait)
return 0;
- }
error = xfs_quiesce_data(mp);
if (error)
Index: xfs/fs/xfs/xfs_sync.c
===================================================================
--- xfs.orig/fs/xfs/xfs_sync.c 2011-11-24 13:44:18.138524837 +0100
+++ xfs/fs/xfs/xfs_sync.c 2011-11-25 11:45:37.472049573 +0100
@@ -359,10 +359,7 @@ xfs_quiesce_data(
{
int error, error2 = 0;
- xfs_qm_sync(mp, SYNC_TRYLOCK);
- xfs_qm_sync(mp, SYNC_WAIT);
-
- /* force out the newly dirtied log buffers */
+ /* force out the log */
xfs_log_force(mp, XFS_LOG_SYNC);
/* write superblock and hoover up shutdown errors */
@@ -470,7 +467,6 @@ xfs_sync_worker(
error = xfs_fs_log_dummy(mp);
else
xfs_log_force(mp, 0);
- error = xfs_qm_sync(mp, SYNC_TRYLOCK);
/* start pushing all the metadata that is currently dirty */
xfs_ail_push_all(mp->m_ail);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 03/16] xfs: remove xfs_qm_sync
2011-11-28 8:27 ` [PATCH 03/16] xfs: remove xfs_qm_sync Christoph Hellwig
@ 2011-12-05 4:09 ` Dave Chinner
0 siblings, 0 replies; 41+ messages in thread
From: Dave Chinner @ 2011-12-05 4:09 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Nov 28, 2011 at 03:27:25AM -0500, Christoph Hellwig wrote:
> Now that we can't have any dirty dquots around that aren't in the AIL we
> can get rid of the explicit dquot syncing from xfssyncd and xfs_fs_sync_fs.
Makes sense - but it does mean that we now completely rely on log
recovery to be correct when someone issues a sync from userspace to
ensure quotas are consistent after a crash. maybe that is worth
mentioning in the commit message....
Otherwise,
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 04/16] xfs: remove the sync_mode argument to xfs_qm_dqflush_all
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
` (2 preceding siblings ...)
2011-11-28 8:27 ` [PATCH 03/16] xfs: remove xfs_qm_sync Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 4:09 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 05/16] xfs: cleanup dquot locking helpers Christoph Hellwig
` (11 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-remove-xfs_qm_dqflush_all-sync-mode --]
[-- Type: text/plain, Size: 1215 bytes --]
It always is zero, and removing it will make future changes easier.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_qm.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c 2011-11-25 11:45:37.468716258 +0100
+++ xfs/fs/xfs/xfs_qm.c 2011-11-25 11:45:39.375372594 +0100
@@ -415,8 +415,7 @@ xfs_qm_unmount_quotas(
*/
STATIC int
xfs_qm_dqflush_all(
- struct xfs_mount *mp,
- int sync_mode)
+ struct xfs_mount *mp)
{
struct xfs_quotainfo *q = mp->m_quotainfo;
int recl;
@@ -451,7 +450,7 @@ again:
* across a disk write.
*/
mutex_unlock(&q->qi_dqlist_lock);
- error = xfs_qm_dqflush(dqp, sync_mode);
+ error = xfs_qm_dqflush(dqp, 0);
xfs_dqunlock(dqp);
if (error)
return error;
@@ -1567,7 +1566,7 @@ xfs_qm_quotacheck(
* successfully.
*/
if (!error)
- error = xfs_qm_dqflush_all(mp, 0);
+ error = xfs_qm_dqflush_all(mp);
/*
* We can get this error if we couldn't do a dquot allocation inside
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* [PATCH 05/16] xfs: cleanup dquot locking helpers
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
` (3 preceding siblings ...)
2011-11-28 8:27 ` [PATCH 04/16] xfs: remove the sync_mode argument to xfs_qm_dqflush_all Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 4:12 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 06/16] xfs: cleanup xfs_qm_dqlookup Christoph Hellwig
` (10 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-quota-clenaup-locking-helpers --]
[-- Type: text/plain, Size: 4708 bytes --]
Mark the trivial lock wrappers as inline, and make the naming consistent
for all of them.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_dquot.c | 31 ++++---------------------------
fs/xfs/xfs_dquot.h | 25 +++++++++++++++++++------
fs/xfs/xfs_dquot_item.c | 2 +-
fs/xfs/xfs_qm.c | 2 +-
4 files changed, 25 insertions(+), 35 deletions(-)
Index: xfs/fs/xfs/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.c 2011-11-25 11:45:32.668742260 +0100
+++ xfs/fs/xfs/xfs_dquot.c 2011-11-25 11:45:41.458694642 +0100
@@ -1257,40 +1257,17 @@ xfs_qm_dqflush(
}
-int
-xfs_qm_dqlock_nowait(
- xfs_dquot_t *dqp)
-{
- return mutex_trylock(&dqp->q_qlock);
-}
-
-void
-xfs_dqlock(
- xfs_dquot_t *dqp)
-{
- mutex_lock(&dqp->q_qlock);
-}
-
void
xfs_dqunlock(
xfs_dquot_t *dqp)
{
- mutex_unlock(&(dqp->q_qlock));
+ xfs_dqunlock_nonotify(dqp);
if (dqp->q_logitem.qli_dquot == dqp) {
- /* Once was dqp->q_mount, but might just have been cleared */
xfs_trans_unlocked_item(dqp->q_logitem.qli_item.li_ailp,
- (xfs_log_item_t*)&(dqp->q_logitem));
+ &dqp->q_logitem.qli_item);
}
}
-
-void
-xfs_dqunlock_nonotify(
- xfs_dquot_t *dqp)
-{
- mutex_unlock(&(dqp->q_qlock));
-}
-
/*
* Lock two xfs_dquot structures.
*
@@ -1370,7 +1347,7 @@ xfs_qm_dqpurge(
* Block on the flush lock after nudging dquot buffer,
* if it is incore.
*/
- xfs_qm_dqflock_pushbuf_wait(dqp);
+ xfs_dqflock_pushbuf_wait(dqp);
}
/*
@@ -1427,7 +1404,7 @@ xfs_qm_dqpurge(
* wait on the flush lock.
*/
void
-xfs_qm_dqflock_pushbuf_wait(
+xfs_dqflock_pushbuf_wait(
xfs_dquot_t *dqp)
{
xfs_mount_t *mp = dqp->q_mount;
Index: xfs/fs/xfs/xfs_dquot.h
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.h 2011-11-05 08:54:00.492993239 +0100
+++ xfs/fs/xfs/xfs_dquot.h 2011-11-25 11:45:41.458694642 +0100
@@ -102,6 +102,21 @@ static inline void xfs_dqfunlock(xfs_dqu
complete(&dqp->q_flush);
}
+static inline int xfs_dqlock_nowait(struct xfs_dquot *dqp)
+{
+ return mutex_trylock(&dqp->q_qlock);
+}
+
+static inline void xfs_dqlock(struct xfs_dquot *dqp)
+{
+ mutex_lock(&dqp->q_qlock);
+}
+
+static inline void xfs_dqunlock_nonotify(struct xfs_dquot *dqp)
+{
+ mutex_unlock(&dqp->q_qlock);
+}
+
#define XFS_DQ_IS_LOCKED(dqp) (mutex_is_locked(&((dqp)->q_qlock)))
#define XFS_DQ_IS_DIRTY(dqp) ((dqp)->dq_flags & XFS_DQ_DIRTY)
#define XFS_QM_ISUDQ(dqp) ((dqp)->dq_flags & XFS_DQ_USER)
@@ -120,8 +135,6 @@ extern void xfs_qm_dqdestroy(xfs_dquot_
extern int xfs_qm_dqflush(xfs_dquot_t *, uint);
extern int xfs_qm_dqpurge(xfs_dquot_t *);
extern void xfs_qm_dqunpin_wait(xfs_dquot_t *);
-extern int xfs_qm_dqlock_nowait(xfs_dquot_t *);
-extern void xfs_qm_dqflock_pushbuf_wait(xfs_dquot_t *dqp);
extern void xfs_qm_adjust_dqtimers(xfs_mount_t *,
xfs_disk_dquot_t *);
extern void xfs_qm_adjust_dqlimits(xfs_mount_t *,
@@ -129,9 +142,9 @@ extern void xfs_qm_adjust_dqlimits(xfs_
extern int xfs_qm_dqget(xfs_mount_t *, xfs_inode_t *,
xfs_dqid_t, uint, uint, xfs_dquot_t **);
extern void xfs_qm_dqput(xfs_dquot_t *);
-extern void xfs_dqlock(xfs_dquot_t *);
-extern void xfs_dqlock2(xfs_dquot_t *, xfs_dquot_t *);
-extern void xfs_dqunlock(xfs_dquot_t *);
-extern void xfs_dqunlock_nonotify(xfs_dquot_t *);
+
+extern void xfs_dqlock2(struct xfs_dquot *, struct xfs_dquot *);
+extern void xfs_dqunlock(struct xfs_dquot *);
+extern void xfs_dqflock_pushbuf_wait(struct xfs_dquot *dqp);
#endif /* __XFS_DQUOT_H__ */
Index: xfs/fs/xfs/xfs_dquot_item.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot_item.c 2011-11-25 11:45:32.668742260 +0100
+++ xfs/fs/xfs/xfs_dquot_item.c 2011-11-25 11:45:41.462027957 +0100
@@ -236,7 +236,7 @@ xfs_qm_dquot_logitem_trylock(
if (atomic_read(&dqp->q_pincount) > 0)
return XFS_ITEM_PINNED;
- if (!xfs_qm_dqlock_nowait(dqp))
+ if (!xfs_dqlock_nowait(dqp))
return XFS_ITEM_LOCKED;
if (!xfs_dqflock_nowait(dqp)) {
Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c 2011-11-25 11:45:39.375372594 +0100
+++ xfs/fs/xfs/xfs_qm.c 2011-11-25 11:45:41.462027957 +0100
@@ -443,7 +443,7 @@ again:
* out immediately. We'll be able to acquire
* the flush lock when the I/O completes.
*/
- xfs_qm_dqflock_pushbuf_wait(dqp);
+ xfs_dqflock_pushbuf_wait(dqp);
}
/*
* Let go of the mplist lock. We don't want to hold it
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* [PATCH 06/16] xfs: cleanup xfs_qm_dqlookup
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
` (4 preceding siblings ...)
2011-11-28 8:27 ` [PATCH 05/16] xfs: cleanup dquot locking helpers Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 4:17 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 07/16] xfs: remove XFS_DQ_INACTIVE Christoph Hellwig
` (9 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-cleanup-xfs_qm_dqlookup --]
[-- Type: text/plain, Size: 1829 bytes --]
Rearrange the code to avoid the conditional locking around the flist_locked
variable. This means we lose a (rather pointless) assert, and hold the
freelist lock a bit longer for one corner case.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_dquot.c | 25 +++++--------------------
1 file changed, 5 insertions(+), 20 deletions(-)
Index: xfs/fs/xfs/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.c 2011-10-27 22:40:02.250173174 +0200
+++ xfs/fs/xfs/xfs_dquot.c 2011-10-27 22:40:02.770171231 +0200
@@ -710,12 +710,9 @@ xfs_qm_dqlookup(
xfs_dquot_t **O_dqpp)
{
xfs_dquot_t *dqp;
- uint flist_locked;
ASSERT(mutex_is_locked(&qh->qh_lock));
- flist_locked = B_FALSE;
-
/*
* Traverse the hashchain looking for a match
*/
@@ -750,31 +747,19 @@ xfs_qm_dqlookup(
xfs_dqlock(dqp);
dqp->dq_flags &= ~(XFS_DQ_WANT);
}
- flist_locked = B_TRUE;
- }
-
- /*
- * id couldn't have changed; we had the hashlock all
- * along
- */
- ASSERT(be32_to_cpu(dqp->q_core.d_id) == id);
- if (flist_locked) {
- if (dqp->q_nrefs != 0) {
- mutex_unlock(&xfs_Gqm->qm_dqfrlist_lock);
- flist_locked = B_FALSE;
- } else {
+ if (dqp->q_nrefs == 0) {
/* take it off the freelist */
trace_xfs_dqlookup_freelist(dqp);
list_del_init(&dqp->q_freelist);
xfs_Gqm->qm_dqfrlist_cnt--;
}
+ XFS_DQHOLD(dqp);
+ mutex_unlock(&xfs_Gqm->qm_dqfrlist_lock);
+ } else {
+ XFS_DQHOLD(dqp);
}
- XFS_DQHOLD(dqp);
-
- if (flist_locked)
- mutex_unlock(&xfs_Gqm->qm_dqfrlist_lock);
/*
* move the dquot to the front of the hashchain
*/
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 06/16] xfs: cleanup xfs_qm_dqlookup
2011-11-28 8:27 ` [PATCH 06/16] xfs: cleanup xfs_qm_dqlookup Christoph Hellwig
@ 2011-12-05 4:17 ` Dave Chinner
0 siblings, 0 replies; 41+ messages in thread
From: Dave Chinner @ 2011-12-05 4:17 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Nov 28, 2011 at 03:27:28AM -0500, Christoph Hellwig wrote:
> Rearrange the code to avoid the conditional locking around the flist_locked
> variable. This means we lose a (rather pointless) assert, and hold the
> freelist lock a bit longer for one corner case.
Hardly worth mentioning that corner case - all we do is hold the
mutex while doing a XFS_DQHOLD() instead of dropping it first, which
we do for the other case, anyway.
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Otherwise,
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 07/16] xfs: remove XFS_DQ_INACTIVE
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
` (5 preceding siblings ...)
2011-11-28 8:27 ` [PATCH 06/16] xfs: cleanup xfs_qm_dqlookup Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 4:23 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 08/16] xfs: implement lazy removal for the dquot freelist Christoph Hellwig
` (8 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-kill-XFS_DQ_INACTIVE --]
[-- Type: text/plain, Size: 5710 bytes --]
Free dquots when purging them during umount instead of keeping them around
on the freelist in a degraded state. The out of order locking in
xfs_qm_dqpurge will be removed again later in this series.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_dquot.c | 25 +++++++++++++--------
fs/xfs/xfs_qm.c | 61 +++++------------------------------------------------
fs/xfs/xfs_quota.h | 4 ---
3 files changed, 23 insertions(+), 67 deletions(-)
Index: xfs/fs/xfs/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.c 2011-11-25 11:45:42.948686569 +0100
+++ xfs/fs/xfs/xfs_dquot.c 2011-11-25 11:45:44.232012950 +0100
@@ -1302,6 +1302,14 @@ xfs_qm_dqpurge(
ASSERT(mutex_is_locked(&mp->m_quotainfo->qi_dqlist_lock));
ASSERT(mutex_is_locked(&dqp->q_hash->qh_lock));
+ /*
+ * XXX(hch): horrible locking order, will get cleaned up ASAP.
+ */
+ if (!mutex_trylock(&xfs_Gqm->qm_dqfrlist_lock)) {
+ mutex_unlock(&dqp->q_hash->qh_lock);
+ return 1;
+ }
+
xfs_dqlock(dqp);
/*
* We really can't afford to purge a dquot that is
@@ -1364,22 +1372,21 @@ xfs_qm_dqpurge(
list_del_init(&dqp->q_hashlist);
qh->qh_version++;
+
list_del_init(&dqp->q_mplist);
mp->m_quotainfo->qi_dqreclaims++;
mp->m_quotainfo->qi_dquots--;
- /*
- * XXX Move this to the front of the freelist, if we can get the
- * freelist lock.
- */
- ASSERT(!list_empty(&dqp->q_freelist));
- dqp->q_mount = NULL;
- dqp->q_hash = NULL;
- dqp->dq_flags = XFS_DQ_INACTIVE;
- memset(&dqp->q_core, 0, sizeof(dqp->q_core));
+ list_del_init(&dqp->q_freelist);
+ xfs_Gqm->qm_dqfrlist_cnt--;
+
xfs_dqfunlock(dqp);
xfs_dqunlock(dqp);
+
+ mutex_unlock(&xfs_Gqm->qm_dqfrlist_lock);
mutex_unlock(&qh->qh_lock);
+
+ xfs_qm_dqdestroy(dqp);
return (0);
}
Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c 2011-11-25 11:45:41.462027957 +0100
+++ xfs/fs/xfs/xfs_qm.c 2011-11-25 11:45:44.235346266 +0100
@@ -154,12 +154,17 @@ STATIC void
xfs_qm_destroy(
struct xfs_qm *xqm)
{
- struct xfs_dquot *dqp, *n;
int hsize, i;
ASSERT(xqm != NULL);
ASSERT(xqm->qm_nrefs == 0);
+
unregister_shrinker(&xfs_qm_shaker);
+
+ mutex_lock(&xqm->qm_dqfrlist_lock);
+ ASSERT(list_empty(&xqm->qm_dqfrlist));
+ mutex_unlock(&xqm->qm_dqfrlist_lock);
+
hsize = xqm->qm_dqhashmask + 1;
for (i = 0; i < hsize; i++) {
xfs_qm_list_destroy(&(xqm->qm_usr_dqhtable[i]));
@@ -171,17 +176,6 @@ xfs_qm_destroy(
xqm->qm_grp_dqhtable = NULL;
xqm->qm_dqhashmask = 0;
- /* frlist cleanup */
- mutex_lock(&xqm->qm_dqfrlist_lock);
- list_for_each_entry_safe(dqp, n, &xqm->qm_dqfrlist, q_freelist) {
- xfs_dqlock(dqp);
- list_del_init(&dqp->q_freelist);
- xfs_Gqm->qm_dqfrlist_cnt--;
- xfs_dqunlock(dqp);
- xfs_qm_dqdestroy(dqp);
- }
- mutex_unlock(&xqm->qm_dqfrlist_lock);
- mutex_destroy(&xqm->qm_dqfrlist_lock);
kmem_free(xqm);
}
@@ -232,34 +226,10 @@ STATIC void
xfs_qm_rele_quotafs_ref(
struct xfs_mount *mp)
{
- xfs_dquot_t *dqp, *n;
-
ASSERT(xfs_Gqm);
ASSERT(xfs_Gqm->qm_nrefs > 0);
/*
- * Go thru the freelist and destroy all inactive dquots.
- */
- mutex_lock(&xfs_Gqm->qm_dqfrlist_lock);
-
- list_for_each_entry_safe(dqp, n, &xfs_Gqm->qm_dqfrlist, q_freelist) {
- xfs_dqlock(dqp);
- if (dqp->dq_flags & XFS_DQ_INACTIVE) {
- ASSERT(dqp->q_mount == NULL);
- ASSERT(! XFS_DQ_IS_DIRTY(dqp));
- ASSERT(list_empty(&dqp->q_hashlist));
- ASSERT(list_empty(&dqp->q_mplist));
- list_del_init(&dqp->q_freelist);
- xfs_Gqm->qm_dqfrlist_cnt--;
- xfs_dqunlock(dqp);
- xfs_qm_dqdestroy(dqp);
- } else {
- xfs_dqunlock(dqp);
- }
- }
- mutex_unlock(&xfs_Gqm->qm_dqfrlist_lock);
-
- /*
* Destroy the entire XQM. If somebody mounts with quotaon, this'll
* be restarted.
*/
@@ -1728,8 +1698,6 @@ again:
* both the dquot and the freelistlock.
*/
if (dqp->dq_flags & XFS_DQ_WANT) {
- ASSERT(! (dqp->dq_flags & XFS_DQ_INACTIVE));
-
trace_xfs_dqreclaim_want(dqp);
XQM_STATS_INC(xqmstats.xs_qm_dqwants);
restarts++;
@@ -1737,23 +1705,6 @@ again:
goto dqunlock;
}
- /*
- * If the dquot is inactive, we are assured that it is
- * not on the mplist or the hashlist, and that makes our
- * life easier.
- */
- if (dqp->dq_flags & XFS_DQ_INACTIVE) {
- ASSERT(mp == NULL);
- ASSERT(! XFS_DQ_IS_DIRTY(dqp));
- ASSERT(list_empty(&dqp->q_hashlist));
- ASSERT(list_empty(&dqp->q_mplist));
- list_del_init(&dqp->q_freelist);
- xfs_Gqm->qm_dqfrlist_cnt--;
- dqpout = dqp;
- XQM_STATS_INC(xqmstats.xs_qm_dqinact_reclaims);
- goto dqunlock;
- }
-
ASSERT(dqp->q_hash);
ASSERT(!list_empty(&dqp->q_mplist));
Index: xfs/fs/xfs/xfs_quota.h
===================================================================
--- xfs.orig/fs/xfs/xfs_quota.h 2011-11-25 11:45:37.468716258 +0100
+++ xfs/fs/xfs/xfs_quota.h 2011-11-25 11:45:44.235346266 +0100
@@ -88,7 +88,6 @@ typedef struct xfs_dqblk {
#define XFS_DQ_GROUP 0x0004 /* a group quota */
#define XFS_DQ_DIRTY 0x0008 /* dquot is dirty */
#define XFS_DQ_WANT 0x0010 /* for lookup/reclaim race */
-#define XFS_DQ_INACTIVE 0x0020 /* dq off mplist & hashlist */
#define XFS_DQ_ALLTYPES (XFS_DQ_USER|XFS_DQ_PROJ|XFS_DQ_GROUP)
@@ -97,8 +96,7 @@ typedef struct xfs_dqblk {
{ XFS_DQ_PROJ, "PROJ" }, \
{ XFS_DQ_GROUP, "GROUP" }, \
{ XFS_DQ_DIRTY, "DIRTY" }, \
- { XFS_DQ_WANT, "WANT" }, \
- { XFS_DQ_INACTIVE, "INACTIVE" }
+ { XFS_DQ_WANT, "WANT" }
/*
* In the worst case, when both user and group quotas are on,
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 07/16] xfs: remove XFS_DQ_INACTIVE
2011-11-28 8:27 ` [PATCH 07/16] xfs: remove XFS_DQ_INACTIVE Christoph Hellwig
@ 2011-12-05 4:23 ` Dave Chinner
2011-12-05 8:37 ` Christoph Hellwig
0 siblings, 1 reply; 41+ messages in thread
From: Dave Chinner @ 2011-12-05 4:23 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Nov 28, 2011 at 03:27:29AM -0500, Christoph Hellwig wrote:
> Free dquots when purging them during umount instead of keeping them around
> on the freelist in a degraded state. The out of order locking in
> xfs_qm_dqpurge will be removed again later in this series.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> - dqp->q_mount = NULL;
> - dqp->q_hash = NULL;
> - dqp->dq_flags = XFS_DQ_INACTIVE;
> - memset(&dqp->q_core, 0, sizeof(dqp->q_core));
> + list_del_init(&dqp->q_freelist);
> + xfs_Gqm->qm_dqfrlist_cnt--;
> +
> xfs_dqfunlock(dqp);
> xfs_dqunlock(dqp);
> +
> + mutex_unlock(&xfs_Gqm->qm_dqfrlist_lock);
> mutex_unlock(&qh->qh_lock);
> +
> + xfs_qm_dqdestroy(dqp);
> return (0);
> }
While there, you may as well make that a "return 0;"
> @@ -171,17 +176,6 @@ xfs_qm_destroy(
> xqm->qm_grp_dqhtable = NULL;
> xqm->qm_dqhashmask = 0;
>
> - /* frlist cleanup */
> - mutex_lock(&xqm->qm_dqfrlist_lock);
> - list_for_each_entry_safe(dqp, n, &xqm->qm_dqfrlist, q_freelist) {
> - xfs_dqlock(dqp);
> - list_del_init(&dqp->q_freelist);
> - xfs_Gqm->qm_dqfrlist_cnt--;
> - xfs_dqunlock(dqp);
> - xfs_qm_dqdestroy(dqp);
> - }
> - mutex_unlock(&xqm->qm_dqfrlist_lock);
> - mutex_destroy(&xqm->qm_dqfrlist_lock);
> kmem_free(xqm);
> }
Don't we still need that mutex_destroy() call there?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 07/16] xfs: remove XFS_DQ_INACTIVE
2011-12-05 4:23 ` Dave Chinner
@ 2011-12-05 8:37 ` Christoph Hellwig
2011-12-06 14:43 ` Christoph Hellwig
0 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-12-05 8:37 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Mon, Dec 05, 2011 at 03:23:51PM +1100, Dave Chinner wrote:
> > + xfs_qm_dqdestroy(dqp);
> > return (0);
> > }
>
> While there, you may as well make that a "return 0;"
Indeed.
> > - mutex_unlock(&xqm->qm_dqfrlist_lock);
> > - mutex_destroy(&xqm->qm_dqfrlist_lock);
> > kmem_free(xqm);
> > }
>
> Don't we still need that mutex_destroy() call there?
We never needed it - Linux does an implicit mutex_destory when freeing
memory containing a mutex.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 07/16] xfs: remove XFS_DQ_INACTIVE
2011-12-05 8:37 ` Christoph Hellwig
@ 2011-12-06 14:43 ` Christoph Hellwig
2011-12-06 20:34 ` Dave Chinner
0 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-12-06 14:43 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Mon, Dec 05, 2011 at 03:37:41AM -0500, Christoph Hellwig wrote:
> On Mon, Dec 05, 2011 at 03:23:51PM +1100, Dave Chinner wrote:
> > > + xfs_qm_dqdestroy(dqp);
> > > return (0);
> > > }
> >
> > While there, you may as well make that a "return 0;"
>
> Indeed.
>
> > > - mutex_unlock(&xqm->qm_dqfrlist_lock);
> > > - mutex_destroy(&xqm->qm_dqfrlist_lock);
> > > kmem_free(xqm);
> > > }
> >
> > Don't we still need that mutex_destroy() call there?
>
> We never needed it - Linux does an implicit mutex_destory when freeing
> memory containing a mutex.
Does this count as a revied-by now?
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 07/16] xfs: remove XFS_DQ_INACTIVE
2011-12-06 14:43 ` Christoph Hellwig
@ 2011-12-06 20:34 ` Dave Chinner
0 siblings, 0 replies; 41+ messages in thread
From: Dave Chinner @ 2011-12-06 20:34 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Tue, Dec 06, 2011 at 09:43:50AM -0500, Christoph Hellwig wrote:
> On Mon, Dec 05, 2011 at 03:37:41AM -0500, Christoph Hellwig wrote:
> > On Mon, Dec 05, 2011 at 03:23:51PM +1100, Dave Chinner wrote:
> > > > + xfs_qm_dqdestroy(dqp);
> > > > return (0);
> > > > }
> > >
> > > While there, you may as well make that a "return 0;"
> >
> > Indeed.
> >
> > > > - mutex_unlock(&xqm->qm_dqfrlist_lock);
> > > > - mutex_destroy(&xqm->qm_dqfrlist_lock);
> > > > kmem_free(xqm);
> > > > }
> > >
> > > Don't we still need that mutex_destroy() call there?
> >
> > We never needed it - Linux does an implicit mutex_destory when freeing
> > memory containing a mutex.
>
> Does this count as a revied-by now?
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 08/16] xfs: implement lazy removal for the dquot freelist
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
` (6 preceding siblings ...)
2011-11-28 8:27 ` [PATCH 07/16] xfs: remove XFS_DQ_INACTIVE Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 4:32 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 09/16] xfs: flatten the dquot lock ordering Christoph Hellwig
` (7 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-quota-lazy-lru --]
[-- Type: text/plain, Size: 6130 bytes --]
Do not remove dquots from the freelist when we grab a reference to them in
xfs_qm_dqlookup, but leave them on the freelist util scanning notices that
they have a reference. This speeds up the lookup fastpath, and greatly
simplifies the lock ordering constraints. Note that the same scheme is
used by the VFS inode and dentry caches.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_dquot.c | 65 +++++++++++++----------------------------------------
fs/xfs/xfs_qm.c | 22 ++++++++---------
fs/xfs/xfs_quota.h | 4 ---
fs/xfs/xfs_trace.h | 2 -
4 files changed, 29 insertions(+), 64 deletions(-)
Index: xfs/fs/xfs/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.c 2011-11-25 11:45:44.232012950 +0100
+++ xfs/fs/xfs/xfs_dquot.c 2011-11-25 11:45:50.205313924 +0100
@@ -722,58 +722,25 @@ xfs_qm_dqlookup(
* dqlock to look at the id field of the dquot, since the
* id can't be modified without the hashlock anyway.
*/
- if (be32_to_cpu(dqp->q_core.d_id) == id && dqp->q_mount == mp) {
- trace_xfs_dqlookup_found(dqp);
+ if (be32_to_cpu(dqp->q_core.d_id) != id || dqp->q_mount != mp)
+ continue;
- /*
- * All in core dquots must be on the dqlist of mp
- */
- ASSERT(!list_empty(&dqp->q_mplist));
+ trace_xfs_dqlookup_found(dqp);
- xfs_dqlock(dqp);
- if (dqp->q_nrefs == 0) {
- ASSERT(!list_empty(&dqp->q_freelist));
- if (!mutex_trylock(&xfs_Gqm->qm_dqfrlist_lock)) {
- trace_xfs_dqlookup_want(dqp);
-
- /*
- * We may have raced with dqreclaim_one()
- * (and lost). So, flag that we don't
- * want the dquot to be reclaimed.
- */
- dqp->dq_flags |= XFS_DQ_WANT;
- xfs_dqunlock(dqp);
- mutex_lock(&xfs_Gqm->qm_dqfrlist_lock);
- xfs_dqlock(dqp);
- dqp->dq_flags &= ~(XFS_DQ_WANT);
- }
-
- if (dqp->q_nrefs == 0) {
- /* take it off the freelist */
- trace_xfs_dqlookup_freelist(dqp);
- list_del_init(&dqp->q_freelist);
- xfs_Gqm->qm_dqfrlist_cnt--;
- }
- XFS_DQHOLD(dqp);
- mutex_unlock(&xfs_Gqm->qm_dqfrlist_lock);
- } else {
- XFS_DQHOLD(dqp);
- }
+ xfs_dqlock(dqp);
+ XFS_DQHOLD(dqp);
- /*
- * move the dquot to the front of the hashchain
- */
- ASSERT(mutex_is_locked(&qh->qh_lock));
- list_move(&dqp->q_hashlist, &qh->qh_list);
- trace_xfs_dqlookup_done(dqp);
- *O_dqpp = dqp;
- return 0;
- }
+ /*
+ * move the dquot to the front of the hashchain
+ */
+ list_move(&dqp->q_hashlist, &qh->qh_list);
+ trace_xfs_dqlookup_done(dqp);
+ *O_dqpp = dqp;
+ return 0;
}
*O_dqpp = NULL;
- ASSERT(mutex_is_locked(&qh->qh_lock));
- return (1);
+ return 1;
}
/*
@@ -1033,8 +1000,10 @@ xfs_qm_dqput(
if (--dqp->q_nrefs == 0) {
trace_xfs_dqput_free(dqp);
- list_add_tail(&dqp->q_freelist, &xfs_Gqm->qm_dqfrlist);
- xfs_Gqm->qm_dqfrlist_cnt++;
+ if (list_empty(&dqp->q_freelist)) {
+ list_add_tail(&dqp->q_freelist, &xfs_Gqm->qm_dqfrlist);
+ xfs_Gqm->qm_dqfrlist_cnt++;
+ }
/*
* If we just added a udquot to the freelist, then
Index: xfs/fs/xfs/xfs_trace.h
===================================================================
--- xfs.orig/fs/xfs/xfs_trace.h 2011-11-24 13:44:18.568522508 +0100
+++ xfs/fs/xfs/xfs_trace.h 2011-11-25 11:45:46.102002820 +0100
@@ -743,8 +743,6 @@ DEFINE_DQUOT_EVENT(xfs_dqtobp_read);
DEFINE_DQUOT_EVENT(xfs_dqread);
DEFINE_DQUOT_EVENT(xfs_dqread_fail);
DEFINE_DQUOT_EVENT(xfs_dqlookup_found);
-DEFINE_DQUOT_EVENT(xfs_dqlookup_want);
-DEFINE_DQUOT_EVENT(xfs_dqlookup_freelist);
DEFINE_DQUOT_EVENT(xfs_dqlookup_done);
DEFINE_DQUOT_EVENT(xfs_dqget_hit);
DEFINE_DQUOT_EVENT(xfs_dqget_miss);
Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c 2011-11-25 11:45:44.235346266 +0100
+++ xfs/fs/xfs/xfs_qm.c 2011-11-25 11:45:50.215313871 +0100
@@ -517,13 +517,12 @@ xfs_qm_dqpurge_int(
* get them off mplist and hashlist, but leave them on freelist.
*/
list_for_each_entry_safe(dqp, n, &q->qi_dqlist, q_mplist) {
- /*
- * It's OK to look at the type without taking dqlock here.
- * We're holding the mplist lock here, and that's needed for
- * a dqreclaim.
- */
- if ((dqp->dq_flags & dqtype) == 0)
+ xfs_dqlock(dqp);
+ if ((dqp->dq_flags & dqtype) == 0) {
+ xfs_dqunlock(dqp);
continue;
+ }
+ xfs_dqunlock(dqp);
if (!mutex_trylock(&dqp->q_hash->qh_lock)) {
nrecl = q->qi_dqreclaims;
@@ -1692,14 +1691,15 @@ again:
xfs_dqlock(dqp);
/*
- * We are racing with dqlookup here. Naturally we don't
- * want to reclaim a dquot that lookup wants. We release the
- * freelist lock and start over, so that lookup will grab
- * both the dquot and the freelistlock.
+ * This dquot has already been grabbed by dqlookup.
+ * Remove it from the freelist and try again.
*/
- if (dqp->dq_flags & XFS_DQ_WANT) {
+ if (dqp->q_nrefs) {
trace_xfs_dqreclaim_want(dqp);
XQM_STATS_INC(xqmstats.xs_qm_dqwants);
+
+ list_del_init(&dqp->q_freelist);
+ xfs_Gqm->qm_dqfrlist_cnt--;
restarts++;
startagain = 1;
goto dqunlock;
Index: xfs/fs/xfs/xfs_quota.h
===================================================================
--- xfs.orig/fs/xfs/xfs_quota.h 2011-11-25 11:45:44.235346266 +0100
+++ xfs/fs/xfs/xfs_quota.h 2011-11-25 11:45:50.228647131 +0100
@@ -87,7 +87,6 @@ typedef struct xfs_dqblk {
#define XFS_DQ_PROJ 0x0002 /* project quota */
#define XFS_DQ_GROUP 0x0004 /* a group quota */
#define XFS_DQ_DIRTY 0x0008 /* dquot is dirty */
-#define XFS_DQ_WANT 0x0010 /* for lookup/reclaim race */
#define XFS_DQ_ALLTYPES (XFS_DQ_USER|XFS_DQ_PROJ|XFS_DQ_GROUP)
@@ -95,8 +94,7 @@ typedef struct xfs_dqblk {
{ XFS_DQ_USER, "USER" }, \
{ XFS_DQ_PROJ, "PROJ" }, \
{ XFS_DQ_GROUP, "GROUP" }, \
- { XFS_DQ_DIRTY, "DIRTY" }, \
- { XFS_DQ_WANT, "WANT" }
+ { XFS_DQ_DIRTY, "DIRTY" }
/*
* In the worst case, when both user and group quotas are on,
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 08/16] xfs: implement lazy removal for the dquot freelist
2011-11-28 8:27 ` [PATCH 08/16] xfs: implement lazy removal for the dquot freelist Christoph Hellwig
@ 2011-12-05 4:32 ` Dave Chinner
2011-12-05 8:38 ` Christoph Hellwig
0 siblings, 1 reply; 41+ messages in thread
From: Dave Chinner @ 2011-12-05 4:32 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Nov 28, 2011 at 03:27:30AM -0500, Christoph Hellwig wrote:
> Do not remove dquots from the freelist when we grab a reference to them in
> xfs_qm_dqlookup, but leave them on the freelist util scanning notices that
> they have a reference. This speeds up the lookup fastpath, and greatly
> simplifies the lock ordering constraints. Note that the same scheme is
> used by the VFS inode and dentry caches.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
That cleans things up nicely, and should be significantly faster if
the VFS cache examples are anything to go by....
Reviewed-by: Dave Chinner <dchinner@redhat.com>
As an aside:
> + /*
> + * move the dquot to the front of the hashchain
> + */
> + list_move(&dqp->q_hashlist, &qh->qh_list);
> + trace_xfs_dqlookup_done(dqp);
> + *O_dqpp = dqp;
> + return 0;
Back when the inode cache used a hash, we found that this moving of
the item to the front of the list actually slowed down lookups - the
impact of dirtying cachelines (i.e. remote CPU cache invalidation)
to move the item in the list was greater than the time saved during
lookups. That was because that when there are no hash chain
modifications taking place, then the frequently hit chains simply
end up shared in all the cpu caches rather than being turfed out on
every successful lookup on a different CPU....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 08/16] xfs: implement lazy removal for the dquot freelist
2011-12-05 4:32 ` Dave Chinner
@ 2011-12-05 8:38 ` Christoph Hellwig
0 siblings, 0 replies; 41+ messages in thread
From: Christoph Hellwig @ 2011-12-05 8:38 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Mon, Dec 05, 2011 at 03:32:31PM +1100, Dave Chinner wrote:
> > + /*
> > + * move the dquot to the front of the hashchain
> > + */
> > + list_move(&dqp->q_hashlist, &qh->qh_list);
> > + trace_xfs_dqlookup_done(dqp);
> > + *O_dqpp = dqp;
> > + return 0;
>
> Back when the inode cache used a hash, we found that this moving of
> the item to the front of the list actually slowed down lookups - the
> impact of dirtying cachelines (i.e. remote CPU cache invalidation)
> to move the item in the list was greater than the time saved during
> lookups. That was because that when there are no hash chain
> modifications taking place, then the frequently hit chains simply
> end up shared in all the cpu caches rather than being turfed out on
> every successful lookup on a different CPU....
Yes, I doubt having this is an overly good idea. But instead of
spending more time on optimizing the cache I'd prefer getting the
patches to move to a tree in after another merge window or two.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 09/16] xfs: flatten the dquot lock ordering
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
` (7 preceding siblings ...)
2011-11-28 8:27 ` [PATCH 08/16] xfs: implement lazy removal for the dquot freelist Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 5:04 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 10/16] xfs: nest qm_dqfrlist_lock insise the dquot qlock Christoph Hellwig
` (6 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-add-DQ_FREEING --]
[-- Type: text/plain, Size: 13738 bytes --]
Introduce a new XFS_DQ_FREEING flag that tells lookup and mplist walks
to skip a dquot that is beeing freed, and use this avoid the trylock
on the hash and mplist locks in xfs_qm_dqreclaim_one. Also simplify
xfs_dqpurge by moving the inodes to a dispose list after marking them
XFS_DQ_FREEING and avoid the locker ordering constraints.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_dquot.c | 110 ++++++++++++++++++---------------------------
fs/xfs/xfs_dquot.h | 2
fs/xfs/xfs_qm.c | 129 ++++++++++++++++++-----------------------------------
fs/xfs/xfs_quota.h | 4 +
4 files changed, 96 insertions(+), 149 deletions(-)
Index: xfs/fs/xfs/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.c 2011-11-25 11:45:50.205313924 +0100
+++ xfs/fs/xfs/xfs_dquot.c 2011-11-25 11:45:53.348630228 +0100
@@ -728,6 +728,12 @@ xfs_qm_dqlookup(
trace_xfs_dqlookup_found(dqp);
xfs_dqlock(dqp);
+ if (dqp->dq_flags & XFS_DQ_FREEING) {
+ *O_dqpp = NULL;
+ xfs_dqunlock(dqp);
+ return -1;
+ }
+
XFS_DQHOLD(dqp);
/*
@@ -781,11 +787,7 @@ xfs_qm_dqget(
return (EIO);
}
}
-#endif
- again:
-
-#ifdef DEBUG
ASSERT(type == XFS_DQ_USER ||
type == XFS_DQ_PROJ ||
type == XFS_DQ_GROUP);
@@ -797,13 +799,21 @@ xfs_qm_dqget(
ASSERT(ip->i_gdquot == NULL);
}
#endif
+
+restart:
mutex_lock(&h->qh_lock);
/*
* Look in the cache (hashtable).
* The chain is kept locked during lookup.
*/
- if (xfs_qm_dqlookup(mp, id, h, O_dqpp) == 0) {
+ switch (xfs_qm_dqlookup(mp, id, h, O_dqpp)) {
+ case -1:
+ XQM_STATS_INC(xqmstats.xs_qm_dquot_dups);
+ mutex_unlock(&h->qh_lock);
+ delay(1);
+ goto restart;
+ case 0:
XQM_STATS_INC(xqmstats.xs_qm_dqcachehits);
/*
* The dquot was found, moved to the front of the chain,
@@ -814,9 +824,11 @@ xfs_qm_dqget(
ASSERT(XFS_DQ_IS_LOCKED(*O_dqpp));
mutex_unlock(&h->qh_lock);
trace_xfs_dqget_hit(*O_dqpp);
- return (0); /* success */
+ return 0; /* success */
+ default:
+ XQM_STATS_INC(xqmstats.xs_qm_dqcachemisses);
+ break;
}
- XQM_STATS_INC(xqmstats.xs_qm_dqcachemisses);
/*
* Dquot cache miss. We don't want to keep the inode lock across
@@ -913,16 +925,21 @@ xfs_qm_dqget(
* lock order between the two dquots here since dqp isn't
* on any findable lists yet.
*/
- if (xfs_qm_dqlookup(mp, id, h, &tmpdqp) == 0) {
+ switch (xfs_qm_dqlookup(mp, id, h, &tmpdqp)) {
+ case 0:
+ case -1:
/*
- * Duplicate found. Just throw away the new dquot
- * and start over.
+ * Duplicate found, either in cache or on its way out.
+ * Just throw away the new dquot and start over.
*/
- xfs_qm_dqput(tmpdqp);
+ if (tmpdqp)
+ xfs_qm_dqput(tmpdqp);
mutex_unlock(&h->qh_lock);
xfs_qm_dqdestroy(dqp);
XQM_STATS_INC(xqmstats.xs_qm_dquot_dups);
- goto again;
+ goto restart;
+ default:
+ break;
}
}
@@ -1250,51 +1267,18 @@ xfs_dqlock2(
}
}
-
/*
- * Take a dquot out of the mount's dqlist as well as the hashlist.
- * This is called via unmount as well as quotaoff, and the purge
- * will always succeed unless there are soft (temp) references
- * outstanding.
- *
- * This returns 0 if it was purged, 1 if it wasn't. It's not an error code
- * that we're returning! XXXsup - not cool.
+ * Take a dquot out of the mount's dqlist as well as the hashlist. This is
+ * called via unmount as well as quotaoff, and the purge will always succeed.
*/
-/* ARGSUSED */
-int
+void
xfs_qm_dqpurge(
- xfs_dquot_t *dqp)
+ struct xfs_dquot *dqp)
{
- xfs_dqhash_t *qh = dqp->q_hash;
- xfs_mount_t *mp = dqp->q_mount;
-
- ASSERT(mutex_is_locked(&mp->m_quotainfo->qi_dqlist_lock));
- ASSERT(mutex_is_locked(&dqp->q_hash->qh_lock));
-
- /*
- * XXX(hch): horrible locking order, will get cleaned up ASAP.
- */
- if (!mutex_trylock(&xfs_Gqm->qm_dqfrlist_lock)) {
- mutex_unlock(&dqp->q_hash->qh_lock);
- return 1;
- }
+ struct xfs_mount *mp = dqp->q_mount;
+ struct xfs_dqhash *qh = dqp->q_hash;
xfs_dqlock(dqp);
- /*
- * We really can't afford to purge a dquot that is
- * referenced, because these are hard refs.
- * It shouldn't happen in general because we went thru _all_ inodes in
- * dqrele_all_inodes before calling this and didn't let the mountlock go.
- * However it is possible that we have dquots with temporary
- * references that are not attached to an inode. e.g. see xfs_setattr().
- */
- if (dqp->q_nrefs != 0) {
- xfs_dqunlock(dqp);
- mutex_unlock(&dqp->q_hash->qh_lock);
- return (1);
- }
-
- ASSERT(!list_empty(&dqp->q_freelist));
/*
* If we're turning off quotas, we have to make sure that, for
@@ -1313,19 +1297,14 @@ xfs_qm_dqpurge(
}
/*
- * XXXIf we're turning this type of quotas off, we don't care
+ * If we are turning this type of quotas off, we don't care
* about the dirty metadata sitting in this dquot. OTOH, if
* we're unmounting, we do care, so we flush it and wait.
*/
if (XFS_DQ_IS_DIRTY(dqp)) {
int error;
- /* dqflush unlocks dqflock */
/*
- * Given that dqpurge is a very rare occurrence, it is OK
- * that we're holding the hashlist and mplist locks
- * across the disk write. But, ... XXXsup
- *
* We don't care about getting disk errors here. We need
* to purge this dquot anyway, so we go ahead regardless.
*/
@@ -1335,31 +1314,34 @@ xfs_qm_dqpurge(
__func__, dqp);
xfs_dqflock(dqp);
}
+
ASSERT(atomic_read(&dqp->q_pincount) == 0);
ASSERT(XFS_FORCED_SHUTDOWN(mp) ||
!(dqp->q_logitem.qli_item.li_flags & XFS_LI_IN_AIL));
+ xfs_dqfunlock(dqp);
+ xfs_dqunlock(dqp);
+
+ mutex_lock(&qh->qh_lock);
list_del_init(&dqp->q_hashlist);
qh->qh_version++;
+ mutex_unlock(&qh->qh_lock);
+ mutex_lock(&mp->m_quotainfo->qi_dqlist_lock);
list_del_init(&dqp->q_mplist);
mp->m_quotainfo->qi_dqreclaims++;
mp->m_quotainfo->qi_dquots--;
+ mutex_unlock(&mp->m_quotainfo->qi_dqlist_lock);
+ mutex_lock(&xfs_Gqm->qm_dqfrlist_lock);
+ ASSERT(!list_empty(&dqp->q_freelist));
list_del_init(&dqp->q_freelist);
xfs_Gqm->qm_dqfrlist_cnt--;
-
- xfs_dqfunlock(dqp);
- xfs_dqunlock(dqp);
-
mutex_unlock(&xfs_Gqm->qm_dqfrlist_lock);
- mutex_unlock(&qh->qh_lock);
xfs_qm_dqdestroy(dqp);
- return (0);
}
-
/*
* Give the buffer a little push if it is incore and
* wait on the flush lock.
Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c 2011-11-25 11:45:50.215313871 +0100
+++ xfs/fs/xfs/xfs_qm.c 2011-11-25 11:45:53.351963543 +0100
@@ -398,7 +398,8 @@ again:
mutex_lock(&q->qi_dqlist_lock);
list_for_each_entry(dqp, &q->qi_dqlist, q_mplist) {
xfs_dqlock(dqp);
- if (! XFS_DQ_IS_DIRTY(dqp)) {
+ if ((dqp->dq_flags & XFS_DQ_FREEING) ||
+ !XFS_DQ_IS_DIRTY(dqp)) {
xfs_dqunlock(dqp);
continue;
}
@@ -437,6 +438,7 @@ again:
/* return ! busy */
return 0;
}
+
/*
* Release the group dquot pointers the user dquots may be
* carrying around as a hint. mplist is locked on entry and exit.
@@ -453,6 +455,13 @@ xfs_qm_detach_gdquots(
ASSERT(mutex_is_locked(&q->qi_dqlist_lock));
list_for_each_entry(dqp, &q->qi_dqlist, q_mplist) {
xfs_dqlock(dqp);
+ if (dqp->dq_flags & XFS_DQ_FREEING) {
+ xfs_dqunlock(dqp);
+ mutex_unlock(&q->qi_dqlist_lock);
+ delay(1);
+ mutex_lock(&q->qi_dqlist_lock);
+ goto again;
+ }
if ((gdqp = dqp->q_gdquot)) {
xfs_dqlock(gdqp);
dqp->q_gdquot = NULL;
@@ -489,8 +498,8 @@ xfs_qm_dqpurge_int(
struct xfs_quotainfo *q = mp->m_quotainfo;
struct xfs_dquot *dqp, *n;
uint dqtype;
- int nrecl;
- int nmisses;
+ int nmisses = 0;
+ LIST_HEAD (dispose_list);
if (!q)
return 0;
@@ -509,46 +518,27 @@ xfs_qm_dqpurge_int(
*/
xfs_qm_detach_gdquots(mp);
- again:
- nmisses = 0;
- ASSERT(mutex_is_locked(&q->qi_dqlist_lock));
/*
* Try to get rid of all of the unwanted dquots. The idea is to
* get them off mplist and hashlist, but leave them on freelist.
*/
list_for_each_entry_safe(dqp, n, &q->qi_dqlist, q_mplist) {
xfs_dqlock(dqp);
- if ((dqp->dq_flags & dqtype) == 0) {
- xfs_dqunlock(dqp);
- continue;
+ if ((dqp->dq_flags & dqtype) != 0 &&
+ !(dqp->dq_flags & XFS_DQ_FREEING)) {
+ if (dqp->q_nrefs == 0) {
+ dqp->dq_flags |= XFS_DQ_FREEING;
+ list_move_tail(&dqp->q_mplist, &dispose_list);
+ } else
+ nmisses++;
}
xfs_dqunlock(dqp);
-
- if (!mutex_trylock(&dqp->q_hash->qh_lock)) {
- nrecl = q->qi_dqreclaims;
- mutex_unlock(&q->qi_dqlist_lock);
- mutex_lock(&dqp->q_hash->qh_lock);
- mutex_lock(&q->qi_dqlist_lock);
-
- /*
- * XXXTheoretically, we can get into a very long
- * ping pong game here.
- * No one can be adding dquots to the mplist at
- * this point, but somebody might be taking things off.
- */
- if (nrecl != q->qi_dqreclaims) {
- mutex_unlock(&dqp->q_hash->qh_lock);
- goto again;
- }
- }
-
- /*
- * Take the dquot off the mplist and hashlist. It may remain on
- * freelist in INACTIVE state.
- */
- nmisses += xfs_qm_dqpurge(dqp);
}
mutex_unlock(&q->qi_dqlist_lock);
+
+ list_for_each_entry_safe(dqp, n, &dispose_list, q_mplist)
+ xfs_qm_dqpurge(dqp);
+
return nmisses;
}
@@ -1667,25 +1657,16 @@ xfs_qm_init_quotainos(
/*
- * Just pop the least recently used dquot off the freelist and
- * recycle it. The returned dquot is locked.
+ * Pop the least recently used dquot off the freelist and recycle it.
*/
-STATIC xfs_dquot_t *
+STATIC struct xfs_dquot *
xfs_qm_dqreclaim_one(void)
{
- xfs_dquot_t *dqpout;
- xfs_dquot_t *dqp;
- int restarts;
- int startagain;
-
- restarts = 0;
- dqpout = NULL;
+ struct xfs_dquot *dqp;
+ int restarts = 0;
- /* lockorder: hashchainlock, freelistlock, mplistlock, dqlock, dqflock */
-again:
- startagain = 0;
mutex_lock(&xfs_Gqm->qm_dqfrlist_lock);
-
+restart:
list_for_each_entry(dqp, &xfs_Gqm->qm_dqfrlist, q_freelist) {
struct xfs_mount *mp = dqp->q_mount;
xfs_dqlock(dqp);
@@ -1701,7 +1682,6 @@ again:
list_del_init(&dqp->q_freelist);
xfs_Gqm->qm_dqfrlist_cnt--;
restarts++;
- startagain = 1;
goto dqunlock;
}
@@ -1737,57 +1717,40 @@ again:
}
goto dqunlock;
}
+ xfs_dqfunlock(dqp);
/*
- * We're trying to get the hashlock out of order. This races
- * with dqlookup; so, we giveup and goto the next dquot if
- * we couldn't get the hashlock. This way, we won't starve
- * a dqlookup process that holds the hashlock that is
- * waiting for the freelist lock.
+ * Prevent lookups now that we are past the point of no return.
*/
- if (!mutex_trylock(&dqp->q_hash->qh_lock)) {
- restarts++;
- goto dqfunlock;
- }
+ dqp->dq_flags |= XFS_DQ_FREEING;
+ xfs_dqunlock(dqp);
- /*
- * This races with dquot allocation code as well as dqflush_all
- * and reclaim code. So, if we failed to grab the mplist lock,
- * giveup everything and start over.
- */
- if (!mutex_trylock(&mp->m_quotainfo->qi_dqlist_lock)) {
- restarts++;
- startagain = 1;
- goto qhunlock;
- }
+ mutex_lock(&dqp->q_hash->qh_lock);
+ list_del_init(&dqp->q_hashlist);
+ dqp->q_hash->qh_version++;
+ mutex_unlock(&dqp->q_hash->qh_lock);
- ASSERT(dqp->q_nrefs == 0);
+ mutex_lock(&mp->m_quotainfo->qi_dqlist_lock);
list_del_init(&dqp->q_mplist);
mp->m_quotainfo->qi_dquots--;
mp->m_quotainfo->qi_dqreclaims++;
- list_del_init(&dqp->q_hashlist);
- dqp->q_hash->qh_version++;
+ mutex_unlock(&mp->m_quotainfo->qi_dqlist_lock);
+
+ ASSERT(dqp->q_nrefs == 0);
list_del_init(&dqp->q_freelist);
xfs_Gqm->qm_dqfrlist_cnt--;
- dqpout = dqp;
- mutex_unlock(&mp->m_quotainfo->qi_dqlist_lock);
-qhunlock:
- mutex_unlock(&dqp->q_hash->qh_lock);
-dqfunlock:
- xfs_dqfunlock(dqp);
+
+ mutex_unlock(&xfs_Gqm->qm_dqfrlist_lock);
+ return dqp;
dqunlock:
xfs_dqunlock(dqp);
- if (dqpout)
- break;
if (restarts >= XFS_QM_RECLAIM_MAX_RESTARTS)
break;
- if (startagain) {
- mutex_unlock(&xfs_Gqm->qm_dqfrlist_lock);
- goto again;
- }
+ goto restart;
}
+
mutex_unlock(&xfs_Gqm->qm_dqfrlist_lock);
- return dqpout;
+ return NULL;
}
/*
Index: xfs/fs/xfs/xfs_quota.h
===================================================================
--- xfs.orig/fs/xfs/xfs_quota.h 2011-11-25 11:45:50.228647131 +0100
+++ xfs/fs/xfs/xfs_quota.h 2011-11-25 11:45:53.351963543 +0100
@@ -87,6 +87,7 @@ typedef struct xfs_dqblk {
#define XFS_DQ_PROJ 0x0002 /* project quota */
#define XFS_DQ_GROUP 0x0004 /* a group quota */
#define XFS_DQ_DIRTY 0x0008 /* dquot is dirty */
+#define XFS_DQ_FREEING 0x0010 /* dquot is beeing torn down */
#define XFS_DQ_ALLTYPES (XFS_DQ_USER|XFS_DQ_PROJ|XFS_DQ_GROUP)
@@ -94,7 +95,8 @@ typedef struct xfs_dqblk {
{ XFS_DQ_USER, "USER" }, \
{ XFS_DQ_PROJ, "PROJ" }, \
{ XFS_DQ_GROUP, "GROUP" }, \
- { XFS_DQ_DIRTY, "DIRTY" }
+ { XFS_DQ_DIRTY, "DIRTY" }, \
+ { XFS_DQ_FREEING, "FREEING" }
/*
* In the worst case, when both user and group quotas are on,
Index: xfs/fs/xfs/xfs_dquot.h
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.h 2011-11-25 11:45:50.241980391 +0100
+++ xfs/fs/xfs/xfs_dquot.h 2011-11-25 11:45:53.351963543 +0100
@@ -133,7 +133,7 @@ static inline void xfs_dqunlock_nonotify
extern void xfs_qm_dqdestroy(xfs_dquot_t *);
extern int xfs_qm_dqflush(xfs_dquot_t *, uint);
-extern int xfs_qm_dqpurge(xfs_dquot_t *);
+extern void xfs_qm_dqpurge(xfs_dquot_t *);
extern void xfs_qm_dqunpin_wait(xfs_dquot_t *);
extern void xfs_qm_adjust_dqtimers(xfs_mount_t *,
xfs_disk_dquot_t *);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 09/16] xfs: flatten the dquot lock ordering
2011-11-28 8:27 ` [PATCH 09/16] xfs: flatten the dquot lock ordering Christoph Hellwig
@ 2011-12-05 5:04 ` Dave Chinner
2011-12-05 9:11 ` Christoph Hellwig
0 siblings, 1 reply; 41+ messages in thread
From: Dave Chinner @ 2011-12-05 5:04 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Nov 28, 2011 at 03:27:31AM -0500, Christoph Hellwig wrote:
> Introduce a new XFS_DQ_FREEING flag that tells lookup and mplist walks
> to skip a dquot that is beeing freed, and use this avoid the trylock
> on the hash and mplist locks in xfs_qm_dqreclaim_one.
Ok, so we now mark dquots being freed with a flag, and then avoid
those inodes during various operations as they dquots are considered
dead.
That means we can use the fact that nothing new will ever use the
dquot being freed while it is still active on lists, so we don't
need to nest locks during reclaim of the dquot to prevent concurrent
lookups from finding it.
Did i understand the intent correctly?
> Also simplify
> xfs_dqpurge by moving the inodes to a dispose list after marking them
> XFS_DQ_FREEING and avoid the locker ordering constraints.
Ok, so changing to a 2 phase reclaim algorithm - mark them for
reclaim and gather them needing only one lock for the list being
walked, then walk that gathered list and free them properly, knowing
that lookups won't find them due to the XFS_DQ_FREEING flag.
Basically the same principles as the VFS cache item reclaim, again?
> Signed-off-by: Christoph Hellwig <hch@lst.de>
......
> @@ -1335,31 +1314,34 @@ xfs_qm_dqpurge(
> __func__, dqp);
> xfs_dqflock(dqp);
> }
> +
> ASSERT(atomic_read(&dqp->q_pincount) == 0);
> ASSERT(XFS_FORCED_SHUTDOWN(mp) ||
> !(dqp->q_logitem.qli_item.li_flags & XFS_LI_IN_AIL));
>
> + xfs_dqfunlock(dqp);
> + xfs_dqunlock(dqp);
> +
> + mutex_lock(&qh->qh_lock);
> list_del_init(&dqp->q_hashlist);
> qh->qh_version++;
> + mutex_unlock(&qh->qh_lock);
>
> + mutex_lock(&mp->m_quotainfo->qi_dqlist_lock);
> list_del_init(&dqp->q_mplist);
> mp->m_quotainfo->qi_dqreclaims++;
> mp->m_quotainfo->qi_dquots--;
> + mutex_unlock(&mp->m_quotainfo->qi_dqlist_lock);
>
> + mutex_lock(&xfs_Gqm->qm_dqfrlist_lock);
> + ASSERT(!list_empty(&dqp->q_freelist));
> list_del_init(&dqp->q_freelist);
That assert could do with a comment - I had to think hard about why
that was correct. It's because when the dquot refcount goes to zero it
is moved onto the free list, so when reclaiming a dquot we should
always find it on the free list....
.....
> @@ -489,8 +498,8 @@ xfs_qm_dqpurge_int(
> struct xfs_quotainfo *q = mp->m_quotainfo;
> struct xfs_dquot *dqp, *n;
> uint dqtype;
> - int nrecl;
> - int nmisses;
> + int nmisses = 0;
> + LIST_HEAD (dispose_list);
Curious style. I haven't seen that before - not sure whether I like
it or not yet...
>
> if (!q)
> return 0;
> @@ -509,46 +518,27 @@ xfs_qm_dqpurge_int(
> */
> xfs_qm_detach_gdquots(mp);
>
> - again:
> - nmisses = 0;
I don't think that nmisses is initialised to zero correctly anymore.
> - ASSERT(mutex_is_locked(&q->qi_dqlist_lock));
> /*
> * Try to get rid of all of the unwanted dquots. The idea is to
> * get them off mplist and hashlist, but leave them on freelist.
> */
That comment is no longer correct - they purged dquots don't remain
on the freelist anymore - they are freed....
.....
> @@ -1737,57 +1717,40 @@ again:
> }
> goto dqunlock;
> }
> + xfs_dqfunlock(dqp);
>
> /*
> - * We're trying to get the hashlock out of order. This races
> - * with dqlookup; so, we giveup and goto the next dquot if
> - * we couldn't get the hashlock. This way, we won't starve
> - * a dqlookup process that holds the hashlock that is
> - * waiting for the freelist lock.
> + * Prevent lookups now that we are past the point of no return.
> */
> - if (!mutex_trylock(&dqp->q_hash->qh_lock)) {
> - restarts++;
> - goto dqfunlock;
> - }
> + dqp->dq_flags |= XFS_DQ_FREEING;
> + xfs_dqunlock(dqp);
Probably worth commenting here that it is safe to access the dquot
unlocked because we own the XFS_DQ_FREEING flag that guarantees
nobody else will start using the dquot once we unlock it.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 09/16] xfs: flatten the dquot lock ordering
2011-12-05 5:04 ` Dave Chinner
@ 2011-12-05 9:11 ` Christoph Hellwig
2011-12-05 9:34 ` Dave Chinner
0 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-12-05 9:11 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Mon, Dec 05, 2011 at 04:04:28PM +1100, Dave Chinner wrote:
> Ok, so we now mark dquots being freed with a flag, and then avoid
> those inodes during various operations as they dquots are considered
> dead.
>
> That means we can use the fact that nothing new will ever use the
> dquot being freed while it is still active on lists, so we don't
> need to nest locks during reclaim of the dquot to prevent concurrent
> lookups from finding it.
>
> Did i understand the intent correctly?
Yes.
> > + mutex_lock(&xfs_Gqm->qm_dqfrlist_lock);
> > + ASSERT(!list_empty(&dqp->q_freelist));
> > list_del_init(&dqp->q_freelist);
>
> That assert could do with a comment - I had to think hard about why
> that was correct. It's because when the dquot refcount goes to zero it
> is moved onto the free list, so when reclaiming a dquot we should
> always find it on the free list....
Indeed. I'll see if I can come up with a sensible comment.
> > @@ -509,46 +518,27 @@ xfs_qm_dqpurge_int(
> > */
> > xfs_qm_detach_gdquots(mp);
> >
> > - again:
> > - nmisses = 0;
>
> I don't think that nmisses is initialised to zero correctly anymore.
We do a
int nmisses = 0;
at the top of the function. Now that there are no retry loops that's
sufficient.
>
> > - ASSERT(mutex_is_locked(&q->qi_dqlist_lock));
> > /*
> > * Try to get rid of all of the unwanted dquots. The idea is to
> > * get them off mplist and hashlist, but leave them on freelist.
> > */
>
> That comment is no longer correct - they purged dquots don't remain
> on the freelist anymore - they are freed....
I'll fix it up.
> > /*
> > + * Prevent lookups now that we are past the point of no return.
> > */
> > - if (!mutex_trylock(&dqp->q_hash->qh_lock)) {
> > - restarts++;
> > - goto dqfunlock;
> > - }
> > + dqp->dq_flags |= XFS_DQ_FREEING;
> > + xfs_dqunlock(dqp);
>
> Probably worth commenting here that it is safe to access the dquot
> unlocked because we own the XFS_DQ_FREEING flag that guarantees
> nobody else will start using the dquot once we unlock it.
I'll make the comment a bit more verbose.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 09/16] xfs: flatten the dquot lock ordering
2011-12-05 9:11 ` Christoph Hellwig
@ 2011-12-05 9:34 ` Dave Chinner
2011-12-05 11:50 ` Christoph Hellwig
0 siblings, 1 reply; 41+ messages in thread
From: Dave Chinner @ 2011-12-05 9:34 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Dec 05, 2011 at 04:11:21AM -0500, Christoph Hellwig wrote:
> On Mon, Dec 05, 2011 at 04:04:28PM +1100, Dave Chinner wrote:
> > > @@ -509,46 +518,27 @@ xfs_qm_dqpurge_int(
> > > */
> > > xfs_qm_detach_gdquots(mp);
> > >
> > > - again:
> > > - nmisses = 0;
> >
> > I don't think that nmisses is initialised to zero correctly anymore.
>
> We do a
>
> int nmisses = 0;
>
> at the top of the function. Now that there are no retry loops that's
> sufficient.
Hmmmm. My unmodified tree just has the declaration without
initialisation, and I didn't find any place where the initialisation
was added in the preceding patches. I'll go back and have another
look.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 09/16] xfs: flatten the dquot lock ordering
2011-12-05 9:34 ` Dave Chinner
@ 2011-12-05 11:50 ` Christoph Hellwig
2011-12-06 0:25 ` Dave Chinner
0 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-12-05 11:50 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Mon, Dec 05, 2011 at 08:34:37PM +1100, Dave Chinner wrote:
> Hmmmm. My unmodified tree just has the declaration without
> initialisation, and I didn't find any place where the initialisation
> was added in the preceding patches. I'll go back and have another
> look.
It's actually in this patch:
@@ -489,8 +498,8 @@ xfs_qm_dqpurge_int(
struct xfs_quotainfo *q = mp->m_quotainfo;
struct xfs_dquot *dqp, *n;
uint dqtype;
- int nrecl;
- int nmisses;
+ int nmisses = 0;
+ LIST_HEAD (dispose_list);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 09/16] xfs: flatten the dquot lock ordering
2011-12-05 11:50 ` Christoph Hellwig
@ 2011-12-06 0:25 ` Dave Chinner
0 siblings, 0 replies; 41+ messages in thread
From: Dave Chinner @ 2011-12-06 0:25 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Dec 05, 2011 at 06:50:21AM -0500, Christoph Hellwig wrote:
> On Mon, Dec 05, 2011 at 08:34:37PM +1100, Dave Chinner wrote:
> > Hmmmm. My unmodified tree just has the declaration without
> > initialisation, and I didn't find any place where the initialisation
> > was added in the preceding patches. I'll go back and have another
> > look.
>
> It's actually in this patch:
>
> @@ -489,8 +498,8 @@ xfs_qm_dqpurge_int(
> struct xfs_quotainfo *q = mp->m_quotainfo;
> struct xfs_dquot *dqp, *n;
> uint dqtype;
> - int nrecl;
> - int nmisses;
> + int nmisses = 0;
> + LIST_HEAD (dispose_list);
>
OK, so I'm blind. :/
Move along, nothing to see here....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 10/16] xfs: nest qm_dqfrlist_lock insise the dquot qlock
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
` (8 preceding siblings ...)
2011-11-28 8:27 ` [PATCH 09/16] xfs: flatten the dquot lock ordering Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 5:10 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 11/16] xfs: simplify xfs_qm_dqattach_grouphint Christoph Hellwig
` (5 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-change-quota-freelist-lock-order --]
[-- Type: text/plain, Size: 4477 bytes --]
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.
Document our new lock order now that it has settled.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
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.
+ */
#ifdef DEBUG
xfs_buftarg_t *xfs_dqerror_target;
@@ -984,69 +983,49 @@ restart:
*/
void
xfs_qm_dqput(
- xfs_dquot_t *dqp)
+ struct xfs_dquot *dqp)
{
- xfs_dquot_t *gdqp;
+ struct xfs_dquot *gdqp;
ASSERT(dqp->q_nrefs > 0);
ASSERT(XFS_DQ_IS_LOCKED(dqp));
trace_xfs_dqput(dqp);
- if (dqp->q_nrefs != 1) {
- dqp->q_nrefs--;
+recurse:
+ if (--dqp->q_nrefs > 0) {
xfs_dqunlock(dqp);
return;
}
- /*
- * drop the dqlock and acquire the freelist and dqlock
- * in the right order; but try to get it out-of-order first
- */
- if (!mutex_trylock(&xfs_Gqm->qm_dqfrlist_lock)) {
- trace_xfs_dqput_wait(dqp);
- xfs_dqunlock(dqp);
- mutex_lock(&xfs_Gqm->qm_dqfrlist_lock);
- xfs_dqlock(dqp);
- }
-
- while (1) {
- gdqp = NULL;
+ trace_xfs_dqput_free(dqp);
- /* We can't depend on nrefs being == 1 here */
- if (--dqp->q_nrefs == 0) {
- trace_xfs_dqput_free(dqp);
-
- if (list_empty(&dqp->q_freelist)) {
- list_add_tail(&dqp->q_freelist, &xfs_Gqm->qm_dqfrlist);
- xfs_Gqm->qm_dqfrlist_cnt++;
- }
+ mutex_lock(&xfs_Gqm->qm_dqfrlist_lock);
+ if (list_empty(&dqp->q_freelist)) {
+ list_add_tail(&dqp->q_freelist, &xfs_Gqm->qm_dqfrlist);
+ xfs_Gqm->qm_dqfrlist_cnt++;
+ }
+ mutex_unlock(&xfs_Gqm->qm_dqfrlist_lock);
- /*
- * If we just added a udquot to the freelist, then
- * we want to release the gdquot reference that
- * it (probably) has. Otherwise it'll keep the
- * gdquot from getting reclaimed.
- */
- if ((gdqp = dqp->q_gdquot)) {
- /*
- * Avoid a recursive dqput call
- */
- xfs_dqlock(gdqp);
- dqp->q_gdquot = NULL;
- }
- }
- xfs_dqunlock(dqp);
+ /*
+ * If we just added a udquot to the freelist, then we want to release
+ * the gdquot reference that it (probably) has. Otherwise it'll keep
+ * the gdquot from getting reclaimed.
+ */
+ gdqp = dqp->q_gdquot;
+ if (gdqp) {
+ xfs_dqlock(gdqp);
+ dqp->q_gdquot = NULL;
+ }
+ xfs_dqunlock(dqp);
- /*
- * If we had a group quota inside the user quota as a hint,
- * release it now.
- */
- if (! gdqp)
- break;
+ /*
+ * If we had a group quota hint, release it now.
+ */
+ if (gdqp) {
dqp = gdqp;
+ goto recurse;
}
- mutex_unlock(&xfs_Gqm->qm_dqfrlist_lock);
}
/*
Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c 2011-11-25 11:45:53.351963543 +0100
+++ xfs/fs/xfs/xfs_qm.c 2011-11-25 11:55:41.538777061 +0100
@@ -1669,7 +1669,9 @@ xfs_qm_dqreclaim_one(void)
restart:
list_for_each_entry(dqp, &xfs_Gqm->qm_dqfrlist, q_freelist) {
struct xfs_mount *mp = dqp->q_mount;
- xfs_dqlock(dqp);
+
+ if (!xfs_dqlock_nowait(dqp))
+ continue;
/*
* This dquot has already been grabbed by dqlookup.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 10/16] xfs: nest qm_dqfrlist_lock insise the dquot qlock
2011-11-28 8:27 ` [PATCH 10/16] xfs: nest qm_dqfrlist_lock insise the dquot qlock Christoph Hellwig
@ 2011-12-05 5:10 ` Dave Chinner
0 siblings, 0 replies; 41+ messages in thread
From: Dave Chinner @ 2011-12-05 5:10 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
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
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 11/16] xfs: simplify xfs_qm_dqattach_grouphint
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
` (9 preceding siblings ...)
2011-11-28 8:27 ` [PATCH 10/16] xfs: nest qm_dqfrlist_lock insise the dquot qlock Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 5:14 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 12/16] xfs: simplify xfs_qm_detach_gdquots Christoph Hellwig
` (4 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-quota-cleanup-xfs_qm_dqattach_grouphint --]
[-- Type: text/plain, Size: 3127 bytes --]
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@lst.de>
---
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-11-25 11:45:54.715289491 +0100
+++ xfs/fs/xfs/xfs_qm.c 2011-11-25 11:45:56.321947454 +0100
@@ -651,11 +651,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(
@@ -666,45 +662,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.
- */
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);
}
@@ -771,17 +743,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.
*/
- 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@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* [PATCH 12/16] xfs: simplify xfs_qm_detach_gdquots
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
` (10 preceding siblings ...)
2011-11-28 8:27 ` [PATCH 11/16] xfs: simplify xfs_qm_dqattach_grouphint Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 5:18 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 13/16] xfs: add a xfs_dqhold helper Christoph Hellwig
` (3 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-quota-simplify-xfs_qm_detach_gdquots --]
[-- Type: text/plain, Size: 1407 bytes --]
With the new lock order there is no reason to drop qi_dqlist_lock around
calls to xfs_qm_dqrele.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_qm.c | 22 +++++-----------------
1 file changed, 5 insertions(+), 17 deletions(-)
Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c 2011-10-27 22:40:07.538179215 +0200
+++ xfs/fs/xfs/xfs_qm.c 2011-10-27 22:40:08.124671538 +0200
@@ -449,7 +449,6 @@ xfs_qm_detach_gdquots(
{
struct xfs_quotainfo *q = mp->m_quotainfo;
struct xfs_dquot *dqp, *gdqp;
- int nrecl;
again:
ASSERT(mutex_is_locked(&q->qi_dqlist_lock));
@@ -462,25 +461,14 @@ xfs_qm_detach_gdquots(
mutex_lock(&q->qi_dqlist_lock);
goto again;
}
- if ((gdqp = dqp->q_gdquot)) {
- xfs_dqlock(gdqp);
+
+ gdqp = dqp->q_gdquot;
+ if (gdqp)
dqp->q_gdquot = NULL;
- }
xfs_dqunlock(dqp);
- if (gdqp) {
- /*
- * Can't hold the mplist lock across a dqput.
- * XXXmust convert to marker based iterations here.
- */
- nrecl = q->qi_dqreclaims;
- mutex_unlock(&q->qi_dqlist_lock);
- xfs_qm_dqput(gdqp);
-
- mutex_lock(&q->qi_dqlist_lock);
- if (nrecl != q->qi_dqreclaims)
- goto again;
- }
+ if (gdqp)
+ xfs_qm_dqrele(gdqp);
}
}
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* [PATCH 13/16] xfs: add a xfs_dqhold helper
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
` (11 preceding siblings ...)
2011-11-28 8:27 ` [PATCH 12/16] xfs: simplify xfs_qm_detach_gdquots Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 5:22 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 14/16] xfs: merge xfs_qm_dqinit_core into the only caller Christoph Hellwig
` (2 subsequent siblings)
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-quota-add-xfs_dqhold --]
[-- Type: text/plain, Size: 4755 bytes --]
Factor the common pattern of:
xfs_dqlock(dqp);
XFS_DQHOLD(dqp);
xfs_dqunlock(dqp);
into a new helper, and remove XFS_DQHOLD now that only two callers are left.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_dquot.c | 2 +-
fs/xfs/xfs_dquot.h | 10 ++++++++--
fs/xfs/xfs_qm.c | 50 +++++++++++++-------------------------------------
3 files changed, 22 insertions(+), 40 deletions(-)
Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c 2011-11-25 11:56:35.841816211 +0100
+++ xfs/fs/xfs/xfs_qm.c 2011-11-25 11:56:36.905143783 +0100
@@ -594,12 +594,9 @@ xfs_qm_dqattach_one(
*/
dqp = udqhint->q_gdquot;
if (dqp && be32_to_cpu(dqp->q_core.d_id) == id) {
- xfs_dqlock(dqp);
- XFS_DQHOLD(dqp);
ASSERT(*IO_idqpp == NULL);
- *IO_idqpp = dqp;
- xfs_dqunlock(dqp);
+ *IO_idqpp = xfs_qm_dqhold(dqp);
xfs_dqunlock(udqhint);
return 0;
}
@@ -659,11 +656,7 @@ xfs_qm_dqattach_grouphint(
xfs_qm_dqrele(tmp);
}
- xfs_dqlock(gdq);
- XFS_DQHOLD(gdq);
- xfs_dqunlock(gdq);
-
- udq->q_gdquot = gdq;
+ udq->q_gdquot = xfs_qm_dqhold(gdq);
done:
xfs_dqunlock(udq);
}
@@ -1928,10 +1921,7 @@ xfs_qm_vop_dqalloc(
* this to caller
*/
ASSERT(ip->i_udquot);
- uq = ip->i_udquot;
- xfs_dqlock(uq);
- XFS_DQHOLD(uq);
- xfs_dqunlock(uq);
+ uq = xfs_qm_dqhold(ip->i_udquot);
}
}
if ((flags & XFS_QMOPT_GQUOTA) && XFS_IS_GQUOTA_ON(mp)) {
@@ -1952,10 +1942,7 @@ xfs_qm_vop_dqalloc(
xfs_ilock(ip, lockflags);
} else {
ASSERT(ip->i_gdquot);
- gq = ip->i_gdquot;
- xfs_dqlock(gq);
- XFS_DQHOLD(gq);
- xfs_dqunlock(gq);
+ gq = xfs_qm_dqhold(ip->i_gdquot);
}
} else if ((flags & XFS_QMOPT_PQUOTA) && XFS_IS_PQUOTA_ON(mp)) {
if (xfs_get_projid(ip) != prid) {
@@ -1975,10 +1962,7 @@ xfs_qm_vop_dqalloc(
xfs_ilock(ip, lockflags);
} else {
ASSERT(ip->i_gdquot);
- gq = ip->i_gdquot;
- xfs_dqlock(gq);
- XFS_DQHOLD(gq);
- xfs_dqunlock(gq);
+ gq = xfs_qm_dqhold(ip->i_gdquot);
}
}
if (uq)
@@ -2028,14 +2012,10 @@ xfs_qm_vop_chown(
xfs_trans_mod_dquot(tp, newdq, XFS_TRANS_DQ_ICOUNT, 1);
/*
- * Take an extra reference, because the inode
- * is going to keep this dquot pointer even
- * after the trans_commit.
- */
- xfs_dqlock(newdq);
- XFS_DQHOLD(newdq);
- xfs_dqunlock(newdq);
- *IO_olddq = newdq;
+ * Take an extra reference, because the inode is going to keep
+ * this dquot pointer even after the trans_commit.
+ */
+ *IO_olddq = xfs_qm_dqhold(newdq);
return prevdq;
}
@@ -2167,25 +2147,21 @@ xfs_qm_vop_create_dqattach(
ASSERT(XFS_IS_QUOTA_RUNNING(mp));
if (udqp) {
- xfs_dqlock(udqp);
- XFS_DQHOLD(udqp);
- xfs_dqunlock(udqp);
ASSERT(ip->i_udquot == NULL);
- ip->i_udquot = udqp;
ASSERT(XFS_IS_UQUOTA_ON(mp));
ASSERT(ip->i_d.di_uid == be32_to_cpu(udqp->q_core.d_id));
+
+ ip->i_udquot = xfs_qm_dqhold(udqp);
xfs_trans_mod_dquot(tp, udqp, XFS_TRANS_DQ_ICOUNT, 1);
}
if (gdqp) {
- xfs_dqlock(gdqp);
- XFS_DQHOLD(gdqp);
- xfs_dqunlock(gdqp);
ASSERT(ip->i_gdquot == NULL);
- ip->i_gdquot = gdqp;
ASSERT(XFS_IS_OQUOTA_ON(mp));
ASSERT((XFS_IS_GQUOTA_ON(mp) ?
ip->i_d.di_gid : xfs_get_projid(ip)) ==
be32_to_cpu(gdqp->q_core.d_id));
+
+ ip->i_gdquot = xfs_qm_dqhold(gdqp);
xfs_trans_mod_dquot(tp, gdqp, XFS_TRANS_DQ_ICOUNT, 1);
}
}
Index: xfs/fs/xfs/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.c 2011-11-25 11:56:08.655296826 +0100
+++ xfs/fs/xfs/xfs_dquot.c 2011-11-25 11:56:36.905143783 +0100
@@ -733,7 +733,7 @@ xfs_qm_dqlookup(
return -1;
}
- XFS_DQHOLD(dqp);
+ dqp->q_nrefs++;
/*
* move the dquot to the front of the hashchain
Index: xfs/fs/xfs/xfs_dquot.h
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.h 2011-11-25 11:55:40.368783400 +0100
+++ xfs/fs/xfs/xfs_dquot.h 2011-11-25 11:56:36.905143783 +0100
@@ -80,8 +80,6 @@ enum {
XFS_QLOCK_NESTED,
};
-#define XFS_DQHOLD(dqp) ((dqp)->q_nrefs++)
-
/*
* Manage the q_flush completion queue embedded in the dquot. This completion
* queue synchronizes processes attempting to flush the in-core dquot back to
@@ -147,4 +145,12 @@ extern void xfs_dqlock2(struct xfs_dquo
extern void xfs_dqunlock(struct xfs_dquot *);
extern void xfs_dqflock_pushbuf_wait(struct xfs_dquot *dqp);
+static inline struct xfs_dquot *xfs_qm_dqhold(struct xfs_dquot *dqp)
+{
+ xfs_dqlock(dqp);
+ dqp->q_nrefs++;
+ xfs_dqunlock(dqp);
+ return dqp;
+}
+
#endif /* __XFS_DQUOT_H__ */
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* [PATCH 14/16] xfs: merge xfs_qm_dqinit_core into the only caller
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
` (12 preceding siblings ...)
2011-11-28 8:27 ` [PATCH 13/16] xfs: add a xfs_dqhold helper Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 5:23 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 15/16] xfs: kill xfs_qm_idtodq Christoph Hellwig
2011-11-28 8:27 ` [PATCH 16/16] xfs: remove XFS_QMOPT_DQSUSER Christoph Hellwig
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-quota-kill-xfs_qm_dqinit_core --]
[-- Type: text/plain, Size: 1743 bytes --]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_dquot.c | 27 +++++++--------------------
1 file changed, 7 insertions(+), 20 deletions(-)
Index: xfs/fs/xfs/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.c 2011-11-25 11:56:36.905143783 +0100
+++ xfs/fs/xfs/xfs_dquot.c 2011-11-25 11:56:40.861789014 +0100
@@ -154,24 +154,6 @@ xfs_qm_dqdestroy(
}
/*
- * This is what a 'fresh' dquot inside a dquot chunk looks like on disk.
- */
-STATIC void
-xfs_qm_dqinit_core(
- xfs_dqid_t id,
- uint type,
- xfs_dqblk_t *d)
-{
- /*
- * Caller has zero'd the entire dquot 'chunk' already.
- */
- d->dd_diskdq.d_magic = cpu_to_be16(XFS_DQUOT_MAGIC);
- d->dd_diskdq.d_version = XFS_DQUOT_VERSION;
- d->dd_diskdq.d_id = cpu_to_be32(id);
- d->dd_diskdq.d_flags = type;
-}
-
-/*
* If default limits are in force, push them into the dquot now.
* We overwrite the dquot limits only if they are zero and this
* is not the root dquot.
@@ -327,8 +309,13 @@ xfs_qm_init_dquot_blk(
curid = id - (id % q->qi_dqperchunk);
ASSERT(curid >= 0);
memset(d, 0, BBTOB(q->qi_dqchunklen));
- for (i = 0; i < q->qi_dqperchunk; i++, d++, curid++)
- xfs_qm_dqinit_core(curid, type, d);
+ for (i = 0; i < q->qi_dqperchunk; i++, d++, curid++) {
+ d->dd_diskdq.d_magic = cpu_to_be16(XFS_DQUOT_MAGIC);
+ d->dd_diskdq.d_version = XFS_DQUOT_VERSION;
+ d->dd_diskdq.d_id = cpu_to_be32(curid);
+ d->dd_diskdq.d_flags = type;
+ }
+
xfs_trans_dquot_buf(tp, bp,
(type & XFS_DQ_USER ? XFS_BLF_UDQUOT_BUF :
((type & XFS_DQ_PROJ) ? XFS_BLF_PDQUOT_BUF :
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* [PATCH 15/16] xfs: kill xfs_qm_idtodq
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
` (13 preceding siblings ...)
2011-11-28 8:27 ` [PATCH 14/16] xfs: merge xfs_qm_dqinit_core into the only caller Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 5:31 ` Dave Chinner
2011-11-28 8:27 ` [PATCH 16/16] xfs: remove XFS_QMOPT_DQSUSER Christoph Hellwig
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-cleanup-quota-read --]
[-- Type: text/plain, Size: 5229 bytes --]
This function doesn't help the code flow, so merge the dquot allocation and
transaction handling into xfs_qm_dqread.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_dquot.c | 134 ++++++++++++++++++-----------------------------------
1 file changed, 47 insertions(+), 87 deletions(-)
Index: xfs/fs/xfs/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.c 2011-11-25 11:56:40.861789014 +0100
+++ xfs/fs/xfs/xfs_dquot.c 2011-11-25 11:56:42.488446869 +0100
@@ -550,36 +550,59 @@ xfs_qm_dqtobp(
* Read in the ondisk dquot using dqtobp() then copy it to an incore version,
* and release the buffer immediately.
*
+ * If XFS_QMOPT_DQALLOC is set, allocate a dquot on disk if it needed.
*/
-/* ARGSUSED */
STATIC int
xfs_qm_dqread(
- xfs_trans_t **tpp,
- xfs_dqid_t id,
- xfs_dquot_t *dqp, /* dquot to get filled in */
- uint flags)
+ struct xfs_mount *mp,
+ xfs_dqid_t id,
+ uint type,
+ uint flags,
+ struct xfs_dquot **O_dqpp)
{
- xfs_disk_dquot_t *ddqp;
- xfs_buf_t *bp;
- int error;
- xfs_trans_t *tp;
+ struct xfs_dquot *dqp;
+ struct xfs_disk_dquot *ddqp;
+ struct xfs_buf *bp;
+ struct xfs_trans *tp = NULL;
+ int error;
+ int cancelflags = 0;
- ASSERT(tpp);
+ dqp = xfs_qm_dqinit(mp, id, type);
trace_xfs_dqread(dqp);
+ if (flags & XFS_QMOPT_DQALLOC) {
+ tp = xfs_trans_alloc(mp, XFS_TRANS_QM_DQALLOC);
+ error = xfs_trans_reserve(tp, XFS_QM_DQALLOC_SPACE_RES(mp),
+ XFS_WRITE_LOG_RES(mp) +
+ BBTOB(mp->m_quotainfo->qi_dqchunklen) - 1 +
+ 128,
+ 0,
+ XFS_TRANS_PERM_LOG_RES,
+ XFS_WRITE_LOG_COUNT);
+ if (error)
+ goto error1;
+ cancelflags = XFS_TRANS_RELEASE_LOG_RES;
+ }
+
/*
* get a pointer to the on-disk dquot and the buffer containing it
* dqp already knows its own type (GROUP/USER).
*/
- if ((error = xfs_qm_dqtobp(tpp, dqp, &ddqp, &bp, flags))) {
- return (error);
+ error = xfs_qm_dqtobp(&tp, dqp, &ddqp, &bp, flags);
+ if (error) {
+ /*
+ * This can happen if quotas got turned off (ESRCH),
+ * or if the dquot didn't exist on disk and we ask to
+ * allocate (ENOENT).
+ */
+ trace_xfs_dqread_fail(dqp);
+ cancelflags |= XFS_TRANS_ABORT;
+ goto error1;
}
- tp = *tpp;
/* copy everything from disk dquot to the incore dquot */
memcpy(&dqp->q_core, ddqp, sizeof(xfs_disk_dquot_t));
- ASSERT(be32_to_cpu(dqp->q_core.d_id) == id);
xfs_qm_dquot_logitem_init(dqp);
/*
@@ -608,77 +631,22 @@ xfs_qm_dqread(
ASSERT(xfs_buf_islocked(bp));
xfs_trans_brelse(tp, bp);
- return (error);
-}
-
-
-/*
- * allocate an incore dquot from the kernel heap,
- * and fill its core with quota information kept on disk.
- * If XFS_QMOPT_DQALLOC is set, it'll allocate a dquot on disk
- * if it wasn't already allocated.
- */
-STATIC int
-xfs_qm_idtodq(
- xfs_mount_t *mp,
- xfs_dqid_t id, /* gid or uid, depending on type */
- uint type, /* UDQUOT or GDQUOT */
- uint flags, /* DQALLOC, DQREPAIR */
- xfs_dquot_t **O_dqpp)/* OUT : incore dquot, not locked */
-{
- xfs_dquot_t *dqp;
- int error;
- xfs_trans_t *tp;
- int cancelflags=0;
-
- dqp = xfs_qm_dqinit(mp, id, type);
- tp = NULL;
- if (flags & XFS_QMOPT_DQALLOC) {
- tp = xfs_trans_alloc(mp, XFS_TRANS_QM_DQALLOC);
- error = xfs_trans_reserve(tp, XFS_QM_DQALLOC_SPACE_RES(mp),
- XFS_WRITE_LOG_RES(mp) +
- BBTOB(mp->m_quotainfo->qi_dqchunklen) - 1 +
- 128,
- 0,
- XFS_TRANS_PERM_LOG_RES,
- XFS_WRITE_LOG_COUNT);
- if (error) {
- cancelflags = 0;
- goto error0;
- }
- cancelflags = XFS_TRANS_RELEASE_LOG_RES;
- }
-
- /*
- * Read it from disk; xfs_dqread() takes care of
- * all the necessary initialization of dquot's fields (locks, etc)
- */
- if ((error = xfs_qm_dqread(&tp, id, dqp, flags))) {
- /*
- * This can happen if quotas got turned off (ESRCH),
- * or if the dquot didn't exist on disk and we ask to
- * allocate (ENOENT).
- */
- trace_xfs_dqread_fail(dqp);
- cancelflags |= XFS_TRANS_ABORT;
- goto error0;
- }
if (tp) {
- if ((error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES)))
- goto error1;
+ error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES);
+ if (error)
+ goto error0;
}
*O_dqpp = dqp;
- return (0);
+ return error;
- error0:
- ASSERT(error);
+error1:
if (tp)
xfs_trans_cancel(tp, cancelflags);
- error1:
+error0:
xfs_qm_dqdestroy(dqp);
*O_dqpp = NULL;
- return (error);
+ return error;
}
/*
@@ -832,19 +800,11 @@ restart:
version = h->qh_version;
mutex_unlock(&h->qh_lock);
- /*
- * Allocate the dquot on the kernel heap, and read the ondisk
- * portion off the disk. Also, do all the necessary initialization
- * This can return ENOENT if dquot didn't exist on disk and we didn't
- * ask it to allocate; ESRCH if quotas got turned off suddenly.
- */
- if ((error = xfs_qm_idtodq(mp, id, type,
- flags & (XFS_QMOPT_DQALLOC|XFS_QMOPT_DQREPAIR|
- XFS_QMOPT_DOWARN),
- &dqp))) {
+ error = xfs_qm_dqread(mp, id, type, flags, &dqp);
+ if (error) {
if (ip)
xfs_ilock(ip, XFS_ILOCK_EXCL);
- return (error);
+ return error;
}
/*
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 15/16] xfs: kill xfs_qm_idtodq
2011-11-28 8:27 ` [PATCH 15/16] xfs: kill xfs_qm_idtodq Christoph Hellwig
@ 2011-12-05 5:31 ` Dave Chinner
0 siblings, 0 replies; 41+ messages in thread
From: Dave Chinner @ 2011-12-05 5:31 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Nov 28, 2011 at 03:27:37AM -0500, Christoph Hellwig wrote:
> This function doesn't help the code flow, so merge the dquot allocation and
> transaction handling into xfs_qm_dqread.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
This version is much easier to follow.
Couple of small things:
> ---
> fs/xfs/xfs_dquot.c | 134 ++++++++++++++++++-----------------------------------
> 1 file changed, 47 insertions(+), 87 deletions(-)
>
> Index: xfs/fs/xfs/xfs_dquot.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_dquot.c 2011-11-25 11:56:40.861789014 +0100
> +++ xfs/fs/xfs/xfs_dquot.c 2011-11-25 11:56:42.488446869 +0100
> @@ -550,36 +550,59 @@ xfs_qm_dqtobp(
> * Read in the ondisk dquot using dqtobp() then copy it to an incore version,
> * and release the buffer immediately.
> *
> + * If XFS_QMOPT_DQALLOC is set, allocate a dquot on disk if it needed.
> */
> -/* ARGSUSED */
> STATIC int
> xfs_qm_dqread(
> - xfs_trans_t **tpp,
> - xfs_dqid_t id,
> - xfs_dquot_t *dqp, /* dquot to get filled in */
> - uint flags)
> + struct xfs_mount *mp,
> + xfs_dqid_t id,
^^^ whitespace damage
> + uint type,
^^^^^ whitespace damage
> + uint flags,
> + struct xfs_dquot **O_dqpp)
> {
> - xfs_disk_dquot_t *ddqp;
> - xfs_buf_t *bp;
> - int error;
> - xfs_trans_t *tp;
> + struct xfs_dquot *dqp;
> + struct xfs_disk_dquot *ddqp;
> + struct xfs_buf *bp;
> + struct xfs_trans *tp = NULL;
> + int error;
> + int cancelflags = 0;
Extra whitespace in the bp/tp/error declarations.
>
> - ASSERT(tpp);
> + dqp = xfs_qm_dqinit(mp, id, type);
>
> trace_xfs_dqread(dqp);
>
> + if (flags & XFS_QMOPT_DQALLOC) {
> + tp = xfs_trans_alloc(mp, XFS_TRANS_QM_DQALLOC);
> + error = xfs_trans_reserve(tp, XFS_QM_DQALLOC_SPACE_RES(mp),
> + XFS_WRITE_LOG_RES(mp) +
> + BBTOB(mp->m_quotainfo->qi_dqchunklen) - 1 +
> + 128,
That reservation value is asking for a macro or separate variable
and a comment explaining it....
therwise, looks OK.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 16/16] xfs: remove XFS_QMOPT_DQSUSER
2011-11-28 8:27 [PATCH 00/16] quota cleanups for Linux 3.3 Christoph Hellwig
` (14 preceding siblings ...)
2011-11-28 8:27 ` [PATCH 15/16] xfs: kill xfs_qm_idtodq Christoph Hellwig
@ 2011-11-28 8:27 ` Christoph Hellwig
2011-12-05 5:34 ` Dave Chinner
15 siblings, 1 reply; 41+ messages in thread
From: Christoph Hellwig @ 2011-11-28 8:27 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-quota-kill-XFS_QMOPT_DQSUSER --]
[-- Type: text/plain, Size: 5054 bytes --]
Just read the id 0 dquot from disk directly in xfs_qm_init_quotainfo instead
of going through dqget and requiring a special flag to not add the dquot to any
lists.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_dquot.c | 27 ++++++---------------------
fs/xfs/xfs_dquot.h | 2 ++
fs/xfs/xfs_qm.c | 22 ++++++++++------------
fs/xfs/xfs_quota.h | 1 -
4 files changed, 18 insertions(+), 34 deletions(-)
Index: xfs/fs/xfs/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.c 2011-11-25 11:56:42.488446869 +0100
+++ xfs/fs/xfs/xfs_dquot.c 2011-11-25 11:56:43.941772330 +0100
@@ -552,7 +552,7 @@ xfs_qm_dqtobp(
*
* If XFS_QMOPT_DQALLOC is set, allocate a dquot on disk if it needed.
*/
-STATIC int
+int
xfs_qm_dqread(
struct xfs_mount *mp,
xfs_dqid_t id,
@@ -801,32 +801,17 @@ restart:
mutex_unlock(&h->qh_lock);
error = xfs_qm_dqread(mp, id, type, flags, &dqp);
- if (error) {
- if (ip)
- xfs_ilock(ip, XFS_ILOCK_EXCL);
- return error;
- }
- /*
- * See if this is mount code calling to look at the overall quota limits
- * which are stored in the id == 0 user or group's dquot.
- * Since we may not have done a quotacheck by this point, just return
- * the dquot without attaching it to any hashtables, lists, etc, or even
- * taking a reference.
- * The caller must dqdestroy this once done.
- */
- if (flags & XFS_QMOPT_DQSUSER) {
- ASSERT(id == 0);
- ASSERT(! ip);
- goto dqret;
- }
+ if (ip)
+ xfs_ilock(ip, XFS_ILOCK_EXCL);
+
+ if (error)
+ return error;
/*
* Dquot lock comes after hashlock in the lock ordering
*/
if (ip) {
- xfs_ilock(ip, XFS_ILOCK_EXCL);
-
/*
* A dquot could be attached to this inode by now, since
* we had dropped the ilock.
Index: xfs/fs/xfs/xfs_dquot.h
===================================================================
--- xfs.orig/fs/xfs/xfs_dquot.h 2011-11-25 11:56:36.905143783 +0100
+++ xfs/fs/xfs/xfs_dquot.h 2011-11-25 11:56:43.945105645 +0100
@@ -129,6 +129,8 @@ static inline void xfs_dqunlock_nonotify
(XFS_IS_UQUOTA_ON((d)->q_mount)) : \
(XFS_IS_OQUOTA_ON((d)->q_mount))))
+extern int xfs_qm_dqread(struct xfs_mount *, xfs_dqid_t, uint,
+ uint, struct xfs_dquot **);
extern void xfs_qm_dqdestroy(xfs_dquot_t *);
extern int xfs_qm_dqflush(xfs_dquot_t *, uint);
extern void xfs_qm_dqpurge(xfs_dquot_t *);
Index: xfs/fs/xfs/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/xfs_qm.c 2011-11-25 11:56:36.905143783 +0100
+++ xfs/fs/xfs/xfs_qm.c 2011-11-25 11:56:43.945105645 +0100
@@ -847,18 +847,21 @@ xfs_qm_init_quotainfo(
/*
* We try to get the limits from the superuser's limits fields.
* This is quite hacky, but it is standard quota practice.
+ *
* We look at the USR dquot with id == 0 first, but if user quotas
* are not enabled we goto the GRP dquot with id == 0.
* We don't really care to keep separate default limits for user
* and group quotas, at least not at this point.
+ *
+ * Since we may not have done a quotacheck by this point, just read
+ * the dquot without attaching it to any hashtables or lists.
*/
- error = xfs_qm_dqget(mp, NULL, (xfs_dqid_t)0,
- XFS_IS_UQUOTA_RUNNING(mp) ? XFS_DQ_USER :
- (XFS_IS_GQUOTA_RUNNING(mp) ? XFS_DQ_GROUP :
- XFS_DQ_PROJ),
- XFS_QMOPT_DQSUSER|XFS_QMOPT_DOWARN,
- &dqp);
- if (! error) {
+ error = xfs_qm_dqread(mp, 0,
+ XFS_IS_UQUOTA_RUNNING(mp) ? XFS_DQ_USER :
+ (XFS_IS_GQUOTA_RUNNING(mp) ? XFS_DQ_GROUP :
+ XFS_DQ_PROJ),
+ XFS_QMOPT_DOWARN, &dqp);
+ if (!error) {
xfs_disk_dquot_t *ddqp = &dqp->q_core;
/*
@@ -885,11 +888,6 @@ xfs_qm_init_quotainfo(
qinf->qi_rtbhardlimit = be64_to_cpu(ddqp->d_rtb_hardlimit);
qinf->qi_rtbsoftlimit = be64_to_cpu(ddqp->d_rtb_softlimit);
- /*
- * We sent the XFS_QMOPT_DQSUSER flag to dqget because
- * we don't want this dquot cached. We haven't done a
- * quotacheck yet, and quotacheck doesn't like incore dquots.
- */
xfs_qm_dqdestroy(dqp);
} else {
qinf->qi_btimelimit = XFS_QM_BTIMELIMIT;
Index: xfs/fs/xfs/xfs_quota.h
===================================================================
--- xfs.orig/fs/xfs/xfs_quota.h 2011-11-25 11:55:38.138795482 +0100
+++ xfs/fs/xfs/xfs_quota.h 2011-11-25 11:56:43.945105645 +0100
@@ -197,7 +197,6 @@ typedef struct xfs_qoff_logformat {
#define XFS_QMOPT_UQUOTA 0x0000004 /* user dquot requested */
#define XFS_QMOPT_PQUOTA 0x0000008 /* project dquot requested */
#define XFS_QMOPT_FORCE_RES 0x0000010 /* ignore quota limits */
-#define XFS_QMOPT_DQSUSER 0x0000020 /* don't cache super users dquot */
#define XFS_QMOPT_SBVERSION 0x0000040 /* change superblock version num */
#define XFS_QMOPT_DOWARN 0x0000400 /* increase warning cnt if needed */
#define XFS_QMOPT_DQREPAIR 0x0001000 /* repair dquot if damaged */
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 41+ messages in thread