From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56138) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XUBLx-0005KQ-VQ for qemu-devel@nongnu.org; Wed, 17 Sep 2014 05:16:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XUBLm-0001B6-DA for qemu-devel@nongnu.org; Wed, 17 Sep 2014 05:16:45 -0400 Received: from averel.grnet-hq.admin.grnet.gr ([195.251.29.3]:48194) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XUBLm-00015N-6S for qemu-devel@nongnu.org; Wed, 17 Sep 2014 05:16:34 -0400 Message-ID: <54195155.2010001@grnet.gr> Date: Wed, 17 Sep 2014 12:16:05 +0300 From: Chrysostomos Nanakos MIME-Version: 1.0 References: <1410890642-6704-1-git-send-email-cnanakos@grnet.gr> <1410890642-6704-2-git-send-email-cnanakos@grnet.gr> <54189228.6090907@redhat.com> In-Reply-To: <54189228.6090907@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3] async: aio_context_new(): Handle event_notifier_init failure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pingfank@linux.vnet.ibm.com, famz@redhat.com, benoit@irqsave.net, jan.kiszka@siemens.com, stefanha@redhat.com, mjt@tls.msk.ru, kroosec@gmail.com, sw@weilnetz.de, pbonzini@redhat.com, afaerber@suse.de, aliguori@amazon.com On 09/16/2014 10:40 PM, Eric Blake wrote: > On 09/16/2014 12:04 PM, Chrysostomos Nanakos wrote: >> If event_notifier_init fails QEMU exits without printing >> any error information to the user. This commit adds an error >> message on failure: >> >> # qemu [...] > Showing the actual command line you used would be helpful. The problem raised after having a system with a low limit of open files and QEMU with 8 iothread objects. Do you believe that we should add such a command line in the commit description? The problem can be easily reproduced with any combination of low limit of open files and iothread objects or even a low limit of open files without the creation of iothreads. > >> qemu: Failed to initialize event notifier: Too many open files in system >> >> Signed-off-by: Chrysostomos Nanakos >> --- >> async.c | 16 +++++++++++----- >> include/block/aio.h | 2 +- >> include/qemu/main-loop.h | 2 +- >> iothread.c | 11 ++++++++++- >> main-loop.c | 9 +++++++-- >> qemu-img.c | 8 +++++++- >> qemu-io.c | 7 ++++++- >> qemu-nbd.c | 6 +++++- >> tests/test-aio.c | 10 +++++++++- >> tests/test-thread-pool.c | 10 +++++++++- >> tests/test-throttle.c | 10 +++++++++- >> vl.c | 5 +++-- >> 12 files changed, 78 insertions(+), 18 deletions(-) >> >> -AioContext *aio_context_new(void) >> +AioContext *aio_context_new(Error **errp) >> { >> + int ret; >> AioContext *ctx; >> ctx = (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioContext)); >> + ret = event_notifier_init(&ctx->notifier, false); >> + if (ret < 0) { >> + g_source_destroy(&ctx->source); > Does g_source_destroy() guarantee that errno is unmolested? If not, > >> + error_setg_errno(errp, -ret, "Failed to initialize event notifier"); > then this logs the wrong error. Swap the lines to be safe. Good catch! I believe that Benoit has covered this point. > >> + return NULL; >> + } >> + aio_set_event_notifier(ctx, &ctx->notifier, >> + (EventNotifierHandler *) >> + event_notifier_test_and_clear); >> ctx->pollfds = g_array_new(FALSE, FALSE, sizeof(GPollFD)); >> ctx->thread_pool = NULL; >> qemu_mutex_init(&ctx->bh_lock); >> rfifolock_init(&ctx->lock, aio_rfifolock_cb, ctx); >> - event_notifier_init(&ctx->notifier, false); > Is hoisting the event notifier init to occur before the mutex init going > to cause any grief? No I can't find any problem, none of the functions use the mutex on initialization. The mutex is being used for adding/removing BH's to/from the BH list. So it's safe at the moment to init mutex after the initialization of the event notifier. > >> +++ b/main-loop.c >> @@ -138,8 +139,12 @@ int qemu_init_main_loop(void) >> return ret; >> } >> >> + qemu_aio_context = aio_context_new(&local_error); >> + if (!qemu_aio_context) { >> + error_propagate(errp, local_error); >> + return -1; >> + } > Can the earlier call to qemu_signal_init() consume any resources that > are now leaked? I can only see signal set manipulation and the setup of a signal handler. Nothing is leaked here. > >> @@ -205,10 +206,17 @@ static void test_cancel(void) >> int main(int argc, char **argv) >> { >> int ret; >> + Error *local_error; >> >> init_clocks(); >> >> - ctx = aio_context_new(); >> + ctx = aio_context_new(&local_error); >> + if (!ctx) { >> + error_report("Failed to create AIO Context: \'%s\'", > Use of \' inside "" is unusual. (Multiple times in your patch) I will fix that and if everyone agrees I will resend the patch. Regards, Chrysostomos.