lock order inversion between zvol_open() and dsl_pool_sync()...zvol_rename_minors()

[bprotopopov]

There appears to be a problem with the following two code paths executed concurrently:

1. resulting from the zfs rename on zvol snapshot, taking place in txg_sync_thread()

#0 [ffff8802a6e7ba20] schedule at ffffffff815299d0
#1 [ffff8802a6e7baf8] __mutex_lock_slowpath at ffffffff8152b3a6
#2 [ffff8802a6e7bb68] mutex_lock at ffffffff8152aecb
#3 [ffff8802a6e7bb88] zvol_rename_minors at ffffffffa02fe634 [zfs]
#4 [ffff8802a6e7bbd8] dsl_dir_rename_sync at ffffffffa027c04d [zfs]
#5 [ffff8802a6e7bc48] dsl_sync_task_sync at ffffffffa02850f2 [zfs]
#6 [ffff8802a6e7bc78] dsl_pool_sync at ffffffffa027d31b [zfs]
#7 [ffff8802a6e7bcf8] spa_sync at ffffffffa0293587 [zfs]
#8 [ffff8802a6e7bdc8] txg_sync_thread at ffffffffa02a890b [zfs]
#9 [ffff8802a6e7beb8] thread_generic_wrapper at ffffffffa0169978 [spl]
#10 [ffff8802a6e7bee8] kthread at ffffffff8109e78e
#11 [ffff8802a6e7bf48] kernel_thread at ffffffff8100c28a

2. resulting from opening zvol for I/O
   #0 [ffff88020e53d9b8] schedule at ffffffff815299d0
   Use Barriers in pre-2.6.24 kernels #1 [ffff88020e53da90] cv_wait_common at ffffffffa0171e65 [spl]
   Implement zeventd daemon #2 [ffff88020e53db20] __cv_wait at ffffffffa0171f75 [spl]
   Use New BIO_RW_FAILFAST_* API #3 [ffff88020e53db30] rrw_enter_read at ffffffffa028df7b [zfs]
   Having ZVOL can cause EBUSY for certain operations #4 [ffff88020e53db60] rrw_enter at ffffffffa028e0d0 [zfs]
   Verify On Disk Compatibility #5 [ffff88020e53db70] dsl_pool_config_enter at ffffffffa027c24d [zfs]
   ZFS Test Suite #6 [ffff88020e53db80] dsl_pool_hold at ffffffffa027c2da [zfs]
   Native ZPL Implementation #7 [ffff88020e53dbc0] dmu_objset_own at ffffffffa025e1a0 [zfs]
   Fuse Implementation #8 [ffff88020e53dc20] zvol_open at ffffffffa02fd4fb [zfs]
   ZVOL Performance #9 [ffff88020e53dc90] __blkdev_get at ffffffff811cbc4e
   Enclosure Management Integration #10 [ffff88020e53dcf0] blkdev_get at ffffffff811cbf70
   Support Debian/Ubuntu Style Packages #11 [ffff88020e53dd00] blkdev_open at ffffffff811cbff1
   Update core ZFS code from OpenSolaris #12 [ffff88020e53dd30] __dentry_open at ffffffff8118b1ba
   Split zpios off in to its own project #13 [ffff88020e53dd90] nameidata_to_filp at ffffffff8118b524
   Use the Autotest Test Suite #14 [ffff88020e53ddb0] do_filp_open at ffffffff811a12d0
   Reduce Stack Usage #15 [ffff88020e53df20] do_sys_open at ffffffff8118af77
   SPL tests fails #16 [ffff88020e53df70] sys_open at ffffffff8118b080
   UTS_RELEASE checking is depricated #17 [ffff88020e53df80] system_call_fastpath at ffffffff8100b0f2

The first code path takes dsl pool config lock, then tries to take zvol_state_lock, while the second, takes the zvol_state_lock, and then tries to get the dsl pool config lock.

It is interesting that zvol_first_open() called from zvol_open() takes care to obtain spa_namespace_lock, and fails if it cannot, in order to avoid the locking order inversion between spa_namespace_lock and the zvol_state_lock.

Yet the txg_sync_thread does not seem to hold the spa_namespace_lock when calling dsl_pool_sync() and further, before trying to get the zvol_state_lock. That is why I believe this defensive check is not effective in this particular case.

[bprotopopov]

I am seriously considering performing zvol__minor related functions invoked from dsl_dataset.c, spa.c, and zfs_ioctl.c, through a taskq, in order to isolate it from any sort of locking conditions in the callers.

[behlendorf]

@bprotopopov what about changing the mutex to a rwlock. The zvol_state_lock is there to primarily protect against addition and removal from the list. For places like zvol_rename_minors where the list is just traversed it would be safe to use a read lock. A proper mutex to lock the individual zvol_state_t member during modification would probably be a good idea as well.

[bprotopopov]

@behlendorf I believe the zvol_first_open() would then have to take the rwlock as a writer whereas the txg_sync_thread would take is as a reader, still resulting in the same deadlock, or am I missing something?

I have prototyped the asynchronous (in a taskq) zvol minor operations, and I think the code is much simplified as far as the analysis of the possible deadlocks. I also think with this approach, there is no longer a need to try-lock the spa_namespace_lock in the zvol_open() path (because everybody now takes the zvol_state_lock first and then other locks). Given that the udev-related side effects of the minor operations are asynchronous as well, would this approach seem like a more straightforward way to get things done?

[behlendorf]

@bprotopopov perhaps the race would be smaller but I think your right. It would still exist. I'm definitely not opposed to doing the minor creation asynchronously I was just tossing out another possible idea. I'd love to see your patch for this once your happy with it.

In fact, what would be ideal is if you could build up a small patch stack with this change and your other zvol improvements. Then we could get them all reviewed in one go and merged.

[bprotopopov]

@behlendorf sure thing

[bprotopopov]

Hope to be done by end of week

[bprotopopov]

@behlendorf, hope you can help, what is the proper way to build SPL and ZFS to enable asserts and other debug checks ?

I did make distclean, ./autogen.sh, ./configure --enable-debug, make rpm-utils && make rpm-kmod, and yum localinstall the RPMs for SPL first, then the same thing for ZFS.

Yet I get a panic
...
SPLError: 2477:0:(list.h:77:list_create()) ASSERTION(size >= offset + sizeof(list_node_t)) failed
SPLError: 2477:0:(list.h:77:list_create()) SPL PANIC
SPL: Showing stack for process 2477
Pid: 2477, comm: modprobe Tainted: P --------------- 2.6.32-504.23.4.el6.x86_64 #1
Call Trace:
[] ? spl_debug_dumpstack+0x46/0x60 [spl]
[] ? spl_debug_bug+0x81/0xd0 [spl]
[] ? spl_PANIC+0xba/0xf0 [spl]
[] ? kmem_alloc_debug+0x251/0x490 [spl]
[] ? spl_kmem_cache_create+0x331/0xf50 [spl]
[] ? __thread_create+0x13f/0x390 [spl]
[] ? mutex_lock+0x1e/0x50
[] ? zfs_znode_cache_constructor+0x0/0x1b0 [zfs]
[] ? spl__init+0x0/0x20 [zfs]
[] ? zvol_init+0x88/0x290 [zfs]
[] ? spl__init+0x0/0x20 [zfs]
[] ? spl__init+0x0/0x20 [zfs]
[] ? _init+0x22/0xb10 [zfs]
[] ? spl__init+0x0/0x20 [zfs]
[] ? spl__init+0x13/0x20 [zfs]
[] ? do_one_initcall+0x3c/0x1d0
[] ? sys_init_module+0xe1/0x250
[] ? system_call_fastpath+0x16/0x1b

What am I missing ?
Should I do make && make install instead of using RPMs ?

[behlendorf]

I'm not sure I follow the question. Passing - - enable-debug as you did will enable the assertions and it looks like your hitting one so that issue will need to be resolved.

[bprotopopov]

Well the assertion looks like mismatch of type sizes in SPL and ZFS modules, e.g. as if one is build with and one without debug support.

Thanks for confirming the procedure, it must be something in my environment.

Typos courtesy of my iPhone

On Sep 18, 2015, at 6:57 PM, Brian Behlendorf <notifications@proxy-git.cwkhome.funmailto:notifications@github.com> wrote:

I'm not sure I follow the question. Passing - - enable-debug as you did will enable the assertions and it looks like your hitting one so that issue will need to be resolved.

Reply to this email directly or view it on GitHubhttps://github.com//issues/3681#issuecomment-141589105.

[bprotopopov]

@behlendorf, wanted to clarify regarding the other zvol improvements you mentioned to be included with this bugfix; I can only think of the zvol minors scalability issue (#2217); I rebased that fix as well and will put it the pull request shortly.

Please let me know if that's what you meant.

[behlendorf]

Thanks for linking to your patches and working on this. I think this improvement will help us close out a lot of long standing issues. I took a quick look at the patch and I have a few questions.

• Why is a necessary to create a taskq per-pool on-demand rather than global one at module load? That definitely complicates the code and I'm not sure what advantage is has over a single global taskq in zvol.c. Are you concerned a deadlock is possible if multiple pools are sharing the same taskq? Or do you feel you'll need more threads when a system contains many pools. If it's the latter I'd suggest just using TASKQ_DYNAMIC and allowing a large number of threads to be created if they're needed.
• It feels to me like the make_async_arg arg logic should be pulled in to zvol_async_dispatch. That would allow all the helper functions to simplified.
• KM_SLEEP can now be used safely, KM_PUSHPAGE isn't required.
• Did you resolve your ASSERT issue?