[Qemu-devel] [PATCH] macio: Use blk_drain instead of blk_drain_all

[Qemu-devel] [PATCH] macio: Use blk_drain instead of blk_drain_all

[Fam Zheng]

We only care about the associated backend, so blk_drain is more
appropriate here.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/ide/macio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 78c10a0..a8c7321 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -400,7 +400,7 @@ static void pmac_ide_flush(DBDMA_io *io)
     IDEState *s = idebus_active_if(&m->bus);
 
     if (s->bus->dma->aiocb) {
-        blk_drain_all();
+        blk_drain(s->blk);
     }
 }
 
-- 
2.8.3

Re: [Qemu-devel] [Qemu-block] [PATCH] macio: Use blk_drain instead of blk_drain_all

[Kevin Wolf]

Am 12.06.2016 um 08:56 hat Fam Zheng geschrieben:
> We only care about the associated backend, so blk_drain is more
> appropriate here.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

[ Cc: John ]

> ---
>  hw/ide/macio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 78c10a0..a8c7321 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -400,7 +400,7 @@ static void pmac_ide_flush(DBDMA_io *io)
>      IDEState *s = idebus_active_if(&m->bus);
>  
>      if (s->bus->dma->aiocb) {
> -        blk_drain_all();
> +        blk_drain(s->blk);
>      }
>  }

Looks good to me:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

However, even this is still doing too much. We only need to drain the
requests that come from this device and can ignore e.g. block job
requests.

Now the part that I'm not completely sure about is whether the problem
is here in the IDE emulation and it should track its own requests or
whether it is blk_drain() that actually shouldn't drain the BDS but just
all requests that came in through this specific BB.

I'm leaning towards the latter, but I'm unsure whether we have cases
where we actually need to drain the whole root BDS. Any opinions?

Kevin

Re: [Qemu-devel] [PATCH] macio: Use blk_drain instead of blk_drain_all

[John Snow]

On 06/12/2016 02:56 AM, Fam Zheng wrote:
> We only care about the associated backend, so blk_drain is more
> appropriate here.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/ide/macio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 78c10a0..a8c7321 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -400,7 +400,7 @@ static void pmac_ide_flush(DBDMA_io *io)
>      IDEState *s = idebus_active_if(&m->bus);
>  
>      if (s->bus->dma->aiocb) {
> -        blk_drain_all();
> +        blk_drain(s->blk);
>      }
>  }
>  
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Shall I take this through my tree?

Re: [Qemu-devel] [Qemu-block] [PATCH] macio: Use blk_drain instead of blk_drain_all

[Fam Zheng]

On Mon, 06/13 11:39, Kevin Wolf wrote:
> Am 12.06.2016 um 08:56 hat Fam Zheng geschrieben:
> > We only care about the associated backend, so blk_drain is more
> > appropriate here.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> [ Cc: John ]
> 
> > ---
> >  hw/ide/macio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> > index 78c10a0..a8c7321 100644
> > --- a/hw/ide/macio.c
> > +++ b/hw/ide/macio.c
> > @@ -400,7 +400,7 @@ static void pmac_ide_flush(DBDMA_io *io)
> >      IDEState *s = idebus_active_if(&m->bus);
> >  
> >      if (s->bus->dma->aiocb) {
> > -        blk_drain_all();
> > +        blk_drain(s->blk);
> >      }
> >  }
> 
> Looks good to me:
> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> 
> However, even this is still doing too much. We only need to drain the
> requests that come from this device and can ignore e.g. block job
> requests.
> 
> Now the part that I'm not completely sure about is whether the problem
> is here in the IDE emulation and it should track its own requests or
> whether it is blk_drain() that actually shouldn't drain the BDS but just
> all requests that came in through this specific BB.
> 
> I'm leaning towards the latter, but I'm unsure whether we have cases
> where we actually need to drain the whole root BDS. Any opinions?

I agree with you and think the latter is better in this case. I think the one
in migration/block.c need to drain the whole root BDS, and others don't.

Fam

Re: [Qemu-devel] [PATCH] macio: Use blk_drain instead of blk_drain_all

[Fam Zheng]

On Mon, 06/13 17:33, John Snow wrote:
> 
> 
> On 06/12/2016 02:56 AM, Fam Zheng wrote:
> > We only care about the associated backend, so blk_drain is more
> > appropriate here.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  hw/ide/macio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> > index 78c10a0..a8c7321 100644
> > --- a/hw/ide/macio.c
> > +++ b/hw/ide/macio.c
> > @@ -400,7 +400,7 @@ static void pmac_ide_flush(DBDMA_io *io)
> >      IDEState *s = idebus_active_if(&m->bus);
> >  
> >      if (s->bus->dma->aiocb) {
> > -        blk_drain_all();
> > +        blk_drain(s->blk);
> >      }
> >  }
> >  
> > 
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> 
> Shall I take this through my tree?

I think that MAINTAINERS says so. :)

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH] macio: Use blk_drain instead of blk_drain_all

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]

On Mon, Jun 13, 2016 at 11:39:48AM +0200, Kevin Wolf wrote:
> Am 12.06.2016 um 08:56 hat Fam Zheng geschrieben:
> > We only care about the associated backend, so blk_drain is more
> > appropriate here.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> 
> [ Cc: John ]
> 
> > ---
> >  hw/ide/macio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> > index 78c10a0..a8c7321 100644
> > --- a/hw/ide/macio.c
> > +++ b/hw/ide/macio.c
> > @@ -400,7 +400,7 @@ static void pmac_ide_flush(DBDMA_io *io)
> >      IDEState *s = idebus_active_if(&m->bus);
> >  
> >      if (s->bus->dma->aiocb) {
> > -        blk_drain_all();
> > +        blk_drain(s->blk);
> >      }
> >  }
> 
> Looks good to me:
> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> 
> However, even this is still doing too much. We only need to drain the
> requests that come from this device and can ignore e.g. block job
> requests.
> 
> Now the part that I'm not completely sure about is whether the problem
> is here in the IDE emulation and it should track its own requests or
> whether it is blk_drain() that actually shouldn't drain the BDS but just
> all requests that came in through this specific BB.
> 
> I'm leaning towards the latter, but I'm unsure whether we have cases
> where we actually need to drain the whole root BDS. Any opinions?

There seem to be two distinct drain use cases:

1. Complete all requests that have been submitted via this BB.

2. Quiesce the BDS sub-graph so operations can be performed without
   in-flight I/O intefering.

Device emulation usually needs #1.  Live migration and monitor commands
need #2.

We could distinguish the two by making blk_drain() have #1 semantics
only.  Users that need #2 must use new
blk_drained_begin()/blk_drained_end() functions so the quiescent region
is explicit.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] macio: Use blk_drain instead of blk_drain_all

[John Snow]

On 06/13/2016 09:21 PM, Fam Zheng wrote:
> On Mon, 06/13 17:33, John Snow wrote:
>>
>>
>> On 06/12/2016 02:56 AM, Fam Zheng wrote:
>>> We only care about the associated backend, so blk_drain is more
>>> appropriate here.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>  hw/ide/macio.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>>> index 78c10a0..a8c7321 100644
>>> --- a/hw/ide/macio.c
>>> +++ b/hw/ide/macio.c
>>> @@ -400,7 +400,7 @@ static void pmac_ide_flush(DBDMA_io *io)
>>>      IDEState *s = idebus_active_if(&m->bus);
>>>  
>>>      if (s->bus->dma->aiocb) {
>>> -        blk_drain_all();
>>> +        blk_drain(s->blk);
>>>      }
>>>  }
>>>  
>>>
>>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>>
>> Shall I take this through my tree?
> 
> I think that MAINTAINERS says so. :)
> 
> Fam
> 

For now, then:

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js