| Message ID | E1mT2Js-0000DW-OH@lizzy.crudebyte.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 9pfs: fix wrong I/O block size in Rgetattr | expand |
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; >
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;
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; > >
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;
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;
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(-)