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

[Stefan Hajnoczi]

On Thu, May 1, 2014 at 5:03 PM, Richard W.M. Jones <address@hidden> wrote:
> 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.

Yes.  I wrote about the reason for the changes and what to look out
for in the cover letter of this series if you want to know more.

Stefan