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

[Christian Schoenebeck]

On Donnerstag, 23. September 2021 10:40:58 CEST Greg Kurz wrote:
> On Wed, 22 Sep 2021 17:55:02 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > 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/2c3e83f92d93fbab071b8a96b8ab769b0190247
> > 5/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.co
> > m/
> > 
> > And I wanted to keep things simple by only requiring one patch on QEMU
> > side
> > for now.
> 
> Fair enough and you're the maintainer anyway so this is your
> call. :-)
> 
> Subsequent cleanup would be to switch to QEMU_ALIGN_DOWN() like
> Philippe suggested but also to consolidate the logic in a common
> helper in order to avoid the code duplication.

It will include code deduplication as well, yes.

> The patch is correct and simple enough to be merged as is :
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>

Queued on 9p.next:
https://github.com/cschoenebeck/qemu/commits/9p.next

Thanks!

> > > > +    }
> > > > +    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;