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

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

[Christian Schoenebeck]

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

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

[Philippe Mathieu-Daudé]

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);

?

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

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;

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

[Greg Kurz]

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/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.
> 

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.

The patch is correct and simple enough to be merged as is :

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

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

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;