| Message ID | 1370271492-5259-1-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 03/06/2013 16:58, Stefan Hajnoczi ha scritto: > Paolo Bonzini <pbonzini@redhat.com> suggested the following test case: > > 1. Launch a guest and wait at the GRUB boot menu: > > qemu-system-x86_64 -enable-kvm -m 1024 \ > -drive if=none,cache=none,file=test.img,id=foo,werror=stop,rerror=stop > -device virtio-blk-pci,drive=foo,id=virtio0,addr=4 > > 2. Hot unplug the device: > > (qemu) drive_del foo > > 3. Select the first boot menu entry > > Without this patch the guest pauses due to ENOMEDIUM. But it is not > possible to resolve this situation - the drive has become anonymous. > > With this patch the guest the guest gets the ENOMEDIUM error. > > Note that this scenario actually happens sometimes during libvirt disk > hot unplug, where device_del is followed by drive_del. I/O may still be > submitted to the drive after drive_del if the guest does not process the > PCI hot unplug notification. > > Reported-by: Dafna Ron <dron@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > blockdev.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/blockdev.c b/blockdev.c > index d1ec99a..6eb81a3 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1180,6 +1180,10 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) > */ > if (bdrv_get_attached_dev(bs)) { > bdrv_make_anon(bs); > + > + /* Further I/O must not pause the guest */ > + bdrv_set_on_error(bs, BLOCKDEV_ON_ERROR_REPORT, > + BLOCKDEV_ON_ERROR_REPORT); > } else { > drive_uninit(drive_get_by_blockdev(bs)); > } > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Paolo
Stefan Hajnoczi <stefanha@redhat.com> writes: > Paolo Bonzini <pbonzini@redhat.com> suggested the following test case: > > 1. Launch a guest and wait at the GRUB boot menu: > > qemu-system-x86_64 -enable-kvm -m 1024 \ > -drive if=none,cache=none,file=test.img,id=foo,werror=stop,rerror=stop > -device virtio-blk-pci,drive=foo,id=virtio0,addr=4 > > 2. Hot unplug the device: > > (qemu) drive_del foo > > 3. Select the first boot menu entry > > Without this patch the guest pauses due to ENOMEDIUM. But it is not > possible to resolve this situation - the drive has become anonymous. > > With this patch the guest the guest gets the ENOMEDIUM error. > > Note that this scenario actually happens sometimes during libvirt disk > hot unplug, where device_del is followed by drive_del. I/O may still be > submitted to the drive after drive_del if the guest does not process the > PCI hot unplug notification. > > Reported-by: Dafna Ron <dron@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > blockdev.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/blockdev.c b/blockdev.c > index d1ec99a..6eb81a3 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1180,6 +1180,10 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) > */ > if (bdrv_get_attached_dev(bs)) { > bdrv_make_anon(bs); > + > + /* Further I/O must not pause the guest */ > + bdrv_set_on_error(bs, BLOCKDEV_ON_ERROR_REPORT, > + BLOCKDEV_ON_ERROR_REPORT); > } else { > drive_uninit(drive_get_by_blockdev(bs)); > } The user gets exactly what he ordered. He ordered "stop on error", then provoked errors by turning the virtual block device into a virtual pile of scrap metal. Because that's exactly what drive_del does when used while a device model is attached to the drive. The only sane use case for drive_del I can think of is revoking access to an image violently, after the guest failed to honor a hot unplug. Even then, using drive_del when the block device is removable is unnecessary. Just rip out the medium with eject -f. Look ma, no scrap metal. I'm not sure what you mean by "it is not possible to resolve this situation". The device is shot! Can't see how that could be resolved. I figure the bit that can't be resolved now is letting the user switch off "stop on error" safely before a drive_del. Even if we had a command for that, there'd still be a window between that command's execution and drive_del's. Your patch solves the problem by having drive_del switch it off unconditionally. Oookay, but please document it, because it's not exactly obvious. Re "the guest gets the ENOMEDIUM error": depends on the device. I doubt disks can signal "no medium", and even if they could, I doubt device drivers are prepared for it. Re "this scenario actually happens sometimes during libvirt disk hot unplug, where device_del is followed by drive_del": if I remember correctly, libvirt disk hot unplug runs drive_del right after device_del, opening a window where the guest sees a dead device. That's asking for trouble, and trouble is known to oblige.
Il 04/06/2013 18:37, Markus Armbruster ha scritto: > I figure the bit that can't be resolved now is letting the user switch > off "stop on error" safely before a drive_del. Even if we had a command > for that, there'd still be a window between that command's execution and > drive_del's. Your patch solves the problem by having drive_del switch > it off unconditionally. Oookay, but please document it, because it's > not exactly obvious. It is not obvious, but it is not surprising either when you see it (i.e. you won't really be surprised by the errors in the guest and won't need to know that, under the hood, rerror has been changed from the value you specified). > Re "the guest gets the ENOMEDIUM error": depends on the device. I doubt > disks can signal "no medium", and even if they could, I doubt device > drivers are prepared for it. SCSI disks can signal whatever they want. Device drivers will just treat it as any other error (sense code) they don't recognize. > Re "this scenario actually happens sometimes during libvirt disk hot > unplug, where device_del is followed by drive_del": if I remember > correctly, libvirt disk hot unplug runs drive_del right after > device_del, opening a window where the guest sees a dead device. That's > asking for trouble, and trouble is known to oblige. I think it's causing too much trouble though, and Stefan's patch is making the trouble evident to the guest. Surprise removal is a fact of life, I don't think it makes sense to stop the machine on surprise removal. It's very different from I/O errors. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 04/06/2013 18:37, Markus Armbruster ha scritto: >> I figure the bit that can't be resolved now is letting the user switch >> off "stop on error" safely before a drive_del. Even if we had a command >> for that, there'd still be a window between that command's execution and >> drive_del's. Your patch solves the problem by having drive_del switch >> it off unconditionally. Oookay, but please document it, because it's >> not exactly obvious. > > It is not obvious, but it is not surprising either when you see it (i.e. > you won't really be surprised by the errors in the guest and won't need > to know that, under the hood, rerror has been changed from the value you > specified). > >> Re "the guest gets the ENOMEDIUM error": depends on the device. I doubt >> disks can signal "no medium", and even if they could, I doubt device >> drivers are prepared for it. > > SCSI disks can signal whatever they want. Device drivers will just > treat it as any other error (sense code) they don't recognize. My point is: the commit message claims "the guest gets the ENOMEDIUM error", which isn't really true. No biggie, of course. >> Re "this scenario actually happens sometimes during libvirt disk hot >> unplug, where device_del is followed by drive_del": if I remember >> correctly, libvirt disk hot unplug runs drive_del right after >> device_del, opening a window where the guest sees a dead device. That's >> asking for trouble, and trouble is known to oblige. > > I think it's causing too much trouble though, and Stefan's patch is > making the trouble evident to the guest. Surprise removal is a fact of > life, I don't think it makes sense to stop the machine on surprise > removal. It's very different from I/O errors. I don't disagree with Stefan's patch, or your defense of it. Except I'm reluctant to not document something non-obvious because "you don't need to know" when I can document it in less time it would take me to overcome my resistance to "you don't need to know" arguments ;) This is drive_add's documentation in hmp-commands.hx: Remove host block device. The result is that guest generated IO is no longer submitted against the host device underlying the disk. Once a drive has been deleted, the QEMU Block layer returns -EIO which results in IO errors in the guest for applications that are reading/writing to the device. Suggest to add: These errors are always reported to the guest, regardless of the drive's error actions (drive options rerror, werror). Independently, libvirt needs fixing.
On 06/04/2013 01:24 PM, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Il 04/06/2013 18:37, Markus Armbruster ha scritto: >>> I figure the bit that can't be resolved now is letting the user switch >>> off "stop on error" safely before a drive_del. Even if we had a command >>> for that, there'd still be a window between that command's execution and >>> drive_del's. Your patch solves the problem by having drive_del switch >>> it off unconditionally. Oookay, but please document it, because it's >>> not exactly obvious. >> >> It is not obvious, but it is not surprising either when you see it (i.e. >> you won't really be surprised by the errors in the guest and won't need >> to know that, under the hood, rerror has been changed from the value you >> specified). >> > This is drive_add's documentation in hmp-commands.hx: > > Remove host block device. The result is that guest generated IO is > no longer submitted against the host device underlying the disk. > Once a drive has been deleted, the QEMU Block layer returns -EIO > which results in IO errors in the guest for applications that are > reading/writing to the device. > > Suggest to add: > > These errors are always reported to the guest, regardless of the > drive's error actions (drive options rerror, werror). > > Independently, libvirt needs fixing. Total agreement that libvirt needs to use a saner disk hot-unplug sequence when it is known that qemu provides one. I've filed https://bugzilla.redhat.com/show_bug.cgi?id=970761 to remind us to fix libvirt.
Eric Blake <eblake@redhat.com> writes: > On 06/04/2013 01:24 PM, Markus Armbruster wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> Il 04/06/2013 18:37, Markus Armbruster ha scritto: >>>> I figure the bit that can't be resolved now is letting the user switch >>>> off "stop on error" safely before a drive_del. Even if we had a command >>>> for that, there'd still be a window between that command's execution and >>>> drive_del's. Your patch solves the problem by having drive_del switch >>>> it off unconditionally. Oookay, but please document it, because it's >>>> not exactly obvious. >>> >>> It is not obvious, but it is not surprising either when you see it (i.e. >>> you won't really be surprised by the errors in the guest and won't need >>> to know that, under the hood, rerror has been changed from the value you >>> specified). >>> > >> This is drive_add's documentation in hmp-commands.hx: >> >> Remove host block device. The result is that guest generated IO is >> no longer submitted against the host device underlying the disk. >> Once a drive has been deleted, the QEMU Block layer returns -EIO >> which results in IO errors in the guest for applications that are >> reading/writing to the device. >> >> Suggest to add: >> >> These errors are always reported to the guest, regardless of the >> drive's error actions (drive options rerror, werror). >> >> Independently, libvirt needs fixing. > > Total agreement that libvirt needs to use a saner disk hot-unplug > sequence when it is known that qemu provides one. I've filed > https://bugzilla.redhat.com/show_bug.cgi?id=970761 > to remind us to fix libvirt. Sane sequence 1. device_del 2. Wait for DEVICE_DELETED 3. if timeout, drive_del Might make sense to offer a choice in the API between "fail" and "destroy the block device" if guest doesn't cooperate.
On Tue, Jun 04, 2013 at 06:37:27PM +0200, Markus Armbruster wrote: > Stefan Hajnoczi <stefanha@redhat.com> writes: > > > Paolo Bonzini <pbonzini@redhat.com> suggested the following test case: > > > > 1. Launch a guest and wait at the GRUB boot menu: > > > > qemu-system-x86_64 -enable-kvm -m 1024 \ > > -drive if=none,cache=none,file=test.img,id=foo,werror=stop,rerror=stop > > -device virtio-blk-pci,drive=foo,id=virtio0,addr=4 > > > > 2. Hot unplug the device: > > > > (qemu) drive_del foo > > > > 3. Select the first boot menu entry > > > > Without this patch the guest pauses due to ENOMEDIUM. But it is not > > possible to resolve this situation - the drive has become anonymous. > > > > With this patch the guest the guest gets the ENOMEDIUM error. > > > > Note that this scenario actually happens sometimes during libvirt disk > > hot unplug, where device_del is followed by drive_del. I/O may still be > > submitted to the drive after drive_del if the guest does not process the > > PCI hot unplug notification. > > > > Reported-by: Dafna Ron <dron@redhat.com> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > blockdev.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/blockdev.c b/blockdev.c > > index d1ec99a..6eb81a3 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -1180,6 +1180,10 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) > > */ > > if (bdrv_get_attached_dev(bs)) { > > bdrv_make_anon(bs); > > + > > + /* Further I/O must not pause the guest */ > > + bdrv_set_on_error(bs, BLOCKDEV_ON_ERROR_REPORT, > > + BLOCKDEV_ON_ERROR_REPORT); > > } else { > > drive_uninit(drive_get_by_blockdev(bs)); > > } > > The user gets exactly what he ordered. He ordered "stop on error", then > provoked errors by turning the virtual block device into a virtual pile > of scrap metal. Because that's exactly what drive_del does when used > while a device model is attached to the drive. > > The only sane use case for drive_del I can think of is revoking access > to an image violently, after the guest failed to honor a hot unplug. > > Even then, using drive_del when the block device is removable is > unnecessary. Just rip out the medium with eject -f. Look ma, no scrap > metal. > > I'm not sure what you mean by "it is not possible to resolve this > situation". The device is shot! Can't see how that could be resolved. This is the critical part: the guest is paused and there is no way to resolve the continuous pause loop. The drive is gone but the guest hasn't PCI hot unplugged the storage controller. As a user, there's nothing you can do on the QEMU monitor to resume the guest - it will just pause itself again. This behavior is really bad, QEMU has basically wedged the guest into an unrecoverable state and that's what I was trying to describe. > I figure the bit that can't be resolved now is letting the user switch > off "stop on error" safely before a drive_del. Even if we had a command > for that, there'd still be a window between that command's execution and > drive_del's. Your patch solves the problem by having drive_del switch > it off unconditionally. Oookay, but please document it, because it's > not exactly obvious. Thanks for the documentation suggestion, will add it in v2. > Re "the guest gets the ENOMEDIUM error": depends on the device. I doubt > disks can signal "no medium", and even if they could, I doubt device > drivers are prepared for it. Yep, error reporting depends on the emulated storage controller. virtio-blk and IDE just report a generic error status. > Re "this scenario actually happens sometimes during libvirt disk hot > unplug, where device_del is followed by drive_del": if I remember > correctly, libvirt disk hot unplug runs drive_del right after > device_del, opening a window where the guest sees a dead device. That's > asking for trouble, and trouble is known to oblige. Agreed.
On Mon, 06/03 16:58, Stefan Hajnoczi wrote: > Paolo Bonzini <pbonzini@redhat.com> suggested the following test case: > > 1. Launch a guest and wait at the GRUB boot menu: > > qemu-system-x86_64 -enable-kvm -m 1024 \ > -drive if=none,cache=none,file=test.img,id=foo,werror=stop,rerror=stop > -device virtio-blk-pci,drive=foo,id=virtio0,addr=4 > > 2. Hot unplug the device: > > (qemu) drive_del foo > > 3. Select the first boot menu entry > > Without this patch the guest pauses due to ENOMEDIUM. But it is not > possible to resolve this situation - the drive has become anonymous. > > With this patch the guest the guest gets the ENOMEDIUM error. s/the guest the guest/the guest/
diff --git a/blockdev.c b/blockdev.c index d1ec99a..6eb81a3 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1180,6 +1180,10 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) */ if (bdrv_get_attached_dev(bs)) { bdrv_make_anon(bs); + + /* Further I/O must not pause the guest */ + bdrv_set_on_error(bs, BLOCKDEV_ON_ERROR_REPORT, + BLOCKDEV_ON_ERROR_REPORT); } else { drive_uninit(drive_get_by_blockdev(bs)); }
Paolo Bonzini <pbonzini@redhat.com> suggested the following test case: 1. Launch a guest and wait at the GRUB boot menu: qemu-system-x86_64 -enable-kvm -m 1024 \ -drive if=none,cache=none,file=test.img,id=foo,werror=stop,rerror=stop -device virtio-blk-pci,drive=foo,id=virtio0,addr=4 2. Hot unplug the device: (qemu) drive_del foo 3. Select the first boot menu entry Without this patch the guest pauses due to ENOMEDIUM. But it is not possible to resolve this situation - the drive has become anonymous. With this patch the guest the guest gets the ENOMEDIUM error. Note that this scenario actually happens sometimes during libvirt disk hot unplug, where device_del is followed by drive_del. I/O may still be submitted to the drive after drive_del if the guest does not process the PCI hot unplug notification. Reported-by: Dafna Ron <dron@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- blockdev.c | 4 ++++ 1 file changed, 4 insertions(+)