On Fri, Jun 29, 2018 at 09:02:17 +0200, Michal Privoznik wrote:
> On 06/29/2018 05:46 AM, dujiancheng wrote:
> > The patch add support L2 table cache for qcow2 disk.
> > L2 table cache can improve IO read and write performance for qcow2 img.
> > Example: random 4K read requests on a fully populated 100GB image (SSD backend and vm with directsync cacha mode),
> > IOPS increased by 7 times.
> >
>
> This commit message has some very long lines and does not explain the
> new element you are introducing. Also, you are missing Signed-off-by
> line which is required.
>
> > ---
> > src/conf/domain_conf.c | 25 +++++++++++++++++++++++++
> > src/conf/domain_conf.h | 4 ++++
> > src/qemu/qemu_command.c | 7 +++++++
> > src/qemu/qemu_domain.c | 4 ++++
> > 4 files changed, 40 insertions(+)
>
> Any new element that is introduced must go hand in hand with RNG schema
> adjustment, documentation and a test case. I believe virsh was
> complaining when you edited your domain and added <diskCache/>:
>
> virsh # edit fedora
> error: Reconnected to the hypervisor
> error: XML document failed to validate against schema: Unable to validate doc against //docs/schemas/domain.rng
> Extra element devices in interleave
> Element domain failed to validate content
>
> Failed. Try again? [y,n,i,f,?]:
>
>
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index b8b5345..cb9fb05 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -9502,6 +9502,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> > char *vendor = NULL;
> > char *product = NULL;
> > char *domain_name = NULL;
> > + char *disk_l2_cache_size = NULL;
> > + char *disk_cache_clean_interval = NULL;
> >
> > if (!(def = virDomainDiskDefNew(xmlopt)))
> > return NULL;
> > @@ -9701,6 +9703,27 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
> > }
> > } else if (virXMLNodeNameEqual(cur, "boot")) {
> > /* boot is parsed as part of virDomainDeviceInfoParseXML */
> > + } else if (virXMLNodeNameEqual(cur, "diskCache")) {
> > + disk_l2_cache_size =
> > + virXMLPropString(cur, "disk_l2_cache_size");
>
> This is rather unfortunate name for attribute. Also, is there a reason
> you have not put the assignment onto one line?
>
> > + if (disk_l2_cache_size &&
> > + virStrToLong_ui(disk_l2_cache_size, NULL, 0,
> > + &def->disk_cache.disk_l2_cache_size) < 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("invalid disk L2 cache size '%s'"),
> > + disk_l2_cache_size);
> > + goto error;
> > + }
> > + disk_cache_clean_interval =
> > + virXMLPropString(cur, "disk_cache_clean_interval");
> > + if (disk_cache_clean_interval &&
> > + virStrToLong_ui(disk_cache_clean_interval, NULL, 0,
> > + &def->disk_cache.disk_cache_clean_interval) < 0) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("invalid disk cache clean interval '%s'"),
> > + disk_cache_clean_interval);
> > + goto error;
> > + }
> > }
> > }
>
> So what this adds is:
>
> <disk type='file' device='disk'>
> <driver name='qemu' type='qcow2' discard='unmap'/>
> <source file='/var/lib/libvirt/images/fedora.qcow2'/>
>
> <diskCache disk_l2_cache_size='128' disk_cache_clean_interval='8'/>
> <!-- This is the new element ^^ -->
>
> <target dev='sda' bus='scsi'/>
> <boot order='1'/>
> <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> </disk>
>
> Well, from my example, what units are "128" and "8" in? Also, in libvirt
> we give users possibility to specify other units than KiB (even though
> we report back size in KiB), for instance look at <memory/> numa
> <cell/>. Also, your proposal is not that future proof. My suggestion is:
>
> <cache>
> <cache level='2'>
> <size unit='KiB'>128</size>
> </cache>
> <clean interval='8'/>
> </cache>
>
> But I'm sure there's even better approach.
There were at least two attempts to implement this in the past which
we've rejected as the configuration of this is more of a black magic
than anything which users could do and there was no solid documentation
how to tackle it or make it useful for higher level management apps.
IIRC John provided a link to the latest discussion which also had
patches. Those were much more complete and documented than this and had
better naming of those.
It may be worth reopening the discussion whether to include this but I'd
certainly go with one of the older versions.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
