Skip to contentSkip to conversation

: drop(unlock) dentry before iput in ocfs2_do_drop_dentry_lock

PATCH New Wengang Wang 2 participants 2 messages Oct 20, 2014 OCFS2

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 : drop(unlock) dentry before iput in ocfs2_do_drop_dentry_lock

There is dead lock case like this(the Dentry is a dentry to the Inode):

node 1 node 2
------------------- --------------------------

granted inode lock

requesting dentry lock
responding to node 1 request on dentry lock, it calls ocfs2_do_drop_dentry_lock with the call trace:
#4 wait_for_completion
#5 __ocfs2_cluster_lock
#6 ocfs2_inode_lock_full_nested
#7 ocfs2_evict_inode
#8 evict
#9 iput
#10 ocfs2_do_drop_dentry_lock

You can see node 2 is requesting the inode lock before unlocking dentry lock thus a two nodes dead lock formed.

The story is like this:

Responding node 1's request, Node 2, ocfs2_dentry_convert_worker, returned UNBLOCK_STOP_POST which means don't downconvert but do post_unlock action. It was doing so because it thought it doesn't need to downconvert but will do an unlock instead in the post_unlock action ocfs2_dentry_post_unlock(). By doing so it can save a clusted downconvert request for performace consideration. But in ocfs2_dentry_post_unlock() before unlock the dentry lock, it was requesting the inode lock forming the deadlock expectedly.

Fix:
unlock dentry lock first before the requesting inode lock in ocfs2_do_drop_dentry_lock.

---
 fs/ocfs2/dcache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/ocfs2/dcache.c +1 -1
index e2e05a1..19a7d6c 100644
@@ -369,9 +369,9 @@ out_attach:
 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);
+	iput(dl->dl_inode);
 	kfree(dl);
 }
 
Signature · 1 line
1.8.3.1
#1 lore raw

PATCH 1/1 : drop(unlock) dentry before iput in ocfs2_do_drop_dentry_lock

This fix will cause problems for regular file unlinks. If the inode is not cleared on node 2, then both node 1 and node 2 will fail to get try open lock and fail to clear the orphan inodes.

I am assuming the deadlock you have seen is because of quota's enabled and fix e3a767b60fd8a9f5e133f42f4970cff77ec43173 should have avoided this deadlock by delaying the dropping of the dquot reference. That didn't happen ? Why else is downconvert task taking the inode lock ?

Thanks,
--Srini

Quoted 56 lines · Wengang Wang

There is dead lock case like this(the Dentry is a dentry to the Inode):

node 1 node 2
------------------- --------------------------

granted inode lock

requesting dentry lock
responding to node 1 request on dentry lock,
it calls ocfs2_do_drop_dentry_lock with the call trace:
#4 wait_for_completion
#5 __ocfs2_cluster_lock
#6 ocfs2_inode_lock_full_nested
#7 ocfs2_evict_inode
#8 evict
#9 iput
#10 ocfs2_do_drop_dentry_lock

You can see node 2 is requesting the inode lock before unlocking dentry lock thus a two nodes dead lock formed.

The story is like this:

Responding node 1's request, Node 2, ocfs2_dentry_convert_worker, returned UNBLOCK_STOP_POST which means don't downconvert but do post_unlock action. It was doing so because it thought it doesn't need to downconvert but will do an unlock instead in the post_unlock action ocfs2_dentry_post_unlock(). By doing so it can save a clusted downconvert request for performace consideration. But in ocfs2_dentry_post_unlock() before unlock the dentry lock, it was requesting the inode lock forming the deadlock expectedly.

Fix:
unlock dentry lock first before the requesting inode lock in ocfs2_do_drop_dentry_lock.

---
  fs/ocfs2/dcache.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
fs/ocfs2/dcache.c +1 -1
index e2e05a1..19a7d6c 100644
@@ -369,9 +369,9 @@ out_attach:
  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);
+	iput(dl->dl_inode);
  	kfree(dl);
  }