Fix deadlock between zfs umount & snapentry_expire #7752
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7752 +/- ##
==========================================
+ Coverage 77.68% 78.1% +0.41%
==========================================
Files 359 368 +9
Lines 111916 111981 +65
==========================================
+ Hits 86945 87461 +516
+ Misses 24971 24520 -451
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for getting to the bottom on this! Your analysis looks good to me but I've suggested a slightly different approach to handle this case.
It'd be great if you could add the test case you came up with to the functional/snapshot/ tests. The more coverage we have of corner cases like this the better.
module/zfs/zfs_ctldir.c
Outdated
| if ((se = zfsctl_snapshot_find_by_objsetid(spa, objsetid)) | ||
| != NULL) { | ||
| zfsctl_snapshot_unmount_cancel(se); | ||
| if (!rw_tryupgrade(&zfs_snapshot_lock)) { | ||
| rw_exit(&zfs_snapshot_lock); |
There was a problem hiding this comment.
My concern with this is that dropping and reacquiring the lock here it leaves a small window where the zfs_snapentry_t can still be found on the _by_name and _by_objsetid list just before it's going to be destroyed.
Since the RW_WRITER lock is there to protect the lookup and and list removal. it makes sense to do both of those things first while holding the lock. Then drop the lock, and call zfsctl_snapshot_unmount_cancel. Since the validity of the se is being protected by the se->se_refcount. It looks like you will need to remove the ASSERT from zfsctl_snapshot_unmount_cancel.
There was a problem hiding this comment.
Fair concern, reworked the change as suggested.
The test case is to reproduce actually, needs to add a sleep for few seconds in snapentry_expire(), before se->se_taskqid = TASKQID_INVALID; and during this sleep the zfs umount zfs_name( for which the snapshot existed/created) has to be fired.
But this sleep is only needed for testing purposes. But i do not see any flags under which I can add this sleep. Any idea?
rohan-puri
force-pushed
the
snap_umount_deadlock_fix
branch
from
26d4de9
to
55ee838
Compare
module/zfs/zfs_ctldir.c
Outdated
| zfsctl_snapshot_remove(se); | ||
| zfsctl_snapshot_rele(se); | ||
| } | ||
| rw_exit(&zfs_snapshot_lock); | ||
| if (se) { | ||
| zfsctl_snapshot_unmount_cancel(se); |
There was a problem hiding this comment.
You want to release the reference taken in zfsctl_snapshot_find_by_objsetid() with zfsctl_snapshot_rele() after calling zfsctl_snapshot_unmount_cancel(). This way the object can't be freed out from underneath you.
There was a problem hiding this comment.
Fair point, made the changes. Thanks for the clarification on testing. This latest updated patch is manually tested.
zfs umount -> zfsctl_destroy() takes zfs_snapshot_lock WRITER & calls zfsctl_snapshot_unmount_cancel() which waits for snapentry_expire() if present(only when snap is automounted). This snapentry_expire() itself then waits for zfs_snapshot_lock READER, resulting in a deadlock. Fix is, in zfsctl_destroy() do avl_tree lookup & removal with WRITER zfs_snapshot_lock & leave this lock before zfsctl_snapshot_unmount_cancel() call, since the validity of se is protected by se->se_refcount. Also remove the corresponding lock assertion from zfsctl_snapshot_unmount_cancel() too. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Rohan Puri <rohan.puri15@gmail.com> Closes openzfs#7751
rohan-puri
force-pushed
the
snap_umount_deadlock_fix
branch
from
55ee838
to
8e4d086
Compare
behlendorf
added
Reviewed
and removed
Status: Work in Progress
zfs umount -> zfsctl_destroy() takes zfs_snapshot_lock WRITER &
calls zfsctl_snapshot_unmount_cancel() which waits for
snapentry_expire() if present(only when snap is automounted).
This snapentry_expire() itself then waits for zfs_snapshot_lock
READER, resulting in a deadlock.
Fix is to take READER in zfsctl_destroy() & upgrade to WRITER
after call to zfsctl_snapshot_unmount_cancel() before calling
zfsctl_snapshot_remove(). This allows snapentry_expire() to
progress too with the READER.
Signed-off-by: Rohan Puri rohan.puri15@gmail.com
Motivation and Context
This change fixes the deadlock between zfs umount and snapentry_expire() #7751Description
Fix is to take READER in zfsctl_destroy() & upgrade to WRITERHow Has This Been Tested?
I reproduced the problem by instrumenting the code, details are mentioned in the bug details. With the repro instrumentation of the code, I tested the fix to see that the deadlock does not happen again.Types of changes
x] Bug fix (non-breaking change which fixes an issue)Checklist:
x] My code follows the ZFS on Linux code style requirements.x] I have read the contributing document.x] All commit messages are properly formatted and containSigned-off-by.