Re: [PATCH] qemu: virtiofs: Add pool element to limit thread pool
On Mon, Oct 26, 2020 at 09:50:22AM +0100, Peter Krempa wrote: > On Tue, Oct 20, 2020 at 13:54:33 -0400, Masayoshi Mizuma wrote: > > From: Masayoshi Mizuma > > > > virtiofsd has --thread-pool-size=NUM option to limit the > > thread pool size. It will be useful to tune the performance. > > Usually it's great to document or at least link to a document which > outlines how changing the parameter influences performance. In many > cases theres just one good option and that should become default. In > other cases it depends on deployment and in such case it's very good to > prevent users from having to guess what it actually does. > > In the end it serves as a justification that adding a tuning knob is > worth it. > > > Add pool element to use the option like as: > > > > > > > > > > I'd prefer if the attribute is named 'threads' as that seems to me to be > more clear what it does without having to refer back to the > documentation. Got it, thanks. I'll change the parameter to 'threads'. Thanks! Masa
Re: [PATCH] qemu: virtiofs: Add pool element to limit thread pool
On Tue, Oct 20, 2020 at 13:54:33 -0400, Masayoshi Mizuma wrote: > From: Masayoshi Mizuma > > virtiofsd has --thread-pool-size=NUM option to limit the > thread pool size. It will be useful to tune the performance. Usually it's great to document or at least link to a document which outlines how changing the parameter influences performance. In many cases theres just one good option and that should become default. In other cases it depends on deployment and in such case it's very good to prevent users from having to guess what it actually does. In the end it serves as a justification that adding a tuning knob is worth it. > Add pool element to use the option like as: > > > > I'd prefer if the attribute is named 'threads' as that seems to me to be more clear what it does without having to refer back to the documentation. > > > > > Signed-off-by: Masayoshi Mizuma > ---
Re: [PATCH] qemu: virtiofs: Add pool element to limit thread pool
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
> >
> > 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:
> >
> >
> >
> >
> >
> >
> >
> >
> > Signed-off-by: Masayoshi Mizuma
> > ---
>
> 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.
> >
> >
> >
> > -
> > +
> >
> >
> >
> > @@ -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 @@
> >
> >
> >
> > +
> > +
> > +
> > +
> > +
> >
> >
> >
> > 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,
> > >queue_size) < 0) {
> > @@ -11597,6 +11598,14 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr
> > xmlopt,
> > goto error;
> > }
> > +if (thread_pool_size &&
> > +virStrToLong_ull(thread_pool_size, NULL, 10,
> > >thread_pool_size) < 0) {
> > +virReportError(VIR_ERR_XML_ERROR,
> > + _("cannot parse thread pool size '%s' for
> > virtiofs"),
> > + thread_pool_size);
> > +
Re: [PATCH] qemu: virtiofs: Add pool element to limit thread pool
On 10/20/20 2:54 PM, Masayoshi Mizuma wrote:
From: Masayoshi Mizuma
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:
Signed-off-by: Masayoshi Mizuma
---
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.
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.
-
+
@@ -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 @@
+
+
+
+
+
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, >queue_size) < 0) {
@@ -11597,6 +11598,14 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
goto error;
}
+if (thread_pool_size &&
+virStrToLong_ull(thread_pool_size, NULL, 10, >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(, " pool='%llu'",
def->thread_pool_size);
+
if (def->cache != VIR_DOMAIN_FS_CACHE_MODE_DEFAULT) {
virBufferAsprintf(, "\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
