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@xxxxxxxxxx> >> --- >> 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(); >> + } -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list