| Message ID | cover.1635340713.git.qemu_oss@crudebyte.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote: > The following changes since commit 931ce30859176f0f7daac6bac255dae5eb21284e: > > Merge remote-tracking branch 'remotes/dagrh/tags/pull-virtiofs-20211026' > into staging (2021-10-26 07:38:41 -0700) > > are available in the Git repository at: > > https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027 > > for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e: > > 9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200) > > ---------------------------------------------------------------- > 9pfs: performance fix and cleanup > > * First patch fixes suboptimal I/O performance on guest due to previously > incorrect block size being transmitted to 9p client. > > * Subsequent patches are cleanup ones intended to reduce code complexity. > > ---------------------------------------------------------------- > Christian Schoenebeck (8): > 9pfs: fix wrong I/O block size in Rgetattr > 9pfs: deduplicate iounit code > 9pfs: simplify blksize_to_iounit() > 9pfs: introduce P9Array > fsdev/p9array.h: check scalar type in P9ARRAY_NEW() > 9pfs: make V9fsString usable via P9Array API > 9pfs: make V9fsPath usable via P9Array API > 9pfs: use P9Array in v9fs_walk() > > fsdev/9p-marshal.c | 2 + > fsdev/9p-marshal.h | 3 + > fsdev/file-op-9p.h | 2 + > fsdev/p9array.h | 160 > +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c | > 70 +++++++++++++---------- > 5 files changed, 208 insertions(+), 29 deletions(-) > create mode 100644 fsdev/p9array.h Regarding last 5 patches: Daniel raised a concern that not using g_autoptr would deviate from current QEMU coding patterns: https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html Unfortunately I saw no way to address his concern without adding unnecessary code complexity, so I decided to make this a 9p local type (QArray -> P9Array) for now, which can easily be replaced in future (e.g. when there will be something appropriate on glib side). Best regards, Christian Schoenebeck
Hi Christian, On 10/27/21 16:05, Christian Schoenebeck wrote: > On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote: >> The following changes since commit 931ce30859176f0f7daac6bac255dae5eb21284e: >> >> Merge remote-tracking branch 'remotes/dagrh/tags/pull-virtiofs-20211026' >> into staging (2021-10-26 07:38:41 -0700) >> >> are available in the Git repository at: >> >> https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027 >> >> for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e: >> >> 9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200) >> >> ---------------------------------------------------------------- >> 9pfs: performance fix and cleanup >> >> * First patch fixes suboptimal I/O performance on guest due to previously >> incorrect block size being transmitted to 9p client. >> >> * Subsequent patches are cleanup ones intended to reduce code complexity. >> >> ---------------------------------------------------------------- >> Christian Schoenebeck (8): >> 9pfs: fix wrong I/O block size in Rgetattr >> 9pfs: deduplicate iounit code >> 9pfs: simplify blksize_to_iounit() >> 9pfs: introduce P9Array >> fsdev/p9array.h: check scalar type in P9ARRAY_NEW() >> 9pfs: make V9fsString usable via P9Array API >> 9pfs: make V9fsPath usable via P9Array API >> 9pfs: use P9Array in v9fs_walk() >> >> fsdev/9p-marshal.c | 2 + >> fsdev/9p-marshal.h | 3 + >> fsdev/file-op-9p.h | 2 + >> fsdev/p9array.h | 160 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c | >> 70 +++++++++++++---------- >> 5 files changed, 208 insertions(+), 29 deletions(-) >> create mode 100644 fsdev/p9array.h > > Regarding last 5 patches: Daniel raised a concern that not using g_autoptr > would deviate from current QEMU coding patterns: > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html > > Unfortunately I saw no way to address his concern without adding unnecessary > code complexity, so I decided to make this a 9p local type (QArray -> P9Array) > for now, which can easily be replaced in future (e.g. when there will be > something appropriate on glib side). Hmm various patches aren't reviewed yet... In particular patch #5 has a Suggested-by tag without Reviewed-by, this looks odd. See https://wiki.qemu.org/Contribute/SubmitAPullRequest: Don't send pull requests for code that hasn't passed review. A pull request says these patches are ready to go into QEMU now, so they must have passed the standard code review processes. In particular if you've corrected issues in one round of code review, you need to send your fixed patch series as normal to the list; you can't put it in a pull request until it's gone through. (Extremely trivial fixes may be OK to just fix in passing, but if in doubt err on the side of not.)
On Wed, 27 Oct 2021 16:05:39 +0200 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote: > > The following changes since commit 931ce30859176f0f7daac6bac255dae5eb21284e: > > > > Merge remote-tracking branch 'remotes/dagrh/tags/pull-virtiofs-20211026' > > into staging (2021-10-26 07:38:41 -0700) > > > > are available in the Git repository at: > > > > https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027 > > > > for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e: > > > > 9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200) > > > > ---------------------------------------------------------------- > > 9pfs: performance fix and cleanup > > > > * First patch fixes suboptimal I/O performance on guest due to previously > > incorrect block size being transmitted to 9p client. > > > > * Subsequent patches are cleanup ones intended to reduce code complexity. > > > > ---------------------------------------------------------------- > > Christian Schoenebeck (8): > > 9pfs: fix wrong I/O block size in Rgetattr > > 9pfs: deduplicate iounit code > > 9pfs: simplify blksize_to_iounit() > > 9pfs: introduce P9Array > > fsdev/p9array.h: check scalar type in P9ARRAY_NEW() > > 9pfs: make V9fsString usable via P9Array API > > 9pfs: make V9fsPath usable via P9Array API > > 9pfs: use P9Array in v9fs_walk() > > > > fsdev/9p-marshal.c | 2 + > > fsdev/9p-marshal.h | 3 + > > fsdev/file-op-9p.h | 2 + > > fsdev/p9array.h | 160 > > +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c | > > 70 +++++++++++++---------- > > 5 files changed, 208 insertions(+), 29 deletions(-) > > create mode 100644 fsdev/p9array.h > > Regarding last 5 patches: Daniel raised a concern that not using g_autoptr > would deviate from current QEMU coding patterns: > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html > > Unfortunately I saw no way to address his concern without adding unnecessary > code complexity, so I decided to make this a 9p local type (QArray -> P9Array) > for now, which can easily be replaced in future (e.g. when there will be > something appropriate on glib side). > Christian, Given that the P9Array is controversial and that the current benefit in patch 8 is relatively small, I'd suggest that you just drop patches 4 to 8 for now. Cheers, -- Greg > Best regards, > Christian Schoenebeck > >
On Mittwoch, 27. Oktober 2021 17:36:03 CEST Philippe Mathieu-Daudé wrote: > Hi Christian, > > On 10/27/21 16:05, Christian Schoenebeck wrote: > > On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote: > >> The following changes since commit 931ce30859176f0f7daac6bac255dae5eb21284e: > >> Merge remote-tracking branch > >> 'remotes/dagrh/tags/pull-virtiofs-20211026' > >> > >> into staging (2021-10-26 07:38:41 -0700) > >> > >> are available in the Git repository at: > >> https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027 > >> > >> for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e: > >> 9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200) > >> > >> ---------------------------------------------------------------- > >> 9pfs: performance fix and cleanup > >> > >> * First patch fixes suboptimal I/O performance on guest due to previously > >> > >> incorrect block size being transmitted to 9p client. > >> > >> * Subsequent patches are cleanup ones intended to reduce code complexity. > >> > >> ---------------------------------------------------------------- > >> > >> Christian Schoenebeck (8): > >> 9pfs: fix wrong I/O block size in Rgetattr > >> 9pfs: deduplicate iounit code > >> 9pfs: simplify blksize_to_iounit() > >> 9pfs: introduce P9Array > >> fsdev/p9array.h: check scalar type in P9ARRAY_NEW() > >> 9pfs: make V9fsString usable via P9Array API > >> 9pfs: make V9fsPath usable via P9Array API > >> 9pfs: use P9Array in v9fs_walk() > >> > >> fsdev/9p-marshal.c | 2 + > >> fsdev/9p-marshal.h | 3 + > >> fsdev/file-op-9p.h | 2 + > >> fsdev/p9array.h | 160 > >> > >> +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c > >> | > >> 70 +++++++++++++---------- > >> > >> 5 files changed, 208 insertions(+), 29 deletions(-) > >> create mode 100644 fsdev/p9array.h > > > > Regarding last 5 patches: Daniel raised a concern that not using g_autoptr > > would deviate from current QEMU coding patterns: > > https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html > > > > Unfortunately I saw no way to address his concern without adding > > unnecessary code complexity, so I decided to make this a 9p local type > > (QArray -> P9Array) for now, which can easily be replaced in future (e.g. > > when there will be something appropriate on glib side). > > Hmm various patches aren't reviewed yet... In particular > patch #5 has a Suggested-by tag without Reviewed-by, this > looks odd. > > See https://wiki.qemu.org/Contribute/SubmitAPullRequest: > > Don't send pull requests for code that hasn't passed review. > A pull request says these patches are ready to go into QEMU now, > so they must have passed the standard code review processes. In > particular if you've corrected issues in one round of code review, > you need to send your fixed patch series as normal to the list; > you can't put it in a pull request until it's gone through. > (Extremely trivial fixes may be OK to just fix in passing, but > if in doubt err on the side of not.) There are in general exactly two persons adding their RBs to 9p patches, which is either Greg or me, and Greg made it already clear that he barely has time for anything above trivial set. So what do you suggest? You want to participate and review 9p patches? Best regards, Christian Schoenebeck
On 10/27/21 18:21, Christian Schoenebeck wrote: > On Mittwoch, 27. Oktober 2021 17:36:03 CEST Philippe Mathieu-Daudé wrote: >> Hi Christian, >> >> On 10/27/21 16:05, Christian Schoenebeck wrote: >>> On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote: >>>> The following changes since commit > 931ce30859176f0f7daac6bac255dae5eb21284e: >>>> Merge remote-tracking branch >>>> 'remotes/dagrh/tags/pull-virtiofs-20211026' >>>> >>>> into staging (2021-10-26 07:38:41 -0700) >>>> >>>> are available in the Git repository at: >>>> https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027 >>>> >>>> for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e: >>>> 9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200) >>>> >>>> ---------------------------------------------------------------- >>>> 9pfs: performance fix and cleanup >>>> >>>> * First patch fixes suboptimal I/O performance on guest due to previously >>>> >>>> incorrect block size being transmitted to 9p client. >>>> >>>> * Subsequent patches are cleanup ones intended to reduce code complexity. >>>> >>>> ---------------------------------------------------------------- >>>> >>>> Christian Schoenebeck (8): >>>> 9pfs: fix wrong I/O block size in Rgetattr >>>> 9pfs: deduplicate iounit code >>>> 9pfs: simplify blksize_to_iounit() >>>> 9pfs: introduce P9Array >>>> fsdev/p9array.h: check scalar type in P9ARRAY_NEW() >>>> 9pfs: make V9fsString usable via P9Array API >>>> 9pfs: make V9fsPath usable via P9Array API >>>> 9pfs: use P9Array in v9fs_walk() >>>> >>>> fsdev/9p-marshal.c | 2 + >>>> fsdev/9p-marshal.h | 3 + >>>> fsdev/file-op-9p.h | 2 + >>>> fsdev/p9array.h | 160 >>>> >>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c >>>> | >>>> 70 +++++++++++++---------- >>>> >>>> 5 files changed, 208 insertions(+), 29 deletions(-) >>>> create mode 100644 fsdev/p9array.h >>> >>> Regarding last 5 patches: Daniel raised a concern that not using g_autoptr >>> would deviate from current QEMU coding patterns: >>> https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html >>> >>> Unfortunately I saw no way to address his concern without adding >>> unnecessary code complexity, so I decided to make this a 9p local type >>> (QArray -> P9Array) for now, which can easily be replaced in future (e.g. >>> when there will be something appropriate on glib side). >> >> Hmm various patches aren't reviewed yet... In particular >> patch #5 has a Suggested-by tag without Reviewed-by, this >> looks odd. >> >> See https://wiki.qemu.org/Contribute/SubmitAPullRequest: >> >> Don't send pull requests for code that hasn't passed review. >> A pull request says these patches are ready to go into QEMU now, >> so they must have passed the standard code review processes. In >> particular if you've corrected issues in one round of code review, >> you need to send your fixed patch series as normal to the list; >> you can't put it in a pull request until it's gone through. >> (Extremely trivial fixes may be OK to just fix in passing, but >> if in doubt err on the side of not.) > > There are in general exactly two persons adding their RBs to 9p patches, which > is either Greg or me, and Greg made it already clear that he barely has time > for anything above trivial set. > > So what do you suggest? You want to participate and review 9p patches? Well I am a bit surprised... $ git log --oneline \ --grep='Reviewed-by: Philippe Mathieu-Daudé' -- hw/9pfs/ | wc -l 18 I also reviewed patch #3 if this pull request... Now I see you posted this 4 times in 2 months, so indeed eventual reviewers had plenty of time to look at your patches. Note I haven't said I'd NAck your pull request, I noticed your own concern wrt Daniel comment, so I looked at the patch and realized it was not reviewed, and simply said this is this is odd. Regards, Phil.
On Mittwoch, 27. Oktober 2021 18:48:10 CEST Philippe Mathieu-Daudé wrote: > On 10/27/21 18:21, Christian Schoenebeck wrote: > > On Mittwoch, 27. Oktober 2021 17:36:03 CEST Philippe Mathieu-Daudé wrote: > >> Hi Christian, > >> > >> On 10/27/21 16:05, Christian Schoenebeck wrote: > >>> On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote: > >>>> The following changes since commit > > > > 931ce30859176f0f7daac6bac255dae5eb21284e: > >>>> Merge remote-tracking branch > >>>> 'remotes/dagrh/tags/pull-virtiofs-20211026' > >>>> > >>>> into staging (2021-10-26 07:38:41 -0700) > >>>> > >>>> are available in the Git repository at: > >>>> https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027 > >>>> > >>>> for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e: > >>>> 9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200) > >>>> > >>>> ---------------------------------------------------------------- > >>>> 9pfs: performance fix and cleanup > >>>> > >>>> * First patch fixes suboptimal I/O performance on guest due to > >>>> previously > >>>> > >>>> incorrect block size being transmitted to 9p client. > >>>> > >>>> * Subsequent patches are cleanup ones intended to reduce code > >>>> complexity. > >>>> > >>>> ---------------------------------------------------------------- > >>>> > >>>> Christian Schoenebeck (8): > >>>> 9pfs: fix wrong I/O block size in Rgetattr > >>>> 9pfs: deduplicate iounit code > >>>> 9pfs: simplify blksize_to_iounit() > >>>> 9pfs: introduce P9Array > >>>> fsdev/p9array.h: check scalar type in P9ARRAY_NEW() > >>>> 9pfs: make V9fsString usable via P9Array API > >>>> 9pfs: make V9fsPath usable via P9Array API > >>>> 9pfs: use P9Array in v9fs_walk() > >>>> > >>>> fsdev/9p-marshal.c | 2 + > >>>> fsdev/9p-marshal.h | 3 + > >>>> fsdev/file-op-9p.h | 2 + > >>>> fsdev/p9array.h | 160 > >>>> > >>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c > >>>> > >>>> 70 +++++++++++++---------- > >>>> > >>>> 5 files changed, 208 insertions(+), 29 deletions(-) > >>>> create mode 100644 fsdev/p9array.h > >>> > >>> Regarding last 5 patches: Daniel raised a concern that not using > >>> g_autoptr > >>> would deviate from current QEMU coding patterns: > >>> https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html > >>> > >>> Unfortunately I saw no way to address his concern without adding > >>> unnecessary code complexity, so I decided to make this a 9p local type > >>> (QArray -> P9Array) for now, which can easily be replaced in future > >>> (e.g. > >>> when there will be something appropriate on glib side). > >> > >> Hmm various patches aren't reviewed yet... In particular > >> patch #5 has a Suggested-by tag without Reviewed-by, this > >> looks odd. > >> > >> See https://wiki.qemu.org/Contribute/SubmitAPullRequest: > >> Don't send pull requests for code that hasn't passed review. > >> A pull request says these patches are ready to go into QEMU now, > >> so they must have passed the standard code review processes. In > >> particular if you've corrected issues in one round of code review, > >> you need to send your fixed patch series as normal to the list; > >> you can't put it in a pull request until it's gone through. > >> (Extremely trivial fixes may be OK to just fix in passing, but > >> if in doubt err on the side of not.) > > > > There are in general exactly two persons adding their RBs to 9p patches, > > which is either Greg or me, and Greg made it already clear that he barely > > has time for anything above trivial set. > > > > So what do you suggest? You want to participate and review 9p patches? > > Well I am a bit surprised... > > $ git log --oneline \ > --grep='Reviewed-by: Philippe Mathieu-Daudé' -- hw/9pfs/ | wc -l > 18 > > I also reviewed patch #3 if this pull request... > > > Now I see you posted this 4 times in 2 months, so indeed eventual > reviewers had plenty of time to look at your patches. > > Note I haven't said I'd NAck your pull request, I noticed your own > concern wrt Daniel comment, so I looked at the patch and realized > it was not reviewed, and simply said this is this is odd. > > Regards, > > Phil. Philippe, of course I understand why this looks odd to you, and yes you reviewed that particular patch. But the situation on the 9p front is like this for >2 years now: people quickly come by to nack patches, but even after having addressed their concerns they barely add their RBs afterwards. That means I am currently forced to send out PRs without RBs once in a while. The mentioned 5 patches look like overkill on first sight, but they are just intended as preparatory ones. I actually should fix a protocol implementation deficit in "Twalk" request handling, and that in turn means I will have to add complexity to function v9fs_walk(). But before even daring to do so, I should get rid of as much of complexity as possible. Because we already had a bunch of issues in that function before. I believe these are trivial 5 patches. But I can also accompany them with test cases if somebody is worried. Greg, I could also drop them now, but the general issue will retain: Reality is that I am the only person working on 9p right now and a fact that I cannot change. The rest is for other people to decide. Best regards, Christian Schoenebeck
Hi Richard, On 10/27/21 19:29, Christian Schoenebeck wrote: > On Mittwoch, 27. Oktober 2021 18:48:10 CEST Philippe Mathieu-Daudé wrote: >> On 10/27/21 18:21, Christian Schoenebeck wrote: >>> On Mittwoch, 27. Oktober 2021 17:36:03 CEST Philippe Mathieu-Daudé wrote: >>>> On 10/27/21 16:05, Christian Schoenebeck wrote: >>>>> Regarding last 5 patches: Daniel raised a concern that not using >>>>> g_autoptr >>>>> would deviate from current QEMU coding patterns: >>>>> https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html >>>>> >>>>> Unfortunately I saw no way to address his concern without adding >>>>> unnecessary code complexity, so I decided to make this a 9p local type >>>>> (QArray -> P9Array) for now, which can easily be replaced in future >>>>> (e.g. >>>>> when there will be something appropriate on glib side). >>>> >>>> Hmm various patches aren't reviewed yet... In particular >>>> patch #5 has a Suggested-by tag without Reviewed-by, this >>>> looks odd. >>>> >>>> See https://wiki.qemu.org/Contribute/SubmitAPullRequest: >>>> Don't send pull requests for code that hasn't passed review. >>>> A pull request says these patches are ready to go into QEMU now, >>>> so they must have passed the standard code review processes. In >>>> particular if you've corrected issues in one round of code review, >>>> you need to send your fixed patch series as normal to the list; >>>> you can't put it in a pull request until it's gone through. >>>> (Extremely trivial fixes may be OK to just fix in passing, but >>>> if in doubt err on the side of not.) >>> >>> There are in general exactly two persons adding their RBs to 9p patches, >>> which is either Greg or me, and Greg made it already clear that he barely >>> has time for anything above trivial set. >>> >>> So what do you suggest? You want to participate and review 9p patches? >> >> Well I am a bit surprised... >> >> $ git log --oneline \ >> --grep='Reviewed-by: Philippe Mathieu-Daudé' -- hw/9pfs/ | wc -l >> 18 >> >> I also reviewed patch #3 if this pull request... >> >> >> Now I see you posted this 4 times in 2 months, so indeed eventual >> reviewers had plenty of time to look at your patches. >> >> Note I haven't said I'd NAck your pull request, I noticed your own >> concern wrt Daniel comment, so I looked at the patch and realized >> it was not reviewed, and simply said this is this is odd. >> >> Regards, >> >> Phil. > > Philippe, of course I understand why this looks odd to you, and yes you > reviewed that particular patch. But the situation on the 9p front is like this > for >2 years now: people quickly come by to nack patches, but even after > having addressed their concerns they barely add their RBs afterwards. That > means I am currently forced to send out PRs without RBs once in a while. > > The mentioned 5 patches look like overkill on first sight, but they are just > intended as preparatory ones. I actually should fix a protocol implementation > deficit in "Twalk" request handling, and that in turn means I will have to add > complexity to function v9fs_walk(). But before even daring to do so, I should > get rid of as much of complexity as possible. Because we already had a bunch > of issues in that function before. I believe these are trivial 5 patches. But > I can also accompany them with test cases if somebody is worried. > > Greg, I could also drop them now, but the general issue will retain: Reality > is that I am the only person working on 9p right now and a fact that I cannot > change. The rest is for other people to decide. To be explicit, I just asked clarifications to Christian. I understand the 9pfs subsystem situation, and I am not against (Nacking) this pull request being merged. Thanks both, Phil.
On 10/27/21 10:29 AM, Christian Schoenebeck wrote: > On Mittwoch, 27. Oktober 2021 18:48:10 CEST Philippe Mathieu-Daudé wrote: >> On 10/27/21 18:21, Christian Schoenebeck wrote: >>> On Mittwoch, 27. Oktober 2021 17:36:03 CEST Philippe Mathieu-Daudé wrote: >>>> Hi Christian, >>>> >>>> On 10/27/21 16:05, Christian Schoenebeck wrote: >>>>> On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote: >>>>>> The following changes since commit >>> >>> 931ce30859176f0f7daac6bac255dae5eb21284e: >>>>>> Merge remote-tracking branch >>>>>> 'remotes/dagrh/tags/pull-virtiofs-20211026' >>>>>> >>>>>> into staging (2021-10-26 07:38:41 -0700) >>>>>> >>>>>> are available in the Git repository at: >>>>>> https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027 >>>>>> >>>>>> for you to fetch changes up to > 7e985780aaab93d2c5be9b62d8d386568dfb071e: >>>>>> 9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200) >>>>>> >>>>>> ---------------------------------------------------------------- >>>>>> 9pfs: performance fix and cleanup >>>>>> >>>>>> * First patch fixes suboptimal I/O performance on guest due to >>>>>> previously >>>>>> >>>>>> incorrect block size being transmitted to 9p client. >>>>>> >>>>>> * Subsequent patches are cleanup ones intended to reduce code >>>>>> complexity. >>>>>> >>>>>> ---------------------------------------------------------------- >>>>>> >>>>>> Christian Schoenebeck (8): >>>>>> 9pfs: fix wrong I/O block size in Rgetattr >>>>>> 9pfs: deduplicate iounit code >>>>>> 9pfs: simplify blksize_to_iounit() >>>>>> 9pfs: introduce P9Array >>>>>> fsdev/p9array.h: check scalar type in P9ARRAY_NEW() >>>>>> 9pfs: make V9fsString usable via P9Array API >>>>>> 9pfs: make V9fsPath usable via P9Array API >>>>>> 9pfs: use P9Array in v9fs_walk() >>>>>> >>>>>> fsdev/9p-marshal.c | 2 + >>>>>> fsdev/9p-marshal.h | 3 + >>>>>> fsdev/file-op-9p.h | 2 + >>>>>> fsdev/p9array.h | 160 >>>>>> >>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c >>>>>> >>>>>> 70 +++++++++++++---------- >>>>>> >>>>>> 5 files changed, 208 insertions(+), 29 deletions(-) >>>>>> create mode 100644 fsdev/p9array.h >>>>> >>>>> Regarding last 5 patches: Daniel raised a concern that not using >>>>> g_autoptr >>>>> would deviate from current QEMU coding patterns: >>>>> https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html >>>>> >>>>> Unfortunately I saw no way to address his concern without adding >>>>> unnecessary code complexity, so I decided to make this a 9p local type >>>>> (QArray -> P9Array) for now, which can easily be replaced in future >>>>> (e.g. >>>>> when there will be something appropriate on glib side). >>>> >>>> Hmm various patches aren't reviewed yet... In particular >>>> patch #5 has a Suggested-by tag without Reviewed-by, this >>>> looks odd. >>>> >>>> See https://wiki.qemu.org/Contribute/SubmitAPullRequest: >>>> Don't send pull requests for code that hasn't passed review. >>>> A pull request says these patches are ready to go into QEMU now, >>>> so they must have passed the standard code review processes. In >>>> particular if you've corrected issues in one round of code review, >>>> you need to send your fixed patch series as normal to the list; >>>> you can't put it in a pull request until it's gone through. >>>> (Extremely trivial fixes may be OK to just fix in passing, but >>>> if in doubt err on the side of not.) >>> >>> There are in general exactly two persons adding their RBs to 9p patches, >>> which is either Greg or me, and Greg made it already clear that he barely >>> has time for anything above trivial set. >>> >>> So what do you suggest? You want to participate and review 9p patches? >> >> Well I am a bit surprised... >> >> $ git log --oneline \ >> --grep='Reviewed-by: Philippe Mathieu-Daudé' -- hw/9pfs/ | wc -l >> 18 >> >> I also reviewed patch #3 if this pull request... >> >> >> Now I see you posted this 4 times in 2 months, so indeed eventual >> reviewers had plenty of time to look at your patches. >> >> Note I haven't said I'd NAck your pull request, I noticed your own >> concern wrt Daniel comment, so I looked at the patch and realized >> it was not reviewed, and simply said this is this is odd. >> >> Regards, >> >> Phil. > > Philippe, of course I understand why this looks odd to you, and yes you > reviewed that particular patch. But the situation on the 9p front is like this > for >2 years now: people quickly come by to nack patches, but even after > having addressed their concerns they barely add their RBs afterwards. That > means I am currently forced to send out PRs without RBs once in a while. In know the feeling -- it takes quite some prodding to get tcg patches reviewed, and I have also sent out PRs that are incompletely reviewed. I see that patch 5 was something I suggested myself, which I then failed to review afterward. In recompense, I have reviewed the whole patch set: Reviewed-by: Richard Henderson <richard.henderson@linaro.org> I think the P9Array is fairly clever, and I do prefer it over glib ugliness. I can imagine only C++ being an improvement over what you've created. Rather than force you to re-create the PR, I'll simply apply this along with the S-o-b, to the merge object. r~
On 10/27/21 6:18 AM, Christian Schoenebeck wrote: > The following changes since commit 931ce30859176f0f7daac6bac255dae5eb21284e: > > Merge remote-tracking branch 'remotes/dagrh/tags/pull-virtiofs-20211026' into staging (2021-10-26 07:38:41 -0700) > > are available in the Git repository at: > > https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027 > > for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e: > > 9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200) > > ---------------------------------------------------------------- > 9pfs: performance fix and cleanup > > * First patch fixes suboptimal I/O performance on guest due to previously > incorrect block size being transmitted to 9p client. > > * Subsequent patches are cleanup ones intended to reduce code complexity. > > ---------------------------------------------------------------- > Christian Schoenebeck (8): > 9pfs: fix wrong I/O block size in Rgetattr > 9pfs: deduplicate iounit code > 9pfs: simplify blksize_to_iounit() > 9pfs: introduce P9Array > fsdev/p9array.h: check scalar type in P9ARRAY_NEW() > 9pfs: make V9fsString usable via P9Array API > 9pfs: make V9fsPath usable via P9Array API > 9pfs: use P9Array in v9fs_walk() > > fsdev/9p-marshal.c | 2 + > fsdev/9p-marshal.h | 3 + > fsdev/file-op-9p.h | 2 + > fsdev/p9array.h | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > hw/9pfs/9p.c | 70 +++++++++++++---------- > 5 files changed, 208 insertions(+), 29 deletions(-) > create mode 100644 fsdev/p9array.h Applied, with my R-b as I promised upthread. Thanks. r~
On Mittwoch, 27. Oktober 2021 20:44:52 CEST Richard Henderson wrote: > On 10/27/21 10:29 AM, Christian Schoenebeck wrote: > > On Mittwoch, 27. Oktober 2021 18:48:10 CEST Philippe Mathieu-Daudé wrote: > >> On 10/27/21 18:21, Christian Schoenebeck wrote: > >>> On Mittwoch, 27. Oktober 2021 17:36:03 CEST Philippe Mathieu-Daudé wrote: > >>>> Hi Christian, > >>>> > >>>> On 10/27/21 16:05, Christian Schoenebeck wrote: > >>>>> On Mittwoch, 27. Oktober 2021 15:18:33 CEST Christian Schoenebeck wrote: > >>>>>> The following changes since commit > >>> > >>> 931ce30859176f0f7daac6bac255dae5eb21284e: > >>>>>> Merge remote-tracking branch > >>>>>> 'remotes/dagrh/tags/pull-virtiofs-20211026' > >>>>>> > >>>>>> into staging (2021-10-26 07:38:41 -0700) > >>>>>> > >>>>>> are available in the Git repository at: > >>>>>> https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027 > >>>>>> > >>>>>> for you to fetch changes up to > > > > 7e985780aaab93d2c5be9b62d8d386568dfb071e: > >>>>>> 9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200) > >>>>>> > >>>>>> ---------------------------------------------------------------- > >>>>>> 9pfs: performance fix and cleanup > >>>>>> > >>>>>> * First patch fixes suboptimal I/O performance on guest due to > >>>>>> previously > >>>>>> > >>>>>> incorrect block size being transmitted to 9p client. > >>>>>> > >>>>>> * Subsequent patches are cleanup ones intended to reduce code > >>>>>> complexity. > >>>>>> > >>>>>> ---------------------------------------------------------------- > >>>>>> > >>>>>> Christian Schoenebeck (8): > >>>>>> 9pfs: fix wrong I/O block size in Rgetattr > >>>>>> 9pfs: deduplicate iounit code > >>>>>> 9pfs: simplify blksize_to_iounit() > >>>>>> 9pfs: introduce P9Array > >>>>>> fsdev/p9array.h: check scalar type in P9ARRAY_NEW() > >>>>>> 9pfs: make V9fsString usable via P9Array API > >>>>>> 9pfs: make V9fsPath usable via P9Array API > >>>>>> 9pfs: use P9Array in v9fs_walk() > >>>>>> > >>>>>> fsdev/9p-marshal.c | 2 + > >>>>>> fsdev/9p-marshal.h | 3 + > >>>>>> fsdev/file-op-9p.h | 2 + > >>>>>> fsdev/p9array.h | 160 > >>>>>> > >>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c > >>>>>> > >>>>>> 70 +++++++++++++---------- > >>>>>> > >>>>>> 5 files changed, 208 insertions(+), 29 deletions(-) > >>>>>> create mode 100644 fsdev/p9array.h > >>>>> > >>>>> Regarding last 5 patches: Daniel raised a concern that not using > >>>>> g_autoptr > >>>>> would deviate from current QEMU coding patterns: > >>>>> https://lists.gnu.org/archive/html/qemu-devel/2021-10/msg00081.html > >>>>> > >>>>> Unfortunately I saw no way to address his concern without adding > >>>>> unnecessary code complexity, so I decided to make this a 9p local type > >>>>> (QArray -> P9Array) for now, which can easily be replaced in future > >>>>> (e.g. > >>>>> when there will be something appropriate on glib side). > >>>> > >>>> Hmm various patches aren't reviewed yet... In particular > >>>> patch #5 has a Suggested-by tag without Reviewed-by, this > >>>> looks odd. > >>>> > >>>> See https://wiki.qemu.org/Contribute/SubmitAPullRequest: > >>>> Don't send pull requests for code that hasn't passed review. > >>>> A pull request says these patches are ready to go into QEMU now, > >>>> so they must have passed the standard code review processes. In > >>>> particular if you've corrected issues in one round of code review, > >>>> you need to send your fixed patch series as normal to the list; > >>>> you can't put it in a pull request until it's gone through. > >>>> (Extremely trivial fixes may be OK to just fix in passing, but > >>>> if in doubt err on the side of not.) > >>> > >>> There are in general exactly two persons adding their RBs to 9p patches, > >>> which is either Greg or me, and Greg made it already clear that he > >>> barely > >>> has time for anything above trivial set. > >>> > >>> So what do you suggest? You want to participate and review 9p patches? > >> > >> Well I am a bit surprised... > >> > >> $ git log --oneline \ > >> > >> --grep='Reviewed-by: Philippe Mathieu-Daudé' -- hw/9pfs/ | wc -l > >> > >> 18 > >> > >> I also reviewed patch #3 if this pull request... > >> > >> > >> Now I see you posted this 4 times in 2 months, so indeed eventual > >> reviewers had plenty of time to look at your patches. > >> > >> Note I haven't said I'd NAck your pull request, I noticed your own > >> concern wrt Daniel comment, so I looked at the patch and realized > >> it was not reviewed, and simply said this is this is odd. > >> > >> Regards, > >> > >> Phil. > > > > Philippe, of course I understand why this looks odd to you, and yes you > > reviewed that particular patch. But the situation on the 9p front is like > > this for >2 years now: people quickly come by to nack patches, but even > > after having addressed their concerns they barely add their RBs > > afterwards. That means I am currently forced to send out PRs without RBs > > once in a while. > In know the feeling -- it takes quite some prodding to get tcg patches > reviewed, and I have also sent out PRs that are incompletely reviewed. > > I see that patch 5 was something I suggested myself, which I then failed to > review afterward. In recompense, I have reviewed the whole patch set: > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > I think the P9Array is fairly clever, and I do prefer it over glib ugliness. > I can imagine only C++ being an improvement over what you've created. > > Rather than force you to re-create the PR, I'll simply apply this along with > the S-o-b, to the merge object. > > > r~ Thanks Richard, I highly appreciate that! Best regards, Christian Schoenebeck
The following changes since commit 931ce30859176f0f7daac6bac255dae5eb21284e: Merge remote-tracking branch 'remotes/dagrh/tags/pull-virtiofs-20211026' into staging (2021-10-26 07:38:41 -0700) are available in the Git repository at: https://github.com/cschoenebeck/qemu.git tags/pull-9p-20211027 for you to fetch changes up to 7e985780aaab93d2c5be9b62d8d386568dfb071e: 9pfs: use P9Array in v9fs_walk() (2021-10-27 14:45:22 +0200) ---------------------------------------------------------------- 9pfs: performance fix and cleanup * First patch fixes suboptimal I/O performance on guest due to previously incorrect block size being transmitted to 9p client. * Subsequent patches are cleanup ones intended to reduce code complexity. ---------------------------------------------------------------- Christian Schoenebeck (8): 9pfs: fix wrong I/O block size in Rgetattr 9pfs: deduplicate iounit code 9pfs: simplify blksize_to_iounit() 9pfs: introduce P9Array fsdev/p9array.h: check scalar type in P9ARRAY_NEW() 9pfs: make V9fsString usable via P9Array API 9pfs: make V9fsPath usable via P9Array API 9pfs: use P9Array in v9fs_walk() fsdev/9p-marshal.c | 2 + fsdev/9p-marshal.h | 3 + fsdev/file-op-9p.h | 2 + fsdev/p9array.h | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/9pfs/9p.c | 70 +++++++++++++---------- 5 files changed, 208 insertions(+), 29 deletions(-) create mode 100644 fsdev/p9array.h