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