On Fri, Oct 23, 2020 at 09:10:23AM -0300, Daniel Henrique Barboza wrote:
>
>
> On 10/20/20 2:54 PM, Masayoshi Mizuma wrote:
> > From: Masayoshi Mizuma <m.mizuma@xxxxxxxxxxxxxx>
> >
> > virtiofsd has --thread-pool-size=NUM option to limit the
> > thread pool size. It will be useful to tune the performance.
> >
> > Add pool element to use the option like as:
> >
> > <filesystem type='mount' accessmode='passthrough'>
> > <driver type='virtiofs' queue='1024'/>
> > <binary path='/usr/libexec/virtiofsd' xattr='on' pool='32'/>
> > <source dir='/path'/>
> > <target dir='mount_tag'/>
> > </filesystem>
> >
> > Signed-off-by: Masayoshi Mizuma <m.mizuma@xxxxxxxxxxxxxx>
> > ---
>
> Code looks good. We generally split the docs from the parser/qemu logic
> in the commits though. For new options it's also good to have unit tests
> covering them as well.
>
> I suggest you split this patch in at least 2 commits. First commit contains
> the changes in the /docs files, introducing the new 'pool' option. The
> second commit contains the changes in domain_conf and qemu_virtiofs that
> implements it.
>
> For unit tests, you'll want something in the lines of what is done in the
> case of 'vhost-user-fs-fd-memory' test in qemuxml2argvtest.c. You'll need
> a new XML file (e.g. vhost-user-fs-pool.xml) in qemuxml2argvdata that
> implements the 'pool' option, then a .args file that will contains the
> expected QEMU command line generated by that XML. You can see how it is being
> done for 'vhost-user-fs-fd-memory' and go from there. All the extra files
> and changes for this new test goes in another patch.
>
> Last but not the least, if you want to make the maintainers life easier, you
> can also add an extra patch documenting this new feature in NEWS.rst.
Thank you for the helpful advice!
I'll split the patch to the followings:
1. docs/formatdomain.rst and docs/schemas/domaincommon.rng
2. src/conf/domain_conf.[ch] and src/qemu/qemu_virtiofs.c
3. The unit test
4. NEWS.rst
Thanks!
Masa
>
>
>
> Thanks,
>
>
> DHB
>
>
>
>
> > docs/formatdomain.rst | 6 ++++--
> > docs/schemas/domaincommon.rng | 5 +++++
> > src/conf/domain_conf.c | 12 ++++++++++++
> > src/conf/domain_conf.h | 1 +
> > src/qemu/qemu_virtiofs.c | 3 +++
> > 5 files changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > index bfa80e4bc2..0f66af5dc3 100644
> > --- a/docs/formatdomain.rst
> > +++ b/docs/formatdomain.rst
> > @@ -3070,7 +3070,7 @@ A directory on the host that can be accessed directly from the guest.
> > </filesystem>
> > <filesystem type='mount' accessmode='passthrough'>
> > <driver type='virtiofs' queue='1024'/>
> > - <binary path='/usr/libexec/virtiofsd' xattr='on'>
> > + <binary path='/usr/libexec/virtiofsd' xattr='on' pool='32'>
> > <cache mode='always'/>
> > <lock posix='on' flock='on'/>
> > </binary>
> > @@ -3188,7 +3188,9 @@ A directory on the host that can be accessed directly from the guest.
> > the use of filesystem extended attributes. Caching can be tuned via the
> > ``cache`` element, possible ``mode`` values being ``none`` and ``always``.
> > Locking can be controlled via the ``lock`` element - attributes ``posix`` and
> > - ``flock`` both accepting values ``on`` or ``off``. ( :since:`Since 6.2.0` )
> > + ``flock`` both accepting values ``on`` or ``off``. ( :since:`Since 6.2.0` ).
> > + Thread pool size limit can be tuned via the ``pool`` element.
> > + ( :since:`Since 6.9.0` )
> > ``source``
> > The resource on the host that is being accessed in the guest. The ``name``
> > attribute must be used with ``type='template'``, and the ``dir`` attribute
> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> > index c25a742581..cd5de2ba80 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -2839,6 +2839,11 @@
> > <ref name="virOnOff"/>
> > </attribute>
> > </optional>
> > + <optional>
> > + <attribute name="pool">
> > + <ref name="unsignedInt"/>
> > + </attribute>
> > + </optional>
> > <optional>
> > <element name="cache">
> > <optional>
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index efa5ac527b..9d06f8c75f 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -11588,6 +11588,7 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
> > g_autofree char *cache = virXPathString("string(./binary/cache/@mode)", ctxt);
> > g_autofree char *posix_lock = virXPathString("string(./binary/lock/@posix)", ctxt);
> > g_autofree char *flock = virXPathString("string(./binary/lock/@flock)", ctxt);
> > + g_autofree char *thread_pool_size = virXPathString("string(./binary/@pool)", ctxt);
> > int val;
> > if (queue_size && virStrToLong_ull(queue_size, NULL, 10, &def->queue_size) < 0) {
> > @@ -11597,6 +11598,14 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
> > goto error;
> > }
> > + if (thread_pool_size &&
> > + virStrToLong_ull(thread_pool_size, NULL, 10, &def->thread_pool_size) < 0) {
> > + virReportError(VIR_ERR_XML_ERROR,
> > + _("cannot parse thread pool size '%s' for virtiofs"),
> > + thread_pool_size);
> > + goto error;
> > + }
> > +
> > if (binary)
> > def->binary = virFileSanitizePath(binary);
> > @@ -26191,6 +26200,9 @@ virDomainFSDefFormat(virBufferPtr buf,
> > virTristateSwitchTypeToString(def->xattr));
> > }
> > + if (def->thread_pool_size)
> > + virBufferAsprintf(&binaryAttrBuf, " pool='%llu'", def->thread_pool_size);
> > +
> > if (def->cache != VIR_DOMAIN_FS_CACHE_MODE_DEFAULT) {
> > virBufferAsprintf(&binaryBuf, "<cache mode='%s'/>\n",
> > virDomainFSCacheModeTypeToString(def->cache));
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index cd344716a3..98ab0a51c7 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -860,6 +860,7 @@ struct _virDomainFSDef {
> > bool symlinksResolved;
> > char *binary;
> > unsigned long long queue_size;
> > + unsigned long long thread_pool_size;
> > virTristateSwitch xattr;
> > virDomainFSCacheMode cache;
> > virTristateSwitch posix_lock;
> > diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
> > index 2e239cad66..e35c278a1b 100644
> > --- a/src/qemu/qemu_virtiofs.c
> > +++ b/src/qemu/qemu_virtiofs.c
> > @@ -126,6 +126,9 @@ qemuVirtioFSBuildCommandLine(virQEMUDriverConfigPtr cfg,
> > virCommandPassFD(cmd, *fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
> > *fd = -1;
> > + if (fs->thread_pool_size)
> > + virCommandAddArgFormat(cmd, "--thread-pool-size=%llu", fs->thread_pool_size);
> > +
> > virCommandAddArg(cmd, "-o");
> > virBufferAddLit(&opts, "source=");
> > virQEMUBuildBufferEscapeComma(&opts, fs->src->path);
> >
