Discussion:
NULL deference in vdev_mirror_child_select
Xin Li
2013-09-18 19:44:43 UTC
Permalink
Hi,

We have seen this panic a few times in field but have not yet figured
out a way to reproduce it. The backtrace is usually
vdev_mirror_io_start() -> vdev_mirror_child_select() ->
vdev_is_dead()+0x1.

Looking at the code, it seems like:

In vdev_mirror_child_select():

for (i = 0, c = mm->mm_preferred; i < mm->mm_children; i++, c++) {
(...)
if (!vdev_readable(mc->mc_vd)) {

Where, in vdev_readable, it calls vdev_is_dead() with NULL (based on
the fault pointer we have seen).

My question is, should we expect mc->mc_vd be not NULL? (Apparently
vdev_lookup_top could return NULL). If so, maybe we should add a test
in vdev_is_dead?

Cheers,
- --
Xin LI <***@delphij.net> https://www.delphij.net/
FreeBSD - The Power to Serve! Live free or die
Steven Hartland
2013-09-18 20:16:57 UTC
Permalink
----- Original Message -----
Post by Xin Li
We have seen this panic a few times in field but have not yet figured
out a way to reproduce it. The backtrace is usually
vdev_mirror_io_start() -> vdev_mirror_child_select() ->
vdev_is_dead()+0x1.
for (i = 0, c = mm->mm_preferred; i < mm->mm_children; i++, c++) {
(...)
if (!vdev_readable(mc->mc_vd)) {
Where, in vdev_readable, it calls vdev_is_dead() with NULL (based on
the fault pointer we have seen).
My question is, should we expect mc->mc_vd be not NULL? (Apparently
vdev_lookup_top could return NULL). If so, maybe we should add a test
in vdev_is_dead?
It reads to me that while vdev_lookup_top can return NULL it only does
so only if vdev >= rvd->vdev_children so if the requested vdev has gone
away.

This is used in metaslab_alloc_dva where the passed in vdev is a hint
and such I don't believe its the intention that vdev_lookup_top should
ever return NULL in its use in vdev_mirror_map_alloc.

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.
George Wilson
2013-09-18 21:37:44 UTC
Permalink
Yes, mc_vd should not be null in this case. This is a where you're doing
a read so you should have a block pointer in hand. From this bp we can
get the vdev that we need to perform the read and that vdev better be a
part of this pool. Can you share more information about the pool
configuration and also the blkptr that is in the zio_t?

Thanks,
George
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Hi,
We have seen this panic a few times in field but have not yet figured
out a way to reproduce it. The backtrace is usually
vdev_mirror_io_start() -> vdev_mirror_child_select() ->
vdev_is_dead()+0x1.
for (i = 0, c = mm->mm_preferred; i < mm->mm_children; i++, c++) {
(...)
if (!vdev_readable(mc->mc_vd)) {
Where, in vdev_readable, it calls vdev_is_dead() with NULL (based on
the fault pointer we have seen).
My question is, should we expect mc->mc_vd be not NULL? (Apparently
vdev_lookup_top could return NULL). If so, maybe we should add a test
in vdev_is_dead?
Cheers,
- --
FreeBSD - The Power to Serve! Live free or die
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.21 (FreeBSD)
iQEcBAEBCgAGBQJSOgKrAAoJEG80Jeu8UPuztVQH/R63m4ww0LuDDeMIckuKB0nP
ZKwih9qPQHCscAdeCSQkkVHd8L9kUxVoVAIwxMHPBtYWLOmHVB5KT/ONSQNE7isS
lK3f1vmSvLyzGVpB2zlwElsJ01j79ka/HmZvK8JmDoqy+kDx6jlz/+k/IgV7rVO3
TAQPGeDdo7bAy1mZRkFACUTFdXk9Ou2oKZjIJ6TXXmecwfhCCDbOa7Feqz2jCaHs
xkqDsCAwMA/X+W/hWmuY6AQK83TiA3dHhpsn5x4NR0HdRD0u3c4WEFpsgK6VMBZ/
2TdiuH3ncBoixhwMK9mSIhKZyR1VMaAIF8x+4as15bMN6FCvUszCpRZi+jd9+bA=
=aNRL
-----END PGP SIGNATURE-----
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/22008002-303f2ff4
Modify Your Subscription: https://www.listbox.com/member/?&
Powered by Listbox: http://www.listbox.com
Xin Li
2013-09-19 00:30:23 UTC
Permalink
Post by George Wilson
Yes, mc_vd should not be null in this case. This is a where you're
doing a read so you should have a block pointer in hand. From this
bp we can get the vdev that we need to perform the read and that
vdev better be a part of this pool. Can you share more information
about the pool configuration and also the blkptr that is in the
zio_t?
Well, we don't "own" these systems in question so we can't really
"debug" them interactively, we can only see these backtraces in crash
reports where the actual data structures are not being reported to
protect privacy for customers. To make the situation worse, we do not
yet know any way to reproduce it at lab, do you have any idea that
would worth a try?

I'm thinking about instrumenting the code so it prints some
information before proceeding to panic so we can catch it next time,
so maybe we print out the contents of zio->io_bp as a start? Any
other information that can be collected to aid debugging? We could
add these to our codebase and see what we would get.
Post by George Wilson
Thanks, George
On 9/18/13 3:44 PM, Xin Li wrote: Hi,
We have seen this panic a few times in field but have not yet
figured out a way to reproduce it. The backtrace is usually
vdev_mirror_io_start() -> vdev_mirror_child_select() ->
vdev_is_dead()+0x1.
for (i = 0, c = mm->mm_preferred; i < mm->mm_children; i++, c++) {
(...) if (!vdev_readable(mc->mc_vd)) {
Where, in vdev_readable, it calls vdev_is_dead() with NULL (based
on the fault pointer we have seen).
My question is, should we expect mc->mc_vd be not NULL?
(Apparently vdev_lookup_top could return NULL). If so, maybe we
should add a test in vdev_is_dead?
https://www.delphij.net/ FreeBSD - The Power to Serve!
Live free or die
https://www.listbox.com/member/archive/rss/182191/22008002-303f2ff4
Modify Your Subscription: https://www.listbox.com/member/?&
Post by George Wilson
Powered by Listbox: http://www.listbox.com
https://www.listbox.com/member/archive/rss/182191/22993488-e3cd949b
https://www.listbox.com/member/?&
Powered by Listbox: http://www.listbox.com
- --
Xin LI <***@delphij.net> https://www.delphij.net/
FreeBSD - The Power to Serve! Live free or die
George Wilson
2013-09-19 15:48:35 UTC
Permalink
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Post by George Wilson
Yes, mc_vd should not be null in this case. This is a where you're
doing a read so you should have a block pointer in hand. From this
bp we can get the vdev that we need to perform the read and that
vdev better be a part of this pool. Can you share more information
about the pool configuration and also the blkptr that is in the
zio_t?
Well, we don't "own" these systems in question so we can't really
"debug" them interactively, we can only see these backtraces in crash
reports where the actual data structures are not being reported to
protect privacy for customers. To make the situation worse, we do not
yet know any way to reproduce it at lab, do you have any idea that
would worth a try?
I've only seen this type of panic in situations where the pool config is
somehow stale or corrupted so I also don't have a good way to reproduce
this. BTW, I suspect that once someone gets in this situation that there
is no recovery. In other words, there is a blkptr that has an invalid
vdev value and they will continue to see panics.
I'm thinking about instrumenting the code so it prints some
information before proceeding to panic so we can catch it next time,
so maybe we print out the contents of zio->io_bp as a start? Any
other information that can be collected to aid debugging? We could
add these to our codebase and see what we would get.
I would be interested in dumping out the blkptr so use sprintf_blkptr to
dump that out. I would also want to know if the zio->io_vd is NULL (I
suspect it is and we're dealing with a ditto block read). You'll also
want to get the pool config and the vdev and vdev id that it's panicking on.

I believe the pool is corrupted so you could make some changes to print
this information out and then build libzpool and have the customer run
'zdb -bbbbb <pool>' and collect the data that way.

- George
Post by George Wilson
Thanks, George
On 9/18/13 3:44 PM, Xin Li wrote: Hi,
We have seen this panic a few times in field but have not yet
figured out a way to reproduce it. The backtrace is usually
vdev_mirror_io_start() -> vdev_mirror_child_select() ->
vdev_is_dead()+0x1.
for (i = 0, c = mm->mm_preferred; i < mm->mm_children; i++, c++) {
(...) if (!vdev_readable(mc->mc_vd)) {
Where, in vdev_readable, it calls vdev_is_dead() with NULL (based
on the fault pointer we have seen).
My question is, should we expect mc->mc_vd be not NULL?
(Apparently vdev_lookup_top could return NULL). If so, maybe we
should add a test in vdev_is_dead?
https://www.delphij.net/ FreeBSD - The Power to Serve!
Live free or die
https://www.listbox.com/member/archive/rss/182191/22008002-303f2ff4
Modify Your Subscription: https://www.listbox.com/member/?&
Post by George Wilson
Powered by Listbox: http://www.listbox.com
https://www.listbox.com/member/archive/rss/182191/22993488-e3cd949b
https://www.listbox.com/member/?&
Powered by Listbox: http://www.listbox.com
- --
FreeBSD - The Power to Serve! Live free or die
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.21 (FreeBSD)
iQEcBAEBCgAGBQJSOkWfAAoJEG80Jeu8UPuzqdcH/idVk072Jj2FEAenTA2133M9
tQxRiOZy2gohOwYMGw5wE4RhVe5bxi5VlhZ15wFPTM0WoQmU58VOjcwEGKuQA4yE
yqbNmERfWLVYEZAhd6Ah/7QCowepKIEgD7NEcHIdRHvEDzNqhaYVtrQaiNWDbHzH
Uk26qJUNO9jlYnVDYTAZ7CgVBY71CAqaaLOE09nzH30BxfnRdiGKDYqVsufVQ1JE
H2kAw9pkLZabsNqPNhzZc1HaiK72rI57dJZCMIZ3E6p9VXWxvo2g4gybEk9CMQlR
ZlNM4UH/qbsi2fpzQmSh9JPH9vsAeYKoxI0RFl9gT8AuFWKyDsUfh0tCQkuJslY=
=ccAp
-----END PGP SIGNATURE-----
Loading...