review: 4322 ZFS deadlock on dp_config

Discussion:

review: 4322 ZFS deadlock on dp_config_rwlock

Steven Hartland

2013-11-17 00:22:26 UTC

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 ***@multiplay.co.uk.

Matthew Ahrens

2013-11-17 00:38:34 UTC

Permalink

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

Post by Steven Hartland
http://cr.illumos.org/~webrev/steveh/illumos-4322/
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
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);
}
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
-------------------------------------------
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/?&
Powered by Listbox: http://www.listbox.com

-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/23047029-187a0c8d
Modify Your Subscription: https://www.listbox.com/member/?member_id=23047029&id_secret=23047029-2e85923f
Powered by Listbox: http://www.listbox.com

Ilya Usvyatsky

2013-11-17 01:04:10 UTC

Permalink

I must be missing something, but it seems that line 574 is excessive here.

Post by Matthew Ahrens
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

Post by Steven Hartland
http://cr.illumos.org/~webrev/steveh/illumos-4322/
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
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);
}
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
-------------------------------------------
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/?&>Your Subscription
<http://www.listbox.com>

Steven Hartland

2013-11-17 01:24:25 UTC

Permalink

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" <***@nexenta.com>
To: <***@lists.illumos.org>
Cc: "developer" <***@open-zfs.org>
Sent: Sunday, November 17, 2013 1:04 AM
Subject: Re: [OpenZFS Developer] [zfs] review: 4322 ZFS deadlock ondp_config_rwlock

Post by Ilya Usvyatsky
I must be missing something, but it seems that line 574 is excessive here.

Post by Steven Hartland
http://cr.illumos.org/~webrev/steveh/illumos-4322/
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
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);
}
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
-------------------------------------------
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

--------------------------------------------------------------------------------

Post by Ilya Usvyatsky
_______________________________________________
developer mailing list
http://lists.open-zfs.org/mailman/listinfo/developer

Steven Hartland

2013-11-17 01:26:04 UTC

Permalink

I thought there may have been some reason for the old flow.

webrev updated:
http://cr.illumos.org/~webrev/steveh/illumos-4322/

Regards
Steve
----- Original Message -----
From: "Matthew Ahrens" <***@delphix.com>
To: "illumos-zfs" <***@lists.illumos.org>
Cc: "developer" <***@open-zfs.org>
Sent: Sunday, November 17, 2013 12:38 AM
Subject: Re: [OpenZFS Developer] [zfs] review: 4322 ZFS deadlock ondp_config_rwlock

Post by Matthew Ahrens
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

--------------------------------------------------------------------------------

Post by Matthew Ahrens
_______________________________________________
developer mailing list
http://lists.open-zfs.org/mailman/listinfo/developer

Ilya Usvyatsky

2013-11-17 02:18:39 UTC

Permalink

LGTM now.

Post by Steven Hartland
I thought there may have been some reason for the old flow.
http://cr.illumos.org/~webrev/steveh/illumos-4322/
Regards
Steve
Sent: Sunday, November 17, 2013 12:38 AM
Subject: Re: [OpenZFS Developer] [zfs] review: 4322 ZFS deadlock ondp_config_rwlock
Steven, good observation, and thanks for the quick turnaround on this fix.

Post by Matthew Ahrens
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

------------------------------------------------------------
--------------------
_______________________________________________

Post by Matthew Ahrens
developer mailing list
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
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/
24013066-c76c6d4d
Modify Your Subscription: https://www.listbox.com/
member/?&
Powered by Listbox: http://www.listbox.com

Matthew Ahrens

2013-11-17 03:25:16 UTC

Permalink

Looks great, thanks Steven!

--matt

On Sat, Nov 16, 2013 at 6:18 PM, Ilya Usvyatsky

Post by Ilya Usvyatsky
LGTM now.

Post by Matthew Ahrens
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 <

------------------------------------------------------------
--------------------
_______________________________________________

Post by Matthew Ahrens
developer mailing list
http://lists.open-zfs.org/mailman/listinfo/developer