[Ocfs2-devel] [PATCH 0/5] ocfs2: Avoid pending orphaned inodes

[Ocfs2-devel] [PATCH 0/5] ocfs2: Avoid pending orphaned inodes

[Jan Kara]

  Hello,

  this is how I would imagine we can solve a deadlock with quotas after we do
not defer dropping inode reference from downconvert worker. The patches are
just compile tested so be careful. Goldwyn, do you have any ocfs2 test cluster
where I can test these patches?

								Honza

[Ocfs2-devel] [PATCH 1/5] ocfs2: Remove OCFS2_INODE_SKIP_DELETE flag

[Jan Kara]

The flag was never set, delete it.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ocfs2/inode.c   | 6 ------
 fs/ocfs2/inode.h   | 8 +++-----
 fs/ocfs2/journal.c | 6 ------
 3 files changed, 3 insertions(+), 17 deletions(-)

diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
index f29a90fde619..b4baaefe4dd4 100644
--- a/fs/ocfs2/inode.c
+++ b/fs/ocfs2/inode.c
@@ -822,12 +822,6 @@ static int ocfs2_inode_is_valid_to_delete(struct inode *inode)
 		goto bail_unlock;
 	}
 
-	/* If we have allowd wipe of this inode for another node, it
-	 * will be marked here so we can safely skip it. Recovery will
-	 * cleanup any inodes we might inadvertently skip here. */
-	if (oi->ip_flags & OCFS2_INODE_SKIP_DELETE)
-		goto bail_unlock;
-
 	ret = 1;
 bail_unlock:
 	spin_unlock(&oi->ip_lock);
diff --git a/fs/ocfs2/inode.h b/fs/ocfs2/inode.h
index 621fc73bf23d..f60bc314ee0a 100644
--- a/fs/ocfs2/inode.h
+++ b/fs/ocfs2/inode.h
@@ -84,8 +84,6 @@ struct ocfs2_inode_info
 #define OCFS2_INODE_BITMAP		0x00000004
 /* This inode has been wiped from disk */
 #define OCFS2_INODE_DELETED		0x00000008
-/* Another node is deleting, so our delete is a nop */
-#define OCFS2_INODE_SKIP_DELETE		0x00000010
 /* Has the inode been orphaned on another node?
  *
  * This hints to ocfs2_drop_inode that it should clear i_nlink before
@@ -100,11 +98,11 @@ struct ocfs2_inode_info
  * rely on ocfs2_delete_inode to sort things out under the proper
  * cluster locks.
  */
-#define OCFS2_INODE_MAYBE_ORPHANED	0x00000020
+#define OCFS2_INODE_MAYBE_ORPHANED	0x00000010
 /* Does someone have the file open O_DIRECT */
-#define OCFS2_INODE_OPEN_DIRECT		0x00000040
+#define OCFS2_INODE_OPEN_DIRECT		0x00000020
 /* Tell the inode wipe code it's not in orphan dir */
-#define OCFS2_INODE_SKIP_ORPHAN_DIR     0x00000080
+#define OCFS2_INODE_SKIP_ORPHAN_DIR     0x00000040
 
 static inline struct ocfs2_inode_info *OCFS2_I(struct inode *inode)
 {
diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index 44fc3e530c3d..03ea9314fecd 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -2132,12 +2132,6 @@ static int ocfs2_recover_orphans(struct ocfs2_super *osb,
 		iter = oi->ip_next_orphan;
 
 		spin_lock(&oi->ip_lock);
-		/* The remote delete code may have set these on the
-		 * assumption that the other node would wipe them
-		 * successfully.  If they are still in the node's
-		 * orphan dir, we need to reset that state. */
-		oi->ip_flags &= ~(OCFS2_INODE_DELETED|OCFS2_INODE_SKIP_DELETE);
-
 		/* Set the proper information to get us going into
 		 * ocfs2_delete_inode. */
 		oi->ip_flags |= OCFS2_INODE_MAYBE_ORPHANED;
-- 
1.8.1.4

[Ocfs2-devel] [PATCH 2/5] ocfs2: Move dquot_initialize() in ocfs2_delete_inode() somewhat later

[Jan Kara]

Move dquot_initalize() call in ocfs2_delete_inode() after the moment we
verify inode is actually a sane one to delete. We certainly don't want
to initialize quota for system inodes etc. This also avoids calling into
quota code from downconvert thread.

Add more details into the comment why bailing out from
ocfs2_delete_inode() when we are in downconvert thread is OK.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ocfs2/inode.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
index b4baaefe4dd4..3b0d722de35e 100644
--- a/fs/ocfs2/inode.c
+++ b/fs/ocfs2/inode.c
@@ -804,11 +804,13 @@ static int ocfs2_inode_is_valid_to_delete(struct inode *inode)
 		goto bail;
 	}
 
-	/* If we're coming from downconvert_thread we can't go into our own
-	 * voting [hello, deadlock city!], so unforuntately we just
-	 * have to skip deleting this guy. That's OK though because
-	 * the node who's doing the actual deleting should handle it
-	 * anyway. */
+	/*
+	 * If we're coming from downconvert_thread we can't go into our own
+	 * voting [hello, deadlock city!] so we cannot delete the inode. But
+	 * since we dropped last inode ref when downconverting dentry lock,
+	 * we cannot have the file open and thus the node doing unlink will
+	 * take care of deleting the inode.
+	 */
 	if (current == osb->dc_task)
 		goto bail;
 
@@ -954,8 +956,6 @@ static void ocfs2_delete_inode(struct inode *inode)
 	if (is_bad_inode(inode) || !OCFS2_I(inode)->ip_blkno)
 		goto bail;
 
-	dquot_initialize(inode);
-
 	if (!ocfs2_inode_is_valid_to_delete(inode)) {
 		/* It's probably not necessary to truncate_inode_pages
 		 * here but we do it for safety anyway (it will most
@@ -964,6 +964,8 @@ static void ocfs2_delete_inode(struct inode *inode)
 		goto bail;
 	}
 
+	dquot_initialize(inode);
+
 	/* We want to block signals in delete_inode as the lock and
 	 * messaging paths may return us -ERESTARTSYS. Which would
 	 * cause us to exit early, resulting in inodes being orphaned
-- 
1.8.1.4

[Ocfs2-devel] [PATCH 3/5] Revert iput deferring code in ocfs2_drop_dentry_lock

[Jan Kara]

From: Goldwyn Rodrigues <rgoldwyn@suse.de>

The following patches are reverted in this patch because these
patches caused regression in the unlink() calls.

ea455f8ab68338ba69f5d3362b342c115bea8e13 - ocfs2: Push out dropping
of dentry lock to ocfs2_wq
f7b1aa69be138ad9d7d3f31fa56f4c9407f56b6a - ocfs2: Fix deadlock on umount
5fd131893793567c361ae64cbeb28a2a753bbe35 - ocfs2: Don't oops in
ocfs2_kill_sb on a failed mount

The regression is caused because these patches delay the iput() in case
of dentry unlocks. This also delays the unlocking of the open lockres.
The open lockresource is required to test if the inode can be wiped from
disk on not. When the deleting node does not get the open lock, it marks
it as orphan (even though it is not in use by another node/process)
and causes a journal checkpoint. This delays operations following the
inode eviction. This also moves the inode to the orphaned inode
which further causes more I/O and a lot of unneccessary orphans.

As for the fix, I tried reproducing the problem by using quotas and enabling
lockdep, but the issue could not be reproduced.

Signed-off-by: Srinivas Eeda <srinivas.eeda@oracle.com>
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ocfs2/dcache.c | 61 +++----------------------------------------------------
 fs/ocfs2/dcache.h | 12 +----------
 fs/ocfs2/ocfs2.h  | 28 ++++---------------------
 fs/ocfs2/super.c  | 30 +--------------------------
 4 files changed, 9 insertions(+), 122 deletions(-)

diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
index 0d3a97d2d5f6..e2e05a106beb 100644
--- a/fs/ocfs2/dcache.c
+++ b/fs/ocfs2/dcache.c
@@ -37,7 +37,6 @@
 #include "dlmglue.h"
 #include "file.h"
 #include "inode.h"
-#include "super.h"
 #include "ocfs2_trace.h"
 
 void ocfs2_dentry_attach_gen(struct dentry *dentry)
@@ -346,52 +345,6 @@ out_attach:
 	return ret;
 }
 
-DEFINE_SPINLOCK(dentry_list_lock);
-
-/* We limit the number of dentry locks to drop in one go. We have
- * this limit so that we don't starve other users of ocfs2_wq. */
-#define DL_INODE_DROP_COUNT 64
-
-/* Drop inode references from dentry locks */
-static void __ocfs2_drop_dl_inodes(struct ocfs2_super *osb, int drop_count)
-{
-	struct ocfs2_dentry_lock *dl;
-
-	spin_lock(&dentry_list_lock);
-	while (osb->dentry_lock_list && (drop_count < 0 || drop_count--)) {
-		dl = osb->dentry_lock_list;
-		osb->dentry_lock_list = dl->dl_next;
-		spin_unlock(&dentry_list_lock);
-		iput(dl->dl_inode);
-		kfree(dl);
-		spin_lock(&dentry_list_lock);
-	}
-	spin_unlock(&dentry_list_lock);
-}
-
-void ocfs2_drop_dl_inodes(struct work_struct *work)
-{
-	struct ocfs2_super *osb = container_of(work, struct ocfs2_super,
-					       dentry_lock_work);
-
-	__ocfs2_drop_dl_inodes(osb, DL_INODE_DROP_COUNT);
-	/*
-	 * Don't queue dropping if umount is in progress. We flush the
-	 * list in ocfs2_dismount_volume
-	 */
-	spin_lock(&dentry_list_lock);
-	if (osb->dentry_lock_list &&
-	    !ocfs2_test_osb_flag(osb, OCFS2_OSB_DROP_DENTRY_LOCK_IMMED))
-		queue_work(ocfs2_wq, &osb->dentry_lock_work);
-	spin_unlock(&dentry_list_lock);
-}
-
-/* Flush the whole work queue */
-void ocfs2_drop_all_dl_inodes(struct ocfs2_super *osb)
-{
-	__ocfs2_drop_dl_inodes(osb, -1);
-}
-
 /*
  * ocfs2_dentry_iput() and friends.
  *
@@ -416,24 +369,16 @@ void ocfs2_drop_all_dl_inodes(struct ocfs2_super *osb)
 static void ocfs2_drop_dentry_lock(struct ocfs2_super *osb,
 				   struct ocfs2_dentry_lock *dl)
 {
+	iput(dl->dl_inode);
 	ocfs2_simple_drop_lockres(osb, &dl->dl_lockres);
 	ocfs2_lock_res_free(&dl->dl_lockres);
-
-	/* We leave dropping of inode reference to ocfs2_wq as that can
-	 * possibly lead to inode deletion which gets tricky */
-	spin_lock(&dentry_list_lock);
-	if (!osb->dentry_lock_list &&
-	    !ocfs2_test_osb_flag(osb, OCFS2_OSB_DROP_DENTRY_LOCK_IMMED))
-		queue_work(ocfs2_wq, &osb->dentry_lock_work);
-	dl->dl_next = osb->dentry_lock_list;
-	osb->dentry_lock_list = dl;
-	spin_unlock(&dentry_list_lock);
+	kfree(dl);
 }
 
 void ocfs2_dentry_lock_put(struct ocfs2_super *osb,
 			   struct ocfs2_dentry_lock *dl)
 {
-	int unlock;
+	int unlock = 0;
 
 	BUG_ON(dl->dl_count == 0);
 
diff --git a/fs/ocfs2/dcache.h b/fs/ocfs2/dcache.h
index b79eff709958..55f58892b153 100644
--- a/fs/ocfs2/dcache.h
+++ b/fs/ocfs2/dcache.h
@@ -29,13 +29,8 @@
 extern const struct dentry_operations ocfs2_dentry_ops;
 
 struct ocfs2_dentry_lock {
-	/* Use count of dentry lock */
 	unsigned int		dl_count;
-	union {
-		/* Linked list of dentry locks to release */
-		struct ocfs2_dentry_lock *dl_next;
-		u64			dl_parent_blkno;
-	};
+	u64			dl_parent_blkno;
 
 	/*
 	 * The ocfs2_dentry_lock keeps an inode reference until
@@ -49,14 +44,9 @@ struct ocfs2_dentry_lock {
 int ocfs2_dentry_attach_lock(struct dentry *dentry, struct inode *inode,
 			     u64 parent_blkno);
 
-extern spinlock_t dentry_list_lock;
-
 void ocfs2_dentry_lock_put(struct ocfs2_super *osb,
 			   struct ocfs2_dentry_lock *dl);
 
-void ocfs2_drop_dl_inodes(struct work_struct *work);
-void ocfs2_drop_all_dl_inodes(struct ocfs2_super *osb);
-
 struct dentry *ocfs2_find_local_alias(struct inode *inode, u64 parent_blkno,
 				      int skip_unhashed);
 
diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
index 3a903470c794..ca81f6b49236 100644
--- a/fs/ocfs2/ocfs2.h
+++ b/fs/ocfs2/ocfs2.h
@@ -274,19 +274,16 @@ enum ocfs2_mount_options
 	OCFS2_MOUNT_HB_GLOBAL = 1 << 14, /* Global heartbeat */
 };
 
-#define OCFS2_OSB_SOFT_RO			0x0001
-#define OCFS2_OSB_HARD_RO			0x0002
-#define OCFS2_OSB_ERROR_FS			0x0004
-#define OCFS2_OSB_DROP_DENTRY_LOCK_IMMED	0x0008
-
-#define OCFS2_DEFAULT_ATIME_QUANTUM		60
+#define OCFS2_OSB_SOFT_RO	0x0001
+#define OCFS2_OSB_HARD_RO	0x0002
+#define OCFS2_OSB_ERROR_FS	0x0004
+#define OCFS2_DEFAULT_ATIME_QUANTUM	60
 
 struct ocfs2_journal;
 struct ocfs2_slot_info;
 struct ocfs2_recovery_map;
 struct ocfs2_replay_map;
 struct ocfs2_quota_recovery;
-struct ocfs2_dentry_lock;
 struct ocfs2_super
 {
 	struct task_struct *commit_task;
@@ -413,11 +410,6 @@ struct ocfs2_super
 	struct list_head blocked_lock_list;
 	unsigned long blocked_lock_count;
 
-	/* List of dentry locks to release. Anyone can add locks to
-	 * the list, ocfs2_wq processes the list  */
-	struct ocfs2_dentry_lock *dentry_lock_list;
-	struct work_struct dentry_lock_work;
-
 	wait_queue_head_t		osb_mount_event;
 
 	/* Truncate log info */
@@ -578,18 +570,6 @@ static inline void ocfs2_set_osb_flag(struct ocfs2_super *osb,
 	spin_unlock(&osb->osb_lock);
 }
 
-
-static inline unsigned long  ocfs2_test_osb_flag(struct ocfs2_super *osb,
-						 unsigned long flag)
-{
-	unsigned long ret;
-
-	spin_lock(&osb->osb_lock);
-	ret = osb->osb_flags & flag;
-	spin_unlock(&osb->osb_lock);
-	return ret;
-}
-
 static inline void ocfs2_set_ro_flag(struct ocfs2_super *osb,
 				     int hard)
 {
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index c41492957aa5..c7f71360666f 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -1238,30 +1238,11 @@ static struct dentry *ocfs2_mount(struct file_system_type *fs_type,
 	return mount_bdev(fs_type, flags, dev_name, data, ocfs2_fill_super);
 }
 
-static void ocfs2_kill_sb(struct super_block *sb)
-{
-	struct ocfs2_super *osb = OCFS2_SB(sb);
-
-	/* Failed mount? */
-	if (!osb || atomic_read(&osb->vol_state) == VOLUME_DISABLED)
-		goto out;
-
-	/* Prevent further queueing of inode drop events */
-	spin_lock(&dentry_list_lock);
-	ocfs2_set_osb_flag(osb, OCFS2_OSB_DROP_DENTRY_LOCK_IMMED);
-	spin_unlock(&dentry_list_lock);
-	/* Wait for work to finish and/or remove it */
-	cancel_work_sync(&osb->dentry_lock_work);
-out:
-	kill_block_super(sb);
-}
-
 static struct file_system_type ocfs2_fs_type = {
 	.owner          = THIS_MODULE,
 	.name           = "ocfs2",
 	.mount          = ocfs2_mount,
-	.kill_sb        = ocfs2_kill_sb,
-
+	.kill_sb        = kill_block_super,
 	.fs_flags       = FS_REQUIRES_DEV|FS_RENAME_DOES_D_MOVE,
 	.next           = NULL
 };
@@ -1934,12 +1915,6 @@ static void ocfs2_dismount_volume(struct super_block *sb, int mnt_err)
 
 	debugfs_remove(osb->osb_ctxt);
 
-	/*
-	 * Flush inode dropping work queue so that deletes are
-	 * performed while the filesystem is still working
-	 */
-	ocfs2_drop_all_dl_inodes(osb);
-
 	/* Orphan scan should be stopped as early as possible */
 	ocfs2_orphan_scan_stop(osb);
 
@@ -2272,9 +2247,6 @@ static int ocfs2_initialize_super(struct super_block *sb,
 	INIT_WORK(&journal->j_recovery_work, ocfs2_complete_recovery);
 	journal->j_state = OCFS2_JOURNAL_FREE;
 
-	INIT_WORK(&osb->dentry_lock_work, ocfs2_drop_dl_inodes);
-	osb->dentry_lock_list = NULL;
-
 	/* get some pseudo constants for clustersize bits */
 	osb->s_clustersize_bits =
 		le32_to_cpu(di->id2.i_super.s_clustersize_bits);
-- 
1.8.1.4

[Ocfs2-devel] [PATCH 4/5] quota: Provide function to grab quota structure reference

[Jan Kara]

Provide dqgrab() function to get quota structure reference when we are
sure it already has at least one active reference. Make use of this
function inside quota code.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/quota/dquot.c         | 4 ++--
 include/linux/quotaops.h | 8 ++++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 831d49a4111f..e3f09e34d0b2 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -528,7 +528,7 @@ restart:
 		if (atomic_read(&dquot->dq_count)) {
 			DEFINE_WAIT(wait);
 
-			atomic_inc(&dquot->dq_count);
+			dqgrab(dquot);
 			prepare_to_wait(&dquot->dq_wait_unused, &wait,
 					TASK_UNINTERRUPTIBLE);
 			spin_unlock(&dq_list_lock);
@@ -624,7 +624,7 @@ int dquot_writeback_dquots(struct super_block *sb, int type)
 			/* Now we have active dquot from which someone is
  			 * holding reference so we can safely just increase
 			 * use count */
-			atomic_inc(&dquot->dq_count);
+			dqgrab(dquot);
 			spin_unlock(&dq_list_lock);
 			dqstats_inc(DQST_LOOKUPS);
 			err = sb->dq_op->write_dquot(dquot);
diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
index 6965fe394c3b..1d3eee594cd6 100644
--- a/include/linux/quotaops.h
+++ b/include/linux/quotaops.h
@@ -46,6 +46,14 @@ void inode_reclaim_rsv_space(struct inode *inode, qsize_t number);
 void dquot_initialize(struct inode *inode);
 void dquot_drop(struct inode *inode);
 struct dquot *dqget(struct super_block *sb, struct kqid qid);
+static inline struct dquot *dqgrab(struct dquot *dquot)
+{
+	/* Make sure someone else has active reference to dquot */
+	WARN_ON_ONCE(!atomic_read(&dquot->dq_count));
+	WARN_ON_ONCE(!test_bit(DQ_ACTIVE_B, &dquot->dq_flags));
+	atomic_inc(&dquot->dq_count);
+	return dquot;
+}
 void dqput(struct dquot *dquot);
 int dquot_scan_active(struct super_block *sb,
 		      int (*fn)(struct dquot *dquot, unsigned long priv),
-- 
1.8.1.4

[Ocfs2-devel] [PATCH 5/5] ocfs2: Implement delayed dropping of last dquot reference

[Jan Kara]

We cannot drop last dquot reference from downconvert thread as that
creates the following deadlock:

NODE 1                                  NODE2
holds dentry lock for 'foo'
holds inode lock for GLOBAL_BITMAP_SYSTEM_INODE
                                        dquot_initialize(bar)
                                          ocfs2_dquot_acquire()
                                            ocfs2_inode_lock(USER_QUOTA_SYSTEM_INODE)
                                            ...
downconvert thread (triggered from another
node or a different process from NODE2)
  ocfs2_dentry_post_unlock()
    ...
    iput(foo)
      ocfs2_evict_inode(foo)
        ocfs2_clear_inode(foo)
          dquot_drop(inode)
            ...
	    ocfs2_dquot_release()
              ocfs2_inode_lock(USER_QUOTA_SYSTEM_INODE)
               - blocks
                                            finds we need more space in
                                            quota file
                                            ...
                                            ocfs2_extend_no_holes()
                                              ocfs2_inode_lock(GLOBAL_BITMAP_SYSTEM_INODE)
                                                - deadlocks waiting for
                                                  downconvert thread

We solve the problem by postponing dropping of the last dquot reference
to a workqueue if it happens from the downconvert thread.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ocfs2/ocfs2.h        |  5 +++++
 fs/ocfs2/quota.h        |  2 ++
 fs/ocfs2/quota_global.c | 35 +++++++++++++++++++++++++++++++++++
 fs/ocfs2/super.c        |  8 ++++++++
 4 files changed, 50 insertions(+)

diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
index ca81f6b49236..f6134345fe42 100644
--- a/fs/ocfs2/ocfs2.h
+++ b/fs/ocfs2/ocfs2.h
@@ -30,6 +30,7 @@
 #include <linux/sched.h>
 #include <linux/wait.h>
 #include <linux/list.h>
+#include <linux/llist.h>
 #include <linux/rbtree.h>
 #include <linux/workqueue.h>
 #include <linux/kref.h>
@@ -410,6 +411,10 @@ struct ocfs2_super
 	struct list_head blocked_lock_list;
 	unsigned long blocked_lock_count;
 
+	/* List of dquot structures to drop last reference to */
+	struct llist_head dquot_drop_list;
+	struct work_struct dquot_drop_work;
+
 	wait_queue_head_t		osb_mount_event;
 
 	/* Truncate log info */
diff --git a/fs/ocfs2/quota.h b/fs/ocfs2/quota.h
index d5ab56cbe5c5..f266d67df3c6 100644
--- a/fs/ocfs2/quota.h
+++ b/fs/ocfs2/quota.h
@@ -28,6 +28,7 @@ struct ocfs2_dquot {
 	unsigned int dq_use_count;	/* Number of nodes having reference to this entry in global quota file */
 	s64 dq_origspace;	/* Last globally synced space usage */
 	s64 dq_originodes;	/* Last globally synced inode usage */
+	struct llist_node list;	/* Member of list of dquots to drop */
 };
 
 /* Description of one chunk to recover in memory */
@@ -110,6 +111,7 @@ int ocfs2_read_quota_phys_block(struct inode *inode, u64 p_block,
 int ocfs2_create_local_dquot(struct dquot *dquot);
 int ocfs2_local_release_dquot(handle_t *handle, struct dquot *dquot);
 int ocfs2_local_write_dquot(struct dquot *dquot);
+void ocfs2_drop_dquot_refs(struct work_struct *work);
 
 extern const struct dquot_operations ocfs2_quota_operations;
 extern struct quota_format_type ocfs2_quota_format;
diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
index aaa50611ec66..7921e209c64b 100644
--- a/fs/ocfs2/quota_global.c
+++ b/fs/ocfs2/quota_global.c
@@ -10,6 +10,7 @@
 #include <linux/jiffies.h>
 #include <linux/writeback.h>
 #include <linux/workqueue.h>
+#include <linux/llist.h>
 
 #include <cluster/masklog.h>
 
@@ -679,6 +680,27 @@ static int ocfs2_calc_qdel_credits(struct super_block *sb, int type)
 	       OCFS2_INODE_UPDATE_CREDITS;
 }
 
+void ocfs2_drop_dquot_refs(struct work_struct *work)
+{
+	struct ocfs2_super *osb = container_of(work, struct ocfs2_super,
+					       dquot_drop_work);
+	struct llist_node *list;
+	struct ocfs2_dquot *odquot, *next_odquot;
+
+	list = llist_del_all(&osb->dquot_drop_list);
+	llist_for_each_entry_safe(odquot, next_odquot, list, list) {
+		/* Drop the reference we acquired in ocfs2_dquot_release() */
+		dqput(&odquot->dq_dquot);
+	}
+}
+
+/*
+ * Called when the last reference to dquot is dropped. If we are called from
+ * downconvert thread, we cannot do all the handling here because grabbing
+ * quota lock could deadlock (the node holding the quota lock could need some
+ * other cluster lock to proceed but with blocked downconvert thread we cannot
+ * release any lock).
+ */
 static int ocfs2_release_dquot(struct dquot *dquot)
 {
 	handle_t *handle;
@@ -694,6 +716,19 @@ static int ocfs2_release_dquot(struct dquot *dquot)
 	/* Check whether we are not racing with some other dqget() */
 	if (atomic_read(&dquot->dq_count) > 1)
 		goto out;
+	/* Running from downconvert thread? Postpone quota processing to wq */
+	if (current == osb->dc_task) {
+		/*
+		 * Grab our own reference to dquot and queue it for delayed
+		 * dropping.  Quota code rechecks after calling
+		 * ->release_dquot() and won't free dquot structure.
+		 */
+		dqgrab(dquot);
+		/* First entry on list -> queue work */
+		if (llist_add(&OCFS2_DQUOT(dquot)->list, &osb->dquot_drop_list))
+			queue_work(ocfs2_wq, &osb->dquot_drop_work);
+		goto out;
+	}
 	status = ocfs2_lock_global_qf(oinfo, 1);
 	if (status < 0)
 		goto out;
diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
index c7f71360666f..0c5ea9454967 100644
--- a/fs/ocfs2/super.c
+++ b/fs/ocfs2/super.c
@@ -1920,6 +1920,11 @@ static void ocfs2_dismount_volume(struct super_block *sb, int mnt_err)
 
 	ocfs2_disable_quotas(osb);
 
+	/* All dquots should be freed by now */
+	WARN_ON(!llist_empty(&osb->dquot_drop_list));
+	/* Wait for worker to be done with the work structure in osb */
+	cancel_work_sync(&osb->dquot_drop_work);
+
 	ocfs2_shutdown_local_alloc(osb);
 
 	ocfs2_truncate_log_shutdown(osb);
@@ -2247,6 +2252,9 @@ static int ocfs2_initialize_super(struct super_block *sb,
 	INIT_WORK(&journal->j_recovery_work, ocfs2_complete_recovery);
 	journal->j_state = OCFS2_JOURNAL_FREE;
 
+	INIT_WORK(&osb->dquot_drop_work, ocfs2_drop_dquot_refs);
+	init_llist_head(&osb->dquot_drop_list);
+
 	/* get some pseudo constants for clustersize bits */
 	osb->s_clustersize_bits =
 		le32_to_cpu(di->id2.i_super.s_clustersize_bits);
-- 
1.8.1.4

[Ocfs2-devel] [PATCH 5/5] ocfs2: Implement delayed dropping of last dquot reference

[Jan Kara]

On Thu 16-01-14 23:28:49, Jan Kara wrote:
> We cannot drop last dquot reference from downconvert thread as that
> creates the following deadlock:
> 
> NODE 1                                  NODE2
> holds dentry lock for 'foo'
> holds inode lock for GLOBAL_BITMAP_SYSTEM_INODE
>                                         dquot_initialize(bar)
>                                           ocfs2_dquot_acquire()
>                                             ocfs2_inode_lock(USER_QUOTA_SYSTEM_INODE)
>                                             ...
> downconvert thread (triggered from another
> node or a different process from NODE2)
>   ocfs2_dentry_post_unlock()
>     ...
>     iput(foo)
>       ocfs2_evict_inode(foo)
>         ocfs2_clear_inode(foo)
>           dquot_drop(inode)
>             ...
> 	    ocfs2_dquot_release()
>               ocfs2_inode_lock(USER_QUOTA_SYSTEM_INODE)
>                - blocks
>                                             finds we need more space in
>                                             quota file
>                                             ...
>                                             ocfs2_extend_no_holes()
>                                               ocfs2_inode_lock(GLOBAL_BITMAP_SYSTEM_INODE)
>                                                 - deadlocks waiting for
>                                                   downconvert thread
> 
> We solve the problem by postponing dropping of the last dquot reference
> to a workqueue if it happens from the downconvert thread.
  Hum, now looking again into ocfs2_clear_inode() there are more problems
than I originally thought. Look for example at
ocfs2_mark_lockres_freeing(). That will block on rw/inode/open lock if
there is downconvert pending waiting for that downconvert to finish.
However that never happens when ocfs2_clear_inode() is called from the
downconvert thread.

So we are back to square one - I don't see a way how to fix these deadlocks
without postponing dropping of inode reference to a workqueue :(.

								Honza


> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ocfs2/ocfs2.h        |  5 +++++
>  fs/ocfs2/quota.h        |  2 ++
>  fs/ocfs2/quota_global.c | 35 +++++++++++++++++++++++++++++++++++
>  fs/ocfs2/super.c        |  8 ++++++++
>  4 files changed, 50 insertions(+)
> 
> diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
> index ca81f6b49236..f6134345fe42 100644
> --- a/fs/ocfs2/ocfs2.h
> +++ b/fs/ocfs2/ocfs2.h
> @@ -30,6 +30,7 @@
>  #include <linux/sched.h>
>  #include <linux/wait.h>
>  #include <linux/list.h>
> +#include <linux/llist.h>
>  #include <linux/rbtree.h>
>  #include <linux/workqueue.h>
>  #include <linux/kref.h>
> @@ -410,6 +411,10 @@ struct ocfs2_super
>  	struct list_head blocked_lock_list;
>  	unsigned long blocked_lock_count;
>  
> +	/* List of dquot structures to drop last reference to */
> +	struct llist_head dquot_drop_list;
> +	struct work_struct dquot_drop_work;
> +
>  	wait_queue_head_t		osb_mount_event;
>  
>  	/* Truncate log info */
> diff --git a/fs/ocfs2/quota.h b/fs/ocfs2/quota.h
> index d5ab56cbe5c5..f266d67df3c6 100644
> --- a/fs/ocfs2/quota.h
> +++ b/fs/ocfs2/quota.h
> @@ -28,6 +28,7 @@ struct ocfs2_dquot {
>  	unsigned int dq_use_count;	/* Number of nodes having reference to this entry in global quota file */
>  	s64 dq_origspace;	/* Last globally synced space usage */
>  	s64 dq_originodes;	/* Last globally synced inode usage */
> +	struct llist_node list;	/* Member of list of dquots to drop */
>  };
>  
>  /* Description of one chunk to recover in memory */
> @@ -110,6 +111,7 @@ int ocfs2_read_quota_phys_block(struct inode *inode, u64 p_block,
>  int ocfs2_create_local_dquot(struct dquot *dquot);
>  int ocfs2_local_release_dquot(handle_t *handle, struct dquot *dquot);
>  int ocfs2_local_write_dquot(struct dquot *dquot);
> +void ocfs2_drop_dquot_refs(struct work_struct *work);
>  
>  extern const struct dquot_operations ocfs2_quota_operations;
>  extern struct quota_format_type ocfs2_quota_format;
> diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
> index aaa50611ec66..7921e209c64b 100644
> --- a/fs/ocfs2/quota_global.c
> +++ b/fs/ocfs2/quota_global.c
> @@ -10,6 +10,7 @@
>  #include <linux/jiffies.h>
>  #include <linux/writeback.h>
>  #include <linux/workqueue.h>
> +#include <linux/llist.h>
>  
>  #include <cluster/masklog.h>
>  
> @@ -679,6 +680,27 @@ static int ocfs2_calc_qdel_credits(struct super_block *sb, int type)
>  	       OCFS2_INODE_UPDATE_CREDITS;
>  }
>  
> +void ocfs2_drop_dquot_refs(struct work_struct *work)
> +{
> +	struct ocfs2_super *osb = container_of(work, struct ocfs2_super,
> +					       dquot_drop_work);
> +	struct llist_node *list;
> +	struct ocfs2_dquot *odquot, *next_odquot;
> +
> +	list = llist_del_all(&osb->dquot_drop_list);
> +	llist_for_each_entry_safe(odquot, next_odquot, list, list) {
> +		/* Drop the reference we acquired in ocfs2_dquot_release() */
> +		dqput(&odquot->dq_dquot);
> +	}
> +}
> +
> +/*
> + * Called when the last reference to dquot is dropped. If we are called from
> + * downconvert thread, we cannot do all the handling here because grabbing
> + * quota lock could deadlock (the node holding the quota lock could need some
> + * other cluster lock to proceed but with blocked downconvert thread we cannot
> + * release any lock).
> + */
>  static int ocfs2_release_dquot(struct dquot *dquot)
>  {
>  	handle_t *handle;
> @@ -694,6 +716,19 @@ static int ocfs2_release_dquot(struct dquot *dquot)
>  	/* Check whether we are not racing with some other dqget() */
>  	if (atomic_read(&dquot->dq_count) > 1)
>  		goto out;
> +	/* Running from downconvert thread? Postpone quota processing to wq */
> +	if (current == osb->dc_task) {
> +		/*
> +		 * Grab our own reference to dquot and queue it for delayed
> +		 * dropping.  Quota code rechecks after calling
> +		 * ->release_dquot() and won't free dquot structure.
> +		 */
> +		dqgrab(dquot);
> +		/* First entry on list -> queue work */
> +		if (llist_add(&OCFS2_DQUOT(dquot)->list, &osb->dquot_drop_list))
> +			queue_work(ocfs2_wq, &osb->dquot_drop_work);
> +		goto out;
> +	}
>  	status = ocfs2_lock_global_qf(oinfo, 1);
>  	if (status < 0)
>  		goto out;
> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> index c7f71360666f..0c5ea9454967 100644
> --- a/fs/ocfs2/super.c
> +++ b/fs/ocfs2/super.c
> @@ -1920,6 +1920,11 @@ static void ocfs2_dismount_volume(struct super_block *sb, int mnt_err)
>  
>  	ocfs2_disable_quotas(osb);
>  
> +	/* All dquots should be freed by now */
> +	WARN_ON(!llist_empty(&osb->dquot_drop_list));
> +	/* Wait for worker to be done with the work structure in osb */
> +	cancel_work_sync(&osb->dquot_drop_work);
> +
>  	ocfs2_shutdown_local_alloc(osb);
>  
>  	ocfs2_truncate_log_shutdown(osb);
> @@ -2247,6 +2252,9 @@ static int ocfs2_initialize_super(struct super_block *sb,
>  	INIT_WORK(&journal->j_recovery_work, ocfs2_complete_recovery);
>  	journal->j_state = OCFS2_JOURNAL_FREE;
>  
> +	INIT_WORK(&osb->dquot_drop_work, ocfs2_drop_dquot_refs);
> +	init_llist_head(&osb->dquot_drop_list);
> +
>  	/* get some pseudo constants for clustersize bits */
>  	osb->s_clustersize_bits =
>  		le32_to_cpu(di->id2.i_super.s_clustersize_bits);
> -- 
> 1.8.1.4
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[Ocfs2-devel] [PATCH 5/5] ocfs2: Implement delayed dropping of last dquot reference

[Goldwyn Rodrigues]

On 01/16/2014 04:58 PM, Jan Kara wrote:
> On Thu 16-01-14 23:28:49, Jan Kara wrote:
>> We cannot drop last dquot reference from downconvert thread as that
>> creates the following deadlock:
>>
>> NODE 1                                  NODE2
>> holds dentry lock for 'foo'
>> holds inode lock for GLOBAL_BITMAP_SYSTEM_INODE
>>                                          dquot_initialize(bar)
>>                                            ocfs2_dquot_acquire()
>>                                              ocfs2_inode_lock(USER_QUOTA_SYSTEM_INODE)
>>                                              ...
>> downconvert thread (triggered from another
>> node or a different process from NODE2)
>>    ocfs2_dentry_post_unlock()
>>      ...
>>      iput(foo)
>>        ocfs2_evict_inode(foo)
>>          ocfs2_clear_inode(foo)
>>            dquot_drop(inode)
>>              ...
>> 	    ocfs2_dquot_release()
>>                ocfs2_inode_lock(USER_QUOTA_SYSTEM_INODE)
>>                 - blocks
>>                                              finds we need more space in
>>                                              quota file
>>                                              ...
>>                                              ocfs2_extend_no_holes()
>>                                                ocfs2_inode_lock(GLOBAL_BITMAP_SYSTEM_INODE)
>>                                                  - deadlocks waiting for
>>                                                    downconvert thread
>>
>> We solve the problem by postponing dropping of the last dquot reference
>> to a workqueue if it happens from the downconvert thread.
>    Hum, now looking again into ocfs2_clear_inode() there are more problems
> than I originally thought. Look for example at
> ocfs2_mark_lockres_freeing(). That will block on rw/inode/open lock if
> there is downconvert pending waiting for that downconvert to finish.
> However that never happens when ocfs2_clear_inode() is called from the
> downconvert thread.
>
> So we are back to square one - I don't see a way how to fix these deadlocks
> without postponing dropping of inode reference to a workqueue :(.
>

Since the reason of the unlink performance is the delay in calling 
ocfs2_open_unlock(), and the ocfs2_mark_lockres_freeing() comes after 
ocfs2_open_unlock(): can we move the call to ocfs2_open_unlock() to 
ocfs2_evict_inode() and then perform ocfs2_clear_inode() in a deferred way?


> 								Honza
>
>
>>
>> Signed-off-by: Jan Kara <jack@suse.cz>
>> ---
>>   fs/ocfs2/ocfs2.h        |  5 +++++
>>   fs/ocfs2/quota.h        |  2 ++
>>   fs/ocfs2/quota_global.c | 35 +++++++++++++++++++++++++++++++++++
>>   fs/ocfs2/super.c        |  8 ++++++++
>>   4 files changed, 50 insertions(+)
>>
>> diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
>> index ca81f6b49236..f6134345fe42 100644
>> --- a/fs/ocfs2/ocfs2.h
>> +++ b/fs/ocfs2/ocfs2.h
>> @@ -30,6 +30,7 @@
>>   #include <linux/sched.h>
>>   #include <linux/wait.h>
>>   #include <linux/list.h>
>> +#include <linux/llist.h>
>>   #include <linux/rbtree.h>
>>   #include <linux/workqueue.h>
>>   #include <linux/kref.h>
>> @@ -410,6 +411,10 @@ struct ocfs2_super
>>   	struct list_head blocked_lock_list;
>>   	unsigned long blocked_lock_count;
>>
>> +	/* List of dquot structures to drop last reference to */
>> +	struct llist_head dquot_drop_list;
>> +	struct work_struct dquot_drop_work;
>> +
>>   	wait_queue_head_t		osb_mount_event;
>>
>>   	/* Truncate log info */
>> diff --git a/fs/ocfs2/quota.h b/fs/ocfs2/quota.h
>> index d5ab56cbe5c5..f266d67df3c6 100644
>> --- a/fs/ocfs2/quota.h
>> +++ b/fs/ocfs2/quota.h
>> @@ -28,6 +28,7 @@ struct ocfs2_dquot {
>>   	unsigned int dq_use_count;	/* Number of nodes having reference to this entry in global quota file */
>>   	s64 dq_origspace;	/* Last globally synced space usage */
>>   	s64 dq_originodes;	/* Last globally synced inode usage */
>> +	struct llist_node list;	/* Member of list of dquots to drop */
>>   };
>>
>>   /* Description of one chunk to recover in memory */
>> @@ -110,6 +111,7 @@ int ocfs2_read_quota_phys_block(struct inode *inode, u64 p_block,
>>   int ocfs2_create_local_dquot(struct dquot *dquot);
>>   int ocfs2_local_release_dquot(handle_t *handle, struct dquot *dquot);
>>   int ocfs2_local_write_dquot(struct dquot *dquot);
>> +void ocfs2_drop_dquot_refs(struct work_struct *work);
>>
>>   extern const struct dquot_operations ocfs2_quota_operations;
>>   extern struct quota_format_type ocfs2_quota_format;
>> diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
>> index aaa50611ec66..7921e209c64b 100644
>> --- a/fs/ocfs2/quota_global.c
>> +++ b/fs/ocfs2/quota_global.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/jiffies.h>
>>   #include <linux/writeback.h>
>>   #include <linux/workqueue.h>
>> +#include <linux/llist.h>
>>
>>   #include <cluster/masklog.h>
>>
>> @@ -679,6 +680,27 @@ static int ocfs2_calc_qdel_credits(struct super_block *sb, int type)
>>   	       OCFS2_INODE_UPDATE_CREDITS;
>>   }
>>
>> +void ocfs2_drop_dquot_refs(struct work_struct *work)
>> +{
>> +	struct ocfs2_super *osb = container_of(work, struct ocfs2_super,
>> +					       dquot_drop_work);
>> +	struct llist_node *list;
>> +	struct ocfs2_dquot *odquot, *next_odquot;
>> +
>> +	list = llist_del_all(&osb->dquot_drop_list);
>> +	llist_for_each_entry_safe(odquot, next_odquot, list, list) {
>> +		/* Drop the reference we acquired in ocfs2_dquot_release() */
>> +		dqput(&odquot->dq_dquot);
>> +	}
>> +}
>> +
>> +/*
>> + * Called when the last reference to dquot is dropped. If we are called from
>> + * downconvert thread, we cannot do all the handling here because grabbing
>> + * quota lock could deadlock (the node holding the quota lock could need some
>> + * other cluster lock to proceed but with blocked downconvert thread we cannot
>> + * release any lock).
>> + */
>>   static int ocfs2_release_dquot(struct dquot *dquot)
>>   {
>>   	handle_t *handle;
>> @@ -694,6 +716,19 @@ static int ocfs2_release_dquot(struct dquot *dquot)
>>   	/* Check whether we are not racing with some other dqget() */
>>   	if (atomic_read(&dquot->dq_count) > 1)
>>   		goto out;
>> +	/* Running from downconvert thread? Postpone quota processing to wq */
>> +	if (current == osb->dc_task) {
>> +		/*
>> +		 * Grab our own reference to dquot and queue it for delayed
>> +		 * dropping.  Quota code rechecks after calling
>> +		 * ->release_dquot() and won't free dquot structure.
>> +		 */
>> +		dqgrab(dquot);
>> +		/* First entry on list -> queue work */
>> +		if (llist_add(&OCFS2_DQUOT(dquot)->list, &osb->dquot_drop_list))
>> +			queue_work(ocfs2_wq, &osb->dquot_drop_work);
>> +		goto out;
>> +	}
>>   	status = ocfs2_lock_global_qf(oinfo, 1);
>>   	if (status < 0)
>>   		goto out;
>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>> index c7f71360666f..0c5ea9454967 100644
>> --- a/fs/ocfs2/super.c
>> +++ b/fs/ocfs2/super.c
>> @@ -1920,6 +1920,11 @@ static void ocfs2_dismount_volume(struct super_block *sb, int mnt_err)
>>
>>   	ocfs2_disable_quotas(osb);
>>
>> +	/* All dquots should be freed by now */
>> +	WARN_ON(!llist_empty(&osb->dquot_drop_list));
>> +	/* Wait for worker to be done with the work structure in osb */
>> +	cancel_work_sync(&osb->dquot_drop_work);
>> +
>>   	ocfs2_shutdown_local_alloc(osb);
>>
>>   	ocfs2_truncate_log_shutdown(osb);
>> @@ -2247,6 +2252,9 @@ static int ocfs2_initialize_super(struct super_block *sb,
>>   	INIT_WORK(&journal->j_recovery_work, ocfs2_complete_recovery);
>>   	journal->j_state = OCFS2_JOURNAL_FREE;
>>
>> +	INIT_WORK(&osb->dquot_drop_work, ocfs2_drop_dquot_refs);
>> +	init_llist_head(&osb->dquot_drop_list);
>> +
>>   	/* get some pseudo constants for clustersize bits */
>>   	osb->s_clustersize_bits =
>>   		le32_to_cpu(di->id2.i_super.s_clustersize_bits);
>> --
>> 1.8.1.4
>>


-- 
Goldwyn

[Ocfs2-devel] [PATCH 5/5] ocfs2: Implement delayed dropping of last dquot reference

[Srinivas Eeda]

On 01/20/2014 07:31 AM, Goldwyn Rodrigues wrote:
> On 01/16/2014 04:58 PM, Jan Kara wrote:
>> On Thu 16-01-14 23:28:49, Jan Kara wrote:
>>> We cannot drop last dquot reference from downconvert thread as that
>>> creates the following deadlock:
>>>
>>> NODE 1                                  NODE2
>>> holds dentry lock for 'foo'
>>> holds inode lock for GLOBAL_BITMAP_SYSTEM_INODE
>>>                                          dquot_initialize(bar)
>>> ocfs2_dquot_acquire()
>>> ocfs2_inode_lock(USER_QUOTA_SYSTEM_INODE)
>>>                                              ...
>>> downconvert thread (triggered from another
>>> node or a different process from NODE2)
>>>    ocfs2_dentry_post_unlock()
>>>      ...
>>>      iput(foo)
>>>        ocfs2_evict_inode(foo)
>>>          ocfs2_clear_inode(foo)
>>>            dquot_drop(inode)
>>>              ...
>>>         ocfs2_dquot_release()
>>>                ocfs2_inode_lock(USER_QUOTA_SYSTEM_INODE)
>>>                 - blocks
>>>                                              finds we need more 
>>> space in
>>>                                              quota file
>>>                                              ...
>>> ocfs2_extend_no_holes()
>>> ocfs2_inode_lock(GLOBAL_BITMAP_SYSTEM_INODE)
>>>                                                  - deadlocks waiting 
>>> for
>>>                                                    downconvert thread
>>>
>>> We solve the problem by postponing dropping of the last dquot reference
>>> to a workqueue if it happens from the downconvert thread.
>>    Hum, now looking again into ocfs2_clear_inode() there are more 
>> problems
>> than I originally thought. Look for example at
>> ocfs2_mark_lockres_freeing(). That will block on rw/inode/open lock if
>> there is downconvert pending waiting for that downconvert to finish.
>> However that never happens when ocfs2_clear_inode() is called from the
>> downconvert thread.
>>
>> So we are back to square one - I don't see a way how to fix these 
>> deadlocks
>> without postponing dropping of inode reference to a workqueue :(.
>>
>
> Since the reason of the unlink performance is the delay in calling 
> ocfs2_open_unlock(), and the ocfs2_mark_lockres_freeing() comes after 
> ocfs2_open_unlock(): can we move the call to ocfs2_open_unlock() to 
> ocfs2_evict_inode() and then perform ocfs2_clear_inode() in a deferred 
> way?
once ocfs2_evict_inode is returned, vfs would destroy the inode, so I 
think we should do the cleanup before that and hence we cannot differ 
ocfs2_clear_inode from inside ocfs2_evict_inode

May be we should queue ocfs2_blocking_ast call itself to down convert 
thread. That way it doesn't prevent down convert thread from clearing 
the inode. Once when down convert thread comes to processes the queued 
ast/bast and finds lockres cleared it can just return.


>
>
>>                                 Honza
>>
>>
>>>
>>> Signed-off-by: Jan Kara <jack@suse.cz>
>>> ---
>>>   fs/ocfs2/ocfs2.h        |  5 +++++
>>>   fs/ocfs2/quota.h        |  2 ++
>>>   fs/ocfs2/quota_global.c | 35 +++++++++++++++++++++++++++++++++++
>>>   fs/ocfs2/super.c        |  8 ++++++++
>>>   4 files changed, 50 insertions(+)
>>>
>>> diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
>>> index ca81f6b49236..f6134345fe42 100644
>>> --- a/fs/ocfs2/ocfs2.h
>>> +++ b/fs/ocfs2/ocfs2.h
>>> @@ -30,6 +30,7 @@
>>>   #include <linux/sched.h>
>>>   #include <linux/wait.h>
>>>   #include <linux/list.h>
>>> +#include <linux/llist.h>
>>>   #include <linux/rbtree.h>
>>>   #include <linux/workqueue.h>
>>>   #include <linux/kref.h>
>>> @@ -410,6 +411,10 @@ struct ocfs2_super
>>>       struct list_head blocked_lock_list;
>>>       unsigned long blocked_lock_count;
>>>
>>> +    /* List of dquot structures to drop last reference to */
>>> +    struct llist_head dquot_drop_list;
>>> +    struct work_struct dquot_drop_work;
>>> +
>>>       wait_queue_head_t        osb_mount_event;
>>>
>>>       /* Truncate log info */
>>> diff --git a/fs/ocfs2/quota.h b/fs/ocfs2/quota.h
>>> index d5ab56cbe5c5..f266d67df3c6 100644
>>> --- a/fs/ocfs2/quota.h
>>> +++ b/fs/ocfs2/quota.h
>>> @@ -28,6 +28,7 @@ struct ocfs2_dquot {
>>>       unsigned int dq_use_count;    /* Number of nodes having 
>>> reference to this entry in global quota file */
>>>       s64 dq_origspace;    /* Last globally synced space usage */
>>>       s64 dq_originodes;    /* Last globally synced inode usage */
>>> +    struct llist_node list;    /* Member of list of dquots to drop */
>>>   };
>>>
>>>   /* Description of one chunk to recover in memory */
>>> @@ -110,6 +111,7 @@ int ocfs2_read_quota_phys_block(struct inode 
>>> *inode, u64 p_block,
>>>   int ocfs2_create_local_dquot(struct dquot *dquot);
>>>   int ocfs2_local_release_dquot(handle_t *handle, struct dquot *dquot);
>>>   int ocfs2_local_write_dquot(struct dquot *dquot);
>>> +void ocfs2_drop_dquot_refs(struct work_struct *work);
>>>
>>>   extern const struct dquot_operations ocfs2_quota_operations;
>>>   extern struct quota_format_type ocfs2_quota_format;
>>> diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
>>> index aaa50611ec66..7921e209c64b 100644
>>> --- a/fs/ocfs2/quota_global.c
>>> +++ b/fs/ocfs2/quota_global.c
>>> @@ -10,6 +10,7 @@
>>>   #include <linux/jiffies.h>
>>>   #include <linux/writeback.h>
>>>   #include <linux/workqueue.h>
>>> +#include <linux/llist.h>
>>>
>>>   #include <cluster/masklog.h>
>>>
>>> @@ -679,6 +680,27 @@ static int ocfs2_calc_qdel_credits(struct 
>>> super_block *sb, int type)
>>>              OCFS2_INODE_UPDATE_CREDITS;
>>>   }
>>>
>>> +void ocfs2_drop_dquot_refs(struct work_struct *work)
>>> +{
>>> +    struct ocfs2_super *osb = container_of(work, struct ocfs2_super,
>>> +                           dquot_drop_work);
>>> +    struct llist_node *list;
>>> +    struct ocfs2_dquot *odquot, *next_odquot;
>>> +
>>> +    list = llist_del_all(&osb->dquot_drop_list);
>>> +    llist_for_each_entry_safe(odquot, next_odquot, list, list) {
>>> +        /* Drop the reference we acquired in ocfs2_dquot_release() */
>>> +        dqput(&odquot->dq_dquot);
>>> +    }
>>> +}
>>> +
>>> +/*
>>> + * Called when the last reference to dquot is dropped. If we are 
>>> called from
>>> + * downconvert thread, we cannot do all the handling here because 
>>> grabbing
>>> + * quota lock could deadlock (the node holding the quota lock could 
>>> need some
>>> + * other cluster lock to proceed but with blocked downconvert 
>>> thread we cannot
>>> + * release any lock).
>>> + */
>>>   static int ocfs2_release_dquot(struct dquot *dquot)
>>>   {
>>>       handle_t *handle;
>>> @@ -694,6 +716,19 @@ static int ocfs2_release_dquot(struct dquot 
>>> *dquot)
>>>       /* Check whether we are not racing with some other dqget() */
>>>       if (atomic_read(&dquot->dq_count) > 1)
>>>           goto out;
>>> +    /* Running from downconvert thread? Postpone quota processing 
>>> to wq */
>>> +    if (current == osb->dc_task) {
>>> +        /*
>>> +         * Grab our own reference to dquot and queue it for delayed
>>> +         * dropping.  Quota code rechecks after calling
>>> +         * ->release_dquot() and won't free dquot structure.
>>> +         */
>>> +        dqgrab(dquot);
>>> +        /* First entry on list -> queue work */
>>> +        if (llist_add(&OCFS2_DQUOT(dquot)->list, 
>>> &osb->dquot_drop_list))
>>> +            queue_work(ocfs2_wq, &osb->dquot_drop_work);
>>> +        goto out;
>>> +    }
>>>       status = ocfs2_lock_global_qf(oinfo, 1);
>>>       if (status < 0)
>>>           goto out;
>>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
>>> index c7f71360666f..0c5ea9454967 100644
>>> --- a/fs/ocfs2/super.c
>>> +++ b/fs/ocfs2/super.c
>>> @@ -1920,6 +1920,11 @@ static void ocfs2_dismount_volume(struct 
>>> super_block *sb, int mnt_err)
>>>
>>>       ocfs2_disable_quotas(osb);
>>>
>>> +    /* All dquots should be freed by now */
>>> +    WARN_ON(!llist_empty(&osb->dquot_drop_list));
>>> +    /* Wait for worker to be done with the work structure in osb */
>>> +    cancel_work_sync(&osb->dquot_drop_work);
>>> +
>>>       ocfs2_shutdown_local_alloc(osb);
>>>
>>>       ocfs2_truncate_log_shutdown(osb);
>>> @@ -2247,6 +2252,9 @@ static int ocfs2_initialize_super(struct 
>>> super_block *sb,
>>>       INIT_WORK(&journal->j_recovery_work, ocfs2_complete_recovery);
>>>       journal->j_state = OCFS2_JOURNAL_FREE;
>>>
>>> +    INIT_WORK(&osb->dquot_drop_work, ocfs2_drop_dquot_refs);
>>> +    init_llist_head(&osb->dquot_drop_list);
>>> +
>>>       /* get some pseudo constants for clustersize bits */
>>>       osb->s_clustersize_bits =
>>>           le32_to_cpu(di->id2.i_super.s_clustersize_bits);
>>> -- 
>>> 1.8.1.4
>>>
>
>

[Ocfs2-devel] [PATCH 5/5] ocfs2: Implement delayed dropping of last dquot reference

[Jan Kara]

On Mon 20-01-14 22:47:43, Srinivas Eeda wrote:
> On 01/20/2014 07:31 AM, Goldwyn Rodrigues wrote:
> >On 01/16/2014 04:58 PM, Jan Kara wrote:
> >>On Thu 16-01-14 23:28:49, Jan Kara wrote:
> >>>We cannot drop last dquot reference from downconvert thread as that
> >>>creates the following deadlock:
> >>>
> >>>NODE 1                                  NODE2
> >>>holds dentry lock for 'foo'
> >>>holds inode lock for GLOBAL_BITMAP_SYSTEM_INODE
> >>>                                         dquot_initialize(bar)
> >>>ocfs2_dquot_acquire()
> >>>ocfs2_inode_lock(USER_QUOTA_SYSTEM_INODE)
> >>>                                             ...
> >>>downconvert thread (triggered from another
> >>>node or a different process from NODE2)
> >>>   ocfs2_dentry_post_unlock()
> >>>     ...
> >>>     iput(foo)
> >>>       ocfs2_evict_inode(foo)
> >>>         ocfs2_clear_inode(foo)
> >>>           dquot_drop(inode)
> >>>             ...
> >>>        ocfs2_dquot_release()
> >>>               ocfs2_inode_lock(USER_QUOTA_SYSTEM_INODE)
> >>>                - blocks
> >>>                                             finds we need
> >>>more space in
> >>>                                             quota file
> >>>                                             ...
> >>>ocfs2_extend_no_holes()
> >>>ocfs2_inode_lock(GLOBAL_BITMAP_SYSTEM_INODE)
> >>>                                                 - deadlocks
> >>>waiting for
> >>>                                                   downconvert thread
> >>>
> >>>We solve the problem by postponing dropping of the last dquot reference
> >>>to a workqueue if it happens from the downconvert thread.
> >>   Hum, now looking again into ocfs2_clear_inode() there are
> >>more problems
> >>than I originally thought. Look for example at
> >>ocfs2_mark_lockres_freeing(). That will block on rw/inode/open lock if
> >>there is downconvert pending waiting for that downconvert to finish.
> >>However that never happens when ocfs2_clear_inode() is called from the
> >>downconvert thread.
> >>
> >>So we are back to square one - I don't see a way how to fix
> >>these deadlocks
> >>without postponing dropping of inode reference to a workqueue :(.
> >>
> >
> >Since the reason of the unlink performance is the delay in calling
> >ocfs2_open_unlock(), and the ocfs2_mark_lockres_freeing() comes
> >after ocfs2_open_unlock(): can we move the call to
> >ocfs2_open_unlock() to ocfs2_evict_inode() and then perform
> >ocfs2_clear_inode() in a deferred way?
> once ocfs2_evict_inode is returned, vfs would destroy the inode, so
> I think we should do the cleanup before that and hence we cannot
> differ ocfs2_clear_inode from inside ocfs2_evict_inode
  Yes.

> May be we should queue ocfs2_blocking_ast call itself to down
> convert thread. That way it doesn't prevent down convert thread from
> clearing the inode. Once when down convert thread comes to processes
> the queued ast/bast and finds lockres cleared it can just return.
  Yes, I was thinking about some trick like this myself. But I don't really
know all the constraints in that area so I'm not sure what will work and
what not and I didn't have time to look deep into that yet.

								Honza

> >>>Signed-off-by: Jan Kara <jack@suse.cz>
> >>>---
> >>>  fs/ocfs2/ocfs2.h        |  5 +++++
> >>>  fs/ocfs2/quota.h        |  2 ++
> >>>  fs/ocfs2/quota_global.c | 35 +++++++++++++++++++++++++++++++++++
> >>>  fs/ocfs2/super.c        |  8 ++++++++
> >>>  4 files changed, 50 insertions(+)
> >>>
> >>>diff --git a/fs/ocfs2/ocfs2.h b/fs/ocfs2/ocfs2.h
> >>>index ca81f6b49236..f6134345fe42 100644
> >>>--- a/fs/ocfs2/ocfs2.h
> >>>+++ b/fs/ocfs2/ocfs2.h
> >>>@@ -30,6 +30,7 @@
> >>>  #include <linux/sched.h>
> >>>  #include <linux/wait.h>
> >>>  #include <linux/list.h>
> >>>+#include <linux/llist.h>
> >>>  #include <linux/rbtree.h>
> >>>  #include <linux/workqueue.h>
> >>>  #include <linux/kref.h>
> >>>@@ -410,6 +411,10 @@ struct ocfs2_super
> >>>      struct list_head blocked_lock_list;
> >>>      unsigned long blocked_lock_count;
> >>>
> >>>+    /* List of dquot structures to drop last reference to */
> >>>+    struct llist_head dquot_drop_list;
> >>>+    struct work_struct dquot_drop_work;
> >>>+
> >>>      wait_queue_head_t        osb_mount_event;
> >>>
> >>>      /* Truncate log info */
> >>>diff --git a/fs/ocfs2/quota.h b/fs/ocfs2/quota.h
> >>>index d5ab56cbe5c5..f266d67df3c6 100644
> >>>--- a/fs/ocfs2/quota.h
> >>>+++ b/fs/ocfs2/quota.h
> >>>@@ -28,6 +28,7 @@ struct ocfs2_dquot {
> >>>      unsigned int dq_use_count;    /* Number of nodes having
> >>>reference to this entry in global quota file */
> >>>      s64 dq_origspace;    /* Last globally synced space usage */
> >>>      s64 dq_originodes;    /* Last globally synced inode usage */
> >>>+    struct llist_node list;    /* Member of list of dquots to drop */
> >>>  };
> >>>
> >>>  /* Description of one chunk to recover in memory */
> >>>@@ -110,6 +111,7 @@ int ocfs2_read_quota_phys_block(struct
> >>>inode *inode, u64 p_block,
> >>>  int ocfs2_create_local_dquot(struct dquot *dquot);
> >>>  int ocfs2_local_release_dquot(handle_t *handle, struct dquot *dquot);
> >>>  int ocfs2_local_write_dquot(struct dquot *dquot);
> >>>+void ocfs2_drop_dquot_refs(struct work_struct *work);
> >>>
> >>>  extern const struct dquot_operations ocfs2_quota_operations;
> >>>  extern struct quota_format_type ocfs2_quota_format;
> >>>diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
> >>>index aaa50611ec66..7921e209c64b 100644
> >>>--- a/fs/ocfs2/quota_global.c
> >>>+++ b/fs/ocfs2/quota_global.c
> >>>@@ -10,6 +10,7 @@
> >>>  #include <linux/jiffies.h>
> >>>  #include <linux/writeback.h>
> >>>  #include <linux/workqueue.h>
> >>>+#include <linux/llist.h>
> >>>
> >>>  #include <cluster/masklog.h>
> >>>
> >>>@@ -679,6 +680,27 @@ static int ocfs2_calc_qdel_credits(struct
> >>>super_block *sb, int type)
> >>>             OCFS2_INODE_UPDATE_CREDITS;
> >>>  }
> >>>
> >>>+void ocfs2_drop_dquot_refs(struct work_struct *work)
> >>>+{
> >>>+    struct ocfs2_super *osb = container_of(work, struct ocfs2_super,
> >>>+                           dquot_drop_work);
> >>>+    struct llist_node *list;
> >>>+    struct ocfs2_dquot *odquot, *next_odquot;
> >>>+
> >>>+    list = llist_del_all(&osb->dquot_drop_list);
> >>>+    llist_for_each_entry_safe(odquot, next_odquot, list, list) {
> >>>+        /* Drop the reference we acquired in ocfs2_dquot_release() */
> >>>+        dqput(&odquot->dq_dquot);
> >>>+    }
> >>>+}
> >>>+
> >>>+/*
> >>>+ * Called when the last reference to dquot is dropped. If we
> >>>are called from
> >>>+ * downconvert thread, we cannot do all the handling here
> >>>because grabbing
> >>>+ * quota lock could deadlock (the node holding the quota lock
> >>>could need some
> >>>+ * other cluster lock to proceed but with blocked downconvert
> >>>thread we cannot
> >>>+ * release any lock).
> >>>+ */
> >>>  static int ocfs2_release_dquot(struct dquot *dquot)
> >>>  {
> >>>      handle_t *handle;
> >>>@@ -694,6 +716,19 @@ static int ocfs2_release_dquot(struct
> >>>dquot *dquot)
> >>>      /* Check whether we are not racing with some other dqget() */
> >>>      if (atomic_read(&dquot->dq_count) > 1)
> >>>          goto out;
> >>>+    /* Running from downconvert thread? Postpone quota
> >>>processing to wq */
> >>>+    if (current == osb->dc_task) {
> >>>+        /*
> >>>+         * Grab our own reference to dquot and queue it for delayed
> >>>+         * dropping.  Quota code rechecks after calling
> >>>+         * ->release_dquot() and won't free dquot structure.
> >>>+         */
> >>>+        dqgrab(dquot);
> >>>+        /* First entry on list -> queue work */
> >>>+        if (llist_add(&OCFS2_DQUOT(dquot)->list,
> >>>&osb->dquot_drop_list))
> >>>+            queue_work(ocfs2_wq, &osb->dquot_drop_work);
> >>>+        goto out;
> >>>+    }
> >>>      status = ocfs2_lock_global_qf(oinfo, 1);
> >>>      if (status < 0)
> >>>          goto out;
> >>>diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c
> >>>index c7f71360666f..0c5ea9454967 100644
> >>>--- a/fs/ocfs2/super.c
> >>>+++ b/fs/ocfs2/super.c
> >>>@@ -1920,6 +1920,11 @@ static void
> >>>ocfs2_dismount_volume(struct super_block *sb, int mnt_err)
> >>>
> >>>      ocfs2_disable_quotas(osb);
> >>>
> >>>+    /* All dquots should be freed by now */
> >>>+    WARN_ON(!llist_empty(&osb->dquot_drop_list));
> >>>+    /* Wait for worker to be done with the work structure in osb */
> >>>+    cancel_work_sync(&osb->dquot_drop_work);
> >>>+
> >>>      ocfs2_shutdown_local_alloc(osb);
> >>>
> >>>      ocfs2_truncate_log_shutdown(osb);
> >>>@@ -2247,6 +2252,9 @@ static int ocfs2_initialize_super(struct
> >>>super_block *sb,
> >>>      INIT_WORK(&journal->j_recovery_work, ocfs2_complete_recovery);
> >>>      journal->j_state = OCFS2_JOURNAL_FREE;
> >>>
> >>>+    INIT_WORK(&osb->dquot_drop_work, ocfs2_drop_dquot_refs);
> >>>+    init_llist_head(&osb->dquot_drop_list);
> >>>+
> >>>      /* get some pseudo constants for clustersize bits */
> >>>      osb->s_clustersize_bits =
> >>>          le32_to_cpu(di->id2.i_super.s_clustersize_bits);
> >>>-- 
> >>>1.8.1.4
> >>>
> >
> >
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR