Re: [PATCH] qemu: virtiofs: Add pool element to limit thread pool

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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);
> > 




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux