Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report()

[Stefan Hajnoczi]

On Mon, May 26, 2014 at 05:59:03PM +0200, Markus Armbruster wrote:
> Kevin Wolf <address@hidden> writes:
> 
> > Am 26.05.2014 um 17:02 hat Markus Armbruster geschrieben:
> >> Stefan Hajnoczi <address@hidden> writes:
> >> 
> >> > On Mon, May 26, 2014 at 09:44:03AM +0800, Le Tan wrote:
> >> >> Replace fprintf(stderr,...) with error_report() in files block/*, 
> >> >> block.c,
> >> >> block-migration.c and blockdev.c. The trailing "\n"s of the @fmt 
> >> >> argument
> >> >> have been removed because @fmt of error_report() should not
> >> >> contain newline.
> >> >> 
> >> >> Signed-off-by: Le Tan <address@hidden>
> >> >> ---
> >> >>  block-migration.c      |    6 +--
> >> >>  block.c                |    4 +-
> >> >>  block/qcow2-refcount.c |  115 
> >> >> +++++++++++++++++++++---------------------
> >> >>  block/qcow2.c          |   16 +++---
> >> >>  block/raw-posix.c      |    8 ++-
> >> >>  block/raw-win32.c      |    6 +--
> >> >>  block/ssh.c            |    2 +-
> >> >>  block/vdi.c            |   14 +++---
> >> >>  block/vmdk.c           |   15 +++---
> >> >>  block/vpc.c            |    4 +-
> >> >>  block/vvfat.c | 129
> >> >> ++++++++++++++++++++++++------------------------
> >> >>  blockdev.c             |    6 +--
> >> >>  12 files changed, 159 insertions(+), 166 deletions(-)
> >> >
> >> > There is one thing that worries me about this:
> >> >
> >> > error_report() assumes that the QEMU global mutex is held in order to
> >> > access the "current monitor".
> >> 
> >> Global variable cur_mon, non-null while we're executing a monitor
> >> command.
> >
> > The important part here is that it's indeed global and not thread-local.
> >
> >> > This is problematic for code in the read/write/flush path (like qcow2
> >> > refcount allocation) since it can be invoked from a virtio-blk
> >> > data-plane thread (where the QEMU global mutex is not held).
> >> 
> >> error_report() reports to the current monitor when "in monitor context",
> >> i.e. while executing a monitor command, i.e. while cur_mon isn't null.
> >> 
> >> Can we ever be in monitor context (cur_mon not null) without holding the
> >> global mutex?
> >
> > The right question is: Can a thread (= the main loop thread) ever be in
> > monitor context while another thread (= dataplane thread) is executing
> > block driver code and doesn't hold the global mutex?
> >
> > If I understand dataplane correctly, the whole point of it is that the
> > answer to this is yes.
> 
> Well, the answer used to be "no".  Once upon a time because there was
> just a single thread, later on because only one thread ever executed
> "interesting" code.
> 
> I'm *not* saying this should remain the case.  I'm trying to find out
> what needs to be done around cur_mon when we break the assumption behind
> it.
> 
> Would making cur_mon thread-local suffice?

It would suffice.

I think there is no code that invokes error_report() from another thread
on purpose (i.e. it knows the main thread is waiting), so making it
thread-local should not introduce a regression.

Stefan