Re: [libvirt] [PATCH 2/3] storage: optional 'refresh_volume_allocation' attribute on pool

On 3/15/19 4:17 PM, jdillama(a)redhat.com wrote: > From: Jason Dillaman <dillaman(a)redhat.com&gt; > > When this attribute is set to 'capacity', refreshing volume > allocations within the pool will report the capacity for the > allocation. This is useful for certain backends where computing > the actual allocation of a volume might be an expensive > operation. > > Signed-off-by: Jason Dillaman <dillaman(a)redhat.com&gt; > --- > docs/formatstorage.html.in | 15 +++++++++--- > docs/schemas/storagecommon.rng | 7 ++++++ > docs/schemas/storagepool.rng | 5 ++++ > src/conf/storage_conf.c | 23 +++++++++++++++++++ > src/conf/storage_conf.h | 9 ++++++++ > .../pool-rbd-refresh-volume-allocation.xml | 12 ++++++++++ > .../pool-rbd-refresh-volume-allocation.xml | 15 ++++++++++++ > tests/storagepoolxml2xmltest.c | 1 + > 8 files changed, 84 insertions(+), 3 deletions(-) > create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-refresh-volume-allocation.xml > create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-refresh-volume-allocation.xml > > diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in > index 968651330f..d40e463167 100644 > --- a/docs/formatstorage.html.in > +++ b/docs/formatstorage.html.in > @@ -16,7 +16,7 @@ > </p> > <p> > The top level tag for a storage pool document is 'pool'. It has > - a single attribute <code>type</code>, which is one of <code>dir</code>, > + an attribute <code>type</code>, which is one of <code>dir</code>, > <code>fs</code>, <code>netfs</code>, <code>disk</code>, > <code>iscsi</code>, <code>logical</code>, <code>scsi</code> > (all <span class="since">since 0.4.1</span>), > @@ -27,8 +27,17 @@ > <code>zfs</code> (<span class="since">since 1.2.8</span>), > <code>vstorage</code> (<span class="since">since 3.1.0</span>), > or <code>iscsi-direct</code> (<span class="since">since 4.7.0</span>). > - This corresponds to the > - storage backend drivers listed further along in this document. > + This corresponds to the storage backend drivers listed further along in > + this document. > + </p> > + <p> > + The 'pool' element may also have an optional > + <code>refresh_volume_allocation</code> attribute to control how > + volume allocation is computed during a refresh. Valid values > + are <code>default</code> to compute the actual usage or > + <code>capacity</code> to default the allocation to the logical > + capacity for cases where computing the allocation is too expensive. > + <span class="since">Since 5.1.1</span> Since 5.2.0 ;-) We change minor for every release, and micro is for asynchronous releases, where we backport some patches (not that we've ever done that, except maybe for v3.2.1). Anyway, I don't like the name of this attribute. In my book, if an attribute has to consist of two or more words that calls for having an element for each of the words but the last which then can be as an attribute. So how about: <pool> <refresh method='default|capacity'/> ... </pool>

This is also more future proof IMO as it allows us to expand <refresh/> should we ever need that. The coode looks good otherwise. Michal