Re: [Qemu-block] [Qemu-devel] [PATCH 1/1 V2] Add vhost-pci-blk driver
On 11/09/2018 12:47 AM, Michael S. Tsirkin wrote: > On Thu, Nov 08, 2018 at 10:09:00PM +0800, Dongli Zhang wrote: >> It looks the kernel space vhost-blk can only process raw image. >> >> How about to verify that only raw image is used in the drive command line >> when >> vhost-blk-pci is paired with it? >> >> Otherwise, vhost-blk-pci might be working with qcow2 image without any >> warning >> on qemu side. >> >> Dongli Zhang > > raw being raw can you really verify that? > I meant to verify the property 'format=' of '-drive', e.g., to check if BlockBackend->root->bs->drv->format_name is raw? We allow the user to erroneously give a qcow2 file with 'format=raw'. However, if 'format=qcow2' is set explicitly, vhots-blk-pci would exit with error as only raw is supported in kernel space. Dongli Zhang
Re: [Qemu-block] [Qemu-devel] [PATCH] tests: Fix Python 3 detection on older GNU make versions
On 11/8/18 1:26 PM, Eduardo Habkost wrote: > On Thu, Nov 08, 2018 at 12:36:37PM -0500, Cleber Rosa wrote: >> >> >> On 11/8/18 11:51 AM, Eduardo Habkost wrote: >> >>> I'm not sure I agree with the "is an important thing to keep >>> track of" part. I don't think we'll have any need to keep track >>> of the Python version in shell script or makefiles after we start >>> requiring Python 3. >>> >> >> Well, "Python 3" is not a uniform thing. There are many changes across >> the 3.x spectrum that can bite you. I'm speaking out of the experience >> with Avocado, that supports Python 3.4, 3.5 and 3.6 (and that reminds me >> we should add tests for 3.7). > > Do you expect us to need to keep track of the exact Python > version before running the Python interpreter? Code written in > Python doesn't count, because it can simply use sys.version_info. > > You're right, Python code can do that. My proposal is really to facilitate other parts of the test automation, and as described later, the debugging on remote systems. >> >>> Extra cleanups (like moving version checks to ./configure) are >>> still welcome, but keep in mind that this will probably be thrown >>> away once we drop Python 2 support. >>> >> >> I don't think it should. Let me give the example of a "Python 3.0" job >> on Travis, related to the following snippet: >> >> # Python builds >> - env: CONFIG="--target-list=x86_64-softmmu" >> python: >> - "3.0" >> >> If you look at https://travis-ci.org/clebergnu/qemu/jobs/452033247#L983 >> you'll see that the intended goal was missed. That has to do with how >> Travis makes the requested Python version available, and it was only >> obvious because this branch prints the Python version used. >> >> Developers writing Python code, for instance tests, may assume a given >> API that doesn't exist or behave like they expect, and not knowing the >> Python version used on a remote environment makes debugging extra hard. > > I'm not sure I get your point. We can surely add diagnostic > messages to show the Python version, and we can make ./configure > fail if Python 3 is unavailable. I'm talking about adding code > to ./configure to save Python version info for makefile rules and > shell scripts. I expect the extra code to be unnecessary in the > future. > I'm definitely happy if you agree with printing the Python version, that's where most of the value is indeed. Then, if we need to keep the version in the generate Makefile is something to be seen. - Cleber.
Re: [Qemu-block] [Qemu-devel] [PATCH] tests: Fix Python 3 detection on older GNU make versions
On Thu, Nov 08, 2018 at 12:36:37PM -0500, Cleber Rosa wrote: > > > On 11/8/18 11:51 AM, Eduardo Habkost wrote: > > > I'm not sure I agree with the "is an important thing to keep > > track of" part. I don't think we'll have any need to keep track > > of the Python version in shell script or makefiles after we start > > requiring Python 3. > > > > Well, "Python 3" is not a uniform thing. There are many changes across > the 3.x spectrum that can bite you. I'm speaking out of the experience > with Avocado, that supports Python 3.4, 3.5 and 3.6 (and that reminds me > we should add tests for 3.7). Do you expect us to need to keep track of the exact Python version before running the Python interpreter? Code written in Python doesn't count, because it can simply use sys.version_info. > > > Extra cleanups (like moving version checks to ./configure) are > > still welcome, but keep in mind that this will probably be thrown > > away once we drop Python 2 support. > > > > I don't think it should. Let me give the example of a "Python 3.0" job > on Travis, related to the following snippet: > > # Python builds > - env: CONFIG="--target-list=x86_64-softmmu" > python: > - "3.0" > > If you look at https://travis-ci.org/clebergnu/qemu/jobs/452033247#L983 > you'll see that the intended goal was missed. That has to do with how > Travis makes the requested Python version available, and it was only > obvious because this branch prints the Python version used. > > Developers writing Python code, for instance tests, may assume a given > API that doesn't exist or behave like they expect, and not knowing the > Python version used on a remote environment makes debugging extra hard. I'm not sure I get your point. We can surely add diagnostic messages to show the Python version, and we can make ./configure fail if Python 3 is unavailable. I'm talking about adding code to ./configure to save Python version info for makefile rules and shell scripts. I expect the extra code to be unnecessary in the future. -- Eduardo
Re: [Qemu-block] [Qemu-devel] [PATCH] tests: Fix Python 3 detection on older GNU make versions
On 11/8/18 11:51 AM, Eduardo Habkost wrote: > I'm not sure I agree with the "is an important thing to keep > track of" part. I don't think we'll have any need to keep track > of the Python version in shell script or makefiles after we start > requiring Python 3. > Well, "Python 3" is not a uniform thing. There are many changes across the 3.x spectrum that can bite you. I'm speaking out of the experience with Avocado, that supports Python 3.4, 3.5 and 3.6 (and that reminds me we should add tests for 3.7). > Extra cleanups (like moving version checks to ./configure) are > still welcome, but keep in mind that this will probably be thrown > away once we drop Python 2 support. > I don't think it should. Let me give the example of a "Python 3.0" job on Travis, related to the following snippet: # Python builds - env: CONFIG="--target-list=x86_64-softmmu" python: - "3.0" If you look at https://travis-ci.org/clebergnu/qemu/jobs/452033247#L983 you'll see that the intended goal was missed. That has to do with how Travis makes the requested Python version available, and it was only obvious because this branch prints the Python version used. Developers writing Python code, for instance tests, may assume a given API that doesn't exist or behave like they expect, and not knowing the Python version used on a remote environment makes debugging extra hard. - Cleber.
Re: [Qemu-block] [Qemu-devel] xen_disk qdevification
> -Original Message- > From: Xen-devel [mailto:xen-devel-boun...@lists.xenproject.org] On Behalf > Of Paul Durrant > Sent: 08 November 2018 15:44 > To: 'Kevin Wolf' > Cc: Stefano Stabellini ; qemu-block@nongnu.org; > Tim Smith ; qemu-de...@nongnu.org; 'Markus > Armbruster' ; Anthony Perard > ; xen-de...@lists.xenproject.org; Max Reitz > > Subject: Re: [Xen-devel] [Qemu-devel] xen_disk qdevification > > > -Original Message- > > From: Kevin Wolf [mailto:kw...@redhat.com] > > Sent: 08 November 2018 15:21 > > To: Paul Durrant > > Cc: 'Markus Armbruster' ; Anthony Perard > > ; Tim Smith ; Stefano > > Stabellini ; qemu-block@nongnu.org; qemu- > > de...@nongnu.org; Max Reitz ; xen- > > de...@lists.xenproject.org > > Subject: Re: [Qemu-devel] xen_disk qdevification > > > > Am 08.11.2018 um 15:00 hat Paul Durrant geschrieben: > > > > -Original Message- > > > > From: Markus Armbruster [mailto:arm...@redhat.com] > > > > Sent: 05 November 2018 15:58 > > > > To: Paul Durrant > > > > Cc: 'Kevin Wolf' ; Tim Smith > ; > > > > Stefano Stabellini ; qemu-block@nongnu.org; > > qemu- > > > > de...@nongnu.org; Max Reitz ; Anthony Perard > > > > ; xen-de...@lists.xenproject.org > > > > Subject: Re: [Qemu-devel] xen_disk qdevification > > > > > > > > Paul Durrant writes: > > > > > > > > >> -Original Message- > > > > >> From: Kevin Wolf [mailto:kw...@redhat.com] > > > > >> Sent: 02 November 2018 11:04 > > > > >> To: Tim Smith > > > > >> Cc: xen-de...@lists.xenproject.org; qemu-de...@nongnu.org; qemu- > > > > >> bl...@nongnu.org; Anthony Perard ; > Paul > > > > Durrant > > > > >> ; Stefano Stabellini > > ; > > > > >> Max Reitz ; arm...@redhat.com > > > > >> Subject: xen_disk qdevification (was: [PATCH 0/3] Performance > > > > improvements > > > > >> for xen_disk v2) > > > > >> > > > > >> Am 02.11.2018 um 11:00 hat Tim Smith geschrieben: > > > > >> > A series of performance improvements for disks using the Xen PV > > ring. > > > > >> > > > > > >> > These have had fairly extensive testing. > > > > >> > > > > > >> > The batching and latency improvements together boost the > > throughput > > > > >> > of small reads and writes by two to six percent (measured using > > fio > > > > >> > in the guest) > > > > >> > > > > > >> > Avoiding repeated calls to posix_memalign() reduced the dirty > > heap > > > > >> > from 25MB to 5MB in the case of a single datapath process while > > also > > > > >> > improving performance. > > > > >> > > > > > >> > v2 removes some checkpatch complaints and fixes the CCs > > > > >> > > > > >> Completely unrelated, but since you're the first person touching > > > > >> xen_disk in a while, you're my victim: > > > > >> > > > > >> At KVM Forum we discussed sending a patch to deprecate xen_disk > > because > > > > >> after all those years, it still hasn't been converted to qdev. > > Markus > > > > is > > > > >> currently fixing some other not yet qdevified block device, but > > after > > > > >> that xen_disk will be the only one left. > > > > >> > > > > >> A while ago, a downstream patch review found out that there are > > some > > > > QMP > > > > >> commands that would immediately crash if a xen_disk device were > > present > > > > >> because of the lacking qdevification. This is not the code > quality > > > > >> standard I envision for QEMU. It's time for non-qdev devices to > go. > > > > >> > > > > >> So if you guys are still interested in the device, could someone > > please > > > > >> finally look into converting it? > > > > >> > > > > > > > > > > I have a patch series to do exactly this. It's somewhat involved > as > > I > > > > > need to convert the whole PV backend infrastructure. I will try to > > > > > rebase and clean up my series a.s.a.p. > > > > > > > > Awesome! Please coordinate with Anthony Prerard to avoid > duplicating > > > > work if you haven't done so already. > > > > > > I've come across a bit of a problem that I'm not sure how best to deal > > > with and so am looking for some advice. > > > > > > I now have a qdevified PV disk backend but I can't bring it up because > > > it fails to acquire a write lock on the qcow2 it is pointing at. This > > > is because there is also an emulated IDE drive using the same qcow2. > > > This does not appear to be a problem for the non-qdev xen-disk, > > > presumably because it is not opening the qcow2 until the emulated > > > device is unplugged and I don't really want to introduce similar > > > hackery in my new backend (i.e. I want it to attach to its drive, and > > > hence open the qcow2, during realize). > > > > > > So, I'm not sure what to do... It is not a problem that both a PV > > > backend and an emulated device are using the same qcow2 because they > > > will never actually operate simultaneously so is there any way I can > > > bypass the qcow2 lock check when I create the drive for my PV backend? > > > (BTW I tried re-using the drive created for the emulated device, but > > > that doesn't work because there is a c
Re: [Qemu-block] [Qemu-devel] [PATCH] tests: Fix Python 3 detection on older GNU make versions
On Thu, Nov 08, 2018 at 11:06:45AM -0500, Cleber Rosa wrote: > > > On 11/8/18 3:45 AM, Markus Armbruster wrote: > > Cleber Rosa writes: > > > >> On 11/7/18 1:05 AM, Markus Armbruster wrote: > >>> Eduardo Habkost writes: > >>> > The $(SHELLSTATUS) variable requires GNU make >= 4.2, but Travis > seems to provide an older version. Change the existing rules to > use command output instead of exit code, to make it compatible > with older GNU make versions. > > Signed-off-by: Eduardo Habkost > --- > I think that's the cause of the Travis failures. I have > submitted a test job right now, at: > https://travis-ci.org/ehabkost/qemu-hacks/jobs/451387962 > Let's see if it fixes the issue. > --- > tests/Makefile.include | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index d2e577eabb..074eece558 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -913,8 +913,8 @@ TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results > # information please refer to "avocado --help". > AVOCADO_SHOW=none > > -$(shell $(PYTHON) -c 'import sys; assert sys.version_info >= (3,0)' > >/dev/null 2>&1) > -ifeq ($(.SHELLSTATUS),0) > +PYTHON3 = $(shell $(PYTHON) -c 'import sys; print(1 if sys.version_info > >= (3, 0) else 0)') > +ifeq ($(PYTHON3), 1) > $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) > $(call quiet-command, \ > $(PYTHON) -m venv --system-site-packages $@, \ > >>> > >>> PEP 394 recommends software distributions install Python 3 into the > >>> default path as python3, and users use that instead of python, except > >>> for programs that are source compatible with both 2 and 3. So, is > >>> finding out whether python is a Python 3 really appropriate? Why can't > >>> we just use python3 and be done with it? > >>> > >> > >> I mentioned that before, when you pointed out the issue you fix here, > >> that configure may be the best place to get the Python version (not only > >> the major version) and make it available elsewhere. Even if not used > >> for other purposes, it is IMO important information to show on the > >> resulting "configure" output. > >> > >> I'm sending patches to do that in a few. > >> > >>> If we can't: isn't this a configure problem? > >>> > >> > >> I believe adhering to PEP394 is a good thing, but even that document > >> recognizes that the real world is not a perfect place: "however, end > >> users should be aware that python refers to python3 on at least Arch > >> Linux". And it only covers *nix systems, so if we hope to care for > >> non-*nix systems, we have to check the Python version manually. > >> > >> So, I guess the safest approach from QEMU's side is to check for the > >> version indeed. > > > > If somebody can point to a system people still use where python3 doesn't > > get you a Python 3, but python does, catering for such (crappy) systems > > in configure makes sense. Until then, it's a waste of brain waves and > > configure run time. > > > > PEP 394 mentions Arch Linux. It's been seven years. What's the most > > recent version of Arch Linux that's still crappy in this regard? > > > > I'm not taking the mission of looking for those odd distros, I agree > it's not productive. My point is that the Python version is an > important thing to keep track of, specially with the advent of running > Python dependent tests on environments we don't fully control. > > Supposing the Python version is captured and tracked by configure, then > the "python" binary name becomes a non-issue. I'm not sure I agree with the "is an important thing to keep track of" part. I don't think we'll have any need to keep track of the Python version in shell script or makefiles after we start requiring Python 3. Extra cleanups (like moving version checks to ./configure) are still welcome, but keep in mind that this will probably be thrown away once we drop Python 2 support. -- Eduardo
Re: [Qemu-block] [Qemu-devel] [PATCH 1/1 V2] Add vhost-pci-blk driver
On Thu, Nov 08, 2018 at 10:09:00PM +0800, Dongli Zhang wrote: > It looks the kernel space vhost-blk can only process raw image. > > How about to verify that only raw image is used in the drive command line when > vhost-blk-pci is paired with it? > > Otherwise, vhost-blk-pci might be working with qcow2 image without any warning > on qemu side. > > Dongli Zhang raw being raw can you really verify that?
Re: [Qemu-block] [Qemu-devel] [PATCH] tests: Fix Python 3 detection on older GNU make versions
On 11/8/18 3:45 AM, Markus Armbruster wrote: > Cleber Rosa writes: > >> On 11/7/18 1:05 AM, Markus Armbruster wrote: >>> Eduardo Habkost writes: >>> The $(SHELLSTATUS) variable requires GNU make >= 4.2, but Travis seems to provide an older version. Change the existing rules to use command output instead of exit code, to make it compatible with older GNU make versions. Signed-off-by: Eduardo Habkost --- I think that's the cause of the Travis failures. I have submitted a test job right now, at: https://travis-ci.org/ehabkost/qemu-hacks/jobs/451387962 Let's see if it fixes the issue. --- tests/Makefile.include | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index d2e577eabb..074eece558 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -913,8 +913,8 @@ TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results # information please refer to "avocado --help". AVOCADO_SHOW=none -$(shell $(PYTHON) -c 'import sys; assert sys.version_info >= (3,0)' >/dev/null 2>&1) -ifeq ($(.SHELLSTATUS),0) +PYTHON3 = $(shell $(PYTHON) -c 'import sys; print(1 if sys.version_info >= (3, 0) else 0)') +ifeq ($(PYTHON3), 1) $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) $(call quiet-command, \ $(PYTHON) -m venv --system-site-packages $@, \ >>> >>> PEP 394 recommends software distributions install Python 3 into the >>> default path as python3, and users use that instead of python, except >>> for programs that are source compatible with both 2 and 3. So, is >>> finding out whether python is a Python 3 really appropriate? Why can't >>> we just use python3 and be done with it? >>> >> >> I mentioned that before, when you pointed out the issue you fix here, >> that configure may be the best place to get the Python version (not only >> the major version) and make it available elsewhere. Even if not used >> for other purposes, it is IMO important information to show on the >> resulting "configure" output. >> >> I'm sending patches to do that in a few. >> >>> If we can't: isn't this a configure problem? >>> >> >> I believe adhering to PEP394 is a good thing, but even that document >> recognizes that the real world is not a perfect place: "however, end >> users should be aware that python refers to python3 on at least Arch >> Linux". And it only covers *nix systems, so if we hope to care for >> non-*nix systems, we have to check the Python version manually. >> >> So, I guess the safest approach from QEMU's side is to check for the >> version indeed. > > If somebody can point to a system people still use where python3 doesn't > get you a Python 3, but python does, catering for such (crappy) systems > in configure makes sense. Until then, it's a waste of brain waves and > configure run time. > > PEP 394 mentions Arch Linux. It's been seven years. What's the most > recent version of Arch Linux that's still crappy in this regard? > I'm not taking the mission of looking for those odd distros, I agree it's not productive. My point is that the Python version is an important thing to keep track of, specially with the advent of running Python dependent tests on environments we don't fully control. Supposing the Python version is captured and tracked by configure, then the "python" binary name becomes a non-issue. - Cleber.
Re: [Qemu-block] [Qemu-devel] xen_disk qdevification
> -Original Message- > From: Kevin Wolf [mailto:kw...@redhat.com] > Sent: 08 November 2018 15:21 > To: Paul Durrant > Cc: 'Markus Armbruster' ; Anthony Perard > ; Tim Smith ; Stefano > Stabellini ; qemu-block@nongnu.org; qemu- > de...@nongnu.org; Max Reitz ; xen- > de...@lists.xenproject.org > Subject: Re: [Qemu-devel] xen_disk qdevification > > Am 08.11.2018 um 15:00 hat Paul Durrant geschrieben: > > > -Original Message- > > > From: Markus Armbruster [mailto:arm...@redhat.com] > > > Sent: 05 November 2018 15:58 > > > To: Paul Durrant > > > Cc: 'Kevin Wolf' ; Tim Smith ; > > > Stefano Stabellini ; qemu-block@nongnu.org; > qemu- > > > de...@nongnu.org; Max Reitz ; Anthony Perard > > > ; xen-de...@lists.xenproject.org > > > Subject: Re: [Qemu-devel] xen_disk qdevification > > > > > > Paul Durrant writes: > > > > > > >> -Original Message- > > > >> From: Kevin Wolf [mailto:kw...@redhat.com] > > > >> Sent: 02 November 2018 11:04 > > > >> To: Tim Smith > > > >> Cc: xen-de...@lists.xenproject.org; qemu-de...@nongnu.org; qemu- > > > >> bl...@nongnu.org; Anthony Perard ; Paul > > > Durrant > > > >> ; Stefano Stabellini > ; > > > >> Max Reitz ; arm...@redhat.com > > > >> Subject: xen_disk qdevification (was: [PATCH 0/3] Performance > > > improvements > > > >> for xen_disk v2) > > > >> > > > >> Am 02.11.2018 um 11:00 hat Tim Smith geschrieben: > > > >> > A series of performance improvements for disks using the Xen PV > ring. > > > >> > > > > >> > These have had fairly extensive testing. > > > >> > > > > >> > The batching and latency improvements together boost the > throughput > > > >> > of small reads and writes by two to six percent (measured using > fio > > > >> > in the guest) > > > >> > > > > >> > Avoiding repeated calls to posix_memalign() reduced the dirty > heap > > > >> > from 25MB to 5MB in the case of a single datapath process while > also > > > >> > improving performance. > > > >> > > > > >> > v2 removes some checkpatch complaints and fixes the CCs > > > >> > > > >> Completely unrelated, but since you're the first person touching > > > >> xen_disk in a while, you're my victim: > > > >> > > > >> At KVM Forum we discussed sending a patch to deprecate xen_disk > because > > > >> after all those years, it still hasn't been converted to qdev. > Markus > > > is > > > >> currently fixing some other not yet qdevified block device, but > after > > > >> that xen_disk will be the only one left. > > > >> > > > >> A while ago, a downstream patch review found out that there are > some > > > QMP > > > >> commands that would immediately crash if a xen_disk device were > present > > > >> because of the lacking qdevification. This is not the code quality > > > >> standard I envision for QEMU. It's time for non-qdev devices to go. > > > >> > > > >> So if you guys are still interested in the device, could someone > please > > > >> finally look into converting it? > > > >> > > > > > > > > I have a patch series to do exactly this. It's somewhat involved as > I > > > > need to convert the whole PV backend infrastructure. I will try to > > > > rebase and clean up my series a.s.a.p. > > > > > > Awesome! Please coordinate with Anthony Prerard to avoid duplicating > > > work if you haven't done so already. > > > > I've come across a bit of a problem that I'm not sure how best to deal > > with and so am looking for some advice. > > > > I now have a qdevified PV disk backend but I can't bring it up because > > it fails to acquire a write lock on the qcow2 it is pointing at. This > > is because there is also an emulated IDE drive using the same qcow2. > > This does not appear to be a problem for the non-qdev xen-disk, > > presumably because it is not opening the qcow2 until the emulated > > device is unplugged and I don't really want to introduce similar > > hackery in my new backend (i.e. I want it to attach to its drive, and > > hence open the qcow2, during realize). > > > > So, I'm not sure what to do... It is not a problem that both a PV > > backend and an emulated device are using the same qcow2 because they > > will never actually operate simultaneously so is there any way I can > > bypass the qcow2 lock check when I create the drive for my PV backend? > > (BTW I tried re-using the drive created for the emulated device, but > > that doesn't work because there is a check if a drive is already > > attached to something). > > > > Any ideas? > > I think the clean solution is to keep the BlockBackend open in xen-disk > from the beginning, but not requesting write permissions yet. > > The BlockBackend is created in parse_drive(), when qdev parses the > -device drive=... option. At this point, no permissions are requested > yet. That is done in blkconf_apply_backend_options(), which is manually > called from the devices; specifically from ide_dev_initfn() in IDE, and > I assume you call the function from xen-disk as well. Yes, I call it during realize. > > xen-disk should then call this function with
Re: [Qemu-block] [Qemu-devel] xen_disk qdevification
Am 08.11.2018 um 15:00 hat Paul Durrant geschrieben: > > -Original Message- > > From: Markus Armbruster [mailto:arm...@redhat.com] > > Sent: 05 November 2018 15:58 > > To: Paul Durrant > > Cc: 'Kevin Wolf' ; Tim Smith ; > > Stefano Stabellini ; qemu-block@nongnu.org; qemu- > > de...@nongnu.org; Max Reitz ; Anthony Perard > > ; xen-de...@lists.xenproject.org > > Subject: Re: [Qemu-devel] xen_disk qdevification > > > > Paul Durrant writes: > > > > >> -Original Message- > > >> From: Kevin Wolf [mailto:kw...@redhat.com] > > >> Sent: 02 November 2018 11:04 > > >> To: Tim Smith > > >> Cc: xen-de...@lists.xenproject.org; qemu-de...@nongnu.org; qemu- > > >> bl...@nongnu.org; Anthony Perard ; Paul > > Durrant > > >> ; Stefano Stabellini ; > > >> Max Reitz ; arm...@redhat.com > > >> Subject: xen_disk qdevification (was: [PATCH 0/3] Performance > > improvements > > >> for xen_disk v2) > > >> > > >> Am 02.11.2018 um 11:00 hat Tim Smith geschrieben: > > >> > A series of performance improvements for disks using the Xen PV ring. > > >> > > > >> > These have had fairly extensive testing. > > >> > > > >> > The batching and latency improvements together boost the throughput > > >> > of small reads and writes by two to six percent (measured using fio > > >> > in the guest) > > >> > > > >> > Avoiding repeated calls to posix_memalign() reduced the dirty heap > > >> > from 25MB to 5MB in the case of a single datapath process while also > > >> > improving performance. > > >> > > > >> > v2 removes some checkpatch complaints and fixes the CCs > > >> > > >> Completely unrelated, but since you're the first person touching > > >> xen_disk in a while, you're my victim: > > >> > > >> At KVM Forum we discussed sending a patch to deprecate xen_disk because > > >> after all those years, it still hasn't been converted to qdev. Markus > > is > > >> currently fixing some other not yet qdevified block device, but after > > >> that xen_disk will be the only one left. > > >> > > >> A while ago, a downstream patch review found out that there are some > > QMP > > >> commands that would immediately crash if a xen_disk device were present > > >> because of the lacking qdevification. This is not the code quality > > >> standard I envision for QEMU. It's time for non-qdev devices to go. > > >> > > >> So if you guys are still interested in the device, could someone please > > >> finally look into converting it? > > >> > > > > > > I have a patch series to do exactly this. It's somewhat involved as I > > > need to convert the whole PV backend infrastructure. I will try to > > > rebase and clean up my series a.s.a.p. > > > > Awesome! Please coordinate with Anthony Prerard to avoid duplicating > > work if you haven't done so already. > > I've come across a bit of a problem that I'm not sure how best to deal > with and so am looking for some advice. > > I now have a qdevified PV disk backend but I can't bring it up because > it fails to acquire a write lock on the qcow2 it is pointing at. This > is because there is also an emulated IDE drive using the same qcow2. > This does not appear to be a problem for the non-qdev xen-disk, > presumably because it is not opening the qcow2 until the emulated > device is unplugged and I don't really want to introduce similar > hackery in my new backend (i.e. I want it to attach to its drive, and > hence open the qcow2, during realize). > > So, I'm not sure what to do... It is not a problem that both a PV > backend and an emulated device are using the same qcow2 because they > will never actually operate simultaneously so is there any way I can > bypass the qcow2 lock check when I create the drive for my PV backend? > (BTW I tried re-using the drive created for the emulated device, but > that doesn't work because there is a check if a drive is already > attached to something). > > Any ideas? I think the clean solution is to keep the BlockBackend open in xen-disk from the beginning, but not requesting write permissions yet. The BlockBackend is created in parse_drive(), when qdev parses the -device drive=... option. At this point, no permissions are requested yet. That is done in blkconf_apply_backend_options(), which is manually called from the devices; specifically from ide_dev_initfn() in IDE, and I assume you call the function from xen-disk as well. xen-disk should then call this function with readonly=true, and at the point of the handover (when the IDE device is already gone) it can call blk_set_perm() to request BLK_PERM_WRITE in addition to the permissions it already holds. The other option I see would be that you simply create both devices with share-rw=on (which results in conf->share_rw == true and therefore shared BLK_PERM_WRITE in blkconf_apply_backend_options()), but that feels like a hack because you don't actually want to have two writers at the same time. Kevin
Re: [Qemu-block] [PATCH 1/1 V2] Add vhost-pci-blk driver
Am 05.11.2018 um 21:56 hat Vitaly Mayatskikh geschrieben: > This driver uses the kernel-mode acceleration for virtio-blk and > allows to get a near bare metal disk performance inside a VM. > > Signed-off-by: Vitaly Mayatskikh > --- > configure | 10 + > default-configs/virtio.mak| 1 + > hw/block/Makefile.objs| 1 + > hw/block/vhost-blk.c | 429 ++ > hw/virtio/virtio-pci.c| 60 + > hw/virtio/virtio-pci.h| 19 ++ > include/hw/virtio/vhost-blk.h | 43 > 7 files changed, 563 insertions(+) > create mode 100644 hw/block/vhost-blk.c > create mode 100644 include/hw/virtio/vhost-blk.h > > diff --git a/configure b/configure > index 46ae1e8c76..787bc780da 100755 > --- a/configure > +++ b/configure > @@ -371,6 +371,7 @@ vhost_crypto="no" > vhost_scsi="no" > vhost_vsock="no" > vhost_user="" > +vhost_blk="" > kvm="no" > hax="no" > hvf="no" > @@ -869,6 +870,7 @@ Linux) >vhost_crypto="yes" >vhost_scsi="yes" >vhost_vsock="yes" > + vhost_blk="yes" >QEMU_INCLUDES="-I\$(SRC_PATH)/linux-headers -I$(pwd)/linux-headers > $QEMU_INCLUDES" >supported_os="yes" >libudev="yes" > @@ -1263,6 +1265,10 @@ for opt do >;; >--enable-vhost-vsock) vhost_vsock="yes" >;; > + --disable-vhost-blk) vhost_blk="no" > + ;; > + --enable-vhost-blk) vhost_blk="yes" > + ;; >--disable-opengl) opengl="no" >;; >--enable-opengl) opengl="yes" > @@ -6000,6 +6006,7 @@ echo "vhost-crypto support $vhost_crypto" > echo "vhost-scsi support $vhost_scsi" > echo "vhost-vsock support $vhost_vsock" > echo "vhost-user support $vhost_user" > +echo "vhost-blk support $vhost_blk" > echo "Trace backends$trace_backends" > if have_backend "simple"; then > echo "Trace output file $trace_file-" > @@ -6461,6 +6468,9 @@ fi > if test "$vhost_user" = "yes" ; then >echo "CONFIG_VHOST_USER=y" >> $config_host_mak > fi > +if test "$vhost_blk" = "yes" ; then > + echo "CONFIG_VHOST_BLK=y" >> $config_host_mak > +fi > if test "$blobs" = "yes" ; then >echo "INSTALL_BLOBS=yes" >> $config_host_mak > fi > diff --git a/default-configs/virtio.mak b/default-configs/virtio.mak > index 1304849018..765c0a2a04 100644 > --- a/default-configs/virtio.mak > +++ b/default-configs/virtio.mak > @@ -1,5 +1,6 @@ > CONFIG_VHOST_USER_SCSI=$(call land,$(CONFIG_VHOST_USER),$(CONFIG_LINUX)) > CONFIG_VHOST_USER_BLK=$(call land,$(CONFIG_VHOST_USER),$(CONFIG_LINUX)) > +CONFIG_VHOST_BLK=$(CONFIG_LINUX) > CONFIG_VIRTIO=y > CONFIG_VIRTIO_9P=y > CONFIG_VIRTIO_BALLOON=y > diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs > index 53ce5751ae..857ce823fc 100644 > --- a/hw/block/Makefile.objs > +++ b/hw/block/Makefile.objs > @@ -14,3 +14,4 @@ obj-$(CONFIG_SH4) += tc58128.o > obj-$(CONFIG_VIRTIO_BLK) += virtio-blk.o > obj-$(CONFIG_VIRTIO_BLK) += dataplane/ > obj-$(CONFIG_VHOST_USER_BLK) += vhost-user-blk.o > +obj-$(CONFIG_VHOST_BLK) += vhost-blk.o > diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c > new file mode 100644 > index 00..4ca8040ee7 > --- /dev/null > +++ b/hw/block/vhost-blk.c > @@ -0,0 +1,429 @@ > +/* > + * vhost-blk host device > + * > + * Copyright(C) 2018 IBM Corporation > + * > + * Authors: > + * Vitaly Mayatskikh > + * > + * Largely based on the "vhost-user-blk.c" implemented by: > + * Changpeng Liu You mean contrib/vhost-user-blk/vhost-user-blk.c in the QEMU tree? That one has the following license: * This work is licensed under the terms of the GNU GPL, version 2 only. * See the COPYING file in the top-level directory. You have this on the other hand: > + * This work is licensed under the terms of the GNU LGPL, version 2 or later. > + * See the COPYING.LIB file in the top-level directory. I think you need to get rid of any vhost-user-blk.c parts before you can make it LGPL2+. Kevin
Re: [Qemu-block] [Qemu-devel] [PATCH 1/1 V2] Add vhost-pci-blk driver
It looks the kernel space vhost-blk can only process raw image.
How about to verify that only raw image is used in the drive command line when
vhost-blk-pci is paired with it?
Otherwise, vhost-blk-pci might be working with qcow2 image without any warning
on qemu side.
Dongli Zhang
On 11/06/2018 04:56 AM, Vitaly Mayatskikh wrote:
> This driver uses the kernel-mode acceleration for virtio-blk and
> allows to get a near bare metal disk performance inside a VM.
>
> Signed-off-by: Vitaly Mayatskikh
> ---
> configure | 10 +
> default-configs/virtio.mak| 1 +
> hw/block/Makefile.objs| 1 +
> hw/block/vhost-blk.c | 429 ++
> hw/virtio/virtio-pci.c| 60 +
> hw/virtio/virtio-pci.h| 19 ++
> include/hw/virtio/vhost-blk.h | 43
> 7 files changed, 563 insertions(+)
> create mode 100644 hw/block/vhost-blk.c
> create mode 100644 include/hw/virtio/vhost-blk.h
>
> diff --git a/configure b/configure
> index 46ae1e8c76..787bc780da 100755
> --- a/configure
> +++ b/configure
> @@ -371,6 +371,7 @@ vhost_crypto="no"
> vhost_scsi="no"
> vhost_vsock="no"
> vhost_user=""
> +vhost_blk=""
> kvm="no"
> hax="no"
> hvf="no"
> @@ -869,6 +870,7 @@ Linux)
>vhost_crypto="yes"
>vhost_scsi="yes"
>vhost_vsock="yes"
> + vhost_blk="yes"
>QEMU_INCLUDES="-I\$(SRC_PATH)/linux-headers -I$(pwd)/linux-headers
> $QEMU_INCLUDES"
>supported_os="yes"
>libudev="yes"
> @@ -1263,6 +1265,10 @@ for opt do
>;;
>--enable-vhost-vsock) vhost_vsock="yes"
>;;
> + --disable-vhost-blk) vhost_blk="no"
> + ;;
> + --enable-vhost-blk) vhost_blk="yes"
> + ;;
>--disable-opengl) opengl="no"
>;;
>--enable-opengl) opengl="yes"
> @@ -6000,6 +6006,7 @@ echo "vhost-crypto support $vhost_crypto"
> echo "vhost-scsi support $vhost_scsi"
> echo "vhost-vsock support $vhost_vsock"
> echo "vhost-user support $vhost_user"
> +echo "vhost-blk support $vhost_blk"
> echo "Trace backends$trace_backends"
> if have_backend "simple"; then
> echo "Trace output file $trace_file-"
> @@ -6461,6 +6468,9 @@ fi
> if test "$vhost_user" = "yes" ; then
>echo "CONFIG_VHOST_USER=y" >> $config_host_mak
> fi
> +if test "$vhost_blk" = "yes" ; then
> + echo "CONFIG_VHOST_BLK=y" >> $config_host_mak
> +fi
> if test "$blobs" = "yes" ; then
>echo "INSTALL_BLOBS=yes" >> $config_host_mak
> fi
> diff --git a/default-configs/virtio.mak b/default-configs/virtio.mak
> index 1304849018..765c0a2a04 100644
> --- a/default-configs/virtio.mak
> +++ b/default-configs/virtio.mak
> @@ -1,5 +1,6 @@
> CONFIG_VHOST_USER_SCSI=$(call land,$(CONFIG_VHOST_USER),$(CONFIG_LINUX))
> CONFIG_VHOST_USER_BLK=$(call land,$(CONFIG_VHOST_USER),$(CONFIG_LINUX))
> +CONFIG_VHOST_BLK=$(CONFIG_LINUX)
> CONFIG_VIRTIO=y
> CONFIG_VIRTIO_9P=y
> CONFIG_VIRTIO_BALLOON=y
> diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
> index 53ce5751ae..857ce823fc 100644
> --- a/hw/block/Makefile.objs
> +++ b/hw/block/Makefile.objs
> @@ -14,3 +14,4 @@ obj-$(CONFIG_SH4) += tc58128.o
> obj-$(CONFIG_VIRTIO_BLK) += virtio-blk.o
> obj-$(CONFIG_VIRTIO_BLK) += dataplane/
> obj-$(CONFIG_VHOST_USER_BLK) += vhost-user-blk.o
> +obj-$(CONFIG_VHOST_BLK) += vhost-blk.o
> diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
> new file mode 100644
> index 00..4ca8040ee7
> --- /dev/null
> +++ b/hw/block/vhost-blk.c
> @@ -0,0 +1,429 @@
> +/*
> + * vhost-blk host device
> + *
> + * Copyright(C) 2018 IBM Corporation
> + *
> + * Authors:
> + * Vitaly Mayatskikh
> + *
> + * Largely based on the "vhost-user-blk.c" implemented by:
> + * Changpeng Liu
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/error-report.h"
> +#include "qemu/cutils.h"
> +#include "qom/object.h"
> +#include "hw/qdev-core.h"
> +#include "hw/virtio/vhost.h"
> +#include "hw/virtio/vhost-blk.h"
> +#include "hw/virtio/virtio.h"
> +#include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
> +#include
> +#include
> +
> +static const int feature_bits[] = {
> +VIRTIO_BLK_F_SIZE_MAX,
> +VIRTIO_BLK_F_SEG_MAX,
> +VIRTIO_BLK_F_BLK_SIZE,
> +VIRTIO_BLK_F_TOPOLOGY,
> +VIRTIO_BLK_F_MQ,
> +VIRTIO_BLK_F_RO,
> +VIRTIO_BLK_F_FLUSH,
> +VIRTIO_BLK_F_CONFIG_WCE,
> +VIRTIO_F_VERSION_1,
> +VIRTIO_RING_F_INDIRECT_DESC,
> +VIRTIO_RING_F_EVENT_IDX,
> +VIRTIO_F_NOTIFY_ON_EMPTY,
> +VHOST_INVALID_FEATURE_BIT
> +};
> +
> +static void vhost_blk_get_config(VirtIODevice *vdev, uint8_t *config)
> +{
> +VHostBlk *s = VHOST_BLK(vdev);
> +memcpy(config, &s->blkcfg, sizeof(struct virtio_blk_config));
> +}
> +
> +static void vhost_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
> +{
> +VHostBlk *s = VHOST_BLK(vdev);
> +struct virtio_blk_config *blkcfg = (
Re: [Qemu-block] [Qemu-devel] xen_disk qdevification
> -Original Message- > From: Markus Armbruster [mailto:arm...@redhat.com] > Sent: 05 November 2018 15:58 > To: Paul Durrant > Cc: 'Kevin Wolf' ; Tim Smith ; > Stefano Stabellini ; qemu-block@nongnu.org; qemu- > de...@nongnu.org; Max Reitz ; Anthony Perard > ; xen-de...@lists.xenproject.org > Subject: Re: [Qemu-devel] xen_disk qdevification > > Paul Durrant writes: > > >> -Original Message- > >> From: Kevin Wolf [mailto:kw...@redhat.com] > >> Sent: 02 November 2018 11:04 > >> To: Tim Smith > >> Cc: xen-de...@lists.xenproject.org; qemu-de...@nongnu.org; qemu- > >> bl...@nongnu.org; Anthony Perard ; Paul > Durrant > >> ; Stefano Stabellini ; > >> Max Reitz ; arm...@redhat.com > >> Subject: xen_disk qdevification (was: [PATCH 0/3] Performance > improvements > >> for xen_disk v2) > >> > >> Am 02.11.2018 um 11:00 hat Tim Smith geschrieben: > >> > A series of performance improvements for disks using the Xen PV ring. > >> > > >> > These have had fairly extensive testing. > >> > > >> > The batching and latency improvements together boost the throughput > >> > of small reads and writes by two to six percent (measured using fio > >> > in the guest) > >> > > >> > Avoiding repeated calls to posix_memalign() reduced the dirty heap > >> > from 25MB to 5MB in the case of a single datapath process while also > >> > improving performance. > >> > > >> > v2 removes some checkpatch complaints and fixes the CCs > >> > >> Completely unrelated, but since you're the first person touching > >> xen_disk in a while, you're my victim: > >> > >> At KVM Forum we discussed sending a patch to deprecate xen_disk because > >> after all those years, it still hasn't been converted to qdev. Markus > is > >> currently fixing some other not yet qdevified block device, but after > >> that xen_disk will be the only one left. > >> > >> A while ago, a downstream patch review found out that there are some > QMP > >> commands that would immediately crash if a xen_disk device were present > >> because of the lacking qdevification. This is not the code quality > >> standard I envision for QEMU. It's time for non-qdev devices to go. > >> > >> So if you guys are still interested in the device, could someone please > >> finally look into converting it? > >> > > > > I have a patch series to do exactly this. It's somewhat involved as I > > need to convert the whole PV backend infrastructure. I will try to > > rebase and clean up my series a.s.a.p. > > Awesome! Please coordinate with Anthony Prerard to avoid duplicating > work if you haven't done so already. I've come across a bit of a problem that I'm not sure how best to deal with and so am looking for some advice. I now have a qdevified PV disk backend but I can't bring it up because it fails to acquire a write lock on the qcow2 it is pointing at. This is because there is also an emulated IDE drive using the same qcow2. This does not appear to be a problem for the non-qdev xen-disk, presumably because it is not opening the qcow2 until the emulated device is unplugged and I don't really want to introduce similar hackery in my new backend (i.e. I want it to attach to its drive, and hence open the qcow2, during realize). So, I'm not sure what to do... It is not a problem that both a PV backend and an emulated device are using the same qcow2 because they will never actually operate simultaneously so is there any way I can bypass the qcow2 lock check when I create the drive for my PV backend? (BTW I tried re-using the drive created for the emulated device, but that doesn't work because there is a check if a drive is already attached to something). Any ideas? Paul
Re: [Qemu-block] [Qemu-devel] [PATCH] tests: Fix Python 3 detection on older GNU make versions
Philippe Mathieu-Daudé writes: > Hi Markus, > > > Le jeu. 8 nov. 2018 09:46, Markus Armbruster a écrit : > >> Cleber Rosa writes: >> >> > On 11/7/18 1:05 AM, Markus Armbruster wrote: [...] >> >> PEP 394 recommends software distributions install Python 3 into the >> >> default path as python3, and users use that instead of python, except >> >> for programs that are source compatible with both 2 and 3. So, is >> >> finding out whether python is a Python 3 really appropriate? Why can't >> >> we just use python3 and be done with it? >> >> >> > >> > I mentioned that before, when you pointed out the issue you fix here, >> > that configure may be the best place to get the Python version (not only >> > the major version) and make it available elsewhere. Even if not used >> > for other purposes, it is IMO important information to show on the >> > resulting "configure" output. >> > >> > I'm sending patches to do that in a few. >> > >> >> If we can't: isn't this a configure problem? >> >> >> > >> > I believe adhering to PEP394 is a good thing, but even that document >> > recognizes that the real world is not a perfect place: "however, end >> > users should be aware that python refers to python3 on at least Arch >> > Linux". And it only covers *nix systems, so if we hope to care for >> > non-*nix systems, we have to check the Python version manually. >> > >> > So, I guess the safest approach from QEMU's side is to check for the >> > version indeed. >> >> If somebody can point to a system people still use where python3 doesn't >> get you a Python 3, but python does, catering for such (crappy) systems >> in configure makes sense. Until then, it's a waste of brain waves and >> configure run time. >> >> PEP 394 mentions Arch Linux. It's been seven years. What's the most >> recent version of Arch Linux that's still crappy in this regard? >> > > Arch doesn't provide python2 by default, thus python points to python3. That's non-crappy as long as python3 also exists, as PEP 394 recommends. Does it? > I think what's crappy is scripts expecting python to be python2. No argument.
Re: [Qemu-block] [PATCH 3/7] qcow2: split out reading normal clusters from qcow2_co_preadv
08.11.2018 13:33, Kevin Wolf wrote:
> Am 08.11.2018 um 11:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 07.11.2018 21:16, Kevin Wolf wrote:
>>> (Broken quoting in text/plain again)
>>>
>>> Am 01.11.2018 um 13:17 hat Vladimir Sementsov-Ogievskiy geschrieben:
27.09.2018 20:35, Max Reitz wrote:
On 07.08.18 19:43, Vladimir Sementsov-Ogievskiy wrote:
Memory allocation may become less efficient for encrypted case.
It's a
payment for further asynchronous scheme.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/qcow2.c | 114
--
1 file changed, 64 insertions(+), 50 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 65e3ca00e2..5e7f2ee318 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1808,6 +1808,67 @@ out:
return ret;
}
+static coroutine_fn int qcow2_co_preadv_normal(BlockDriverState
*bs,
+ uint64_t
file_cluster_offset,
+ uint64_t offset,
+ uint64_t bytes,
+ QEMUIOVector
*qiov,
+ uint64_t
qiov_offset)
+{
+int ret;
+BDRVQcow2State *s = bs->opaque;
+void *crypt_buf = NULL;
+QEMUIOVector hd_qiov;
+int offset_in_cluster = offset_into_cluster(s, offset);
+
+if ((file_cluster_offset & 511) != 0) {
+return -EIO;
+}
+
+qemu_iovec_init(&hd_qiov, qiov->niov);
So you're not just re-allocating the bounce buffer for every single
call, but also this I/O vector. Hm.
>>> And this one is actually at least a little more serious, I think.
>>>
>>> Decryption and decompression (including copying between the original
>>> qiov and the bounce buffer) are already very slow, so allocating a
>>> bounce buffer or not shouldn't make any noticable difference.
>>>
>>> But I'd like to keep allocations out of the fast path as much as we can.
>>>
+if (bs->encrypted) {
+assert(s->crypto);
+assert(bytes <= QCOW_MAX_CRYPT_CLUSTERS *
s->cluster_size);
+
+crypt_buf = qemu_try_blockalign(bs->file->bs, bytes);
1. Why did you remove the comment?
2. The check for whether crypt_buf was successfully allocated is
missing.
3. Do you have any benchmarks for encrypted images? Having to
allocate
the bounce buffer for potentially every single cluster sounds rather
bad
to me.
>>> Actually, benchmarks for normal, fully allocated images would be a
>>> little more interesting. Though I'm not sure if qcow2 actually performs
>>> well enough that we'll see any difference even there (when we measured
>>> memory allocation overhead etc. for raw images in the context of
>>> comparing coroutines to AIO callback styes, the difference was already
>>> hard to see).
>>
>> Ok, I'll benchmark it and/or improve fast path (you mean code path,
>> where we skip coroutines creation, to handle the whole request at once,
>> yes?).
>
> I had less the specific code path in mind, but the case of reading
> unallocated clusters or reading/writing an already allocated area of
> normal or zero clusters (no compression, no encrpytion, no other fancy
> stuff). These are the cases that qcow2 images will hit the most in
> practice.
There are already some benchmarks in the thread started from the cover
letter of the series.
>
>> Hmm, all this staff with hd_qiov's in many places is due to we don't
>> have qiov_offset parameter in .bdrv_co_preadv.. Is it a bad idea to
>> add it?
>
> I actually thought the same yesterday, though I'm not completely sure
> yet about it. It makes the interfaces a bit uglier, but it could indeed
> save us some work in the I/O path, so unless we find bigger reasons
> against it, maybe we should do that.
>
>> Anyway, I now think, it's better to start with decompress-threads
>> series, as compress-threads are already merged, then bring encryption to
>> threads too, and then return to these series. This way will remove some
>> questions about allocation, and may be it would be simpler and more
>> consistent to bring more things to coroutines, not only normal clusters.
>
> Okay, we can do that.
>
> Kevin
>
--
Best regards,
Vladimir
Re: [Qemu-block] [PATCH 3/7] qcow2: split out reading normal clusters from qcow2_co_preadv
Am 08.11.2018 um 11:02 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 07.11.2018 21:16, Kevin Wolf wrote:
> > (Broken quoting in text/plain again)
> >
> > Am 01.11.2018 um 13:17 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> 27.09.2018 20:35, Max Reitz wrote:
> >>
> >> On 07.08.18 19:43, Vladimir Sementsov-Ogievskiy wrote:
> >>
> >> Memory allocation may become less efficient for encrypted case.
> >> It's a
> >> payment for further asynchronous scheme.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy
> >>
> >> ---
> >> block/qcow2.c | 114
> >> --
> >> 1 file changed, 64 insertions(+), 50 deletions(-)
> >>
> >> diff --git a/block/qcow2.c b/block/qcow2.c
> >> index 65e3ca00e2..5e7f2ee318 100644
> >> --- a/block/qcow2.c
> >> +++ b/block/qcow2.c
> >> @@ -1808,6 +1808,67 @@ out:
> >> return ret;
> >> }
> >>
> >> +static coroutine_fn int qcow2_co_preadv_normal(BlockDriverState
> >> *bs,
> >> + uint64_t
> >> file_cluster_offset,
> >> + uint64_t offset,
> >> + uint64_t bytes,
> >> + QEMUIOVector
> >> *qiov,
> >> + uint64_t
> >> qiov_offset)
> >> +{
> >> +int ret;
> >> +BDRVQcow2State *s = bs->opaque;
> >> +void *crypt_buf = NULL;
> >> +QEMUIOVector hd_qiov;
> >> +int offset_in_cluster = offset_into_cluster(s, offset);
> >> +
> >> +if ((file_cluster_offset & 511) != 0) {
> >> +return -EIO;
> >> +}
> >> +
> >> +qemu_iovec_init(&hd_qiov, qiov->niov);
> >>
> >> So you're not just re-allocating the bounce buffer for every single
> >> call, but also this I/O vector. Hm.
> > And this one is actually at least a little more serious, I think.
> >
> > Decryption and decompression (including copying between the original
> > qiov and the bounce buffer) are already very slow, so allocating a
> > bounce buffer or not shouldn't make any noticable difference.
> >
> > But I'd like to keep allocations out of the fast path as much as we can.
> >
> >> +if (bs->encrypted) {
> >> +assert(s->crypto);
> >> +assert(bytes <= QCOW_MAX_CRYPT_CLUSTERS *
> >> s->cluster_size);
> >> +
> >> +crypt_buf = qemu_try_blockalign(bs->file->bs, bytes);
> >>
> >> 1. Why did you remove the comment?
> >>
> >> 2. The check for whether crypt_buf was successfully allocated is
> >> missing.
> >>
> >> 3. Do you have any benchmarks for encrypted images? Having to
> >> allocate
> >> the bounce buffer for potentially every single cluster sounds rather
> >> bad
> >> to me.
> > Actually, benchmarks for normal, fully allocated images would be a
> > little more interesting. Though I'm not sure if qcow2 actually performs
> > well enough that we'll see any difference even there (when we measured
> > memory allocation overhead etc. for raw images in the context of
> > comparing coroutines to AIO callback styes, the difference was already
> > hard to see).
>
> Ok, I'll benchmark it and/or improve fast path (you mean code path,
> where we skip coroutines creation, to handle the whole request at once,
> yes?).
I had less the specific code path in mind, but the case of reading
unallocated clusters or reading/writing an already allocated area of
normal or zero clusters (no compression, no encrpytion, no other fancy
stuff). These are the cases that qcow2 images will hit the most in
practice.
> Hmm, all this staff with hd_qiov's in many places is due to we don't
> have qiov_offset parameter in .bdrv_co_preadv.. Is it a bad idea to
> add it?
I actually thought the same yesterday, though I'm not completely sure
yet about it. It makes the interfaces a bit uglier, but it could indeed
save us some work in the I/O path, so unless we find bigger reasons
against it, maybe we should do that.
> Anyway, I now think, it's better to start with decompress-threads
> series, as compress-threads are already merged, then bring encryption to
> threads too, and then return to these series. This way will remove some
> questions about allocation, and may be it would be simpler and more
> consistent to bring more things to coroutines, not only normal clusters.
Okay, we can do that.
Kevin
Re: [Qemu-block] [PATCH 3/7] qcow2: split out reading normal clusters from qcow2_co_preadv
07.11.2018 21:16, Kevin Wolf wrote:
> (Broken quoting in text/plain again)
>
> Am 01.11.2018 um 13:17 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 27.09.2018 20:35, Max Reitz wrote:
>>
>> On 07.08.18 19:43, Vladimir Sementsov-Ogievskiy wrote:
>>
>> Memory allocation may become less efficient for encrypted case.
>> It's a
>> payment for further asynchronous scheme.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>
>> ---
>> block/qcow2.c | 114
>> --
>> 1 file changed, 64 insertions(+), 50 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 65e3ca00e2..5e7f2ee318 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1808,6 +1808,67 @@ out:
>> return ret;
>> }
>>
>> +static coroutine_fn int qcow2_co_preadv_normal(BlockDriverState
>> *bs,
>> + uint64_t
>> file_cluster_offset,
>> + uint64_t offset,
>> + uint64_t bytes,
>> + QEMUIOVector *qiov,
>> + uint64_t
>> qiov_offset)
>> +{
>> +int ret;
>> +BDRVQcow2State *s = bs->opaque;
>> +void *crypt_buf = NULL;
>> +QEMUIOVector hd_qiov;
>> +int offset_in_cluster = offset_into_cluster(s, offset);
>> +
>> +if ((file_cluster_offset & 511) != 0) {
>> +return -EIO;
>> +}
>> +
>> +qemu_iovec_init(&hd_qiov, qiov->niov);
>>
>> So you're not just re-allocating the bounce buffer for every single
>> call, but also this I/O vector. Hm.
> And this one is actually at least a little more serious, I think.
>
> Decryption and decompression (including copying between the original
> qiov and the bounce buffer) are already very slow, so allocating a
> bounce buffer or not shouldn't make any noticable difference.
>
> But I'd like to keep allocations out of the fast path as much as we can.
>
>> +if (bs->encrypted) {
>> +assert(s->crypto);
>> +assert(bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
>> +
>> +crypt_buf = qemu_try_blockalign(bs->file->bs, bytes);
>>
>> 1. Why did you remove the comment?
>>
>> 2. The check for whether crypt_buf was successfully allocated is
>> missing.
>>
>> 3. Do you have any benchmarks for encrypted images? Having to allocate
>> the bounce buffer for potentially every single cluster sounds rather bad
>> to me.
> Actually, benchmarks for normal, fully allocated images would be a
> little more interesting. Though I'm not sure if qcow2 actually performs
> well enough that we'll see any difference even there (when we measured
> memory allocation overhead etc. for raw images in the context of
> comparing coroutines to AIO callback styes, the difference was already
> hard to see).
Ok, I'll benchmark it and/or improve fast path (you mean code path,
where we skip coroutines creation, to handle the whole request at once,
yes?).
Hmm, all this staff with hd_qiov's in many places is due to we don't
have qiov_offset parameter in .bdrv_co_preadv.. Is it a bad idea to add it?
Anyway, I now think, it's better to start with decompress-threads
series, as compress-threads are already merged, then bring encryption to
threads too, and then return to these series. This way will remove some
questions about allocation, and may be it would be simpler and more
consistent to bring more things to coroutines, not only normal clusters.
>
> Kevin
--
Best regards,
Vladimir
Re: [Qemu-block] [PATCH v5 0/3] file-posix: Simplifications on image locking
Am 11.10.2018 um 09:21 hat Fam Zheng geschrieben: > v5: Address Max's comments (Thanks for reviewing): > - Clean up after test done. > - Add rev-by to patch 1 and 2. > > v4: Fix test on systems without OFD. [Patchew] > > The first patch reduces chances of QEMU crash in unusual (but not unlikely) > cases especially when used by Libvirt (see commit message). > > The second patch halves fd for images. > > The third adds some more test for patch one (would have caught the regression > caused by v2). Thanks, applied to the block branch. Kevin
Re: [Qemu-block] [Qemu-devel] [PATCH] tests: Fix Python 3 detection on older GNU make versions
Hi Markus, Le jeu. 8 nov. 2018 09:46, Markus Armbruster a écrit : > Cleber Rosa writes: > > > On 11/7/18 1:05 AM, Markus Armbruster wrote: > >> Eduardo Habkost writes: > >> > >>> The $(SHELLSTATUS) variable requires GNU make >= 4.2, but Travis > >>> seems to provide an older version. Change the existing rules to > >>> use command output instead of exit code, to make it compatible > >>> with older GNU make versions. > >>> > >>> Signed-off-by: Eduardo Habkost > >>> --- > >>> I think that's the cause of the Travis failures. I have > >>> submitted a test job right now, at: > >>> https://travis-ci.org/ehabkost/qemu-hacks/jobs/451387962 > >>> Let's see if it fixes the issue. > >>> --- > >>> tests/Makefile.include | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/tests/Makefile.include b/tests/Makefile.include > >>> index d2e577eabb..074eece558 100644 > >>> --- a/tests/Makefile.include > >>> +++ b/tests/Makefile.include > >>> @@ -913,8 +913,8 @@ TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results > >>> # information please refer to "avocado --help". > >>> AVOCADO_SHOW=none > >>> > >>> -$(shell $(PYTHON) -c 'import sys; assert sys.version_info >= (3,0)' > >/dev/null 2>&1) > >>> -ifeq ($(.SHELLSTATUS),0) > >>> +PYTHON3 = $(shell $(PYTHON) -c 'import sys; print(1 if > sys.version_info >= (3, 0) else 0)') > >>> +ifeq ($(PYTHON3), 1) > >>> $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) > >>> $(call quiet-command, \ > >>> $(PYTHON) -m venv --system-site-packages $@, \ > >> > >> PEP 394 recommends software distributions install Python 3 into the > >> default path as python3, and users use that instead of python, except > >> for programs that are source compatible with both 2 and 3. So, is > >> finding out whether python is a Python 3 really appropriate? Why can't > >> we just use python3 and be done with it? > >> > > > > I mentioned that before, when you pointed out the issue you fix here, > > that configure may be the best place to get the Python version (not only > > the major version) and make it available elsewhere. Even if not used > > for other purposes, it is IMO important information to show on the > > resulting "configure" output. > > > > I'm sending patches to do that in a few. > > > >> If we can't: isn't this a configure problem? > >> > > > > I believe adhering to PEP394 is a good thing, but even that document > > recognizes that the real world is not a perfect place: "however, end > > users should be aware that python refers to python3 on at least Arch > > Linux". And it only covers *nix systems, so if we hope to care for > > non-*nix systems, we have to check the Python version manually. > > > > So, I guess the safest approach from QEMU's side is to check for the > > version indeed. > > If somebody can point to a system people still use where python3 doesn't > get you a Python 3, but python does, catering for such (crappy) systems > in configure makes sense. Until then, it's a waste of brain waves and > configure run time. > > PEP 394 mentions Arch Linux. It's been seven years. What's the most > recent version of Arch Linux that's still crappy in this regard? > Arch doesn't provide python2 by default, thus python points to python3. I think what's crappy is scripts expecting python to be python2. >
Re: [Qemu-block] [Qemu-devel] [PATCH] tests: Fix Python 3 detection on older GNU make versions
Cleber Rosa writes: > On 11/7/18 1:05 AM, Markus Armbruster wrote: >> Eduardo Habkost writes: >> >>> The $(SHELLSTATUS) variable requires GNU make >= 4.2, but Travis >>> seems to provide an older version. Change the existing rules to >>> use command output instead of exit code, to make it compatible >>> with older GNU make versions. >>> >>> Signed-off-by: Eduardo Habkost >>> --- >>> I think that's the cause of the Travis failures. I have >>> submitted a test job right now, at: >>> https://travis-ci.org/ehabkost/qemu-hacks/jobs/451387962 >>> Let's see if it fixes the issue. >>> --- >>> tests/Makefile.include | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/tests/Makefile.include b/tests/Makefile.include >>> index d2e577eabb..074eece558 100644 >>> --- a/tests/Makefile.include >>> +++ b/tests/Makefile.include >>> @@ -913,8 +913,8 @@ TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results >>> # information please refer to "avocado --help". >>> AVOCADO_SHOW=none >>> >>> -$(shell $(PYTHON) -c 'import sys; assert sys.version_info >= (3,0)' >>> >/dev/null 2>&1) >>> -ifeq ($(.SHELLSTATUS),0) >>> +PYTHON3 = $(shell $(PYTHON) -c 'import sys; print(1 if sys.version_info >= >>> (3, 0) else 0)') >>> +ifeq ($(PYTHON3), 1) >>> $(TESTS_VENV_DIR): $(TESTS_VENV_REQ) >>> $(call quiet-command, \ >>> $(PYTHON) -m venv --system-site-packages $@, \ >> >> PEP 394 recommends software distributions install Python 3 into the >> default path as python3, and users use that instead of python, except >> for programs that are source compatible with both 2 and 3. So, is >> finding out whether python is a Python 3 really appropriate? Why can't >> we just use python3 and be done with it? >> > > I mentioned that before, when you pointed out the issue you fix here, > that configure may be the best place to get the Python version (not only > the major version) and make it available elsewhere. Even if not used > for other purposes, it is IMO important information to show on the > resulting "configure" output. > > I'm sending patches to do that in a few. > >> If we can't: isn't this a configure problem? >> > > I believe adhering to PEP394 is a good thing, but even that document > recognizes that the real world is not a perfect place: "however, end > users should be aware that python refers to python3 on at least Arch > Linux". And it only covers *nix systems, so if we hope to care for > non-*nix systems, we have to check the Python version manually. > > So, I guess the safest approach from QEMU's side is to check for the > version indeed. If somebody can point to a system people still use where python3 doesn't get you a Python 3, but python does, catering for such (crappy) systems in configure makes sense. Until then, it's a waste of brain waves and configure run time. PEP 394 mentions Arch Linux. It's been seven years. What's the most recent version of Arch Linux that's still crappy in this regard?
