Hi,
This is a minor change from v3. It returns in place when racing is detected.
Changwei and Daniel,
I added your Reviewed-by and/or Tested-by with the patch, if you don't
think it's good, pls let me know.
thanks,
wengang
On 05/29/2019 10:46 AM, Wengang Wang wrote:
ocfs2_dentry_attach_lock() can be executed in parallel threads against the
same dentry. Make that race safe.
The race is like this:
thread A thread B
(A1) enter ocfs2_dentry_attach_lock,
seeing dentry->d_fsdata is NULL,
and no alias found by
ocfs2_find_local_alias, so kmalloc
a new ocfs2_dentry_lock structure
to local variable "dl", dl1
.....
(B1) enter ocfs2_dentry_attach_lock,
seeing dentry->d_fsdata is NULL,
and no alias found by
ocfs2_find_local_alias so kmalloc
a new ocfs2_dentry_lock structure
to local variable "dl", dl2.
......
(A2) set dentry->d_fsdata with dl1,
call ocfs2_dentry_lock() and increase
dl1->dl_lockres.l_ro_holders to 1 on
success.
......
(B2) set dentry->d_fsdata with dl2
call ocfs2_dentry_lock() and increase
dl2->dl_lockres.l_ro_holders to 1 on
success.
......
(A3) call ocfs2_dentry_unlock()
and decrease
dl2->dl_lockres.l_ro_holders to 0
on success.
....
(B3) call ocfs2_dentry_unlock(),
decreasing
dl2->dl_lockres.l_ro_holders, but
see it's zero now, panic
Signed-off-by: Wengang Wang <wen.gang.wang@xxxxxxxxxx>
Reported-by: Daniel Sobe <daniel.sobe@xxxxxxx>
Tested-by: Daniel Sobe <daniel.sobe@xxxxxxx>
Reviewed-by: Changwei Ge <gechangwei@xxxxxxx>
---
v4: return in place on race detection.
v3: add Reviewed-by, Reported-by and Tested-by only
v2: 1) removed lock on dentry_attach_lock at the first access of
dentry->d_fsdata since it helps very little.
2) do cleanups before freeing the duplicated dl
3) return after freeing the duplicated dl found.
---
fs/ocfs2/dcache.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
index 290373024d9d..e8ace3b54e9c 100644
--- a/fs/ocfs2/dcache.c
+++ b/fs/ocfs2/dcache.c
@@ -310,6 +310,18 @@ int ocfs2_dentry_attach_lock(struct dentry *dentry,
out_attach:
spin_lock(&dentry_attach_lock);
+ if (unlikely(dentry->d_fsdata && !alias)) {
+ /* d_fsdata is set by a racing thread which is doing
+ * the same thing as this thread is doing. Leave the racing
+ * thread going ahead and we return here.
+ */
+ spin_unlock(&dentry_attach_lock);
+ iput(dl->dl_inode);
+ ocfs2_lock_res_free(&dl->dl_lockres);
+ kfree(dl);
+ return 0;
+ }
+
dentry->d_fsdata = dl;
dl->dl_count++;
spin_unlock(&dentry_attach_lock);
