> On Oct. 6, 2014, 11:31 p.m., Bayard Bell wrote: > > I'm a bit uncomfortable with the level of bug reporting and fix analysis > > here. The changes would seem sufficient to preventing panic in this case of > > import failure, but what's not clear is how we expect this to be handled > > thereafter, given that we don't handle it in these code paths, such that we > > know that we are doing the right thing, minimally for the range of errors > > that metaslab_init() can currently be passed by space_map_open(). > > Prakash Surya wrote: > metaslab_init() is only called from vdev_metaslab_init(). > vdev_metaslab_init() is called from two locations, vdev_load() and > vdev_expand(). vdev_load() handles this error (any vdev_metaslab_init() > error) by setting the the vdev_state, and vdev_expand handles this error > (again, any vdev_metaslab_init() error) by panic'ing. > > Do you have any specific concerns? The error seems to be handled clearly > to me. Am I overlooking something? > > Also, I should note, vdev_metaslab_init() can return an error if > dbuf_read() fails; so callers of vdev_metaslab_init() already had to support > the possiblity of this function failing.
My concerns aren't with the code per se but with the lack of explicit analysis offered about the immediate problems and concerns about generalising a fix beyond those facts. It's not that I don't reckon you've done that, but as a reviewer I'd prefer to have the root cause and fix analysis fleshed out, which I understand to be what advocates also now require (thus what I wrote up for 5133 in the issue tracker is quite extensive by comparison, where most of the detail was provided at RTI). That's to say I grant that what I'm asking is a matter of preference, but I take my preferences to be consistent with the bar for RTI and no less relevant at this stage. - Bayard ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.csiden.org/r/110/#review273 ----------------------------------------------------------- On Oct. 6, 2014, 2:02 p.m., Prakash Surya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.csiden.org/r/110/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2014, 2:02 p.m.) > > > Review request for OpenZFS Developer Mailing List and Christopher Siden. > > > Bugs: 5213 > https://www.illumos.org/projects/illumos-gate//issues/5213 > > > Repository: illumos-gate > > > Description > ------- > > 5213 panic in metaslab_init due to space_map_open returning ENXIO > Reviewed by: Matthew Ahrens <[email protected]> > Reviewed by: George Wilson <[email protected]> > > Original author: Prakash Surya > > > Diffs > ----- > > usr/src/uts/common/fs/zfs/vdev.c 7571b21a5f8cd5c0a1d6406556bcc22fcaceb814 > usr/src/uts/common/fs/zfs/sys/metaslab.h > 7fd47c08c9df5a7a3f6f24c6a4deda84502d0b19 > usr/src/uts/common/fs/zfs/metaslab.c > a33ec7f628108f5c3d2206583b060e2d580a7dca > > Diff: https://reviews.csiden.org/r/110/diff/ > > > Testing > ------- > > > Thanks, > > Prakash Surya > >
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
