Re: [PATCH] 9pfs: fix wrong I/O block size in Rgetattr

[Christian Schoenebeck]

On Mittwoch, 22. September 2021 17:42:08 CEST Philippe Mathieu-Daudé wrote:
> On 9/22/21 15:13, Christian Schoenebeck wrote:
> > When client sent a 9p Tgetattr request then the wrong I/O block
> > size value was returned by 9p server; instead of host file
> > system's I/O block size it should rather return an I/O block
> > size according to 9p session's 'msize' value, because the value
> > returned to client should be an "optimum" block size for I/O
> > (i.e. to maximize performance), it should not reflect the actual
> > physical block size of the underlying storage media.
> > 
> > The I/O block size of a host filesystem is typically 4k, so the
> > value returned was far too low for good 9p I/O performance.
> > 
> > This patch adds stat_to_iounit() with a similar approach as the
> > existing get_iounit() function.
> > 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >   hw/9pfs/9p.c | 21 ++++++++++++++++++++-
> >   1 file changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index c857b31321..708b030474 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1262,6 +1262,25 @@ static int coroutine_fn stat_to_v9stat(V9fsPDU
> > *pdu, V9fsPath *path,> 
> >   #define P9_STATS_ALL           0x00003fffULL /* Mask for All fields
> >   above */> 
> > +static int32_t stat_to_iounit(const V9fsPDU *pdu, const struct stat
> > *stbuf) +{
> > +    int32_t iounit = 0;
> > +    V9fsState *s = pdu->s;
> > +
> > +    /*
> > +     * iounit should be multiples of st_blksize (host filesystem block
> > size) +     * as well as less than (client msize - P9_IOHDRSZ)
> > +     */
> > +    if (stbuf->st_blksize) {
> > +        iounit = stbuf->st_blksize;
> > +        iounit *= (s->msize - P9_IOHDRSZ) / stbuf->st_blksize;
> 
> Is that:
> 
>    iounit = QEMU_ALIGN_DOWN(s->msize - P9_IOHDRSZ, stbuf->st_blksize);
> 
> ?
> 

Yes it is, thanks for the hint! :)

I actually just took the equivalent, already existing code from get_iounit():
https://github.com/qemu/qemu/blob/2c3e83f92d93fbab071b8a96b8ab769b01902475/hw/9pfs/9p.c#L1880

Would it be OK to do that subsequently with cleanup patches? My plan was to
first address this with one patch, and addressing the cleanup issues
separately later on, because this patch is required for testing the following
kernel patches:
https://lore.kernel.org/netdev/cover.1632156835.git.linux_oss@crudebyte.com/

And I wanted to keep things simple by only requiring one patch on QEMU side
for now.


> > +    }
> > +    if (!iounit) {
> > +        iounit = s->msize - P9_IOHDRSZ;
> > +    }
> > +    return iounit;
> > +}
> > +
> > 
> >   static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
> >   
> >                                   V9fsStatDotl *v9lstat)
> >   
> >   {
> > 
> > @@ -1273,7 +1292,7 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const
> > struct stat *stbuf,> 
> >       v9lstat->st_gid = stbuf->st_gid;
> >       v9lstat->st_rdev = stbuf->st_rdev;
> >       v9lstat->st_size = stbuf->st_size;
> > 
> > -    v9lstat->st_blksize = stbuf->st_blksize;
> > +    v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
> > 
> >       v9lstat->st_blocks = stbuf->st_blocks;
> >       v9lstat->st_atime_sec = stbuf->st_atime;
> >       v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;