I think you may have caught the webrev while I was updating it
with Matts feedback theer Ilya.
If you would be so kind as to check now.
Regards
Steve
----- Original Message -----
From: "Ilya Usvyatsky" <[email protected]>
To: <[email protected]>
Cc: "developer" <[email protected]>
Sent: Sunday, November 17, 2013 1:04 AM
Subject: Re: [OpenZFS Developer] [zfs] review: 4322 ZFS deadlock
ondp_config_rwlock
I must be missing something, but it seems that line 574 is excessive here.
On Sat, Nov 16, 2013 at 7:38 PM, Matthew Ahrens <[email protected]> wrote:
Steven, good observation, and thanks for the quick turnaround on this fix.
I think that we should hold the dsl_pool_config lock while calling
dsl_dataset_name(), because otherwise the dd_parent could change (via
dsl_dir_rename_sync()) and the old parent be evicted while we are looking
at it. This would be an exceedingly tight race condition, probably
impossible to exercise in practice. But we should consider adding an
assertion to this effect (having dsl_dir_name assert that the lock is
held), and making sure that all callers adhere to this convention. That is
outside the scope of this specific fix, however.
I'd suggest that we hold the lock until after dsl_dataset_name() is
called, using logic like the old code to ensure we drop the lock in the
error case as well; too bad this code is a bit ugly.
--matt
On Sat, Nov 16, 2013 at 4:22 PM, Steven Hartland <[email protected]>wrote:
Hi all looking for reviewers for:
http://cr.illumos.org/~webrev/steveh/illumos-4322/
Which addresses:
https://www.illumos.org/issues/4322
I've tested to confirm it fixes the issue but I have
a concern that the dsl_pool_config lock should be
held for dsl_dataset_name dsl_dataset_rele.
The reason for this is that both the code path
prior commit: a7a845e4bf22fd1b2a284729ccd95c7370a0438c
did this:
dsl_pool_config_enter(dp, FTAG);
error = dsl_dataset_hold_obj(dp, dsobj, FTAG, &ds);
if (error == 0) {
char name[MAXNAMELEN];
dsl_dataset_name(ds, name);
dsl_dataset_rele(ds, FTAG);
dsl_pool_config_exit(dp, FTAG);
(void) zfs_unmount_snap(name);
} else {
dsl_pool_config_exit(dp, FTAG);
}
As well as at least one other place I've found:
spa.c - spa_prop_get
dsl_pool_config_enter(dp, FTAG);
if (err = dsl_dataset_hold_obj(dp,
za.za_first_integer, FTAG, &ds)) {
dsl_pool_config_exit(dp, FTAG);
break;
}
strval = kmem_alloc(
MAXNAMELEN + strlen(MOS_DIR_NAME) + 1,
KM_SLEEP);
dsl_dataset_name(ds, strval);
dsl_dataset_rele(ds, FTAG);
dsl_pool_config_exit(dp, FTAG);
If the lock was only required for the obj hold I would
have expected both of the above to do so as it would be
cleaner.
I have confirmed a kernel even with full asserts can
quite happily run without deadlock and no other asserts
are trigged during a send of a snapshot which is mounted,
so there are no obvious issues but always best to ask ;-)
Regards
Steve
================================================
This e.mail is private and confidential between Multiplay (UK) Ltd. and
the person or entity to whom it is addressed. In the event of misdirection,
the recipient is prohibited from using, copying, printing or otherwise
disseminating it or any information contained in it.
In the event of misdirection, illegible or incomplete transmission please
telephone +44 845 868 1337
or return the E.mail to [email protected].
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/
21635000-ebd1d460
Modify Your Subscription: https://www.listbox.com/member/?&id_
secret=21635000-73dc201a <https://www.listbox.com/member/?&;>
Powered by Listbox: http://www.listbox.com
*illumos-zfs* | Archives<https://www.listbox.com/member/archive/182191/=now>
<https://www.listbox.com/member/archive/rss/182191/24013066-c76c6d4d> |
Modify<https://www.listbox.com/member/?member_id=24013066&id_secret=24013066-bdf4d28f>Your
Subscription
<http://www.listbox.com>
--------------------------------------------------------------------------------
_______________________________________________
developer mailing list
[email protected]
http://lists.open-zfs.org/mailman/listinfo/developer
================================================
This e.mail is private and confidential between Multiplay (UK) Ltd. and the person or entity to whom it is addressed. In the event of misdirection, the recipient is prohibited from using, copying, printing or otherwise disseminating it or any information contained in it.
In the event of misdirection, illegible or incomplete transmission please
telephone +44 845 868 1337
or return the E.mail to [email protected].
_______________________________________________
developer mailing list
[email protected]
http://lists.open-zfs.org/mailman/listinfo/developer
