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>
> ---
> 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?
It seemed more straightforward to do it all at once, but if you want 2
patches - that's not a problem. I just want to be clear first...
John
Jan