4121 vdev_label_init should treat request as succeeded when pool is read only

[Christopher Siden]

http://code.delphix.com/illumos-4121/

Work by George Wilson. This should also address
https://github.com/zfsonlinux/zfs/issues/1863

Chris

[Saso Kiselkov]

Post by Christopher Siden
http://code.delphix.com/illumos-4121/
Work by George Wilson. This should also address
https://github.com/zfsonlinux/zfs/issues/1863

There appears to be a typo in the changed line:

if (!vd->vdev_ops->vdev_op_leaf || || !spa_writeable(spa))

should read:

if (!vd->vdev_ops->vdev_op_leaf || !spa_writeable(spa))

[George Wilson]

Hmm, I'll ask Chris about this since my repo shows this:

diff --git a/usr/src/uts/common/fs/zfs/vdev_label.c
b/usr/src/uts/common/fs/zfs/vdev_label.c
index 0b4915f..c7ae60c 100644
--- a/usr/src/uts/common/fs/zfs/vdev_label.c
+++ b/usr/src/uts/common/fs/zfs/vdev_label.c
@@ -640,7 +640,7 @@ vdev_label_init(vdev_t *vd, uint64_t crtxg,
vdev_labeltype_t reason)
/* 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);

/*

- George

Post by Saso Kiselkov

Post by Christopher Siden
http://code.delphix.com/illumos-4121/
Work by George Wilson. This should also address
https://github.com/zfsonlinux/zfs/issues/1863

if (!vd->vdev_ops->vdev_op_leaf || || !spa_writeable(spa))
if (!vd->vdev_ops->vdev_op_leaf || !spa_writeable(spa))

[Boris Protopopov]

Aside from a low-prio suggestion to move the check for spa_writable() above
the loop, LGTM.

Boris.


On Wed, Dec 11, 2013 at 12:36 PM, George Wilson

Post by George Wilson
diff --git a/usr/src/uts/common/fs/zfs/vdev_label.c
b/usr/src/uts/common/fs/zfs/vdev_label.c
index 0b4915f..c7ae60c 100644
--- a/usr/src/uts/common/fs/zfs/vdev_label.c
+++ b/usr/src/uts/common/fs/zfs/vdev_label.c
@@ -640,7 +640,7 @@ vdev_label_init(vdev_t *vd, uint64_t crtxg,
vdev_labeltype_t reason)
/* 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);
/*
- George

Post by Saso Kiselkov

Post by Christopher Siden
http://code.delphix.com/illumos-4121/
Work by George Wilson. This should also address
https://github.com/zfsonlinux/zfs/issues/1863

if (!vd->vdev_ops->vdev_op_leaf || || !spa_writeable(spa))
if (!vd->vdev_ops->vdev_op_leaf || !spa_writeable(spa))

-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182191/
23052084-8e2408bc
Modify Your Subscription: https://www.listbox.com/
member/?&
Powered by Listbox: http://www.listbox.com

--
Best regards,

Boris Protopopov
Nexenta Systems

455 El Camino Real, Santa Clara, CA 95050

[d] 408.791.3366 | [c] 978.621.6901
Skype: bprotopopov



-------------------------------------------
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

[Matthew Ahrens]

There is an extra "||". Aside from that, looks good to me.

--matt


On Wed, Dec 11, 2013 at 9:19 AM, Christopher Siden <

Post by Christopher Siden
http://code.delphix.com/illumos-4121/
Work by George Wilson. This should also address
https://github.com/zfsonlinux/zfs/issues/1863
Chris
-------------------------------------------
illumos-zfs
Archives: https://www.listbox.com/member/archive/182191/=now
https://www.listbox.com/member/archive/rss/182191/21635000-ebd1d460
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

[Saso Kiselkov]

Post by Matthew Ahrens
There is an extra "||". Aside from that, looks good to me.

+1, aside from the typo, LGTM

Cheers,