Re: [Libguestfs] [PATCH 2/2] nbd: Add new qemu:joint-allocation metadata context

On Thu, Jun 10, 2021 at 02:52:10AM +0300, Nir Soffer wrote: Yeah, I'm happy to bike-shed the name. Note that dirty-bitmap tracking is harder to merge in: qemu supports exposing more than one dirty bitmap context at once, but where each context uses the same bit (bit 0, which overlaps with NBD_STATE_HOLE). We'd have to decide whether we support merging only a single bitmap, or whether a user can merge in up to 28 bitmaps (where the order in which they specify dirty bitmaps as part of the qemu:mega:bitmap1:bitmap2:... context name matters). And if we decide that merging in even a single dirty bitmap is too difficult, then clients already have to correlate a dirty bitmap with everything else, at which point correlating 3 contexts (dirty bitmap with base:allocation and qemu:allocation-depth) is no harder than correlating 2 contexts (dirty bitmap with qemu:joint-allocation or whatever we name it). Yeah, I just compressed your example. Correct. If all you care about is which portions of the image defer to the backing chain, NBD_STATE_LOCAL is sufficient. Looking at NBD_STATE_HOLE merely tells you which portions of the qcow2 file have not been pre-allocated, but that has no bearing on whether those clusters defer to the backing chain. Unfortunately, that is not how qemu tracks things. Raw files have no backing file, and therefore 100% of the file reports as BDRV_BLOCK_ALLOCATED (which really means the raw file is responsible for 100% of the guest-visible content, and has no bearing to how much filesystem space is allocated in the host). Both a completely sparse and a completely allocated raw file will have the same result of showing the entire file as 'local'. Reporting 'hole,zero,unallocated' would be a lie - it would be claiming that 100% of the raw file defers to the backing chain but that in turn the backing chain has not been allocated. In reality, when bits 2+3 report as 0 (neither NBD_STATE_LOCAL nor NDD_STATE_BACKING was set), qemu is reporting that the cluster defaults to reading as zero because we exhausted the entire backing chain without finding any layer willing to provide those contents. Maybe "unallocated" isn't the best term, and I should use "absent" instead? That way, we don't perpetuate the confusion of HOLE (which is about host storage allocation) with backing chain presence (where a particular layer in a backing chain provides or defers data regardless of host allocation). Again, that's what qemu already uses. A raw file has no backing file, so the entire guest contents come from the current layer in the backing chain (there are no deeper layers to look at). An empty qcow2 file (whether or not a backing file is specified) _does_ defer all clusters to whatever the backing chain would supply. Maybe the name "qemu:allocation-depth" is a misnomer, and we should have called it "qemu:backing-depth", but since that was chosen in 5.2, it's harder to change now. No, that's not a bug, but intentional behavior, and changing it would break how qemu-img convert and friends operate. Correct - the shortest sequence would be first sending NBD_OPT_SET_META_CONTEXT with _just_ "qemu:joint-allocation". If the server answers with a context id, you're set; if it answers with an error, the server doesn't support contexts at all so there's no point trying anything else (your fallbacks would also fail). If it answers with 0 contexts, then the context id was unrecognized (you are not talking to a version of qemu that recognizes it), so your fallback would then be a second NBD_OPT_SET_META_CONTEXT, this time with both "base:allocation" and "qemu:allocation-depth", and see if you get 1 or 2 ids back. (If you can assume that all qemu versions in your downstream ecosystem have been patched such that qemu:joint-allocation is always present if base:allocation does not have 5.2 semantics, then you can simplify the fallback to using just "base:allocation", but that's only safe in a curated downstream ecosystem.) But remember that qemu can expose more than one dirty bitmap at a time, so then we start having to further complicate things by determining _which_ bit in the combined context would represent _which_ dirty bitmap. Merging things that only depend on bdrv_block_status() was easy, but merging multiple bitmaps starts to make the qemu implementation complicated enough that I'm leaning towards requiring the client to do the cross-context correlation rather than burdening the server with the task. Both calls boil down to bdrv_block_status at some point, so you are right that figuring out how to get the information consolidated in 1 call would be more efficient. Indeed, adding yet another wrapper around the core functionality of bdrv_block_status to get what we want in one call is a nice idea. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org