[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report() |
Date: |
Tue, 27 May 2014 17:59:07 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Stefan Hajnoczi <address@hidden> writes:
> 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.
So, who's going to cook up the patch to make it thread-local?
- [Qemu-trivial] [PATCH v4] block: replace fprintf(stderr, ...) with error_report(), Le Tan, 2014/05/25
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report(), Stefan Hajnoczi, 2014/05/26
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report(), Markus Armbruster, 2014/05/26
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report(), Kevin Wolf, 2014/05/26
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report(), Markus Armbruster, 2014/05/26
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report(), Stefan Hajnoczi, 2014/05/27
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report(),
Markus Armbruster <=