[RFC,2/3] sheepdog: pass NULL for io_flush

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 block/sheepdog.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

Il 28/03/2013 22:52, Anthony Liguori ha scritto:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  block/sheepdog.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index bb67c4c..2bccd9b 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -503,13 +503,6 @@ 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;
> @@ -532,14 +525,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, have_co_req, co);
> +    qemu_aio_set_fd_handler(sockfd, NULL, restart_co_req, NULL, co);
>  
>      ret = send_co_req(sockfd, hdr, data, wlen);
>      if (ret < 0) {
>          goto out;
>      }
>  
> -    qemu_aio_set_fd_handler(sockfd, restart_co_req, NULL, have_co_req, co);
> +    qemu_aio_set_fd_handler(sockfd, restart_co_req, NULL, NULL, co);
>  
>      ret = qemu_co_recv(sockfd, hdr, sizeof(*hdr));
>      if (ret < sizeof(*hdr)) {
> 

There is another one in hw/dataplane/virtio-blk.c, btw.

Paolo

Am 28.03.2013 um 22:52 hat Anthony Liguori geschrieben:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  block/sheepdog.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index bb67c4c..2bccd9b 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -503,13 +503,6 @@ 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. */

Now you return 1 even when no request is pending (which is the case in
which no io_flush handler would be set before). Why is this correct?
(This is actually a question about PATCH 1/3, I just noticed it here.
Are there more cases like this?)

Kevin

> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index bb67c4c..2bccd9b 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -503,13 +503,6 @@ 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. */
> 
> Now you return 1 even when no request is pending (which is the case in
> which no io_flush handler would be set before). Why is this correct?
> (This is actually a question about PATCH 1/3, I just noticed it here.
> Are there more cases like this?)

In the dataplane code, the ioeventfd uses an io_flush callback that returns
true.

Paolo

On Tue, Apr 02, 2013 at 10:37:24AM +0200, Kevin Wolf wrote:
> Am 28.03.2013 um 22:52 hat Anthony Liguori geschrieben:
> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > ---
> >  block/sheepdog.c | 11 ++---------
> >  1 file changed, 2 insertions(+), 9 deletions(-)
> > 
> > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > index bb67c4c..2bccd9b 100644
> > --- a/block/sheepdog.c
> > +++ b/block/sheepdog.c
> > @@ -503,13 +503,6 @@ 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. */
> 
> Now you return 1 even when no request is pending (which is the case in
> which no io_flush handler would be set before). Why is this correct?
> (This is actually a question about PATCH 1/3, I just noticed it here.
> Are there more cases like this?)

The trick with return 1 handlers is that they are deleted when there are
no more requests.  The pattern is:

...begin processing request...
qemu_set_fd_handler2(fd, ..., have_co_req);
qemu_coroutine_yield()
qemu_set_fd_handler2(fd, NULL, NULL, NULL, NULL); /* delete handler */
...finish processing request...

Stefan

Am 08.04.2013 um 17:31 hat Stefan Hajnoczi geschrieben:
> On Tue, Apr 02, 2013 at 10:37:24AM +0200, Kevin Wolf wrote:
> > Am 28.03.2013 um 22:52 hat Anthony Liguori geschrieben:
> > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> > > ---
> > >  block/sheepdog.c | 11 ++---------
> > >  1 file changed, 2 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/block/sheepdog.c b/block/sheepdog.c
> > > index bb67c4c..2bccd9b 100644
> > > --- a/block/sheepdog.c
> > > +++ b/block/sheepdog.c
> > > @@ -503,13 +503,6 @@ 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. */
> > 
> > Now you return 1 even when no request is pending (which is the case in
> > which no io_flush handler would be set before). Why is this correct?
> > (This is actually a question about PATCH 1/3, I just noticed it here.
> > Are there more cases like this?)
> 
> The trick with return 1 handlers is that they are deleted when there are
> no more requests.  The pattern is:
> 
> ...begin processing request...
> qemu_set_fd_handler2(fd, ..., have_co_req);
> qemu_coroutine_yield()
> qemu_set_fd_handler2(fd, NULL, NULL, NULL, NULL); /* delete handler */
> ...finish processing request...

Indeed. I thought I had seen a case where only the flush handler is
reset, but it seems I didn't look close enough.

Kevin