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

[Ján Tomko]

On Wed, Dec 16, 2015 at 09:19:07AM -0500, John Ferlan wrote:
> 
> 
> 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".
> 

I was thinking out loud and I did not mean that the documentation
changes should be a part of this patch. Sorry if I was not clear enough.

> 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?
> 

I have a slight preference for treating it as a feature and not
adjusting the docs.

> >> Signed-off-by: John Ferlan <jferlan at 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?
> 

The patch I linked ajdusts the return value for the only code path that
did not follow the 0/-1 convention.

So:
ACK to the non-documenation hunk of this patch, in its entirety.
I would prefer if you could wait for my patch adjusting the return
values before pushing this one, but that's not a hard prerequisite.

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20151216/02107cb2/attachment-0001.sig>