Re: [libvirt] [PATCH 1/2] rbd: Add wiping RBD volumes by using rbd_discard() or rbd_write()

On 12/23/2015 10:06 AM, Wido den Hollander wrote: Found these buried in my todo list of things to look at from during the holiday break. I figure by bumping it'll bring it back into focus... "Semantically" speaking - this patch is a v2 of the original patch series... I'm still a bit conflicted whether to add a new option to Wipe or whether a new API should be developed. I see value in both options. Although perhaps thinking of this as "trim" and not "discard" could make it more palatable for wipe. As a new API, each backend driver could decide whether it supports the discard/trim option, but that's quite a bit more work (essentially mimic the Wipe functionality, but generate Trim). I'll note off the top that if we go with adding a new wipe algorithm and we've updated virsh-volume.c to recognize that, then virsh.pod would also need an update to describe it. Also rather than one patch here - I suggest smaller individual patches to make it easier to debug issues down the line when using git bisect. I see perhaps 4 patches... Patch 1: You probably want to start by adjusting virStorageBackendVolWipeLocal. In particular, the switch statement there needs some tweaking - first to use the "switch ((virStorageVolWipeAlgorithm) algorithm) {" construct, but also fixing a 'bug' I just noted in the current design. If the current 'default:' option is taken, the code reports an error, but still attempts the SCRUB command (which will return/cause a different error). BTW: Instead of default it would the *_LAST case... If you're really ambitious, adding a check for the "expected" 'flags' bits would also be beneficial especially since you'll be adding one. Patch 2: Add wipe support for rbd and add the Zero algorithm. This gives a base. The switch in virStorageBackendRBDVolWipe could still remain, but the Flags would only be for _ZERO Patch 3: Then add a the 'trim' option to libvirt-storage.h, virsh-volume.c, and virsh.pod... Patch 4: This patch would add the 'trim' support to the backend. Also grab virStorageBackendRBDImageInfo from patch 2. You're making the same 'stripe_count' call in this patch, but don't have the same checks. If you're concerned about the perhaps extra unnecessary calls you could allow the 3 return parameters to be NULL, then prior to fetching do "if (param)" type trick. The caller could then provide a NULL if they don't care about features and unit... Assuming we use wipe, I think "TRIM" with the description of the option to be "trimming" the contents of the volume. Whether that's sparse files, thin/sparse logical volumes, or rbd object discarding... The aren't your problem to solve here, unless you have that desire to make those changes too. Also, the 2nd/3rd comments should line up under 1-pass... This isn't necessary I believe. I was able to remove without issue. Newer libvirt convention is: static int virStorage... Add: int ret = -1; Usually it's 'ret' instead of just 'r'... Keeping 'r' for rbd_*() call failures fine though since that will contain (and possibly message) rbd_* specific API call errors... This will generate two spaces "... failed on RBD image..." So is length a "uint64_t" or not? I do note that librdb.h deems it a "size_t"... The query is more why caste to (unsigned long long) other than the %llu (of course). As for offset, IIRC the convention is "%zu", although for this one I note that the librdb.h deems it a "uint64_t". similar comments regarding the castes and the variable types. Here would be: ret = 0; writebuf is leaked. Need a VIR_FREE() and this becomes return ret; static int virStorage... Need int ret = -1 rbd_discard deems 'offset' to also be a uint64_t similar to *Zero - you'll have "...failed on RBD image..." similar comments regarding caste's of length and offset similar comments regarding caste's Here would be ret = 0; And return ret; static int virStorage... Add int ret = -1; BTW: This can be : if ((r = rbd_open(ptr.ioctx, vol->name, &image, NULL)) < 0) { For this and all rbd_* calls... I see the subsequent patch has some extra checks before calling this. Why wouldn't those also need to be made here? Follow the convention of "switch ((virStorageVolWipeAlgorithm) algorithm) {" Then each "case" lines up under "switch". I would change this (and the next one) to: if (virStorageBackendRBDVolWipeZero(image, vol->name, info, stripe_count) < 0) goto cleanup; Also, I ran these patches through Coverity - it complains that 'info' is passed by value of 160 bytes... Although neither API adjusts it, why not just pass "info.size" and "info.obj_size" or pass by reference the whole 'info' (just to be safe). And listing each case allowed - so it's clearer. That way if someone in the future comes along and adds ALG_ONE, the rbd code isn't forgotten to be adjusted... The compiler catches it. This will be unnecessary... This overwrites the errors found in the *WipeZero and *WipeDiscard API's The assumption here being ret = 0; > + if (image) > + rbd_close(image); > + > + virStorageBackendRBDCloseRADOSConn(&ptr); return ret; No need to remove the "," - that way the only diff is the line. I think "trim" will be better. John