[PATCH v2 1/1] 9pfs: log warning if msize <= 8192

[PATCH v2 1/1] 9pfs: log warning if msize <= 8192

[Christian Schoenebeck]

It is essential to choose a reasonable high value for 'msize' to avoid
severely degraded file I/O performance. This parameter can only be
chosen on client/guest side, and a Linux client defaults to an 'msize'
of only 8192 if the user did not explicitly specify a value for 'msize',
which results in very poor file I/O performance.

Unfortunately many users are not aware that they should specify an
appropriate value for 'msize' to avoid severe performance issues, so
log a performance warning (with a QEMU wiki link explaining this issue
in detail) on host side in that case to make it more clear.

Currently a client cannot automatically pick a reasonable value for
'msize', because a good value for 'msize' depends on the file I/O
potential of the underlying storage on host side, i.e. a feature
invisible to the client, and even then a user would still need to trade
off between performance profit and additional RAM costs, i.e. with
growing 'msize' (RAM occupation), performance still increases, but
performance delta will shrink continuously.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 7bb994bbf2..99b6f24fd6 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1353,6 +1353,15 @@ static void coroutine_fn v9fs_version(void *opaque)
         goto out;
     }
 
+    /* 8192 is the default msize of Linux clients */
+    if (s->msize <= 8192) {
+        warn_report_once(
+            "9p: degraded performance: a reasonable high msize should be "
+            "chosen on client/guest side (chosen msize is <= 8192). See "
+            "https://wiki.qemu.org/Documentation/9psetup#msize for details."
+        );
+    }
+
 marshal:
     err = pdu_marshal(pdu, offset, "ds", s->msize, &version);
     if (err < 0) {
-- 
2.20.1

[PATCH v2 0/1] 9pfs: log warning if msize <= 8192

[Christian Schoenebeck]

I have updated the QEMU 9P setup wiki page about this 'msize' issue. For
some reason the dedicated anchor 'msize' does not work though:

https://wiki.qemu.org/Documentation/9psetup#msize

Not sure whether that's a wiki installation problem? When I view the wiki
source, it looks like it is showing some errors there.

v1->v2:

  * Updated commit log message to make it more clear why the client cannot
    auto pick a good value for 'msize'.

  * Added a web link to the log message, pointing to the appropriate QEMU
    wiki page which explains the 'msize' issue in detail.

Message-ID of previous version (v1):
  E1kDR8W-0001s4-Sr@lizzy.crudebyte.com

Christian Schoenebeck (1):
  9pfs: log warning if msize <= 8192

 hw/9pfs/9p.c | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.20.1

Re: [PATCH v2 0/1] 9pfs: log warning if msize <= 8192

[Greg Kurz]

On Thu, 3 Sep 2020 16:41:02 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> I have updated the QEMU 9P setup wiki page about this 'msize' issue. For
> some reason the dedicated anchor 'msize' does not work though:
> 
> https://wiki.qemu.org/Documentation/9psetup#msize
> 

AFAICT the wiki derives the anchor from the section name, ie.

https://wiki.qemu.org/Documentation/9psetup#Performance_Considerations

It's a bit longer than #msize but it works. I don't know if you can
add anchors manually in the wiki.

> Not sure whether that's a wiki installation problem? When I view the wiki
> source, it looks like it is showing some errors there.
> 
> v1->v2:
> 
>   * Updated commit log message to make it more clear why the client cannot
>     auto pick a good value for 'msize'.
> 
>   * Added a web link to the log message, pointing to the appropriate QEMU
>     wiki page which explains the 'msize' issue in detail.
> 
> Message-ID of previous version (v1):
>   E1kDR8W-0001s4-Sr@lizzy.crudebyte.com
> 
> Christian Schoenebeck (1):
>   9pfs: log warning if msize <= 8192
> 
>  hw/9pfs/9p.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 

Re: [PATCH v2 0/1] 9pfs: log warning if msize <= 8192

[Greg Kurz]

On Thu, 3 Sep 2020 17:19:31 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Thu, 3 Sep 2020 16:41:02 +0200
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> 
> > I have updated the QEMU 9P setup wiki page about this 'msize' issue. For
> > some reason the dedicated anchor 'msize' does not work though:
> > 
> > https://wiki.qemu.org/Documentation/9psetup#msize
> > 
> 
> AFAICT the wiki derives the anchor from the section name, ie.
> 
> https://wiki.qemu.org/Documentation/9psetup#Performance_Considerations
> 
> It's a bit longer than #msize but it works. I don't know if you can
> add anchors manually in the wiki.
> 

It seems you could achieve this without the template:

== <span id="msize">Performance Considerations</span> ==

> > Not sure whether that's a wiki installation problem? When I view the wiki
> > source, it looks like it is showing some errors there.
> > 
> > v1->v2:
> > 
> >   * Updated commit log message to make it more clear why the client cannot
> >     auto pick a good value for 'msize'.
> > 
> >   * Added a web link to the log message, pointing to the appropriate QEMU
> >     wiki page which explains the 'msize' issue in detail.
> > 
> > Message-ID of previous version (v1):
> >   E1kDR8W-0001s4-Sr@lizzy.crudebyte.com
> > 
> > Christian Schoenebeck (1):
> >   9pfs: log warning if msize <= 8192
> > 
> >  hw/9pfs/9p.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> 

Re: [PATCH v2 0/1] 9pfs: log warning if msize <= 8192

[Christian Schoenebeck]

On Donnerstag, 3. September 2020 17:41:23 CEST Greg Kurz wrote:
> On Thu, 3 Sep 2020 17:19:31 +0200
> 
> Greg Kurz <groug@kaod.org> wrote:
> > On Thu, 3 Sep 2020 16:41:02 +0200
> > 
> > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > I have updated the QEMU 9P setup wiki page about this 'msize' issue. For
> > > some reason the dedicated anchor 'msize' does not work though:
> > > 
> > > https://wiki.qemu.org/Documentation/9psetup#msize
> > 
> > AFAICT the wiki derives the anchor from the section name, ie.
> > 
> > https://wiki.qemu.org/Documentation/9psetup#Performance_Considerations
> > 
> > It's a bit longer than #msize but it works. I don't know if you can
> > add anchors manually in the wiki.
> 
> It seems you could achieve this without the template:
> 
> == <span id="msize">Performance Considerations</span> ==

What I tried was this (as wiki source):

<!-- NOTE: anchor 'msize' is linked by a QEMU 9pfs log message in 9p.c  -->
{{anchor|msize}}
== Performance Considerations ==

Which "should" work according to:
https://en.wikipedia.org/wiki/Template:Anchor

However after I did those changes I saw some template errors as comment in the 
generated HTML sources, which now are gone at least.

I wait a bit to see if it is maybe just a caching problem. If it still doesn't 
work in a while, I will update it with your solution a bit later.

Thanks for the hint!

Best regards,
Christian Schoenebeck

Re: [PATCH v2 1/1] 9pfs: log warning if msize <= 8192

[Greg Kurz]

On Thu, 3 Sep 2020 16:20:21 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> It is essential to choose a reasonable high value for 'msize' to avoid
> severely degraded file I/O performance. This parameter can only be
> chosen on client/guest side, and a Linux client defaults to an 'msize'
> of only 8192 if the user did not explicitly specify a value for 'msize',
> which results in very poor file I/O performance.
> 
> Unfortunately many users are not aware that they should specify an
> appropriate value for 'msize' to avoid severe performance issues, so
> log a performance warning (with a QEMU wiki link explaining this issue
> in detail) on host side in that case to make it more clear.
> 
> Currently a client cannot automatically pick a reasonable value for
> 'msize', because a good value for 'msize' depends on the file I/O
> potential of the underlying storage on host side, i.e. a feature
> invisible to the client, and even then a user would still need to trade
> off between performance profit and additional RAM costs, i.e. with
> growing 'msize' (RAM occupation), performance still increases, but
> performance delta will shrink continuously.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 7bb994bbf2..99b6f24fd6 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1353,6 +1353,15 @@ static void coroutine_fn v9fs_version(void *opaque)
>          goto out;
>      }
>  
> +    /* 8192 is the default msize of Linux clients */
> +    if (s->msize <= 8192) {

I agree that an msize of 8192 suck from a performance standpoint but
I guess many users are msize agnostic, and they use the default value
without even knowing it. They might not even care for performance at
all, otherwise they'd likely ask google and they will eventually land
on:

https://wiki.qemu.org/Documentation/9psetup#Performance_Considerations

But with this patch, they will now get a warning each time they start
QEMU, maybe freak out and file reports in launchpad. So I suggest you
turn <= into < to avoid bothering these placid users with a warning.

Anyway, it's your choice :) so if you manage to get the #msize anchor to
work as expected:

Reviewed-by: Greg Kurz <groug@kaod.org>

> +        warn_report_once(
> +            "9p: degraded performance: a reasonable high msize should be "
> +            "chosen on client/guest side (chosen msize is <= 8192). See "
> +            "https://wiki.qemu.org/Documentation/9psetup#msize for details."
> +        );
> +    }
> +
>  marshal:
>      err = pdu_marshal(pdu, offset, "ds", s->msize, &version);
>      if (err < 0) {

Re: [PATCH v2 0/1] 9pfs: log warning if msize <= 8192

[Greg Kurz]

On Thu, 03 Sep 2020 17:48:33 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Donnerstag, 3. September 2020 17:41:23 CEST Greg Kurz wrote:
> > On Thu, 3 Sep 2020 17:19:31 +0200
> > 
> > Greg Kurz <groug@kaod.org> wrote:
> > > On Thu, 3 Sep 2020 16:41:02 +0200
> > > 
> > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > > I have updated the QEMU 9P setup wiki page about this 'msize' issue. For
> > > > some reason the dedicated anchor 'msize' does not work though:
> > > > 
> > > > https://wiki.qemu.org/Documentation/9psetup#msize
> > > 
> > > AFAICT the wiki derives the anchor from the section name, ie.
> > > 
> > > https://wiki.qemu.org/Documentation/9psetup#Performance_Considerations
> > > 
> > > It's a bit longer than #msize but it works. I don't know if you can
> > > add anchors manually in the wiki.
> > 
> > It seems you could achieve this without the template:
> > 
> > == <span id="msize">Performance Considerations</span> ==
> 
> What I tried was this (as wiki source):
> 
> <!-- NOTE: anchor 'msize' is linked by a QEMU 9pfs log message in 9p.c  -->
> {{anchor|msize}}
> == Performance Considerations ==
> 
> Which "should" work according to:
> https://en.wikipedia.org/wiki/Template:Anchor
> 
> However after I did those changes I saw some template errors as comment in the 
> generated HTML sources, which now are gone at least.
> 
> I wait a bit to see if it is maybe just a caching problem. If it still doesn't 
> work in a while, I will update it with your solution a bit later.
> 
> Thanks for the hint!
> 

Found here:

https://meta.wikimedia.org/wiki/Help:Link#Manual_anchors

> Best regards,
> Christian Schoenebeck
> 
> 

Re: [PATCH v2 1/1] 9pfs: log warning if msize <= 8192

[Christian Schoenebeck]

On Donnerstag, 3. September 2020 18:02:48 CEST Greg Kurz wrote:
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 7bb994bbf2..99b6f24fd6 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1353,6 +1353,15 @@ static void coroutine_fn v9fs_version(void *opaque)
> > 
> >          goto out;
> >      
> >      }
> > 
> > +    /* 8192 is the default msize of Linux clients */
> > +    if (s->msize <= 8192) {
> 
> I agree that an msize of 8192 suck from a performance standpoint but
> I guess many users are msize agnostic, and they use the default value
> without even knowing it. They might not even care for performance at
> all, otherwise they'd likely ask google and they will eventually land
> on:
> 
> https://wiki.qemu.org/Documentation/9psetup#Performance_Considerations
> 
> But with this patch, they will now get a warning each time they start
> QEMU, maybe freak out and file reports in launchpad. So I suggest you
> turn <= into < to avoid bothering these placid users with a warning.

Mmm, that's actually precisely my intended target group: people who have never 
been aware about the existence of 'msize' before.

I keep '<=' for now, I think the log message is already clear that you simply 
have to make it any 'msize' > 8192 and the warning is gone.

> Anyway, it's your choice :) so if you manage to get the #msize anchor to
> work as expected:

Works now (using raw html anchor instead):
https://wiki.qemu.org/Documentation/9psetup#msize

> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks Greg!

> > +        warn_report_once(
> > +            "9p: degraded performance: a reasonable high msize should be
> > "
> > +            "chosen on client/guest side (chosen msize is <= 8192). See "
> > +            "https://wiki.qemu.org/Documentation/9psetup#msize for
> > details." +        );
> > +    }
> > +
> > 
> >  marshal:
> >      err = pdu_marshal(pdu, offset, "ds", s->msize, &version);
> >      if (err < 0) {

Best regards,
Christian Schoenebeck