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
---
Tim Smith (3):
Improve xen_disk batching behaviour
Improve xen_disk response latency
Avoid repeated memory allocation in xen_disk
hw/block/xen_disk.c | 82 +++++++++++++++++++++++++++++----------------------
1 file changed, 46 insertions(+), 36 deletions(-)
--
Tim Smith <tim.smith@citrix.com>
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? Kevin
> -----Original Message----- > From: Kevin Wolf [mailto:kwolf@redhat.com] > Sent: 02 November 2018 11:04 > To: Tim Smith <tim.smith@citrix.com> > Cc: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org; qemu- > block@nongnu.org; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant > <Paul.Durrant@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; > Max Reitz <mreitz@redhat.com>; armbru@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. Paul > Kevin
Am 02.11.2018 um 12:13 hat Paul Durrant geschrieben: > > -----Original Message----- > > From: Kevin Wolf [mailto:kwolf@redhat.com] > > Sent: 02 November 2018 11:04 > > To: Tim Smith <tim.smith@citrix.com> > > Cc: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org; qemu- > > block@nongnu.org; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant > > <Paul.Durrant@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; > > Max Reitz <mreitz@redhat.com>; armbru@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. Thanks a lot, Paul! This is good news. Kevin
Paul Durrant <Paul.Durrant@citrix.com> writes: >> -----Original Message----- >> From: Kevin Wolf [mailto:kwolf@redhat.com] >> Sent: 02 November 2018 11:04 >> To: Tim Smith <tim.smith@citrix.com> >> Cc: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org; qemu- >> block@nongnu.org; Anthony Perard <anthony.perard@citrix.com>; Paul Durrant >> <Paul.Durrant@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; >> Max Reitz <mreitz@redhat.com>; armbru@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.
> -----Original Message----- > From: Markus Armbruster [mailto:armbru@redhat.com] > Sent: 05 November 2018 15:58 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: 'Kevin Wolf' <kwolf@redhat.com>; Tim Smith <tim.smith@citrix.com>; > Stefano Stabellini <sstabellini@kernel.org>; qemu-block@nongnu.org; qemu- > devel@nongnu.org; Max Reitz <mreitz@redhat.com>; Anthony Perard > <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org > Subject: Re: [Qemu-devel] xen_disk qdevification > > Paul Durrant <Paul.Durrant@citrix.com> writes: > > >> -----Original Message----- > >> From: Kevin Wolf [mailto:kwolf@redhat.com] > >> Sent: 02 November 2018 11:04 > >> To: Tim Smith <tim.smith@citrix.com> > >> Cc: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org; qemu- > >> block@nongnu.org; Anthony Perard <anthony.perard@citrix.com>; Paul > Durrant > >> <Paul.Durrant@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; > >> Max Reitz <mreitz@redhat.com>; armbru@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. Sure. I have already spoken to Anthony. Thanks, Paul
> -----Original Message----- > From: Markus Armbruster [mailto:armbru@redhat.com] > Sent: 05 November 2018 15:58 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: 'Kevin Wolf' <kwolf@redhat.com>; Tim Smith <tim.smith@citrix.com>; > Stefano Stabellini <sstabellini@kernel.org>; qemu-block@nongnu.org; qemu- > devel@nongnu.org; Max Reitz <mreitz@redhat.com>; Anthony Perard > <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org > Subject: Re: [Qemu-devel] xen_disk qdevification > > Paul Durrant <Paul.Durrant@citrix.com> writes: > > >> -----Original Message----- > >> From: Kevin Wolf [mailto:kwolf@redhat.com] > >> Sent: 02 November 2018 11:04 > >> To: Tim Smith <tim.smith@citrix.com> > >> Cc: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org; qemu- > >> block@nongnu.org; Anthony Perard <anthony.perard@citrix.com>; Paul > Durrant > >> <Paul.Durrant@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; > >> Max Reitz <mreitz@redhat.com>; armbru@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
Am 08.11.2018 um 15:00 hat Paul Durrant geschrieben: > > -----Original Message----- > > From: Markus Armbruster [mailto:armbru@redhat.com] > > Sent: 05 November 2018 15:58 > > To: Paul Durrant <Paul.Durrant@citrix.com> > > Cc: 'Kevin Wolf' <kwolf@redhat.com>; Tim Smith <tim.smith@citrix.com>; > > Stefano Stabellini <sstabellini@kernel.org>; qemu-block@nongnu.org; qemu- > > devel@nongnu.org; Max Reitz <mreitz@redhat.com>; Anthony Perard > > <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org > > Subject: Re: [Qemu-devel] xen_disk qdevification > > > > Paul Durrant <Paul.Durrant@citrix.com> writes: > > > > >> -----Original Message----- > > >> From: Kevin Wolf [mailto:kwolf@redhat.com] > > >> Sent: 02 November 2018 11:04 > > >> To: Tim Smith <tim.smith@citrix.com> > > >> Cc: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org; qemu- > > >> block@nongnu.org; Anthony Perard <anthony.perard@citrix.com>; Paul > > Durrant > > >> <Paul.Durrant@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; > > >> Max Reitz <mreitz@redhat.com>; armbru@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
> -----Original Message----- > From: Kevin Wolf [mailto:kwolf@redhat.com] > Sent: 08 November 2018 15:21 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: 'Markus Armbruster' <armbru@redhat.com>; Anthony Perard > <anthony.perard@citrix.com>; Tim Smith <tim.smith@citrix.com>; Stefano > Stabellini <sstabellini@kernel.org>; qemu-block@nongnu.org; qemu- > devel@nongnu.org; Max Reitz <mreitz@redhat.com>; xen- > devel@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:armbru@redhat.com] > > > Sent: 05 November 2018 15:58 > > > To: Paul Durrant <Paul.Durrant@citrix.com> > > > Cc: 'Kevin Wolf' <kwolf@redhat.com>; Tim Smith <tim.smith@citrix.com>; > > > Stefano Stabellini <sstabellini@kernel.org>; qemu-block@nongnu.org; > qemu- > > > devel@nongnu.org; Max Reitz <mreitz@redhat.com>; Anthony Perard > > > <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org > > > Subject: Re: [Qemu-devel] xen_disk qdevification > > > > > > Paul Durrant <Paul.Durrant@citrix.com> writes: > > > > > > >> -----Original Message----- > > > >> From: Kevin Wolf [mailto:kwolf@redhat.com] > > > >> Sent: 02 November 2018 11:04 > > > >> To: Tim Smith <tim.smith@citrix.com> > > > >> Cc: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org; qemu- > > > >> block@nongnu.org; Anthony Perard <anthony.perard@citrix.com>; Paul > > > Durrant > > > >> <Paul.Durrant@citrix.com>; Stefano Stabellini > <sstabellini@kernel.org>; > > > >> Max Reitz <mreitz@redhat.com>; armbru@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 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. > I tried that and it works fine :-) > > 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. > Yes, that does indeed seem like more of a hack. The first option works so I'll go with that. Thanks for your help. Cheers, Paul > Kevin
> -----Original Message----- > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf > Of Paul Durrant > Sent: 08 November 2018 15:44 > To: 'Kevin Wolf' <kwolf@redhat.com> > Cc: Stefano Stabellini <sstabellini@kernel.org>; qemu-block@nongnu.org; > Tim Smith <tim.smith@citrix.com>; qemu-devel@nongnu.org; 'Markus > Armbruster' <armbru@redhat.com>; Anthony Perard > <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org; Max Reitz > <mreitz@redhat.com> > Subject: Re: [Xen-devel] [Qemu-devel] xen_disk qdevification > > > -----Original Message----- > > From: Kevin Wolf [mailto:kwolf@redhat.com] > > Sent: 08 November 2018 15:21 > > To: Paul Durrant <Paul.Durrant@citrix.com> > > Cc: 'Markus Armbruster' <armbru@redhat.com>; Anthony Perard > > <anthony.perard@citrix.com>; Tim Smith <tim.smith@citrix.com>; Stefano > > Stabellini <sstabellini@kernel.org>; qemu-block@nongnu.org; qemu- > > devel@nongnu.org; Max Reitz <mreitz@redhat.com>; xen- > > devel@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:armbru@redhat.com] > > > > Sent: 05 November 2018 15:58 > > > > To: Paul Durrant <Paul.Durrant@citrix.com> > > > > Cc: 'Kevin Wolf' <kwolf@redhat.com>; Tim Smith > <tim.smith@citrix.com>; > > > > Stefano Stabellini <sstabellini@kernel.org>; qemu-block@nongnu.org; > > qemu- > > > > devel@nongnu.org; Max Reitz <mreitz@redhat.com>; Anthony Perard > > > > <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org > > > > Subject: Re: [Qemu-devel] xen_disk qdevification > > > > > > > > Paul Durrant <Paul.Durrant@citrix.com> writes: > > > > > > > > >> -----Original Message----- > > > > >> From: Kevin Wolf [mailto:kwolf@redhat.com] > > > > >> Sent: 02 November 2018 11:04 > > > > >> To: Tim Smith <tim.smith@citrix.com> > > > > >> Cc: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org; qemu- > > > > >> block@nongnu.org; Anthony Perard <anthony.perard@citrix.com>; > Paul > > > > Durrant > > > > >> <Paul.Durrant@citrix.com>; Stefano Stabellini > > <sstabellini@kernel.org>; > > > > >> Max Reitz <mreitz@redhat.com>; armbru@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 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. > > > > I tried that and it works fine :-) Unfortunately I spoke too soon... I still had a patch in place to disable locking checks :-( What I'm trying to do to maintain compatibility with the existing Xen toolstack (which I think is the only feasible way to make the change avoiding chicken and egg problems) is to use a 'compat' function that creates a drive based on the information that the Xen toolstack writes into xenstore. I'm using drive_new() to do this and it is this that fails. So, I have tried setting BDRV_OPT_READ_ONLY and BDRV_OPT_FORCE_SHARE. This allows me to get through drive_new() but later I fail to set the write permission with error "Block node is read-only". > > > > > 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. > > > > Yes, that does indeed seem like more of a hack. The first option works so > I'll go with that. > I'll now see what I can do with this idea. Paul
> -----Original Message----- > From: Paul Durrant > Sent: 08 November 2018 16:44 > To: Paul Durrant <Paul.Durrant@citrix.com>; 'Kevin Wolf' > <kwolf@redhat.com> > Cc: Stefano Stabellini <sstabellini@kernel.org>; qemu-block@nongnu.org; > Tim Smith <tim.smith@citrix.com>; qemu-devel@nongnu.org; 'Markus > Armbruster' <armbru@redhat.com>; Anthony Perard > <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org; Max Reitz > <mreitz@redhat.com> > Subject: RE: [Qemu-devel] xen_disk qdevification > > > -----Original Message----- > > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On > Behalf > > Of Paul Durrant > > Sent: 08 November 2018 15:44 > > To: 'Kevin Wolf' <kwolf@redhat.com> > > Cc: Stefano Stabellini <sstabellini@kernel.org>; qemu-block@nongnu.org; > > Tim Smith <tim.smith@citrix.com>; qemu-devel@nongnu.org; 'Markus > > Armbruster' <armbru@redhat.com>; Anthony Perard > > <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org; Max Reitz > > <mreitz@redhat.com> > > Subject: Re: [Xen-devel] [Qemu-devel] xen_disk qdevification > > > > > -----Original Message----- > > > From: Kevin Wolf [mailto:kwolf@redhat.com] > > > Sent: 08 November 2018 15:21 > > > To: Paul Durrant <Paul.Durrant@citrix.com> > > > Cc: 'Markus Armbruster' <armbru@redhat.com>; Anthony Perard > > > <anthony.perard@citrix.com>; Tim Smith <tim.smith@citrix.com>; Stefano > > > Stabellini <sstabellini@kernel.org>; qemu-block@nongnu.org; qemu- > > > devel@nongnu.org; Max Reitz <mreitz@redhat.com>; xen- > > > devel@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:armbru@redhat.com] > > > > > Sent: 05 November 2018 15:58 > > > > > To: Paul Durrant <Paul.Durrant@citrix.com> > > > > > Cc: 'Kevin Wolf' <kwolf@redhat.com>; Tim Smith > > <tim.smith@citrix.com>; > > > > > Stefano Stabellini <sstabellini@kernel.org>; qemu- > block@nongnu.org; > > > qemu- > > > > > devel@nongnu.org; Max Reitz <mreitz@redhat.com>; Anthony Perard > > > > > <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org > > > > > Subject: Re: [Qemu-devel] xen_disk qdevification > > > > > > > > > > Paul Durrant <Paul.Durrant@citrix.com> writes: > > > > > > > > > > >> -----Original Message----- > > > > > >> From: Kevin Wolf [mailto:kwolf@redhat.com] > > > > > >> Sent: 02 November 2018 11:04 > > > > > >> To: Tim Smith <tim.smith@citrix.com> > > > > > >> Cc: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org; > qemu- > > > > > >> block@nongnu.org; Anthony Perard <anthony.perard@citrix.com>; > > Paul > > > > > Durrant > > > > > >> <Paul.Durrant@citrix.com>; Stefano Stabellini > > > <sstabellini@kernel.org>; > > > > > >> Max Reitz <mreitz@redhat.com>; armbru@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 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. > > > > > > > I tried that and it works fine :-) > > Unfortunately I spoke too soon... I still had a patch in place to disable > locking checks :-( > > What I'm trying to do to maintain compatibility with the existing Xen > toolstack (which I think is the only feasible way to make the change > avoiding chicken and egg problems) is to use a 'compat' function that > creates a drive based on the information that the Xen toolstack writes > into xenstore. I'm using drive_new() to do this and it is this that fails. > > So, I have tried setting BDRV_OPT_READ_ONLY and BDRV_OPT_FORCE_SHARE. This > allows me to get through drive_new() but later I fail to set the write > permission with error "Block node is read-only". > > > > > > > > > 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. > > > > > > > Yes, that does indeed seem like more of a hack. The first option works > so > > I'll go with that. > > > > I'll now see what I can do with this idea. I think I have a reasonably neat solution, as it is restricted to my compat code and can thus go away when the Xen toolstack is re-educated to use QMP to instantiate PV backends (once they are all qdevified). I simply add "file.locking=off" to the options I pass to drive_new(). Paul > > Paul
Am 09.11.2018 um 11:27 hat Paul Durrant geschrieben: > > -----Original Message----- > > From: Paul Durrant > > Sent: 08 November 2018 16:44 > > To: Paul Durrant <Paul.Durrant@citrix.com>; 'Kevin Wolf' > > <kwolf@redhat.com> > > Cc: Stefano Stabellini <sstabellini@kernel.org>; qemu-block@nongnu.org; > > Tim Smith <tim.smith@citrix.com>; qemu-devel@nongnu.org; 'Markus > > Armbruster' <armbru@redhat.com>; Anthony Perard > > <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org; Max Reitz > > <mreitz@redhat.com> > > Subject: RE: [Qemu-devel] xen_disk qdevification > > > > > -----Original Message----- > > > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On > > Behalf > > > Of Paul Durrant > > > Sent: 08 November 2018 15:44 > > > To: 'Kevin Wolf' <kwolf@redhat.com> > > > Cc: Stefano Stabellini <sstabellini@kernel.org>; qemu-block@nongnu.org; > > > Tim Smith <tim.smith@citrix.com>; qemu-devel@nongnu.org; 'Markus > > > Armbruster' <armbru@redhat.com>; Anthony Perard > > > <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org; Max Reitz > > > <mreitz@redhat.com> > > > Subject: Re: [Xen-devel] [Qemu-devel] xen_disk qdevification > > > > > > > -----Original Message----- > > > > From: Kevin Wolf [mailto:kwolf@redhat.com] > > > > Sent: 08 November 2018 15:21 > > > > To: Paul Durrant <Paul.Durrant@citrix.com> > > > > Cc: 'Markus Armbruster' <armbru@redhat.com>; Anthony Perard > > > > <anthony.perard@citrix.com>; Tim Smith <tim.smith@citrix.com>; Stefano > > > > Stabellini <sstabellini@kernel.org>; qemu-block@nongnu.org; qemu- > > > > devel@nongnu.org; Max Reitz <mreitz@redhat.com>; xen- > > > > devel@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:armbru@redhat.com] > > > > > > Sent: 05 November 2018 15:58 > > > > > > To: Paul Durrant <Paul.Durrant@citrix.com> > > > > > > Cc: 'Kevin Wolf' <kwolf@redhat.com>; Tim Smith > > > <tim.smith@citrix.com>; > > > > > > Stefano Stabellini <sstabellini@kernel.org>; qemu- > > block@nongnu.org; > > > > qemu- > > > > > > devel@nongnu.org; Max Reitz <mreitz@redhat.com>; Anthony Perard > > > > > > <anthony.perard@citrix.com>; xen-devel@lists.xenproject.org > > > > > > Subject: Re: [Qemu-devel] xen_disk qdevification > > > > > > > > > > > > Paul Durrant <Paul.Durrant@citrix.com> writes: > > > > > > > > > > > > >> -----Original Message----- > > > > > > >> From: Kevin Wolf [mailto:kwolf@redhat.com] > > > > > > >> Sent: 02 November 2018 11:04 > > > > > > >> To: Tim Smith <tim.smith@citrix.com> > > > > > > >> Cc: xen-devel@lists.xenproject.org; qemu-devel@nongnu.org; > > qemu- > > > > > > >> block@nongnu.org; Anthony Perard <anthony.perard@citrix.com>; > > > Paul > > > > > > Durrant > > > > > > >> <Paul.Durrant@citrix.com>; Stefano Stabellini > > > > <sstabellini@kernel.org>; > > > > > > >> Max Reitz <mreitz@redhat.com>; armbru@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 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. > > > > > > > > > > I tried that and it works fine :-) > > > > Unfortunately I spoke too soon... I still had a patch in place to disable > > locking checks :-( > > > > What I'm trying to do to maintain compatibility with the existing Xen > > toolstack (which I think is the only feasible way to make the change > > avoiding chicken and egg problems) is to use a 'compat' function that > > creates a drive based on the information that the Xen toolstack writes > > into xenstore. I'm using drive_new() to do this and it is this that fails. > > > > So, I have tried setting BDRV_OPT_READ_ONLY and BDRV_OPT_FORCE_SHARE. This > > allows me to get through drive_new() but later I fail to set the write > > permission with error "Block node is read-only". drive_new() is really a legacy interface. It immediately creates a BlockBackend and takes permissions for it. You don't want that here. (And I'd like it to go away in a few releases, so better don't let new code rely on it.) If you can, it would be better to just call qmp_blockdev_add(). This creates only a node (BlockDriverState) without a BlockBackend. You'll get your BlockBackend from the qdev drive property. > > > > 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. > > > > > > > > > > Yes, that does indeed seem like more of a hack. The first option works > > so > > > I'll go with that. > > > > > > > I'll now see what I can do with this idea. > > I think I have a reasonably neat solution, as it is restricted to my > compat code and can thus go away when the Xen toolstack is re-educated > to use QMP to instantiate PV backends (once they are all qdevified). I > simply add "file.locking=off" to the options I pass to drive_new(). I wouldn't agree on "neat", but if you think so... Kevin
On Fri, Nov 02, Kevin Wolf wrote: > 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. Do you have that backwards by any chance? IMO the presence of assert() contributes to bad code quality, not the drivers that trigger those asserts. It is bad enough that two QEMU releases went out while being in bad shape. Anyway, hopefully Paul or whoever will find the time and energy to convert the code at some point. Olaf
> -----Original Message----- > From: Olaf Hering [mailto:olaf@aepfle.de] > Sent: 12 December 2018 09:00 > To: Kevin Wolf <kwolf@redhat.com> > Cc: Tim Smith <tim.smith@citrix.com>; Stefano Stabellini > <sstabellini@kernel.org>; qemu-block@nongnu.org; armbru@redhat.com; qemu- > devel@nongnu.org; Max Reitz <mreitz@redhat.com>; Paul Durrant > <Paul.Durrant@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; > xen-devel@lists.xenproject.org > Subject: Re: [Xen-devel] xen_disk qdevification (was: [PATCH 0/3] > Performance improvements for xen_disk v2) > > On Fri, Nov 02, Kevin Wolf wrote: > > > 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. > > Do you have that backwards by any chance? IMO the presence of assert() > contributes to bad code quality, not the drivers that trigger those > asserts. It is bad enough that two QEMU releases went out while being in > bad shape. > > Anyway, hopefully Paul or whoever will find the time and energy to > convert the code at some point. It's done. V4 of my series has acks from the Xen maintainers. I think it needs some other acks from block maintainers but it's basically ready to go in (and I've verified that no assert is tripped by xentop at least). Also I hope to post the re-based patches from Tim (one of which fixes the memory issues) later today. Paul > > Olaf
Am 12.12.2018 um 09:59 hat Olaf Hering geschrieben: > On Fri, Nov 02, Kevin Wolf wrote: > > > 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. > > Do you have that backwards by any chance? IMO the presence of assert() > contributes to bad code quality, not the drivers that trigger those > asserts. You like shooting the messenger, it seems? Bugs aren't bad, only catching them is? But anyway, in this case, I seem to remember it was a plain old segfault, not a failed assertion. Kevin
Olaf Hering <olaf@aepfle.de> writes: > On Fri, Nov 02, Kevin Wolf wrote: > >> 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. > > Do you have that backwards by any chance? IMO the presence of assert() > contributes to bad code quality, not the drivers that trigger those > asserts. It is bad enough that two QEMU releases went out while being in > bad shape. Converting block devices to the qdev infrastructure (introduced in 2009) has been a longwinded affair. We've repeatedly reminded people in charge of the stragglers to update their code. Neglecting to update code to current infrastructure creates a burden and a risk. The burden is on the maintainers of the infrastructure: they get to drag along outmoded infrastructure. The risk is on the users of the code: it becomes a special case, and eventually acquires its special bugs. Putting the blame for them entirely on the maintainers of the infrastructure is not fair. > Anyway, hopefully Paul or whoever will find the time and energy to > convert the code at some point. Yes, Paul's taking care of it. Much appreciated.
© 2016 - 2024 Red Hat, Inc.