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

[John Ferlan]

On 12/11/2015 10:56 AM, Ján Tomko wrote:
> On Wed, Dec 02, 2015 at 03:22:38PM -0500, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1270709
>>
>> When a volume wipe is successful, a volume refresh should be done 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 would 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.
>>
> 
> I would expect that after wiping a 200 MB volume with zeros, it would
> contain 200 MB of zeros and it would not be shrinkable to 50 MB, not
> even after a volume refresh.
> 
> While it seems ftruncate to 0 bytes and back satisfies the documentation
> of the virStorageVolWipe API:
>   Ensure data previously on a volume is not accessible to future reads
> the ALG_ZERO description says:
>   VIR_STORAGE_VOL_WIPE_ALG_ZERO   =   0   1-pass, all zeroes
> 
> Do we need to update it to reflect that there might not be any pass over
> the old data (which might not happen for non-sparse files either,
> if the filesystem does not overwrite the same sectors)?
> 

If updating the docs is necessary - that's fine. Would it be better to
update (all or some of):

  1. virStorageVolWipe
  2. virStorageVolWipePattern
  3. virStorageVolWipeAlgorithm
  4. virsh.pod (vol-wipe)

or do you think it's a bug that some backend uses ftruncate() rather
than writing zero's to the volume? See commit id '73adc0e5'. I viewed
things as - at some point in time it was deemed acceptable to use
ftruncate(), so at least make sure "update" the volume data after wipe
is done. I think perhaps calling resize is overkill, but also didn't
want to miss out on some "other" backend that did something similar - so
taking the broad brush rather than the targeted approach.

If updating docs is fine, then how about the following wording:

"For some storage backends, the use of the pass all zeros algorithm
VIR_STORAGE_VOL_WIPE_ALG_ZERO (default for virStorageVolWipe()), will
result in a volume truncation rather than writing all zeros to the
volume. After the volume is wiped, libvirt will refresh volume data
usage statistics, including allocation and capacity."

NB:
Calling refreshVol after has the same result as if someone does a "virsh
vol-info" or "virsh vol-list default --details"...


>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/storage/storage_driver.c | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>> index bbf21f6..2e59e39 100644
>> --- a/src/storage/storage_driver.c
>> +++ b/src/storage/storage_driver.c
>> @@ -2436,7 +2436,19 @@ storageVolWipePattern(virStorageVolPtr obj,
>>          goto cleanup;
>>      }
>>  
>> -    ret = backend->wipeVol(obj->conn, pool, vol, algorithm, flags);
>> +    if ((ret = backend->wipeVol(obj->conn, pool, vol, algorithm, flags)) < 0)
>> +        goto cleanup;
> 
> More readable as:
>   if (func() < 0)
>      goto cleanup;
> 
>   ret = 0;
> If the return value does not need to be propagated. (Which it should not
> here, but it is possible in some cases. I will send a patch)
> 

Reviewer dependent I think...  Seems some want to see the one line like
I have above, while others want to see the multi-line.  I used to be in
favor of multi-line, mostly because of the need to be careful about
where you put that right ")"!

In any case, I see more code come through with the single line, so I've
retrained my fingers to use that model.  Doesn't really matter to me
either way and I don't recall seeing anything in the contributor
guidelines...

>> +
>> +    /* Best effort to refresh the volume data. If unsuccessful, we've already
>> +     * wiped the data so there's no going back on that. Best we can do is
>> +     * provide some details over what happened and move on
>> +     */
> 
> The wipe did happen, but if refreshVol fails, there is something
> seriously wrong with the volume. I think returning -1 and reporting an
> error is reasonable here.
> 

Ironically I went with WARNING, but would prefer error; however, flipped
a virtual coin and went with WARN.

I also had, but ditched a patch that would show the sizes on error for
volume shrink. That is in storageVolResize rather than:


        virReportError(VIR_ERR_INVALID_ARG, "%s",
                       _("can't shrink capacity below "
                         "existing allocation"));

I had done something like

  _("cannot shrink capacity of %lld below "
    "existing allocation of %lld"),
  abs_capacity, vol->target.allocation

But of course the value internally is in bytes, so it looks odd and
isn't what the user provided. So I changed to using %lldK and dividing
by 1024, but then thought - damn someone that provided M will not be
happy... Couldn't win, so I just removed it.


John
> Jan
> 
>> +    if (backend->refreshVol &&
>> +        backend->refreshVol(obj->conn, pool, vol) < 0) {
>> +        VIR_WARN("failed to refresh volume '%s' info after volume wipe",
>> +                 vol->name);
>> +        virResetLastError();
>> +    }