Hi Matt, * Matt Keenan ([email protected]) wrote: > On 05/12/11 11:31 PM, Glenn Lagasse wrote: > >Hey Drew, > > > >* Drew Fisher ([email protected]) wrote: > >>Good afternoon! > >> > >>Could I please get a code review for the following two bugs: > >> > >>7044170<http://monaco.us.oracle.com/detail.jsf?cr=7044170> AI > >>should support "s" for sector units > >>7044383<http://monaco.us.oracle.com/detail.jsf?cr=7044383> Zvol > >>deletion should cater for swap/dump deletion > >> > >>http://cr.opensolaris.org/~drewfish/cr_7044383/ > > > >Just a general comment about 7044383. What are we gaining by allowing > >the removal of the dump zvol if we can't simultaneously remove it from > >the dumpadm config? We're essentially 'breaking' dumpadm. We'll also > >need to revisit 'allow deletion of dump zvols' when (really, if > >considering it's state) 6910925 is addressed to complete the changes > >(and so I wonder if we might just want to hold off on allowing dump > >zvols to be destroyed until we really can do support that operation > >fully). > > > > Drew, > > Comment in logical.py : > "(although we can destroy the zvol)" > > This is not true, because we cannot unassign the zvol as a dump > device, the zvol itself cannot be destroyed, and therefore the pool > cannot be destroyed either, so I reckon best to remove this portion > of the comment. > > Glenn, we discussed this in detail with Drew, and CUD AI will trap > on this scenario if a manifest is specifying to remove a dump zvol > device, and put appropriate information to the log stating how to > work around this. Essentially we are not allowing it, until 6910925 > is addressed.
I guess having CUD AI trap for this is 'ok' but it seems less than ideal to me to support something that can't actually be fully accomplished. Requiring all consumers of logical.py to know that dump zvols have to be treated specially until some dumpadm deficiency is addressed could lead to issues imho if that subtlety isn't caught/understood. The code as-is doesn't actually work if the zvol in question is a dump zvol and some higher level consumer doesn't filter it out. So in essence, we aren't really addressing anything to do with dump zvols over what the code already did/tried to do. It would seem to make more sense to me to actually include code to detect if a dump zvol is what we're trying to destroy and actually not allow it since that's the reality until the dumpadm issue is addressed. There's also the issue of how does this code gets 'fixed' when/if the dumpadm issue gets resolved (we're closing our bug that purportedly allows for the deletion of the dump zvol, but not really since we're not removing the zvol from dumpadm first). I guess at a minimum I'd expect a new bug to be filed against logical.py for the TODO to finish the support for removing a dump zvol and link the dumpadm bug with it so that we'll at least have some way to track this. Although I think just not allowing dump zvols to be destroyed since they can't be is really the right thing to do imo. Cheers, -- Glenn _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
