Re: [libvirt] [PATCH v2 1/2] conf, docs: Add qcow2 cache configuration support

On 09/03/2017 10:39 PM, Liu Qing wrote: parameters OK - so it's in the commit message, but someone reading the docs isn't going to read the commit message to find any useful suggestions... What is the value listed in? Seems to me from the sample XML it's in bytes, but you didn't create a "sized" parameter so seeing 2097152 could cause someone to think that's an MB value because you indicate the default is 1MB instead of the byte equivalent. I see that the QEMU code would error on sizing restrictions, so I suppose we can avoid those tyeps of checks. Still just the wording here doesn't really convey enough meaning to allow someone to "guess" a good value to use. What's the basis of what someone should use to have a solid guess? Similar comments here w/r/t sizing and what's a good value. I feel we've recently gone down this path adding tx_queue_size values and not really having a good idea what to tell a user/consumer "how" to realistically use the values. s/defaut/default So again is there a good range or suggested values? The default is to never clean and this allows some type of cleanup. So what's the downside of too short or too long. Is a single second an eternity in a world that uses milliseconds or is it too short because of cache hits? It's almost too bad QEMU didn't implement something like "clean=aggressive" or "clean=passive" in order to choose between 1 or 2 algorithms managed by QEMU to perform the cleanup with "clean=never" being the default. It'll be at least 3.8.0 since 3.7.0 is was released yesterday. It would seem to me that a good caveat for each of these is don't mess with them unless you know what you're doing. But for those that need performance adjustments, then here's some suggestions and guidelines. You know - it almost seems as though this could/should a <driver> sub-element such as: <qcow2_attr l2_cache_size='#' refcount_cache_size='#' clean_interval='#'/qcow2_attr> (or just <qcow2>) Maybe others also have an opinion on this. Just seems "odd" to have attributes starting with "qcow2_" for <driver...> where it'd be required to also ensure that the attribute "type='qcow2'". I also see there are some other qcow2 specific attributes that may be useful now or even in the future. But in the end, they're specific to an implementation of the driver and aren't general attributes for a driver. These should all be virStrToLong_ullp since you're expecting a positive or unsigned value. Furthermore, 'int' or 'unsigned' (e.g. _ui) limits you a bit with byte values, you should follow what QEMU has. You may even want to consider allowing "sized" values (search for virDomainParseScaledValue for examples) so that someone could supply the value in something other than bytes. You still store in bytes, but visually someone can provide 1Mb rather than being required to do the math themselves. Of course that comes with even more XML parsing logic. Also once all the parsing is done we should modify virDomainDiskDefParseValidate in order to validate that these were only added for disk->src->format == VIR_STORAGE_FILE_QCOW2 and error if added to any other format. Again this could/would change if using a subelement as well as using "%llu" as the format string. Also these are some really long lines - consider someone using an 80 column wide terminal... Long lines here too... But this actually belongs in the next patch, not here as all you're doing in the first patch is the xml processing. With the Validation done the format check wouldn't be required either. QEMU defines these as as int64_t int64_t l2_cache_size; int64_t refcount_cache_size; int64_t cache_clean_interval; But uses them as uint64_t, so that's what we should do - define them as uint64_t 900 seconds! I dunno, seems like a long time to me! a/k/a 2 Mb a/k/a 512 Kb a/k/a 2 Mb a/k/a 512 Kb FWIW: You could have gone with just 1 test, but all 3 are fine since they show they can be used separately. In the long run, we're just trying to validate properly read and formatted values. Doing them separately could also imply that each parameter should be added in it's own separate patch... John