Re: [Qemu-devel] [PATCH 17/22] ssh: use BlockDriverState's AioContext

[Richard W.M. Jones]

On Thu, May 01, 2014 at 04:54:41PM +0200, Stefan Hajnoczi wrote:
> Drop the assumption that we're using the main AioContext.  Use
> bdrv_get_aio_context() to register fd handlers in the right AioContext
> for this BlockDriverState.
> 
> The .bdrv_detach_aio_context() and .bdrv_attach_aio_context() interfaces
> are not needed since no fd handlers, timers, or BHs stay registered when
> requests have been drained.
> 
> For now this doesn't make much difference but will allow ssh to work in
> IOThread instances in the future.
> 
> Cc: Richard W.M. Jones <address@hidden>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  block/ssh.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/block/ssh.c b/block/ssh.c
> index aa63c9d..3f4a9fb 100644
> --- a/block/ssh.c
> +++ b/block/ssh.c
> @@ -742,7 +742,7 @@ static void restart_coroutine(void *opaque)
>      qemu_coroutine_enter(co, NULL);
>  }
>  
> -static coroutine_fn void set_fd_handler(BDRVSSHState *s)
> +static coroutine_fn void set_fd_handler(BDRVSSHState *s, BlockDriverState 
> *bs)
>  {
>      int r;
>      IOHandler *rd_handler = NULL, *wr_handler = NULL;
> @@ -760,24 +760,26 @@ static coroutine_fn void set_fd_handler(BDRVSSHState *s)
>      DPRINTF("s->sock=%d rd_handler=%p wr_handler=%p", s->sock,
>              rd_handler, wr_handler);
>  
> -    qemu_aio_set_fd_handler(s->sock, rd_handler, wr_handler, co);
> +    aio_set_fd_handler(bdrv_get_aio_context(bs), s->sock,
> +                       rd_handler, wr_handler, co);
>  }
>  
> -static coroutine_fn void clear_fd_handler(BDRVSSHState *s)
> +static coroutine_fn void clear_fd_handler(BDRVSSHState *s,
> +                                          BlockDriverState *bs)
>  {
>      DPRINTF("s->sock=%d", s->sock);
> -    qemu_aio_set_fd_handler(s->sock, NULL, NULL, NULL);
> +    aio_set_fd_handler(bdrv_get_aio_context(bs), s->sock, NULL, NULL, NULL);
>  }
>  
>  /* A non-blocking call returned EAGAIN, so yield, ensuring the
>   * handlers are set up so that we'll be rescheduled when there is an
>   * interesting event on the socket.
>   */
> -static coroutine_fn void co_yield(BDRVSSHState *s)
> +static coroutine_fn void co_yield(BDRVSSHState *s, BlockDriverState *bs)
>  {
> -    set_fd_handler(s);
> +    set_fd_handler(s, bs);
>      qemu_coroutine_yield();
> -    clear_fd_handler(s);
> +    clear_fd_handler(s, bs);
>  }
>  
>  /* SFTP has a function `libssh2_sftp_seek64' which seeks to a position
> @@ -807,7 +809,7 @@ static void ssh_seek(BDRVSSHState *s, int64_t offset, int 
> flags)
>      }
>  }
>  
> -static coroutine_fn int ssh_read(BDRVSSHState *s,
> +static coroutine_fn int ssh_read(BDRVSSHState *s, BlockDriverState *bs,
>                                   int64_t offset, size_t size,
>                                   QEMUIOVector *qiov)
>  {
> @@ -840,7 +842,7 @@ static coroutine_fn int ssh_read(BDRVSSHState *s,
>          DPRINTF("sftp_read returned %zd", r);
>  
>          if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) {
> -            co_yield(s);
> +            co_yield(s, bs);
>              goto again;
>          }
>          if (r < 0) {
> @@ -875,14 +877,14 @@ static coroutine_fn int ssh_co_readv(BlockDriverState 
> *bs,
>      int ret;
>  
>      qemu_co_mutex_lock(&s->lock);
> -    ret = ssh_read(s, sector_num * BDRV_SECTOR_SIZE,
> +    ret = ssh_read(s, bs, sector_num * BDRV_SECTOR_SIZE,
>                     nb_sectors * BDRV_SECTOR_SIZE, qiov);
>      qemu_co_mutex_unlock(&s->lock);
>  
>      return ret;
>  }
>  
> -static int ssh_write(BDRVSSHState *s,
> +static int ssh_write(BDRVSSHState *s, BlockDriverState *bs,
>                       int64_t offset, size_t size,
>                       QEMUIOVector *qiov)
>  {
> @@ -910,7 +912,7 @@ static int ssh_write(BDRVSSHState *s,
>          DPRINTF("sftp_write returned %zd", r);
>  
>          if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) {
> -            co_yield(s);
> +            co_yield(s, bs);
>              goto again;
>          }
>          if (r < 0) {
> @@ -929,7 +931,7 @@ static int ssh_write(BDRVSSHState *s,
>           */
>          if (r == 0) {
>              ssh_seek(s, offset + written, SSH_SEEK_WRITE|SSH_SEEK_FORCE);
> -            co_yield(s);
> +            co_yield(s, bs);
>              goto again;
>          }
>  
> @@ -957,7 +959,7 @@ static coroutine_fn int ssh_co_writev(BlockDriverState 
> *bs,
>      int ret;
>  
>      qemu_co_mutex_lock(&s->lock);
> -    ret = ssh_write(s, sector_num * BDRV_SECTOR_SIZE,
> +    ret = ssh_write(s, bs, sector_num * BDRV_SECTOR_SIZE,
>                      nb_sectors * BDRV_SECTOR_SIZE, qiov);
>      qemu_co_mutex_unlock(&s->lock);
>  
> @@ -978,7 +980,7 @@ static void unsafe_flush_warning(BDRVSSHState *s, const 
> char *what)
>  
>  #ifdef HAS_LIBSSH2_SFTP_FSYNC
>  
> -static coroutine_fn int ssh_flush(BDRVSSHState *s)
> +static coroutine_fn int ssh_flush(BDRVSSHState *s, BlockDriverState *bs)
>  {
>      int r;
>  
> @@ -986,7 +988,7 @@ static coroutine_fn int ssh_flush(BDRVSSHState *s)
>   again:
>      r = libssh2_sftp_fsync(s->sftp_handle);
>      if (r == LIBSSH2_ERROR_EAGAIN || r == LIBSSH2_ERROR_TIMEOUT) {
> -        co_yield(s);
> +        co_yield(s, bs);
>          goto again;
>      }
>      if (r == LIBSSH2_ERROR_SFTP_PROTOCOL &&
> @@ -1008,7 +1010,7 @@ static coroutine_fn int ssh_co_flush(BlockDriverState 
> *bs)
>      int ret;
>  
>      qemu_co_mutex_lock(&s->lock);
> -    ret = ssh_flush(s);
> +    ret = ssh_flush(s, bs);
>      qemu_co_mutex_unlock(&s->lock);
>  
>      return ret;
> -- 
> 1.9.0

As this appears to simply be about adding a context pointer to several
calls, it seems to be a simple, mechanical change, so ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW