[Qemu-devel] [PATCH] sheepdog: set io_flush handler in do_co_req

[Qemu-devel] [PATCH] sheepdog: set io_flush handler in do_co_req

[MORITA Kazutaka]

If an io_flush handler is not set, qemu_aio_wait doesn't invoke
callbacks.

Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
---
 block/sheepdog.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index e4ec32d..cb0eeed 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -501,6 +501,13 @@ static void restart_co_req(void *opaque)
     qemu_coroutine_enter(co, NULL);
 }
 
+static int have_co_req(void *opaque)
+{
+    /* this handler is set only when there is a pending request, so
+     * always returns 1. */
+    return 1;
+}
+
 typedef struct SheepdogReqCo {
     int sockfd;
     SheepdogReq *hdr;
@@ -523,14 +530,14 @@ static coroutine_fn void do_co_req(void *opaque)
     unsigned int *rlen = srco->rlen;
 
     co = qemu_coroutine_self();
-    qemu_aio_set_fd_handler(sockfd, NULL, restart_co_req, NULL, co);
+    qemu_aio_set_fd_handler(sockfd, NULL, restart_co_req, have_co_req, co);
 
     ret = send_co_req(sockfd, hdr, data, wlen);
     if (ret < 0) {
         goto out;
     }
 
-    qemu_aio_set_fd_handler(sockfd, restart_co_req, NULL, NULL, co);
+    qemu_aio_set_fd_handler(sockfd, restart_co_req, NULL, have_co_req, co);
 
     ret = qemu_co_recv(sockfd, hdr, sizeof(*hdr));
     if (ret < sizeof(*hdr)) {
-- 
1.8.1.3.566.gaa39828

Re: [Qemu-devel] [PATCH] sheepdog: set io_flush handler in do_co_req

[Stefan Hajnoczi]

On Mon, Mar 11, 2013 at 06:01:02PM +0900, MORITA Kazutaka wrote:
> If an io_flush handler is not set, qemu_aio_wait doesn't invoke
> callbacks.
> 
> Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> ---
>  block/sheepdog.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index e4ec32d..cb0eeed 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -501,6 +501,13 @@ static void restart_co_req(void *opaque)
>      qemu_coroutine_enter(co, NULL);
>  }
>  
> +static int have_co_req(void *opaque)
> +{
> +    /* this handler is set only when there is a pending request, so
> +     * always returns 1. */
> +    return 1;
> +}
> +
>  typedef struct SheepdogReqCo {
>      int sockfd;
>      SheepdogReq *hdr;
> @@ -523,14 +530,14 @@ static coroutine_fn void do_co_req(void *opaque)
>      unsigned int *rlen = srco->rlen;
>  
>      co = qemu_coroutine_self();
> -    qemu_aio_set_fd_handler(sockfd, NULL, restart_co_req, NULL, co);
> +    qemu_aio_set_fd_handler(sockfd, NULL, restart_co_req, have_co_req, co);
>  
>      ret = send_co_req(sockfd, hdr, data, wlen);

Which tree is this patch against?  block/sheepdog.c:do_co_req() has a
socket_set_block(sockfd) call before this line.

Is there a guarantee that only one coroutine executes do_co_req() at a
time?  Otherwise the first coroutine that finishes the function sets
io_flush to NULL even though there's still another request processing.

Stefan

Re: [Qemu-devel] [PATCH] sheepdog: set io_flush handler in do_co_req

[MORITA Kazutaka]

At Mon, 11 Mar 2013 15:39:05 +0100,
Stefan Hajnoczi wrote:
> 
> On Mon, Mar 11, 2013 at 06:01:02PM +0900, MORITA Kazutaka wrote:
> > If an io_flush handler is not set, qemu_aio_wait doesn't invoke
> > callbacks.
> > 
> > Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> > ---
> >  block/sheepdog.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index e4ec32d..cb0eeed 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -501,6 +501,13 @@ static void restart_co_req(void *opaque)
> >      qemu_coroutine_enter(co, NULL);
> >  }
> >  
> > +static int have_co_req(void *opaque)
> > +{
> > +    /* this handler is set only when there is a pending request, so
> > +     * always returns 1. */
> > +    return 1;
> > +}
> > +
> >  typedef struct SheepdogReqCo {
> >      int sockfd;
> >      SheepdogReq *hdr;
> > @@ -523,14 +530,14 @@ static coroutine_fn void do_co_req(void *opaque)
> >      unsigned int *rlen = srco->rlen;
> >  
> >      co = qemu_coroutine_self();
> > -    qemu_aio_set_fd_handler(sockfd, NULL, restart_co_req, NULL, co);
> > +    qemu_aio_set_fd_handler(sockfd, NULL, restart_co_req, have_co_req, co);
> >  
> >      ret = send_co_req(sockfd, hdr, data, wlen);
> 
> Which tree is this patch against?  block/sheepdog.c:do_co_req() has a
> socket_set_block(sockfd) call before this line.

Sorry, I have another patch in my local tree that needed to be sent
before this one.  I'll send v2 including the missing patch.

> 
> Is there a guarantee that only one coroutine executes do_co_req() at a
> time?  Otherwise the first coroutine that finishes the function sets
> io_flush to NULL even though there's still another request processing.

Yes - the sockfd is opened just before calling do_req() and is closed
just after returning the function, so the sheepdog driver cannot call
multiple do_co_req() at a time against the same socket descriptor.

Thanks,

Kazutaka