Re: [libvirt] [PATCH v2] storage: Attempt to refresh volume after successful wipe volume

On 12/16/2015 08:48 AM, Ján Tomko wrote: > On Tue, Dec 15, 2015 at 03:03:33PM -0500, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1270709 >> >> When a volume wipe is successful, perform a volume refresh afterwards to >> update any volume data that may be used in future volume commands, such as >> volume resize. For a raw file volume, a wipe could truncate the file and >> a followup volume resize the capacity may fail because the volume target >> allocation isn't updated to reflect the wipe activity. >> >> Since the documentation doesn't mention this side effect of the zero >> algorithm for a raw file volume, update the various documentation to >> describe the results. >> > > The documentation does not belong in this patch. Also, we could > intentionally keep it vague so that we don't have to commit to that > behavior. > Adding the documentation was a reaction to your review of v1: http://www.redhat.com/archives/libvir-list/2015-December/msg00464.html where you queried whether we should update the documentation "to reflect that there might not be any pass over the old data".

Whether the doc changes are kept or not I suppose then becomes "consensus based". I see value in describing what's done and I see value in being vague enough so that we could change to do something else in the future. The question thus remains, should the current truncate be considered a bug? Or a happy consequence/feature?

>> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> v1: >> http://www.redhat.com/archives/libvir-list/2015-December/msg00085.html >> >> Changes since v1: >> * Use the preferred call format from review > >> * Cause error if refreshVol fails > > If my patch adjusting the return value gets pushed before this one: > https://www.redhat.com/archives/libvir-list/2015-December/msg00467.html > > that change is just cosmetic. yep. > Otherwise, I don't think a patch adding refreshVol should be changing > the return value. > So again, your initial review says: More readable as: if (func() < 0) goto cleanup; Which is what I followed. Other than the value of ret being -errno on fdatasync - is the objection based solely on you'd like to see a separate patch to handle the ret differently for the wipeVol call, then a patch for refreshVol?