Re: [Qemu-block] [PATCH v2] rbd: make the code more readable

[Stefan Hajnoczi]

On Sat, Oct 15, 2016 at 04:26:13PM +0800, Xiubo Li wrote:
> Make it a bit clearer and more readable.
> 
> Signed-off-by: Xiubo Li 
> CC: John Snow 
> ---
> 
> V2:
> - Advice from John Snow. Thanks.
> 
> 
>  block/rbd.c | 25 -
>  1 file changed, 12 insertions(+), 13 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 18/18] aio: convert from RFifoLock to QemuRecMutex

[Stefan Hajnoczi]

On Thu, Oct 13, 2016 at 07:34:22PM +0200, Paolo Bonzini wrote:
> It is simpler and a bit faster, and QEMU does not need the contention
> callbacks (and thus the fairness) anymore.
> 
> Reviewed-by: Fam Zheng 
> Signed-off-by: Paolo Bonzini 
> ---
>  async.c  |  8 ++---
>  include/block/aio.h  |  3 +-
>  include/qemu/rfifolock.h | 54 
>  tests/.gitignore |  1 -
>  tests/Makefile.include   |  2 --
>  tests/test-rfifolock.c   | 91 
> 
>  util/Makefile.objs   |  1 -
>  util/rfifolock.c | 78 -
>  8 files changed, 5 insertions(+), 233 deletions(-)
>  delete mode 100644 include/qemu/rfifolock.h
>  delete mode 100644 tests/test-rfifolock.c
>  delete mode 100644 util/rfifolock.c

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 15/18] block: only call aio_poll on the current thread's AioContext

[Stefan Hajnoczi]

On Thu, Oct 13, 2016 at 07:34:19PM +0200, Paolo Bonzini wrote:
> aio_poll is not thread safe; for example bdrv_drain can hang if
> the last in-flight I/O operation is completed in the I/O thread after
> the main thread has checked bs->in_flight.
> 
> The bug remains latent as long as all of it is called within
> aio_context_acquire/aio_context_release, but this will change soon.
> 
> To fix this, if bdrv_drain is called from outside the I/O thread,
> signal the main AioContext through a dummy bottom half.  The event
> loop then only runs in the I/O thread.
> 
> Reviewed-by: Fam Zheng 
> Signed-off-by: Paolo Bonzini 
> ---
>  async.c   |  1 +
>  block.c   |  2 ++
>  block/io.c|  7 +++
>  include/block/block.h | 24 +---
>  include/block/block_int.h |  1 +
>  5 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/async.c b/async.c
> index f30d011..fb37b03 100644
> --- a/async.c
> +++ b/async.c
> @@ -61,6 +61,7 @@ void aio_bh_schedule_oneshot(AioContext *ctx, QEMUBHFunc 
> *cb, void *opaque)
>  smp_wmb();
>  ctx->first_bh = bh;
>  qemu_mutex_unlock(>bh_lock);
> +aio_notify(ctx);
>  }
>  
>  QEMUBH *aio_bh_new(AioContext *ctx, QEMUBHFunc *cb, void *opaque)
> diff --git a/block.c b/block.c
> index fbe485c..a17baab 100644
> --- a/block.c
> +++ b/block.c
> @@ -2090,7 +2090,9 @@ int bdrv_reopen_multiple(AioContext *ctx, 
> BlockReopenQueue *bs_queue, Error **er
>  
>  assert(bs_queue != NULL);
>  
> +aio_context_release(ctx);
>  bdrv_drain_all();
> +aio_context_acquire(ctx);
>  
>  QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
>  if (bdrv_reopen_prepare(_entry->state, bs_queue, _err)) {
> diff --git a/block/io.c b/block/io.c
> index 7d3dcfc..cd28909 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -474,8 +474,15 @@ void bdrv_inc_in_flight(BlockDriverState *bs)
>  atomic_inc(>in_flight);
>  }
>  
> +static void dummy_bh_cb(void *opaque)
> +{
> +}
> +
>  void bdrv_wakeup(BlockDriverState *bs)
>  {
> +if (bs->wakeup) {
> +aio_bh_schedule_oneshot(qemu_get_aio_context(), dummy_bh_cb, NULL);
> +}
>  }

Why use a dummy BH instead of aio_notify()?

>  
>  void bdrv_dec_in_flight(BlockDriverState *bs)
> diff --git a/include/block/block.h b/include/block/block.h
> index ba4318b..72d5d8e 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -343,9 +343,27 @@ void bdrv_drain_all(void);
>  #define bdrv_poll_while(bs, cond) ({   \
>  bool waited_ = false;  \
>  BlockDriverState *bs_ = (bs);  \
> -while ((cond)) {   \
> -aio_poll(bdrv_get_aio_context(bs_), true); \
> -waited_ = true;\
> +AioContext *ctx_ = bdrv_get_aio_context(bs_);  \
> +if (aio_context_in_iothread(ctx_)) {   \
> +while ((cond)) {   \
> +aio_poll(ctx_, true);  \
> +waited_ = true;\
> +}  \
> +} else {   \
> +assert(qemu_get_current_aio_context() ==   \
> +   qemu_get_aio_context());\

The assumption is that IOThread #1 will never call bdrv_poll_while() on
IOThread #2's AioContext.  I believe that is true today.  Is this what
you had in mind?

Please add a comment since it's not completely obvious from the assert
expression.

> +/* Ask bdrv_dec_in_flight to wake up the main  \
> + * QEMU AioContext.\
> + */\
> +assert(!bs_->wakeup);  \
> +bs_->wakeup = true;\
> +while ((cond)) {   \
> +aio_context_release(ctx_); \
> +aio_poll(qemu_get_aio_context(), true);\
> +aio_context_acquire(ctx_); \
> +waited_ = true;\
> +}  \
> +bs_->wakeup = false;   \
>  }  \
>  waited_; })
>  
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 11f877b..0516f62 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -470,6 +470,7 @@ struct BlockDriverState {
>  NotifierWithReturnList before_write_notifiers;
>  
>  /* number of in-flight requests; overall and serialising */
> +bool wakeup;
>  unsigned int in_flight;
>  unsigned int 

Re: [Qemu-block] [Qemu-devel] [PATCH 13/18] replication: pass BlockDriverState to reopen_backing_file

[Stefan Hajnoczi]

On Thu, Oct 13, 2016 at 07:34:17PM +0200, Paolo Bonzini wrote:
> This will be needed in the next patch to retrieve the AioContext.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block/replication.c | 21 -
>  1 file changed, 12 insertions(+), 9 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 14/18] block: prepare bdrv_reopen_multiple to release AioContext

[Stefan Hajnoczi]

On Thu, Oct 13, 2016 at 07:34:18PM +0200, Paolo Bonzini wrote:
> After the next patch bdrv_drain_all will have to be called without holding any
> AioContext.  Prepare to do this by adding an AioContext argument to
> bdrv_reopen_multiple.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block.c   | 4 ++--
>  block/commit.c| 2 +-
>  block/replication.c   | 3 ++-
>  include/block/block.h | 2 +-
>  qemu-io-cmds.c| 2 +-
>  5 files changed, 7 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 11/18] aio: introduce qemu_get_current_aio_context

[Stefan Hajnoczi]

On Thu, Oct 13, 2016 at 07:34:15PM +0200, Paolo Bonzini wrote:
> This will be used by bdrv_poll_while (and thus by bdrv_drain)
> to choose how to wait for I/O completion.
> 
> Reviewed-by: Fam Zheng 
> Signed-off-by: Paolo Bonzini 
> ---
>  include/block/aio.h | 15 +++
>  iothread.c  |  9 +
>  stubs/Makefile.objs |  1 +
>  stubs/iothread.c|  8 
>  4 files changed, 33 insertions(+)
>  create mode 100644 stubs/iothread.c
> 
> diff --git a/include/block/aio.h b/include/block/aio.h
> index b9fe2cb..60a4f21 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -453,6 +453,21 @@ static inline bool aio_node_check(AioContext *ctx, bool 
> is_external)
>  }
>  
>  /**
> + * Return the AioContext whose event loop runs in the current I/O thread.
> + */
> +AioContext *qemu_get_current_aio_context(void);

This doc comment left me wondering what "I/O thread" means.  Looking at
the implementation this function returns the current IOThread
AioContext, otherwise it returns the QEMU main loop AioContext.

I think the following captures the semantics:

"Return the AioContext whose event loop runs in the current thread.

If called from an IOThread this will be the IOThread's AioContext.  If
called from another thread it will be the main loop AioContext."

> +
> +/**
> + * @ctx: the aio context
> + *
> + * Return whether we are running in the I/O thread that manages @ctx.

s%I/O %%

This function works within an IOThread but also in any other thread.

> + */
> +static inline bool aio_context_in_iothread(AioContext *ctx)
> +{
> +return ctx == qemu_get_current_aio_context();
> +}
> +
> +/**
>   * aio_context_setup:
>   * @ctx: the aio context
>   *
> diff --git a/iothread.c b/iothread.c
> index fbeb8de..62c8796 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -20,6 +20,7 @@
>  #include "qmp-commands.h"
>  #include "qemu/error-report.h"
>  #include "qemu/rcu.h"
> +#include "qemu/main-loop.h"
>  
>  typedef ObjectClass IOThreadClass;
>  
> @@ -28,6 +29,13 @@ typedef ObjectClass IOThreadClass;
>  #define IOTHREAD_CLASS(klass) \
> OBJECT_CLASS_CHECK(IOThreadClass, klass, TYPE_IOTHREAD)
>  
> +static __thread IOThread *my_iothread;
> +
> +AioContext *qemu_get_current_aio_context(void)
> +{
> +return my_iothread ? my_iothread->ctx : qemu_get_aio_context();
> +}
> +
>  static void *iothread_run(void *opaque)
>  {
>  IOThread *iothread = opaque;
> @@ -35,6 +43,7 @@ static void *iothread_run(void *opaque)
>  
>  rcu_register_thread();
>  
> +my_iothread = iothread;
>  qemu_mutex_lock(>init_done_lock);
>  iothread->thread_id = qemu_get_thread_id();
>  qemu_cond_signal(>init_done_cond);
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index c5850e8..84b9d9e 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -17,6 +17,7 @@ stub-obj-y += gdbstub.o
>  stub-obj-y += get-fd.o
>  stub-obj-y += get-next-serial.o
>  stub-obj-y += get-vm-name.o
> +stub-obj-y += iothread.o
>  stub-obj-y += iothread-lock.o
>  stub-obj-y += is-daemonized.o
>  stub-obj-y += machine-init-done.o
> diff --git a/stubs/iothread.c b/stubs/iothread.c
> new file mode 100644
> index 000..8cc9e28
> --- /dev/null
> +++ b/stubs/iothread.c
> @@ -0,0 +1,8 @@
> +#include "qemu/osdep.h"
> +#include "block/aio.h"
> +#include "qemu/main-loop.h"
> +
> +AioContext *qemu_get_current_aio_context(void)
> +{
> +return qemu_get_aio_context();
> +}
> -- 
> 2.7.4
> 
> 
> 


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 10/18] sheepdog: use bdrv_poll_while and bdrv_wakeup

[Stefan Hajnoczi]

On Thu, Oct 13, 2016 at 07:34:14PM +0200, Paolo Bonzini wrote:
> These ensure that bdrv_poll_while will exit for a BlockDriverState
> that is not in the main AioContext.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block/sheepdog.c | 67 
> 
>  1 file changed, 38 insertions(+), 29 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 09/18] nfs: use bdrv_poll_while and bdrv_wakeup

[Stefan Hajnoczi]

On Thu, Oct 13, 2016 at 07:34:13PM +0200, Paolo Bonzini wrote:
> These will make it possible to use nfs_get_allocated_file_size on
> a file that is not in the main AioContext.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block/nfs.c | 47 +--
>  1 file changed, 29 insertions(+), 18 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 08/18] nfs: move nfs_set_events out of the while loops

[Stefan Hajnoczi]

On Thu, Oct 13, 2016 at 07:34:12PM +0200, Paolo Bonzini wrote:
> nfs_set_events only needs to be called once before entering the
> while loop; afterwards, nfs_process_read and nfs_process_write
> take care of it.
> 
> Signed-off-by: Paolo Bonzini 
> ---
>  block/nfs.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 07/18] block: introduce bdrv_poll_while and bdrv_wakeup

[Stefan Hajnoczi]

On Thu, Oct 13, 2016 at 07:34:11PM +0200, Paolo Bonzini wrote:
> @@ -485,9 +474,14 @@ void bdrv_inc_in_flight(BlockDriverState *bs)
>  atomic_inc(>in_flight);
>  }
>  
> +void bdrv_wakeup(BlockDriverState *bs)
> +{
> +}

Please write a doc comment explaining the semantics of this new API.

Since it's a nop here it may be better to introduce bdrv_wakeup() in
another patch.


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 02/18] blockjob: introduce .drain callback for jobs

[Stefan Hajnoczi]

On Thu, Oct 13, 2016 at 07:34:06PM +0200, Paolo Bonzini wrote:
> +static void backup_drain(BlockJob *job)
> +{
> +BackupBlockJob *s = container_of(job, BackupBlockJob, common);
> +
> +/* Need to keep a reference in case blk_drain triggers execution
> + * of backup_complete...
> + */
> +if (s->target) {
> +blk_ref(s->target);
> +blk_drain(s->target);
> +blk_unref(s->target);
> +}
[...]
> @@ -331,6 +346,7 @@ static void backup_complete(BlockJob *job, void *opaque)
>  BackupCompleteData *data = opaque;
>  
>  blk_unref(s->target);
> +s->target = NULL;

Will blk_unref(s->target) segfault since backup_complete() has set it to
NULL?  I expected backup_drain() to stash the pointer in a local
variable to avoid using s->target.


signature.asc
Description: PGP signature

Re: [Qemu-block] [v2 2/5] block/ssh: Add InetSocketAddress and accept it

[Ashijeet Acharya]

>>>  /* Check the remote host's key against known_hosts. */
>>> -ret = check_host_key(s, host, port, host_key_check, errp);
>>> +ret = check_host_key(s, s->inet->host, port, host_key_check,
>>
>> But then you're still using the port here... And I can't come up with a
>> way (not even a bad one) to get the numeric port. Maybe interpret the
>> addrinfo in inet_connect_saddr()? But getting that information out would
>> be ugly, if even possible...
>>
>
> Will using strtol() do any good?

Better to use qemu_strtol() from cutils.c ?

Ashijeet

Re: [Qemu-block] [v2 2/5] block/ssh: Add InetSocketAddress and accept it

[Ashijeet Acharya]

On Sun, Oct 16, 2016 at 4:00 AM, Max Reitz  wrote:
> On 15.10.2016 11:04, Ashijeet Acharya wrote:
>> Add InetSocketAddress compatibility to SSH driver.
>>
>> Add a new option "server" to the SSH block driver which then accepts
>> a InetSocketAddress.
>>
>> "host" and "port" are supported as legacy options and are mapped to
>> their InetSocketAddress representation.
>>
>> Signed-off-by: Ashijeet Acharya 
>> ---
>>  block/ssh.c | 83 
>> ++---
>>  1 file changed, 74 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/ssh.c b/block/ssh.c
>> index 75cb7bc..3b18907 100644
>> --- a/block/ssh.c
>> +++ b/block/ssh.c
>> @@ -30,10 +30,14 @@
>>  #include "block/block_int.h"
>>  #include "qapi/error.h"
>>  #include "qemu/error-report.h"
>> +#include "qemu/cutils.h"
>>  #include "qemu/sockets.h"
>>  #include "qemu/uri.h"
>> +#include "qapi-visit.h"
>>  #include "qapi/qmp/qint.h"
>>  #include "qapi/qmp/qstring.h"
>> +#include "qapi/qobject-input-visitor.h"
>> +#include "qapi/qobject-output-visitor.h"
>>
>>  /* DEBUG_SSH=1 enables the DPRINTF (debugging printf) statements in
>>   * this block driver code.
>> @@ -74,6 +78,8 @@ typedef struct BDRVSSHState {
>>   */
>>  LIBSSH2_SFTP_ATTRIBUTES attrs;
>>
>> +InetSocketAddress *inet;
>> +
>>  /* Used to warn if 'flush' is not supported. */
>>  char *hostport;
>>  bool unsafe_flush_warning;
>> @@ -263,7 +269,9 @@ static bool ssh_has_filename_options_conflict(QDict 
>> *options, Error **errp)
>>  !strcmp(qe->key, "port") ||
>>  !strcmp(qe->key, "path") ||
>>  !strcmp(qe->key, "user") ||
>> -!strcmp(qe->key, "host_key_check"))
>> +!strcmp(qe->key, "host_key_check") ||
>> +!strcmp(qe->key, "server") ||
>
> I know I've done the same in my series, but I'll actually drop this
> condition from the next version; I'm not actually handling the case
> where the destination address is not flattened, and neither are you
> (we're both using qdict_extract_subqdict() instead of testing whether
> "server" is an object on its own), so I think you should drop it as well
> and just test for the prefix.
>
> It doesn't harm to test for "server" itself, but I think it's a bit
> confusing still, since you (we) are not dealing with that possibility
> anywhere else.

Yes, I have dropped this.

>> +!strstart(qe->key, "server.", NULL))
>
> It should be just "strstart", not "!strstart", because the function
> returns 1 if the prefix matches.

Fixed.

>
>>  {
>>  error_setg(errp, "Option '%s' cannot be used with a file name",
>> qe->key);
>> @@ -555,13 +563,66 @@ static QemuOptsList ssh_runtime_opts = {
>>  },
>>  };
>>
>> +static bool ssh_process_legacy_socket_options(QDict *output_opts,
>> +  QemuOpts *legacy_opts,
>> +  Error **errp)
>> +{
>> +const char *host = qemu_opt_get(legacy_opts, "host");
>> +const char *port = qemu_opt_get(legacy_opts, "port");
>> +
>> +if (!host && port) {
>> +error_setg(errp, "port may not be used without host");
>> +return false;
>> +} else {
>> +qdict_put(output_opts, "server.host", qstring_from_str(host));
>
> This will segfault if host is NULL. You shouldn't go into this branch at
> all if host is not set.

Yes, I have put this in a different 'if' like:

if (host) {
   qdict_put();
   ...
}

>
>> +qdict_put(output_opts, "server.port",
>> +  qstring_from_str(port ?: stringify(22)));
>> +}
>> +
>> +return true;
>> +}
>> +
>> +static InetSocketAddress *ssh_config(BDRVSSHState *s, QDict *options,
>> + Error **errp)
>> +{
>> +InetSocketAddress *inet = NULL;
>> +QDict *addr = NULL;
>> +QObject *crumpled_addr = NULL;
>> +Visitor *iv = NULL;
>> +Error *local_error = NULL;
>> +
>> +qdict_extract_subqdict(options, , "server.");
>> +if (!qdict_size(addr)) {
>> +error_setg(errp, "SSH server address missing");
>> +goto out;
>> +}
>> +
>> +crumpled_addr = qdict_crumple(addr, true, errp);
>> +if (!crumpled_addr) {
>> +goto out;
>> +}
>> +
>> +iv = qobject_input_visitor_new_autocast(crumpled_addr, true, 1, true);
>
> In contrast to what Kevin said in v1, I think you do not want to use
> autocast here.
>
> Or, to be more specific, it's difficult. The thing is that the autocast
> documentation says: "Any scalar values in the @obj input data structure
> should always be represented as strings".
>
> So if you do use the autocast version, command line works great because
> from there everything comes as a string. But blockdev-add no longer
> works because from there everything comes with the correct type (and you
> cannot give it the wrong type). Case in point, this happens