| Message ID | 6ad371df66eecffccce29a0bd14695f40edd6120.1548171741.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iothread-related fixes for virtio-scsi | expand |
On 22/01/19 16:53, Alberto Garcia wrote: > This patch forbids attaching a disk to a SCSI device if its using a > different AioContext. Test case included. > > Signed-off-by: Alberto Garcia <berto@igalia.com> Should this be handled in common code? However, everything else looks good. Thanks! Paolo > --- > hw/scsi/virtio-scsi.c | 7 +++++++ > tests/qemu-iotests/240 | 22 ++++++++++++++++++++++ > tests/qemu-iotests/240.out | 20 ++++++++++++++++++++ > 3 files changed, 49 insertions(+) > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c > index e1f7b208c7..eb90288f47 100644 > --- a/hw/scsi/virtio-scsi.c > +++ b/hw/scsi/virtio-scsi.c > @@ -791,9 +791,16 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, > SCSIDevice *sd = SCSI_DEVICE(dev); > > if (s->ctx && !s->dataplane_fenced) { > + AioContext *ctx; > if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) { > return; > } > + ctx = blk_get_aio_context(sd->conf.blk); > + if (ctx != s->ctx && ctx != qemu_get_aio_context()) { > + error_setg(errp, "Cannot attach a blockdev that is using " > + "a different iothread"); > + return; > + } > virtio_scsi_acquire(s); > blk_set_aio_context(sd->conf.blk, s->ctx); > virtio_scsi_release(s); > diff --git a/tests/qemu-iotests/240 b/tests/qemu-iotests/240 > index 5d499c9a00..65cc3b39b1 100755 > --- a/tests/qemu-iotests/240 > +++ b/tests/qemu-iotests/240 > @@ -101,6 +101,28 @@ run_qemu <<EOF > { "execute": "quit"} > EOF > > +echo > +echo === Attach two SCSI disks using the same block device but different iothreads === > +echo > + > +run_qemu <<EOF > +{ "execute": "qmp_capabilities" } > +{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "hd0", "read-only": true}} > +{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread0"}} > +{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread1"}} > +{ "execute": "device_add", "arguments": {"id": "scsi0", "driver": "${virtio_scsi}", "iothread": "iothread0"}} > +{ "execute": "device_add", "arguments": {"id": "scsi1", "driver": "${virtio_scsi}", "iothread": "iothread1"}} > +{ "execute": "device_add", "arguments": {"id": "scsi-hd0", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi0.0"}} > +{ "execute": "device_add", "arguments": {"id": "scsi-hd1", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi1.0"}} > +{ "execute": "device_del", "arguments": {"id": "scsi-hd0"}} > +{ "execute": "device_add", "arguments": {"id": "scsi-hd1", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi1.0"}} > +{ "execute": "device_del", "arguments": {"id": "scsi-hd1"}} > +{ "execute": "device_del", "arguments": {"id": "scsi0"}} > +{ "execute": "device_del", "arguments": {"id": "scsi1"}} > +{ "execute": "blockdev-del", "arguments": {"node-name": "hd0"}} > +{ "execute": "quit"} > +EOF > + > # success, all done > echo "*** done" > rm -f $seq.full > diff --git a/tests/qemu-iotests/240.out b/tests/qemu-iotests/240.out > index 701cb5c7d2..d76392966c 100644 > --- a/tests/qemu-iotests/240.out > +++ b/tests/qemu-iotests/240.out > @@ -31,4 +31,24 @@ QMP_VERSION > {"return": {}} > {"return": {}} > {"return": {}} > + > +=== Attach two SCSI disks using the same block device but different iothreads === > + > +Testing: > +QMP_VERSION > +{"return": {}} > +{"return": {}} > +{"return": {}} > +{"return": {}} > +{"return": {}} > +{"return": {}} > +{"return": {}} > +{"error": {"class": "GenericError", "desc": "Cannot attach a blockdev that is using a different iothread"}} > +{"return": {}} > +{"return": {}} > +{"return": {}} > +{"return": {}} > +{"return": {}} > +{"return": {}} > +{"return": {}} > *** done >
On Wed 23 Jan 2019 10:46:54 AM CET, Paolo Bonzini wrote: > On 22/01/19 16:53, Alberto Garcia wrote: >> This patch forbids attaching a disk to a SCSI device if its using a >> different AioContext. Test case included. >> >> Signed-off-by: Alberto Garcia <berto@igalia.com> > > Should this be handled in common code? However, everything else looks > good. Thanks! You mean a common function with the code below? >> + ctx = blk_get_aio_context(sd->conf.blk); >> + if (ctx != s->ctx && ctx != qemu_get_aio_context()) { >> + error_setg(errp, "Cannot attach a blockdev that is using " >> + "a different iothread"); >> + return; >> + } Who else would be the user? Berto
On 23/01/19 16:09, Alberto Garcia wrote: > On Wed 23 Jan 2019 10:46:54 AM CET, Paolo Bonzini wrote: >> On 22/01/19 16:53, Alberto Garcia wrote: >>> This patch forbids attaching a disk to a SCSI device if its using a >>> different AioContext. Test case included. >>> >>> Signed-off-by: Alberto Garcia <berto@igalia.com> >> >> Should this be handled in common code? However, everything else looks >> good. Thanks! > > You mean a common function with the code below? > >>> + ctx = blk_get_aio_context(sd->conf.blk); >>> + if (ctx != s->ctx && ctx != qemu_get_aio_context()) { >>> + error_setg(errp, "Cannot attach a blockdev that is using " >>> + "a different iothread"); >>> + return; >>> + } > > Who else would be the user? If it isn't, probably virtio-blk should be using too. Paolo
On Wed 23 Jan 2019 04:47:30 PM CET, Paolo Bonzini wrote: >> You mean a common function with the code below? >> >>>> + ctx = blk_get_aio_context(sd->conf.blk); >>>> + if (ctx != s->ctx && ctx != qemu_get_aio_context()) { >>>> + error_setg(errp, "Cannot attach a blockdev that is using " >>>> + "a different iothread"); >>>> + return; >>>> + } >> >> Who else would be the user? > > If it isn't, probably virtio-blk should be using too. I agree, but that's the problem that I mentioned on the cover letter. In the virtio-blk case I don't know how to tell that the block device is using a different iothread during device_add, because the iothread is not set during realize() but by virtio_blk_data_plane_start() that happens only when I reboot the guest. Berto
Am 23.01.2019 um 17:16 hat Alberto Garcia geschrieben: > On Wed 23 Jan 2019 04:47:30 PM CET, Paolo Bonzini wrote: > >> You mean a common function with the code below? > >> > >>>> + ctx = blk_get_aio_context(sd->conf.blk); > >>>> + if (ctx != s->ctx && ctx != qemu_get_aio_context()) { > >>>> + error_setg(errp, "Cannot attach a blockdev that is using " > >>>> + "a different iothread"); > >>>> + return; > >>>> + } > >> > >> Who else would be the user? > > > > If it isn't, probably virtio-blk should be using too. > > I agree, but that's the problem that I mentioned on the cover letter. > > In the virtio-blk case I don't know how to tell that the block device is > using a different iothread during device_add, because the iothread is > not set during realize() but by virtio_blk_data_plane_start() that > happens only when I reboot the guest. I think the proper solution on the block layer level would be that AioContext is managed per BdrvChild and only BdrvChild objects with the same AioContext can be attached to the same node. bdrv_set_aio_context() would then fail if another parent is already using the node. Now, Paolo keeps talking about how AioContexts are going to disappear soon (TM), so I wonder whether it's still worth fixing this properly now? Kevin
On Thu 24 Jan 2019 10:12:22 AM CET, Kevin Wolf wrote: > Am 23.01.2019 um 17:16 hat Alberto Garcia geschrieben: >> On Wed 23 Jan 2019 04:47:30 PM CET, Paolo Bonzini wrote: >> >> You mean a common function with the code below? >> >> >> >>>> + ctx = blk_get_aio_context(sd->conf.blk); >> >>>> + if (ctx != s->ctx && ctx != qemu_get_aio_context()) { >> >>>> + error_setg(errp, "Cannot attach a blockdev that is using " >> >>>> + "a different iothread"); >> >>>> + return; >> >>>> + } >> >> >> >> Who else would be the user? >> > >> > If it isn't, probably virtio-blk should be using too. >> >> I agree, but that's the problem that I mentioned on the cover letter. >> >> In the virtio-blk case I don't know how to tell that the block device >> is using a different iothread during device_add, because the iothread >> is not set during realize() but by virtio_blk_data_plane_start() that >> happens only when I reboot the guest. > > I think the proper solution on the block layer level would be that > AioContext is managed per BdrvChild and only BdrvChild objects with > the same AioContext can be attached to the same node. > bdrv_set_aio_context() would then fail if another parent is already > using the node. How would that solve the virtio-blk problem, though? :-? > Now, Paolo keeps talking about how AioContexts are going to disappear > soon (TM), so I wonder whether it's still worth fixing this properly > now? Either way, should we commit this series already and later update it once we have a proper solution for virtio-blk? Berto
Am 28.01.2019 um 15:42 hat Alberto Garcia geschrieben: > On Thu 24 Jan 2019 10:12:22 AM CET, Kevin Wolf wrote: > > Am 23.01.2019 um 17:16 hat Alberto Garcia geschrieben: > >> On Wed 23 Jan 2019 04:47:30 PM CET, Paolo Bonzini wrote: > >> >> You mean a common function with the code below? > >> >> > >> >>>> + ctx = blk_get_aio_context(sd->conf.blk); > >> >>>> + if (ctx != s->ctx && ctx != qemu_get_aio_context()) { > >> >>>> + error_setg(errp, "Cannot attach a blockdev that is using " > >> >>>> + "a different iothread"); > >> >>>> + return; > >> >>>> + } > >> >> > >> >> Who else would be the user? > >> > > >> > If it isn't, probably virtio-blk should be using too. > >> > >> I agree, but that's the problem that I mentioned on the cover letter. > >> > >> In the virtio-blk case I don't know how to tell that the block device > >> is using a different iothread during device_add, because the iothread > >> is not set during realize() but by virtio_blk_data_plane_start() that > >> happens only when I reboot the guest. > > > > I think the proper solution on the block layer level would be that > > AioContext is managed per BdrvChild and only BdrvChild objects with > > the same AioContext can be attached to the same node. > > bdrv_set_aio_context() would then fail if another parent is already > > using the node. > > How would that solve the virtio-blk problem, though? :-? It wouldn't, but it would make clear that bdrv_set_aio_context() can fail and we'd have to handle the problem exactly there. So probably we need to do something to move the data plane initialisation into realize? The only other option I see is downgrading to non-dataplane mode, but this would have to be silent because during machine reset we have no way to report errors. > > Now, Paolo keeps talking about how AioContexts are going to disappear > > soon (TM), so I wonder whether it's still worth fixing this properly > > now? > > Either way, should we commit this series already and later update it > once we have a proper solution for virtio-blk? Fine with me. Kevin
On Mon 28 Jan 2019 03:57:43 PM CET, Kevin Wolf wrote: >> > I think the proper solution on the block layer level would be that >> > AioContext is managed per BdrvChild and only BdrvChild objects with >> > the same AioContext can be attached to the same node. >> > bdrv_set_aio_context() would then fail if another parent is already >> > using the node. >> >> How would that solve the virtio-blk problem, though? :-? > > It wouldn't, but it would make clear that bdrv_set_aio_context() can > fail and we'd have to handle the problem exactly there. Yes, that sound like the proper solution. > The only other option I see is downgrading to non-dataplane mode, but > this would have to be silent because during machine reset we have no > way to report errors. Yes, I actually had a patch doing that, but as you said the user has no way of knowing that it went wrong. If you have 2 or 3 virtio-blk devices with different iothreads using the same BDS then after the guest reboot only one would succeed, but you can't see which one, or can you? Berto
Am 28.01.2019 um 16:46 hat Alberto Garcia geschrieben: > On Mon 28 Jan 2019 03:57:43 PM CET, Kevin Wolf wrote: > > >> > I think the proper solution on the block layer level would be that > >> > AioContext is managed per BdrvChild and only BdrvChild objects with > >> > the same AioContext can be attached to the same node. > >> > bdrv_set_aio_context() would then fail if another parent is already > >> > using the node. > >> > >> How would that solve the virtio-blk problem, though? :-? > > > > It wouldn't, but it would make clear that bdrv_set_aio_context() can > > fail and we'd have to handle the problem exactly there. > > Yes, that sound like the proper solution. > > > The only other option I see is downgrading to non-dataplane mode, but > > this would have to be silent because during machine reset we have no > > way to report errors. > > Yes, I actually had a patch doing that, but as you said the user has no > way of knowing that it went wrong. > > If you have 2 or 3 virtio-blk devices with different iothreads using the > same BDS then after the guest reboot only one would succeed, but you > can't see which one, or can you? I suppose you could add a QOM property that tells whether the assigned iothread is actually in use, but it would still be quite obscure. And you would have to check explicitly, so I don't think that's a good solution. And actually... if you move the first virtio-blk device to an iothread, then downgrading the others isn't going to save us, because that would still be using the backend from two threads (one successfully enabled iothread, and the main thread). Kevin
On Mon 28 Jan 2019 05:01:09 PM CET, Kevin Wolf wrote: > And actually... if you move the first virtio-blk device to an > iothread, then downgrading the others isn't going to save us, because > that would still be using the backend from two threads (one > successfully enabled iothread, and the main thread). ...which makes me think that, since patch 1 from this series moves the BlockBackend back to the main AioContext on unplug, we could have a similar problem if the BDS is being used by other devices. Or am I missing something? Berto
Am 28.01.2019 um 17:15 hat Alberto Garcia geschrieben: > On Mon 28 Jan 2019 05:01:09 PM CET, Kevin Wolf wrote: > > > And actually... if you move the first virtio-blk device to an > > iothread, then downgrading the others isn't going to save us, because > > that would still be using the backend from two threads (one > > successfully enabled iothread, and the main thread). > > ...which makes me think that, since patch 1 from this series moves the > BlockBackend back to the main AioContext on unplug, we could have a > similar problem if the BDS is being used by other devices. Or am I > missing something? Oops. :-) Maybe we do need to implement the proper solution? Kevin
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index e1f7b208c7..eb90288f47 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -791,9 +791,16 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev, SCSIDevice *sd = SCSI_DEVICE(dev); if (s->ctx && !s->dataplane_fenced) { + AioContext *ctx; if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) { return; } + ctx = blk_get_aio_context(sd->conf.blk); + if (ctx != s->ctx && ctx != qemu_get_aio_context()) { + error_setg(errp, "Cannot attach a blockdev that is using " + "a different iothread"); + return; + } virtio_scsi_acquire(s); blk_set_aio_context(sd->conf.blk, s->ctx); virtio_scsi_release(s); diff --git a/tests/qemu-iotests/240 b/tests/qemu-iotests/240 index 5d499c9a00..65cc3b39b1 100755 --- a/tests/qemu-iotests/240 +++ b/tests/qemu-iotests/240 @@ -101,6 +101,28 @@ run_qemu <<EOF { "execute": "quit"} EOF +echo +echo === Attach two SCSI disks using the same block device but different iothreads === +echo + +run_qemu <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "hd0", "read-only": true}} +{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread0"}} +{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread1"}} +{ "execute": "device_add", "arguments": {"id": "scsi0", "driver": "${virtio_scsi}", "iothread": "iothread0"}} +{ "execute": "device_add", "arguments": {"id": "scsi1", "driver": "${virtio_scsi}", "iothread": "iothread1"}} +{ "execute": "device_add", "arguments": {"id": "scsi-hd0", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi0.0"}} +{ "execute": "device_add", "arguments": {"id": "scsi-hd1", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi1.0"}} +{ "execute": "device_del", "arguments": {"id": "scsi-hd0"}} +{ "execute": "device_add", "arguments": {"id": "scsi-hd1", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi1.0"}} +{ "execute": "device_del", "arguments": {"id": "scsi-hd1"}} +{ "execute": "device_del", "arguments": {"id": "scsi0"}} +{ "execute": "device_del", "arguments": {"id": "scsi1"}} +{ "execute": "blockdev-del", "arguments": {"node-name": "hd0"}} +{ "execute": "quit"} +EOF + # success, all done echo "*** done" rm -f $seq.full diff --git a/tests/qemu-iotests/240.out b/tests/qemu-iotests/240.out index 701cb5c7d2..d76392966c 100644 --- a/tests/qemu-iotests/240.out +++ b/tests/qemu-iotests/240.out @@ -31,4 +31,24 @@ QMP_VERSION {"return": {}} {"return": {}} {"return": {}} + +=== Attach two SCSI disks using the same block device but different iothreads === + +Testing: +QMP_VERSION +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"error": {"class": "GenericError", "desc": "Cannot attach a blockdev that is using a different iothread"}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} +{"return": {}} *** done
This patch forbids attaching a disk to a SCSI device if its using a different AioContext. Test case included. Signed-off-by: Alberto Garcia <berto@igalia.com> --- hw/scsi/virtio-scsi.c | 7 +++++++ tests/qemu-iotests/240 | 22 ++++++++++++++++++++++ tests/qemu-iotests/240.out | 20 ++++++++++++++++++++ 3 files changed, 49 insertions(+)