spa_validate_aux called by spa_import in read-only mode

[Xin Li]

Hi,

I have run into an assertion in zio.c on FreeBSD while trying to
import a pool read-only and believes it affects Illumos too:

zio->io_type != ZIO_TYPE_WRITE || spa_writeable(spa)

Looking more closely, it seems that the write was initiated by
vdev_label_init, which is in turn called from spa_import via
spa_validate_aux.

If my understanding is correct, the call of spa_validate_aux is not
really necessary when pool is imported read only, so something like
this would be needed:

diff --git a/usr/src/uts/common/fs/zfs/spa.c
b/usr/src/uts/common/fs/zfs/spa.c
index 7334d39..2ce775d 100644
- --- a/usr/src/uts/common/fs/zfs/spa.c
+++ b/usr/src/uts/common/fs/zfs/spa.c
@@ -3888,12 +3888,14 @@ spa_import(const char *pool, nvlist_t *config,
nvlist_t *props, uint64_t flags)

VERIFY(nvlist_lookup_nvlist(config, ZPOOL_CONFIG_VDEV_TREE,
&nvroot) == 0);
- - if (error == 0)
- - error = spa_validate_aux(spa, nvroot, -1ULL,
- - VDEV_ALLOC_SPARE);
- - if (error == 0)
- - error = spa_validate_aux(spa, nvroot, -1ULL,
- - VDEV_ALLOC_L2CACHE);
+ if (mode & FWRITE) {
+ if (error == 0)
+ error = spa_validate_aux(spa, nvroot, -1ULL,
+ VDEV_ALLOC_SPARE);
+ if (error == 0)
+ error = spa_validate_aux(spa, nvroot, -1ULL,
+ VDEV_ALLOC_L2CACHE);
+ }
spa_config_exit(spa, SCL_ALL, FTAG);

if (props != NULL)


Cheers,
- --
Xin LI <***@delphij.net> https://www.delphij.net/
FreeBSD - The Power to Serve! Live free or die

[George Wilson]

I think a more complete fix would be to do the following:

diff -r 91dbcb77a2f9 usr/src/uts/common/fs/zfs/vdev_label.c
--- a/usr/src/uts/common/fs/zfs/vdev_label.c Tue Oct 02 05:18:23 2012
+0100
+++ b/usr/src/uts/common/fs/zfs/vdev_label.c Wed Aug 14 12:48:42 2013
-0700
@@ -656,7 +656,7 @@
/* Track the creation time for this vdev */
vd->vdev_crtxg = crtxg;

- if (!vd->vdev_ops->vdev_op_leaf)
+ if (!vd->vdev_ops->vdev_op_leaf || !spa_writeable(spa))
return (0);


This will ensure that we don't have add checks in all the consumers of
vdev_label_init().

Thanks,
George

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Hi,
I have run into an assertion in zio.c on FreeBSD while trying to
zio->io_type != ZIO_TYPE_WRITE || spa_writeable(spa)
Looking more closely, it seems that the write was initiated by
vdev_label_init, which is in turn called from spa_import via
spa_validate_aux.
If my understanding is correct, the call of spa_validate_aux is not
really necessary when pool is imported read only, so something like
diff --git a/usr/src/uts/common/fs/zfs/spa.c
b/usr/src/uts/common/fs/zfs/spa.c
index 7334d39..2ce775d 100644
- --- a/usr/src/uts/common/fs/zfs/spa.c
+++ b/usr/src/uts/common/fs/zfs/spa.c
@@ -3888,12 +3888,14 @@ spa_import(const char *pool, nvlist_t *config,
nvlist_t *props, uint64_t flags)
VERIFY(nvlist_lookup_nvlist(config, ZPOOL_CONFIG_VDEV_TREE,
&nvroot) == 0);
- - if (error == 0)
- - error = spa_validate_aux(spa, nvroot, -1ULL,
- - VDEV_ALLOC_SPARE);
- - if (error == 0)
- - error = spa_validate_aux(spa, nvroot, -1ULL,
- - VDEV_ALLOC_L2CACHE);
+ if (mode & FWRITE) {
+ if (error == 0)
+ error = spa_validate_aux(spa, nvroot, -1ULL,
+ VDEV_ALLOC_SPARE);
+ if (error == 0)
+ error = spa_validate_aux(spa, nvroot, -1ULL,
+ VDEV_ALLOC_L2CACHE);
+ }
spa_config_exit(spa, SCL_ALL, FTAG);
if (props != NULL)
Cheers,
- --
FreeBSD - The Power to Serve! Live free or die
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (FreeBSD)
iQEcBAEBCgAGBQJSCpJBAAoJEG80Jeu8UPuz/a0H/jgsGUkEU0WtQT7aHSANqOPz
aH7WMj7Sozkpx9ceRQbR62cFCrg55P+Z9BCWA/Bbgge4bG8AhwZPriTuSV+3WVCt
/L9Q4t4O7hfoxlnx6GWKbRTEanjDZMQVFtaOsFHFMDoEbAxE4uKd+tk/FSd1HrQC
YZQrukEmI09zsldr/u9rtPEGAwGLDT5mr/lHigo9UtlSYNvdvHzjwai/yUKTv2rZ
X8Yv/qUxBSXOJ6OJ1UNezkB8sm2AzuYKT6+1HbuxoRPIUAIeAxr4NvFtFshmcNKF
A36J+0JviUrVG228e3LKu6nEiUSw7TAnOiFPrn6VsEcLRYN2GB3tZZzkC3piOSc=
=noL9
-----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]

diff -r 91dbcb77a2f9 usr/src/uts/common/fs/zfs/vdev_label.c ---
a/usr/src/uts/common/fs/zfs/vdev_label.c Tue Oct 02 05:18:23
2012 +0100 +++ b/usr/src/uts/common/fs/zfs/vdev_label.c Wed Aug
time for this vdev */ vd->vdev_crtxg = crtxg;
- if (!vd->vdev_ops->vdev_op_leaf) + if
(!vd->vdev_ops->vdev_op_leaf || !spa_writeable(spa)) return (0);
This will ensure that we don't have add checks in all the consumers
of vdev_label_init().

Oh that's good point! Yes, I think this is a better solution.

Cheers,
- --
Xin LI <***@delphij.net> https://www.delphij.net/
FreeBSD - The Power to Serve! Live free or die

[Albert Lee]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

diff -r 91dbcb77a2f9 usr/src/uts/common/fs/zfs/vdev_label.c ---
a/usr/src/uts/common/fs/zfs/vdev_label.c Tue Oct 02 05:18:23
2012 +0100 +++ b/usr/src/uts/common/fs/zfs/vdev_label.c Wed Aug
time for this vdev */ vd->vdev_crtxg = crtxg;
- if (!vd->vdev_ops->vdev_op_leaf) + if
(!vd->vdev_ops->vdev_op_leaf || !spa_writeable(spa)) return (0);
This will ensure that we don't have add checks in all the consumers
of vdev_label_init().

Oh that's good point! Yes, I think this is a better solution.

That change makes sense to me. I think I've seen this while testing with
libzpool.

-Albert

Cheers,
- --
FreeBSD - The Power to Serve! Live free or die
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (FreeBSD)
iQEcBAEBCgAGBQJSC+8/AAoJEG80Jeu8UPuz5tQIAJUHQPbc9ybATgeSm7E9+KHu
O+Iej+AyPWKvD97Kruey78erM9hjl8kuqgGk5qhmYCoxMfxfYWUp0vayU63MPkUj
+rsJKNsf6D5fULFQfx5d9tKgUyOdrV1anvK9LdX6QKDVMeEs3DLgYYFQK+CVE4jR
KA5I6jffqlegyd+wL4qlduWyp1Rnjf1NOtnJYzitVvfAG8mMU03WEZwkGw2v1m3Z
TDfSmtdXYx3DFx5CIpeuwl5LIuokJzJJEI42GsMwmZEeL8ecFCPAfagKHL0kAm8F
fkj7PbDQ41XqR2P+ipdxoah8oZdlmSkjl2+e07kO9f2rpfaPvljQV4ODSI58vOk=
=Wk4P
-----END PGP SIGNATURE-----

--
Albert Lee <***@nexenta.com>
Nexenta Systems, Inc. <http://www.nexenta.com>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

diff -r 91dbcb77a2f9 usr/src/uts/common/fs/zfs/vdev_label.c ---
a/usr/src/uts/common/fs/zfs/vdev_label.c Tue Oct 02 05:18:23
2012 +0100 +++ b/usr/src/uts/common/fs/zfs/vdev_label.c Wed Aug
time for this vdev */ vd->vdev_crtxg = crtxg;
- if (!vd->vdev_ops->vdev_op_leaf) + if
(!vd->vdev_ops->vdev_op_leaf || !spa_writeable(spa)) return (0);
This will ensure that we don't have add checks in all the consumers
of vdev_label_init().

Oh that's good point! Yes, I think this is a better solution.
Cheers,
- --
FreeBSD - The Power to Serve! Live free or die
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (FreeBSD)
iQEcBAEBCgAGBQJSC+8/AAoJEG80Jeu8UPuz5tQIAJUHQPbc9ybATgeSm7E9+KHu
O+Iej+AyPWKvD97Kruey78erM9hjl8kuqgGk5qhmYCoxMfxfYWUp0vayU63MPkUj
+rsJKNsf6D5fULFQfx5d9tKgUyOdrV1anvK9LdX6QKDVMeEs3DLgYYFQK+CVE4jR
KA5I6jffqlegyd+wL4qlduWyp1Rnjf1NOtnJYzitVvfAG8mMU03WEZwkGw2v1m3Z
TDfSmtdXYx3DFx5CIpeuwl5LIuokJzJJEI42GsMwmZEeL8ecFCPAfagKHL0kAm8F
fkj7PbDQ41XqR2P+ipdxoah8oZdlmSkjl2+e07kO9f2rpfaPvljQV4ODSI58vOk=
=Wk4P
-----END PGP SIGNATURE-----
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
https://www.listbox.com/member/archive/rss/182191/22051446-3e2d4190
https://www.listbox.com/member/?&
Powered by Listbox: http://www.listbox.com

--
Albert Lee <***@nexenta.com>
Nexenta Systems, Inc. <http://www.nexenta.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

[Yuri Pankov]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

diff -r 91dbcb77a2f9 usr/src/uts/common/fs/zfs/vdev_label.c ---
a/usr/src/uts/common/fs/zfs/vdev_label.c Tue Oct 02 05:18:23
2012 +0100 +++ b/usr/src/uts/common/fs/zfs/vdev_label.c Wed Aug
time for this vdev */ vd->vdev_crtxg = crtxg;
- if (!vd->vdev_ops->vdev_op_leaf) + if
(!vd->vdev_ops->vdev_op_leaf || !spa_writeable(spa)) return (0);
This will ensure that we don't have add checks in all the consumers
of vdev_label_init().

Oh that's good point! Yes, I think this is a better solution.
That change makes sense to me. I think I've seen this while testing
with libzpool.

Thanks for fixing this. This was already reported on the list (see the
"zpool sends write zios to auxiliary vdevs when attempting to import in
readonly mode (was: ZFS panic loop problem on FreeBSD 9.1-RELEASE)"
thread back in March), and is pretty easy to reproduce, just importing a
pool having cache vdevs in readonly mode.

[Yuri Pankov]

Post by George Wilson
diff -r 91dbcb77a2f9 usr/src/uts/common/fs/zfs/vdev_label.c
--- a/usr/src/uts/common/fs/zfs/vdev_label.c Tue Oct 02 05:18:23 2012
+0100
+++ b/usr/src/uts/common/fs/zfs/vdev_label.c Wed Aug 14 12:48:42 2013
-0700
@@ -656,7 +656,7 @@
/* Track the creation time for this vdev */
vd->vdev_crtxg = crtxg;
- if (!vd->vdev_ops->vdev_op_leaf)
+ if (!vd->vdev_ops->vdev_op_leaf || !spa_writeable(spa))
return (0);
This will ensure that we don't have add checks in all the consumers of
vdev_label_init().

Hi George,

Given that this is being hit from time to time, could you please
integrate the fix (or I could take it)?

https://www.illumos.org/issues/4121

Post by George Wilson

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Hi,
I have run into an assertion in zio.c on FreeBSD while trying to
zio->io_type != ZIO_TYPE_WRITE || spa_writeable(spa)
Looking more closely, it seems that the write was initiated by
vdev_label_init, which is in turn called from spa_import via
spa_validate_aux.
If my understanding is correct, the call of spa_validate_aux is not
really necessary when pool is imported read only, so something like
diff --git a/usr/src/uts/common/fs/zfs/spa.c
b/usr/src/uts/common/fs/zfs/spa.c
index 7334d39..2ce775d 100644
- --- a/usr/src/uts/common/fs/zfs/spa.c
+++ b/usr/src/uts/common/fs/zfs/spa.c
@@ -3888,12 +3888,14 @@ spa_import(const char *pool, nvlist_t *config,
nvlist_t *props, uint64_t flags)
VERIFY(nvlist_lookup_nvlist(config, ZPOOL_CONFIG_VDEV_TREE,
&nvroot) == 0);
- - if (error == 0)
- - error = spa_validate_aux(spa, nvroot, -1ULL,
- - VDEV_ALLOC_SPARE);
- - if (error == 0)
- - error = spa_validate_aux(spa, nvroot, -1ULL,
- - VDEV_ALLOC_L2CACHE);
+ if (mode & FWRITE) {
+ if (error == 0)
+ error = spa_validate_aux(spa, nvroot, -1ULL,
+ VDEV_ALLOC_SPARE);
+ if (error == 0)
+ error = spa_validate_aux(spa, nvroot, -1ULL,
+ VDEV_ALLOC_L2CACHE);
+ }
spa_config_exit(spa, SCL_ALL, FTAG);
if (props != NULL)