zfs: Fix lockdep between ds_lock and dd_lock in dsl_dataset_namelen()

[mzhivich]

Motivation and Context

Fix potential locking issue reported by debug kernel build.

Description

Booting debug kernel found an inconsistent lock dependency between
dataset's ds_lock and its directory's dd_lock.

[ 32.215336] ======================================================
[ 32.221859] WARNING: possible circular locking dependency detected
[ 32.221861] 4.14.90+ #8 Tainted: G O
[ 32.221862] ------------------------------------------------------
[ 32.221863] dynamic_kernel_/4667 is trying to acquire lock:
[ 32.221864] (&ds->ds_lock){+.+.}, at: []
dsl_dataset_check_quota+0x9e/0x8a0 [zfs]
[ 32.221941] but task is already holding lock:
[ 32.221941] (&dd->dd_lock){+.+.}, at: []
dsl_dir_tempreserve_space+0x3b9/0x1290 [zfs]
[ 32.221983] which lock already depends on the new lock.
[ 32.221983] the existing dependency chain (in reverse order) is:
[ 32.221984] -> #1 (&dd->dd_lock){+.+.}:
[ 32.221992] __mutex_lock+0xef/0x14c0
[ 32.222049] dsl_dir_namelen+0xd4/0x2d0 [zfs]
[ 32.222093] dsl_dataset_namelen+0x2f1/0x430 [zfs]
[ 32.222142] verify_dataset_name_len+0xd/0x40 [zfs]
[ 32.222184] dmu_objset_find_dp_impl+0x5f5/0xef0 [zfs]
[ 32.222226] dmu_objset_find_dp_cb+0x40/0x60 [zfs]
[ 32.222235] taskq_thread+0x969/0x1460 [spl]
[ 32.222238] kthread+0x2fb/0x400
[ 32.222241] ret_from_fork+0x3a/0x50

[ 32.222241] -> #0 (&ds->ds_lock){+.+.}:
[ 32.222246] lock_acquire+0x14f/0x390
[ 32.222248] __mutex_lock+0xef/0x14c0
[ 32.222291] dsl_dataset_check_quota+0x9e/0x8a0 [zfs]
[ 32.222355] dsl_dir_tempreserve_space+0x5d2/0x1290 [zfs]
[ 32.222392] dmu_tx_assign+0xa61/0xdb0 [zfs]
[ 32.222436] zfs_create+0x4e6/0x11d0 [zfs]
[ 32.222481] zpl_create+0x194/0x340 [zfs]
[ 32.222484] lookup_open+0xa86/0x16f0
[ 32.222486] path_openat+0xe56/0x2490
[ 32.222488] do_filp_open+0x17f/0x260
[ 32.222490] do_sys_open+0x195/0x310
[ 32.222491] SyS_open+0xbf/0xf0
[ 32.222494] do_syscall_64+0x191/0x4f0
[ 32.222496] entry_SYSCALL_64_after_hwframe+0x42/0xb7

[ 32.222497] other info that might help us debug this:

[ 32.222497] Possible unsafe locking scenario:
[ 32.222498] CPU0 CPU1
[ 32.222498] ---- ----
[ 32.222499] lock(&dd->dd_lock);
[ 32.222500] lock(&ds->ds_lock);
[ 32.222502] lock(&dd->dd_lock);
[ 32.222503] lock(&ds->ds_lock);
[ 32.222504] *** DEADLOCK ***
[ 32.222505] 3 locks held by dynamic_kernel_/4667:
[ 32.222506] #0: (sb_writers#9){.+.+}, at:
[] mnt_want_write+0x3c/0xa0
[ 32.222511] #1: (&type->i_mutex_dir_key#8){++++}, at:
[] path_openat+0xe2e/0x2490
[ 32.222515] #2: (&dd->dd_lock){+.+.}, at:
[]
dsl_dir_tempreserve_space+0x3b9/0x1290 [zfs]

The issue is caused by dsl_dataset_namelen() holding ds_lock,
followed by acquiring dd_lock on ds->ds_dir in dsl_dir_namelen().

However, ds->ds_dir should not be protected by ds_lock,
so releasing it before call to dsl_dir_namelen() prevents
the lockdep issue.

Signed-off-by: Michael Zhivich mzhivich@akamai.com

How Has This Been Tested?

Booting debug kernel with the patch does not elicit lockdep error.
Tested using Linux 4.14.90 on x86_64 with ZFS 0.7.12.

Types of changes

• [ x ] Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation (a change to man pages or other documentation)

Checklist:

• [ x ] My code follows the ZFS on Linux code style requirements.
• I have updated the documentation accordingly.
• [ x ] I have read the contributing document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• [ x ] All commit messages are properly formatted and contain Signed-off-by.

[chrisrd]

LGTM

[alek-p]

LGTM, and this seems to be more consistent with the comment in dmu_impl.h regarding the locking strategy.
I guess the reason this doesn't blow up more often is we don't end up in the dmu_objset_find_dp() -> ... -> dsl_dir_namelen() path all that much.

[brandonhaberfeld]

OMG!

…

On Mon, 25 Feb 2019, 18:36 Alek P, ***@***.***> wrote: ***@***.**** approved this pull request. LGTM, and this seems to be more consistent with the comment in dmu_impl.h <https://github.com/zfsonlinux/zfs/blob/65282ee9e06b130f1f0169baf5d9bf0dd8fc1ef9/include/sys/dmu_impl.h#L183-L185> regarding the locking strategy. I guess the reason this doesn't blow up more often is we don't end up in the dmu_objset_find_dp() -> ... -> dsl_dir_namelen() path all that much. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#8413 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ASSZpr8CWY1MF2xl7CLQjlX7oTBNUZiFks5vRBENgaJpZM4a-ZK3> .