[libvirt] [PATCH] storage: Fix problem with disk backend pool allocation calculation

[Ján Tomko]

On Sat, May 23, 2015 at 09:39:58AM -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1224018
> 
> The disk pool recalculates the pool allocation, capacity, and available
> values each time through processing a newly created disk partition.
> However, there were two issues with doing so. The process generally is
> to read the partition table via virStorageBackendDiskReadPartitions and
> process the volume. This is called during create and refresh backend
> API's with the only difference being create passes a specific volume
> to be processed while refresh processes all volumes (passing NULL for
> the volume value).
> 
> The first issue via virStorageBackendDiskCreateVol was that the pool's
> available value was cleared, so when virStorageBackendDiskMakeVol
> processes the (new) volume it will ignore any other partitions on the
> disk, thus after returning the pool allocation would only include the
> newly added volume.
> 
> The second is while processing a partition, the adjustment of the
> available value depends on the type of partition. For "primary" and
> "logical" partitions, the available value was adjusted. However, for
> "extended" partitions, the available value wasn't adjusted. Since the
> calling function(s) storageVolCreateXML[From] will only adjust the
> pool available value when they determine that the backend code didn't
> this resulted in the available value being incorrectly adjusted by
> that code. If a pool refresh was executed, the "correct" value showed up.
> 
> To fix the first issue, only clear the available value when we're
> being processed from the refresh path (vol will be NULL). To fix the
> second issue, increment available by 1 so that the calling function
> won't adjust after we're done. That could leave the appearance of a
> single byte being used by a pool that only has an extended partition,
> but that's better than having it show up as the size of the partition
> until someone refreshes.

It is marginally better because '1' is obviously wrong, but not
adjusting the value at all would be the real fix.

> The refresh path will also nick by the same
> value so it'll be consistent.

Two issues deserve two separate patches, especially if the explanation
is that long.

> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/storage/storage_backend_disk.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
> index 6394dac..bc14aab 100644
> --- a/src/storage/storage_backend_disk.c
> +++ b/src/storage/storage_backend_disk.c
> @@ -164,8 +164,18 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool,
>              return -1;
>      }
>  
> +    /* For "data", adjust the pool's allocation by the size of the volume
> +     * For "metadata" (aka "extended" volume), we haven't yet used the space;
> +     * however, the calling createVol path will adjust the pool allocation
> +     * by the volume allocation if it determines a storage backend doesn't
> +     * adjust the pool's allocation value. So bump the allocation to avoid
> +     * having the calling code misrepresent the value. The refresh path
> +     * would resolve/change the value.

I think the calling code should not be interpreting the value at all -
either it should be the backend's responsibility to update the values
and the caller should not touch it at all, or the caller should only
update them for those backends that are known not to do it themselves.
That way it's clear which code should be responsible for updating the
values.

> +     */
>      if (STRNEQ(groups[2], "metadata"))
>          pool->def->allocation += vol->target.allocation;
> +    else
> +        pool->def->allocation++;
>      if (vol->source.extents[0].end > pool->def->capacity)
>          pool->def->capacity = vol->source.extents[0].end;
>  
> @@ -309,7 +319,13 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool,
>                                 pool->def->source.devices[0].path,
>                                 NULL);
>  
> -    pool->def->allocation = pool->def->capacity = pool->def->available = 0;
> +    /* The reload/restart/refresh path will recalculate everything;
> +     * otherwise, keep allocation as is as capacity and available
> +     * will be adjusted
> +     */

'refresh path' is ambiguous here - technically we're refreshing a
volume if one was passed. And it's not clear to me from the comment if
everything will be recalculated by virStorageBackendDiskMakeVol, or by
some other function called after this function on the (pool) refresh path.

/* If a volume is passed, virStorageBackendDiskMakeVol only updates the pool
 * allocation for that single volume.
 */

Jan

> +    if (!vol)
> +        pool->def->allocation = 0;
> +    pool->def->capacity = pool->def->available = 0;
>  
>      ret = virCommandRunNul(cmd,
>                             6,
> -- 
> 2.1.0
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- 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/20150526/e07cdc37/attachment-0001.sig>