On 10/10/12 01:37, dann frazier wrote: > hey Dmitrijs! > I prepared a similar fix for this a couple of weeks ago that has been > in testing at a customer site. Looks like we crossed paths, but our > fixes are very similar. My patch has been working well for my test > case, and I've verified yours does too. However, I've a question on > your patch by way of review.. and selfishly to find out if I've an > issue in my implementation. > > Is the vgdisplay bit known to be necessary, or is it just > precautionary? I wouldn't think that vgdisplay did anything more than > vgs with respect to scanning - if that is true, it should should make > the vgdisplay redundant with the vgs call in this call path: > > device_remove_lvm() > -> pv_on_device() > -> pvs()) > > If you can dispense w/ the vgdisplay, I'd propose moving the > update-dev and device_remove_lvm calls into the preceding loop instead > of adding a new one, just to improve readability. > I didn't want to put update-dev in the preceding loop, because then it could be run more than once, when only one time is needed (after all partition tables have been written out). Looking at your analysis, the vgdisplay call is indeed currently redundant, due to a side-effect of the device_remove_lvm call. But this is a nasty & confusing bug and I don't want it to come back, if the internal semantics of device_remove_lvm will stop making calls to pvs (e.g. wipe lvm metadata with dd or something). Hence the precautionary call to vgdisplay. Strictly speaking vgdisplay call is not necessary and not having it there is mostly harmless. Thanks for reviewing my patch. -- Regards, Dmitrijs.
Attachment:
signature.asc
Description: OpenPGP digital signature