Patchwork [13/21] dma-helpers: replace bdrv_aio_writev() with bdrv_aio_writev_proxy().

login
register
mail settings
Submitter Yoshiaki Tamura
Date Nov. 25, 2010, 6:06 a.m.
Message ID <1290665220-26478-14-git-send-email-tamura.yoshiaki@lab.ntt.co.jp>
Download mbox | patch
Permalink /patch/72989/
State New
Headers show

Comments

Yoshiaki Tamura - Nov. 25, 2010, 6:06 a.m.
Replace bdrv_aio_writev() with bdrv_aio_writev_proxy() to let
event-tap capture events from dma-helpers.

Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
---
 dma-helpers.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Michael S. Tsirkin - Nov. 28, 2010, 9:33 a.m.
On Thu, Nov 25, 2010 at 03:06:52PM +0900, Yoshiaki Tamura wrote:
> Replace bdrv_aio_writev() with bdrv_aio_writev_proxy() to let
> event-tap capture events from dma-helpers.
> 
> Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>

Same comment as -net here: it's not clear when should
a device use bdrv_aio_writev_proxy and when bdrv_aio_writev.
If all devices should just use _proxy, let's
just make bdrv_aio_writev DTRT instead.

> ---
>  dma-helpers.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/dma-helpers.c b/dma-helpers.c
> index 712ed89..8ab2c26 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -117,8 +117,8 @@ static void dma_bdrv_cb(void *opaque, int ret)
>      }
>  
>      if (dbs->is_write) {
> -        dbs->acb = bdrv_aio_writev(dbs->bs, dbs->sector_num, &dbs->iov,
> -                                   dbs->iov.size / 512, dma_bdrv_cb, dbs);
> +        dbs->acb = bdrv_aio_writev_proxy(dbs->bs, dbs->sector_num, &dbs->iov,
> +                                         dbs->iov.size / 512, dma_bdrv_cb, dbs);
>      } else {
>          dbs->acb = bdrv_aio_readv(dbs->bs, dbs->sector_num, &dbs->iov,
>                                    dbs->iov.size / 512, dma_bdrv_cb, dbs);
> -- 
> 1.7.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yoshiaki Tamura - Nov. 28, 2010, 11:55 a.m.
2010/11/28 Michael S. Tsirkin <mst@redhat.com>:
> On Thu, Nov 25, 2010 at 03:06:52PM +0900, Yoshiaki Tamura wrote:
>> Replace bdrv_aio_writev() with bdrv_aio_writev_proxy() to let
>> event-tap capture events from dma-helpers.
>>
>> Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
>
> Same comment as -net here: it's not clear when should
> a device use bdrv_aio_writev_proxy and when bdrv_aio_writev.
> If all devices should just use _proxy, let's
> just make bdrv_aio_writev DTRT instead.

Same as I replied to the net layer question.  However, I had
troubles with inserting event-tap functions into block.c before.
block.c gets linked with utils like qemu-img, but they don't get
linked with emulators code which event-tap uses in it.  So I want
to avoid linking block and event-tap for utils, but I guess we
don't want to use ifdefs for this.  I'm wondering how I can solve
this problem cleanly.

Kevin, do you have suggestions here?

Yoshi

>
>> ---
>>  dma-helpers.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/dma-helpers.c b/dma-helpers.c
>> index 712ed89..8ab2c26 100644
>> --- a/dma-helpers.c
>> +++ b/dma-helpers.c
>> @@ -117,8 +117,8 @@ static void dma_bdrv_cb(void *opaque, int ret)
>>      }
>>
>>      if (dbs->is_write) {
>> -        dbs->acb = bdrv_aio_writev(dbs->bs, dbs->sector_num, &dbs->iov,
>> -                                   dbs->iov.size / 512, dma_bdrv_cb, dbs);
>> +        dbs->acb = bdrv_aio_writev_proxy(dbs->bs, dbs->sector_num, &dbs->iov,
>> +                                         dbs->iov.size / 512, dma_bdrv_cb, dbs);
>>      } else {
>>          dbs->acb = bdrv_aio_readv(dbs->bs, dbs->sector_num, &dbs->iov,
>>                                    dbs->iov.size / 512, dma_bdrv_cb, dbs);
>> --
>> 1.7.1.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Michael S. Tsirkin - Nov. 28, 2010, 12:28 p.m.
On Sun, Nov 28, 2010 at 08:55:28PM +0900, Yoshiaki Tamura wrote:
> 2010/11/28 Michael S. Tsirkin <mst@redhat.com>:
> > On Thu, Nov 25, 2010 at 03:06:52PM +0900, Yoshiaki Tamura wrote:
> >> Replace bdrv_aio_writev() with bdrv_aio_writev_proxy() to let
> >> event-tap capture events from dma-helpers.
> >>
> >> Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
> >
> > Same comment as -net here: it's not clear when should
> > a device use bdrv_aio_writev_proxy and when bdrv_aio_writev.
> > If all devices should just use _proxy, let's
> > just make bdrv_aio_writev DTRT instead.
> 
> Same as I replied to the net layer question.  However, I had
> troubles with inserting event-tap functions into block.c before.
> block.c gets linked with utils like qemu-img, but they don't get
> linked with emulators code which event-tap uses in it.  So I want
> to avoid linking block and event-tap for utils, but I guess we
> don't want to use ifdefs for this.  I'm wondering how I can solve
> this problem cleanly.

Add stubs same as we have for other functions.

> Kevin, do you have suggestions here?
> 
> Yoshi
> 
> >
> >> ---
> >>  dma-helpers.c |    4 ++--
> >>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/dma-helpers.c b/dma-helpers.c
> >> index 712ed89..8ab2c26 100644
> >> --- a/dma-helpers.c
> >> +++ b/dma-helpers.c
> >> @@ -117,8 +117,8 @@ static void dma_bdrv_cb(void *opaque, int ret)
> >>      }
> >>
> >>      if (dbs->is_write) {
> >> -        dbs->acb = bdrv_aio_writev(dbs->bs, dbs->sector_num, &dbs->iov,
> >> -                                   dbs->iov.size / 512, dma_bdrv_cb, dbs);
> >> +        dbs->acb = bdrv_aio_writev_proxy(dbs->bs, dbs->sector_num, &dbs->iov,
> >> +                                         dbs->iov.size / 512, dma_bdrv_cb, dbs);
> >>      } else {
> >>          dbs->acb = bdrv_aio_readv(dbs->bs, dbs->sector_num, &dbs->iov,
> >>                                    dbs->iov.size / 512, dma_bdrv_cb, dbs);
> >> --
> >> 1.7.1.2
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe kvm" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
Kevin Wolf - Nov. 29, 2010, 9:52 a.m.
Am 28.11.2010 12:55, schrieb Yoshiaki Tamura:
> 2010/11/28 Michael S. Tsirkin <mst@redhat.com>:
>> On Thu, Nov 25, 2010 at 03:06:52PM +0900, Yoshiaki Tamura wrote:
>>> Replace bdrv_aio_writev() with bdrv_aio_writev_proxy() to let
>>> event-tap capture events from dma-helpers.
>>>
>>> Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
>>
>> Same comment as -net here: it's not clear when should
>> a device use bdrv_aio_writev_proxy and when bdrv_aio_writev.
>> If all devices should just use _proxy, let's
>> just make bdrv_aio_writev DTRT instead.
> 
> Same as I replied to the net layer question.  However, I had
> troubles with inserting event-tap functions into block.c before.
> block.c gets linked with utils like qemu-img, but they don't get
> linked with emulators code which event-tap uses in it.  So I want
> to avoid linking block and event-tap for utils, but I guess we
> don't want to use ifdefs for this.  I'm wondering how I can solve
> this problem cleanly.
> 
> Kevin, do you have suggestions here?

Michael's stubs (probably in qemu-tool.c) seem to be the right solution.

Which requests do you actually want to intercept? I assume you're aware
that for example qcow2 internally calls another bdrv_aio_readv/writev
that accesses the image file.

So if you only want to have the requests that come directly from
devices, maybe you'll have to restrict it to BlockDriverStates that
belongs to a drive. I think this is the case if it has a non-empty
device name.

Kevin
Yoshiaki Tamura - Nov. 29, 2010, 12:56 p.m.
2010/11/29 Kevin Wolf <kwolf@redhat.com>:
> Am 28.11.2010 12:55, schrieb Yoshiaki Tamura:
>> 2010/11/28 Michael S. Tsirkin <mst@redhat.com>:
>>> On Thu, Nov 25, 2010 at 03:06:52PM +0900, Yoshiaki Tamura wrote:
>>>> Replace bdrv_aio_writev() with bdrv_aio_writev_proxy() to let
>>>> event-tap capture events from dma-helpers.
>>>>
>>>> Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
>>>
>>> Same comment as -net here: it's not clear when should
>>> a device use bdrv_aio_writev_proxy and when bdrv_aio_writev.
>>> If all devices should just use _proxy, let's
>>> just make bdrv_aio_writev DTRT instead.
>>
>> Same as I replied to the net layer question.  However, I had
>> troubles with inserting event-tap functions into block.c before.
>> block.c gets linked with utils like qemu-img, but they don't get
>> linked with emulators code which event-tap uses in it.  So I want
>> to avoid linking block and event-tap for utils, but I guess we
>> don't want to use ifdefs for this.  I'm wondering how I can solve
>> this problem cleanly.
>>
>> Kevin, do you have suggestions here?
>
> Michael's stubs (probably in qemu-tool.c) seem to be the right solution.

Same here.  I noticed kvm-stub to be a good example.

> Which requests do you actually want to intercept? I assume you're aware
> that for example qcow2 internally calls another bdrv_aio_readv/writev
> that accesses the image file.
>
> So if you only want to have the requests that come directly from
> devices, maybe you'll have to restrict it to BlockDriverStates that
> belongs to a drive. I think this is the case if it has a non-empty
> device name.

Yes, exactly.  I noticed that a little while ago.  Thanks for
making it clear.

>
> Kevin
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Patch

diff --git a/dma-helpers.c b/dma-helpers.c
index 712ed89..8ab2c26 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -117,8 +117,8 @@  static void dma_bdrv_cb(void *opaque, int ret)
     }
 
     if (dbs->is_write) {
-        dbs->acb = bdrv_aio_writev(dbs->bs, dbs->sector_num, &dbs->iov,
-                                   dbs->iov.size / 512, dma_bdrv_cb, dbs);
+        dbs->acb = bdrv_aio_writev_proxy(dbs->bs, dbs->sector_num, &dbs->iov,
+                                         dbs->iov.size / 512, dma_bdrv_cb, dbs);
     } else {
         dbs->acb = bdrv_aio_readv(dbs->bs, dbs->sector_num, &dbs->iov,
                                   dbs->iov.size / 512, dma_bdrv_cb, dbs);