Patchwork [RFC,for-1.4] Revert "block: fix block tray status"

login
register
mail settings
Submitter Luiz Capitulino
Date Feb. 6, 2013, 8:02 p.m.
Message ID <20130206180214.311f670a@redhat.com>
Download mbox | patch
Permalink /patch/218730/
State New
Headers show

Comments

Luiz Capitulino - Feb. 6, 2013, 8:02 p.m.
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(-)
Kevin Wolf - Feb. 7, 2013, 1:12 p.m.
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
Luiz Capitulino - Feb. 7, 2013, 1:15 p.m.
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.
Kevin Wolf - Feb. 7, 2013, 1:28 p.m.
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
Luiz Capitulino - Feb. 7, 2013, 1:31 p.m.
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.
Luiz Capitulino - Feb. 21, 2013, 5:31 p.m.
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.

Patch

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) {