| Message ID | 20130206180214.311f670a@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 06.02.2013 21:02, schrieb Luiz Capitulino: > This reverts commit 9ca111544c64b5abed2e79cf52e19a8f227b347b. > > That commit has added a weird side effect to QMP: on shutdown, > QMP emits the DEVICE_TRAY_MOVED event for all empty drives > that have closed trays. > > This happens because the tray state actually changes in > the following code path: > > main loop exit > bdrv_close_all() > bdrv_close() > bdrv_dev_change_media_cb() > > So, revert the commit as the real fix is to separate eject > from bdrv_close() and also because it's not clear how important > it is to have a 100% correct semantic for eject from HMP. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > > ** IMPORTANT: This is an RFC because I'm not totally sure we should > apply it for 1.4, for the following reasons: > > 1. The worst we get is a gratuitous state change that triggers > an QMP event > > 2. Commit 9ca1115 is a bit old and was introduced before 1.4 > development > > 3. This is not the Right Thing either > > block.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/block.c b/block.c > index 50dab8e..d58c6cc 100644 > --- a/block.c > +++ b/block.c > @@ -1178,9 +1178,9 @@ void bdrv_close(BlockDriverState *bs) > bdrv_delete(bs->file); > bs->file = NULL; > } > - } > > - bdrv_dev_change_media_cb(bs, false); > + bdrv_dev_change_media_cb(bs, false); > + } > > /*throttling disk I/O limits*/ > if (bs->io_limits_enabled) { I think the right solution is to move the bdrv_dev_change_media_cb() call to those callers of bdrv_close() that actually need it. I haven't checked it yet in detail, but at the first sight it looks like eject_device() and do_drive_del() are the only ones that need it. Such a change I'd rather move to 1.5, though. Do you think it's important to get rid of these (admittedly odd) artefacts for 1.4? Kevin
On Thu, 07 Feb 2013 14:12:10 +0100 Kevin Wolf <kwolf@redhat.com> wrote: > Am 06.02.2013 21:02, schrieb Luiz Capitulino: > > This reverts commit 9ca111544c64b5abed2e79cf52e19a8f227b347b. > > > > That commit has added a weird side effect to QMP: on shutdown, > > QMP emits the DEVICE_TRAY_MOVED event for all empty drives > > that have closed trays. > > > > This happens because the tray state actually changes in > > the following code path: > > > > main loop exit > > bdrv_close_all() > > bdrv_close() > > bdrv_dev_change_media_cb() > > > > So, revert the commit as the real fix is to separate eject > > from bdrv_close() and also because it's not clear how important > > it is to have a 100% correct semantic for eject from HMP. > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > > > ** IMPORTANT: This is an RFC because I'm not totally sure we should > > apply it for 1.4, for the following reasons: > > > > 1. The worst we get is a gratuitous state change that triggers > > an QMP event > > > > 2. Commit 9ca1115 is a bit old and was introduced before 1.4 > > development > > > > 3. This is not the Right Thing either > > > > block.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/block.c b/block.c > > index 50dab8e..d58c6cc 100644 > > --- a/block.c > > +++ b/block.c > > @@ -1178,9 +1178,9 @@ void bdrv_close(BlockDriverState *bs) > > bdrv_delete(bs->file); > > bs->file = NULL; > > } > > - } > > > > - bdrv_dev_change_media_cb(bs, false); > > + bdrv_dev_change_media_cb(bs, false); > > + } > > > > /*throttling disk I/O limits*/ > > if (bs->io_limits_enabled) { > > I think the right solution is to move the bdrv_dev_change_media_cb() > call to those callers of bdrv_close() that actually need it. I haven't > checked it yet in detail, but at the first sight it looks like > eject_device() and do_drive_del() are the only ones that need it. > > Such a change I'd rather move to 1.5, though. Do you think it's > important to get rid of these (admittedly odd) artefacts for 1.4? I don't have any reports of this causing issues to QMP clients, so we can defer to 1.5. Worst case we merge this fix for a stable release.
Am 07.02.2013 14:15, schrieb Luiz Capitulino: > On Thu, 07 Feb 2013 14:12:10 +0100 > Kevin Wolf <kwolf@redhat.com> wrote: >> I think the right solution is to move the bdrv_dev_change_media_cb() >> call to those callers of bdrv_close() that actually need it. I haven't >> checked it yet in detail, but at the first sight it looks like >> eject_device() and do_drive_del() are the only ones that need it. >> >> Such a change I'd rather move to 1.5, though. Do you think it's >> important to get rid of these (admittedly odd) artefacts for 1.4? > > I don't have any reports of this causing issues to QMP clients, so > we can defer to 1.5. Worst case we merge this fix for a stable release. Okay, then that's the plan. Are you going to send a patch for 1.5 or should I make sure to have it somewhere on a todo list? Kevin
On Thu, 07 Feb 2013 14:28:33 +0100 Kevin Wolf <kwolf@redhat.com> wrote: > Am 07.02.2013 14:15, schrieb Luiz Capitulino: > > On Thu, 07 Feb 2013 14:12:10 +0100 > > Kevin Wolf <kwolf@redhat.com> wrote: > >> I think the right solution is to move the bdrv_dev_change_media_cb() > >> call to those callers of bdrv_close() that actually need it. I haven't > >> checked it yet in detail, but at the first sight it looks like > >> eject_device() and do_drive_del() are the only ones that need it. > >> > >> Such a change I'd rather move to 1.5, though. Do you think it's > >> important to get rid of these (admittedly odd) artefacts for 1.4? > > > > I don't have any reports of this causing issues to QMP clients, so > > we can defer to 1.5. Worst case we merge this fix for a stable release. > > Okay, then that's the plan. Are you going to send a patch for 1.5 or > should I make sure to have it somewhere on a todo list? I can do it, but not this week.
On Thu, 7 Feb 2013 11:31:08 -0200 Luiz Capitulino <lcapitulino@redhat.com> wrote: > On Thu, 07 Feb 2013 14:28:33 +0100 > Kevin Wolf <kwolf@redhat.com> wrote: > > > Am 07.02.2013 14:15, schrieb Luiz Capitulino: > > > On Thu, 07 Feb 2013 14:12:10 +0100 > > > Kevin Wolf <kwolf@redhat.com> wrote: > > >> I think the right solution is to move the bdrv_dev_change_media_cb() > > >> call to those callers of bdrv_close() that actually need it. I haven't > > >> checked it yet in detail, but at the first sight it looks like > > >> eject_device() and do_drive_del() are the only ones that need it. > > >> > > >> Such a change I'd rather move to 1.5, though. Do you think it's > > >> important to get rid of these (admittedly odd) artefacts for 1.4? > > > > > > I don't have any reports of this causing issues to QMP clients, so > > > we can defer to 1.5. Worst case we merge this fix for a stable release. > > > > Okay, then that's the plan. Are you going to send a patch for 1.5 or > > should I make sure to have it somewhere on a todo list? > > I can do it, but not this week. It turns out I couldn't work on it this week, and I'll be off for some time starting Monday next week.
diff --git a/block.c b/block.c index 50dab8e..d58c6cc 100644 --- a/block.c +++ b/block.c @@ -1178,9 +1178,9 @@ void bdrv_close(BlockDriverState *bs) bdrv_delete(bs->file); bs->file = NULL; } - } - bdrv_dev_change_media_cb(bs, false); + bdrv_dev_change_media_cb(bs, false); + } /*throttling disk I/O limits*/ if (bs->io_limits_enabled) {
This reverts commit 9ca111544c64b5abed2e79cf52e19a8f227b347b. That commit has added a weird side effect to QMP: on shutdown, QMP emits the DEVICE_TRAY_MOVED event for all empty drives that have closed trays. This happens because the tray state actually changes in the following code path: main loop exit bdrv_close_all() bdrv_close() bdrv_dev_change_media_cb() So, revert the commit as the real fix is to separate eject from bdrv_close() and also because it's not clear how important it is to have a 100% correct semantic for eject from HMP. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- ** IMPORTANT: This is an RFC because I'm not totally sure we should apply it for 1.4, for the following reasons: 1. The worst we get is a gratuitous state change that triggers an QMP event 2. Commit 9ca1115 is a bit old and was introduced before 1.4 development 3. This is not the Right Thing either block.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)