From patchwork Wed Jun 29 14:15:19 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900173 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 59939C433EF for ; Wed, 29 Jun 2022 14:31:35 +0000 (UTC) Received: from localhost ([::1]:45472 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6Yj7-0005zq-GU for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:31:33 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36308) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTw-0007uo-A9 for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:52 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:58533) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTr-00020L-R9 for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:51 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512147; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=i+zk7SrzYoSHnf5yoRasYjwBwTIG+52hR950VPuNieg=; b=RgaaofqzSXXF1zosmlQUdRdpGpyDhfcYT+l1SSRb5RkcHeYi1YFun9ZJtbiyNn1ETwSIpG Py2o5eZSz63oCXjctqO8iqhwxZUid6BkD9eaCzY11PCGJEtwD5fMrVBmEd7B+S7jWm/JiX g5rlWkb34VlQFF7JV2dQKImq5TFs6KM= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-380-81twkzbUP-qoa3kGB5cXXQ-1; Wed, 29 Jun 2022 10:15:43 -0400 X-MC-Unique: 81twkzbUP-qoa3kGB5cXXQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 51C093C0F389; Wed, 29 Jun 2022 14:15:43 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id BD7C818EAA; Wed, 29 Jun 2022 14:15:41 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito , Vladimir Sementsov-Ogievskiy Subject: [PATCH v8 01/20] job.c: make job_mutex and job_lock/unlock() public Date: Wed, 29 Jun 2022 10:15:19 -0400 Message-Id: <20220629141538.3400679-2-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" job mutex will be used to protect the job struct elements and list, replacing AioContext locks. Right now use a shared lock for all jobs, in order to keep things simple. Once the AioContext lock is gone, we can introduce per-job locks. To simplify the switch from aiocontext to job lock, introduce *nop* lock/unlock functions and macros. We want to always call job_lock/unlock outside the AioContext locks, and not vice-versa, otherwise we might get a deadlock. This is not straightforward to do, and that's why we start with nop functions. Once everything is protected by job_lock/unlock, we can change the nop into an actual mutex and remove the aiocontext lock. Since job_mutex is already being used, add static real_job_{lock/unlock} for the existing usage. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi Reviewed-by: Vladimir Sementsov-Ogievskiy --- include/qemu/job.h | 24 ++++++++++++++++++++++++ job.c | 35 +++++++++++++++++++++++------------ 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index c105b31076..d1192ffd61 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -303,6 +303,30 @@ typedef enum JobCreateFlags { JOB_MANUAL_DISMISS = 0x04, } JobCreateFlags; +extern QemuMutex job_mutex; + +#define JOB_LOCK_GUARD() /* QEMU_LOCK_GUARD(&job_mutex) */ + +#define WITH_JOB_LOCK_GUARD() /* WITH_QEMU_LOCK_GUARD(&job_mutex) */ + +/** + * job_lock: + * + * Take the mutex protecting the list of jobs and their status. + * Most functions called by the monitor need to call job_lock + * and job_unlock manually. On the other hand, function called + * by the block jobs themselves and by the block layer will take the + * lock for you. + */ +void job_lock(void); + +/** + * job_unlock: + * + * Release the mutex protecting the list of jobs and their status. + */ +void job_unlock(void); + /** * Allocate and return a new job transaction. Jobs can be added to the * transaction using job_txn_add_job(). diff --git a/job.c b/job.c index 075c6f3a20..2b4ffca9d4 100644 --- a/job.c +++ b/job.c @@ -32,6 +32,12 @@ #include "trace/trace-root.h" #include "qapi/qapi-events-job.h" +/* + * job_mutex protects the jobs list, but also makes the + * struct job fields thread-safe. + */ +QemuMutex job_mutex; + static QLIST_HEAD(, Job) jobs = QLIST_HEAD_INITIALIZER(jobs); /* Job State Transition Table */ @@ -74,17 +80,22 @@ struct JobTxn { int refcnt; }; -/* Right now, this mutex is only needed to synchronize accesses to job->busy - * and job->sleep_timer, such as concurrent calls to job_do_yield and - * job_enter. */ -static QemuMutex job_mutex; +void job_lock(void) +{ + /* nop */ +} + +void job_unlock(void) +{ + /* nop */ +} -static void job_lock(void) +static void real_job_lock(void) { qemu_mutex_lock(&job_mutex); } -static void job_unlock(void) +static void real_job_unlock(void) { qemu_mutex_unlock(&job_mutex); } @@ -450,21 +461,21 @@ void job_enter_cond(Job *job, bool(*fn)(Job *job)) return; } - job_lock(); + real_job_lock(); if (job->busy) { - job_unlock(); + real_job_unlock(); return; } if (fn && !fn(job)) { - job_unlock(); + real_job_unlock(); return; } assert(!job->deferred_to_main_loop); timer_del(&job->sleep_timer); job->busy = true; - job_unlock(); + real_job_unlock(); aio_co_enter(job->aio_context, job->co); } @@ -481,13 +492,13 @@ void job_enter(Job *job) * called explicitly. */ static void coroutine_fn job_do_yield(Job *job, uint64_t ns) { - job_lock(); + real_job_lock(); if (ns != -1) { timer_mod(&job->sleep_timer, ns); } job->busy = false; job_event_idle(job); - job_unlock(); + real_job_unlock(); qemu_coroutine_yield(); /* Set by job_enter_cond() before re-entering the coroutine. */ From patchwork Wed Jun 29 14:15:20 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900214 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 71291C433EF for ; Wed, 29 Jun 2022 14:34:15 +0000 (UTC) Received: from localhost ([::1]:53144 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6Yli-0003fd-Eu for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:34:14 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36318) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTx-0007up-GO for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:53 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:44048) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTs-00021J-Lg for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:52 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512147; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=CX0JozOiowan0mYy3TlgKF8JCuNmamOAvf6f9rxg/Zs=; b=YjhRvV30VWD2CAwMjk42UTqrHEF8vRMfLLKodb90n6iLgQhxPyOruwgBOOgillIHgnNLoZ SDkgd6NNn4dABdon9ZOTiYkUfjjvXlu7p70W679MmxIRk742wFFMyRwk9Ft9FE7J88ce01 zPXMjtkqlX1sP4jj5p913jDajVcaDBg= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-554-09Obyv44NImYt_rWRk8m6Q-1; Wed, 29 Jun 2022 10:15:44 -0400 X-MC-Unique: 09Obyv44NImYt_rWRk8m6Q-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id ACFFC1019C8B; Wed, 29 Jun 2022 14:15:43 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id 58F3C18EAA; Wed, 29 Jun 2022 14:15:43 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito , Vladimir Sementsov-Ogievskiy Subject: [PATCH v8 02/20] job.h: categorize fields in struct Job Date: Wed, 29 Jun 2022 10:15:20 -0400 Message-Id: <20220629141538.3400679-3-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Categorize the fields in struct Job to understand which ones need to be protected by the job mutex and which don't. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Stefan Hajnoczi --- include/qemu/job.h | 61 +++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 25 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index d1192ffd61..876e13d549 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -40,27 +40,52 @@ typedef struct JobTxn JobTxn; * Long-running operation. */ typedef struct Job { + + /* Fields set at initialization (job_create), and never modified */ + /** The ID of the job. May be NULL for internal jobs. */ char *id; - /** The type of this job. */ + /** + * The type of this job. + * All callbacks are called with job_mutex *not* held. + */ const JobDriver *driver; - /** Reference count of the block job */ - int refcnt; - - /** Current state; See @JobStatus for details. */ - JobStatus status; - - /** AioContext to run the job coroutine in */ - AioContext *aio_context; - /** * The coroutine that executes the job. If not NULL, it is reentered when * busy is false and the job is cancelled. + * Initialized in job_start() */ Coroutine *co; + /** True if this job should automatically finalize itself */ + bool auto_finalize; + + /** True if this job should automatically dismiss itself */ + bool auto_dismiss; + + /** The completion function that will be called when the job completes. */ + BlockCompletionFunc *cb; + + /** The opaque value that is passed to the completion function. */ + void *opaque; + + /* ProgressMeter API is thread-safe */ + ProgressMeter progress; + + + /** Protected by AioContext lock */ + + /** AioContext to run the job coroutine in */ + AioContext *aio_context; + + /** Reference count of the block job */ + int refcnt; + + /** Current state; See @JobStatus for details. */ + JobStatus status; + /** * Timer that is used by @job_sleep_ns. Accessed under job_mutex (in * job.c). @@ -112,14 +137,6 @@ typedef struct Job { /** Set to true when the job has deferred work to the main loop. */ bool deferred_to_main_loop; - /** True if this job should automatically finalize itself */ - bool auto_finalize; - - /** True if this job should automatically dismiss itself */ - bool auto_dismiss; - - ProgressMeter progress; - /** * Return code from @run and/or @prepare callback(s). * Not final until the job has reached the CONCLUDED status. @@ -134,12 +151,6 @@ typedef struct Job { */ Error *err; - /** The completion function that will be called when the job completes. */ - BlockCompletionFunc *cb; - - /** The opaque value that is passed to the completion function. */ - void *opaque; - /** Notifiers called when a cancelled job is finalised */ NotifierList on_finalize_cancelled; @@ -167,6 +178,7 @@ typedef struct Job { /** * Callbacks and other information about a Job driver. + * All callbacks are invoked with job_mutex *not* held. */ struct JobDriver { @@ -472,7 +484,6 @@ void job_yield(Job *job); */ void coroutine_fn job_sleep_ns(Job *job, int64_t ns); - /** Returns the JobType of a given Job. */ JobType job_type(const Job *job); From patchwork Wed Jun 29 14:15:21 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900250 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D2FB1C43334 for ; Wed, 29 Jun 2022 14:44:59 +0000 (UTC) Received: from localhost ([::1]:47278 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6Yw6-00039s-Nj for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:44:58 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36580) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YU4-0007xX-59 for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:03 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:42056) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTv-000236-K7 for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:57 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512148; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=yTpDBTFfrx7nITUCv8EmSkF6y1fnDWo0Y9hyDRkbKno=; b=HS1maFL0+YCkTStKG4VmLJX73/DUzbZeGsNiRh9+/ZsDn0bgtqubfePArq8imMT6xjojJC N7lT+f9zeCkIHCvZ/uMBqDZAIgIU8vpj/nbyEUB+JJsEJbpfx0TEhFFRW8AWQPL9y3nn3n S8MmueCht3+i/qphH9Q+UWQ4zRn8uco= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-460-O4MX005QMWedPRr-uvntcA-1; Wed, 29 Jun 2022 10:15:44 -0400 X-MC-Unique: O4MX005QMWedPRr-uvntcA-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 1487B3C0F38A; Wed, 29 Jun 2022 14:15:44 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id B4A809D7F; Wed, 29 Jun 2022 14:15:43 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito , Vladimir Sementsov-Ogievskiy Subject: [PATCH v8 03/20] job.c: API functions not used outside should be static Date: Wed, 29 Jun 2022 10:15:21 -0400 Message-Id: <20220629141538.3400679-4-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" job_event_* functions can all be static, as they are not used outside job.c. Same applies for job_txn_add_job(). Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi Reviewed-by: Vladimir Sementsov-Ogievskiy --- include/qemu/job.h | 18 ------------------ job.c | 22 +++++++++++++++++++--- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index 876e13d549..4b64eb15f7 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -358,18 +358,6 @@ JobTxn *job_txn_new(void); */ void job_txn_unref(JobTxn *txn); -/** - * @txn: The transaction (may be NULL) - * @job: Job to add to the transaction - * - * Add @job to the transaction. The @job must not already be in a transaction. - * The caller must call either job_txn_unref() or job_completed() to release - * the reference that is automatically grabbed here. - * - * If @txn is NULL, the function does nothing. - */ -void job_txn_add_job(JobTxn *txn, Job *job); - /** * Create a new long-running job and return it. * @@ -431,12 +419,6 @@ void job_progress_set_remaining(Job *job, uint64_t remaining); */ void job_progress_increase_remaining(Job *job, uint64_t delta); -/** To be called when a cancelled job is finalised. */ -void job_event_cancelled(Job *job); - -/** To be called when a successfully completed job is finalised. */ -void job_event_completed(Job *job); - /** * Conditionally enter the job coroutine if the job is ready to run, not * already busy and fn() returns true. fn() is called while under the job_lock diff --git a/job.c b/job.c index 2b4ffca9d4..cafd597ba4 100644 --- a/job.c +++ b/job.c @@ -125,7 +125,17 @@ void job_txn_unref(JobTxn *txn) } } -void job_txn_add_job(JobTxn *txn, Job *job) +/** + * @txn: The transaction (may be NULL) + * @job: Job to add to the transaction + * + * Add @job to the transaction. The @job must not already be in a transaction. + * The caller must call either job_txn_unref() or job_completed() to release + * the reference that is automatically grabbed here. + * + * If @txn is NULL, the function does nothing. + */ +static void job_txn_add_job(JobTxn *txn, Job *job) { if (!txn) { return; @@ -427,12 +437,18 @@ void job_progress_increase_remaining(Job *job, uint64_t delta) progress_increase_remaining(&job->progress, delta); } -void job_event_cancelled(Job *job) +/** + * To be called when a cancelled job is finalised. + */ +static void job_event_cancelled(Job *job) { notifier_list_notify(&job->on_finalize_cancelled, job); } -void job_event_completed(Job *job) +/** + * To be called when a successfully completed job is finalised. + */ +static void job_event_completed(Job *job) { notifier_list_notify(&job->on_finalize_completed, job); } From patchwork Wed Jun 29 14:15:22 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900161 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 55D59C433EF for ; Wed, 29 Jun 2022 14:20:14 +0000 (UTC) Received: from localhost ([::1]:45454 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6YY9-0002rP-43 for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:20:13 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36238) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTw-0007uC-9d for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:52 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:41289) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTr-0001zG-O2 for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:50 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512146; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=pXXEWuTZjbY86kMHhS2xb1I9DX+F+DsvFH6sb6NeVxc=; b=DMK7wYoRDheQB3My6OECGhh+Ag37+iXMQLckPfbFA8WNchpGX4c/nwLzdjX17uSL6T68oY Lf+WkQBkIy/cDagv8+Mqm7L8LXu+K1V84ugYFhP9u4jVQ2CzZizO6cWunfqTCH8CeJbYUO aR6WISoSAPFKv/6/8BC3rzUoSyyAr0Q= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-638-ov-17yc4Mc6ZZsYaRFsj6Q-1; Wed, 29 Jun 2022 10:15:44 -0400 X-MC-Unique: ov-17yc4Mc6ZZsYaRFsj6Q-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 71D778339AB; Wed, 29 Jun 2022 14:15:44 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1C6A49D63; Wed, 29 Jun 2022 14:15:44 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito , Vladimir Sementsov-Ogievskiy Subject: [PATCH v8 04/20] aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED Date: Wed, 29 Jun 2022 10:15:22 -0400 Message-Id: <20220629141538.3400679-5-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Same as AIO_WAIT_WHILE macro, but if we are in the Main loop do not release and then acquire ctx_ 's aiocontext. Once all Aiocontext locks go away, this macro will replace AIO_WAIT_WHILE. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi Reviewed-by: Vladimir Sementsov-Ogievskiy --- include/block/aio-wait.h | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h index 54840f8622..a61f82c617 100644 --- a/include/block/aio-wait.h +++ b/include/block/aio-wait.h @@ -59,10 +59,13 @@ typedef struct { extern AioWait global_aio_wait; /** - * AIO_WAIT_WHILE: + * _AIO_WAIT_WHILE: * @ctx: the aio context, or NULL if multiple aio contexts (for which the * caller does not hold a lock) are involved in the polling condition. * @cond: wait while this conditional expression is true + * @unlock: whether to unlock and then lock again @ctx. This apples + * only when waiting for another AioContext from the main loop. + * Otherwise it's ignored. * * Wait while a condition is true. Use this to implement synchronous * operations that require event loop activity. @@ -75,7 +78,7 @@ extern AioWait global_aio_wait; * wait on conditions between two IOThreads since that could lead to deadlock, * go via the main loop instead. */ -#define AIO_WAIT_WHILE(ctx, cond) ({ \ +#define _AIO_WAIT_WHILE(ctx, cond, unlock) ({ \ bool waited_ = false; \ AioWait *wait_ = &global_aio_wait; \ AioContext *ctx_ = (ctx); \ @@ -92,11 +95,11 @@ extern AioWait global_aio_wait; assert(qemu_get_current_aio_context() == \ qemu_get_aio_context()); \ while ((cond)) { \ - if (ctx_) { \ + if (unlock && ctx_) { \ aio_context_release(ctx_); \ } \ aio_poll(qemu_get_aio_context(), true); \ - if (ctx_) { \ + if (unlock && ctx_) { \ aio_context_acquire(ctx_); \ } \ waited_ = true; \ @@ -105,6 +108,12 @@ extern AioWait global_aio_wait; qatomic_dec(&wait_->num_waiters); \ waited_; }) +#define AIO_WAIT_WHILE(ctx, cond) \ + _AIO_WAIT_WHILE(ctx, cond, true) + +#define AIO_WAIT_WHILE_UNLOCKED(ctx, cond) \ + _AIO_WAIT_WHILE(ctx, cond, false) + /** * aio_wait_kick: * Wake up the main thread if it is waiting on AIO_WAIT_WHILE(). During From patchwork Wed Jun 29 14:15:23 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900165 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A6E18C433EF for ; Wed, 29 Jun 2022 14:27:25 +0000 (UTC) Received: from localhost ([::1]:36408 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6Yf6-0007yM-MP for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:27:24 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36584) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YU4-0007xZ-5r for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:03 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:42117) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTv-00024W-HV for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:59 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512150; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5IScjRI1hOdiTzez+ZtuDpb8XsjbOSEao6fWM6xdRas=; b=EeBCBeprcpc/0IP+hiDQtuGjnxlECXdne6/qEznVtLeo0XDMrzgkQpPhlEaINzsuSTYJh6 VpO1NkMP5tvGlZZQX9r9KEJpCj9w3GZRO0Aco81UvmzQm/t+UJlyPhblDsYLaLnAxA+nDL GFyO4UHd6FPqt60s8liSZKl81M6PgwE= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-634-aeJWrBAOOaKC-qL3PfXxig-1; Wed, 29 Jun 2022 10:15:46 -0400 X-MC-Unique: aeJWrBAOOaKC-qL3PfXxig-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id D0B57384F804; Wed, 29 Jun 2022 14:15:44 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7A8DE17452; Wed, 29 Jun 2022 14:15:44 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 05/20] job.c: add job_lock/unlock while keeping job.h intact Date: Wed, 29 Jun 2022 10:15:23 -0400 Message-Id: <20220629141538.3400679-6-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" With "intact" we mean that all job.h functions implicitly take the lock. Therefore API callers are unmodified. This means that: - all static functions become _locked, and call _locked functions - all public functions take the lock internally, and call _locked functions - all public functions called internally by other functions in job.c will have a _locked counterpart, to avoid deadlocks (job lock already taken) - public functions called only from exernal files (not job.c) do not have _locked() counterpart and take the lock inside job_{lock/unlock} is independent from real_job_{lock/unlock}. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop* .Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- include/qemu/job.h | 73 +++++- job.c | 607 +++++++++++++++++++++++++++++++-------------- 2 files changed, 499 insertions(+), 181 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index 4b64eb15f7..99960cc9a3 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -358,6 +358,9 @@ JobTxn *job_txn_new(void); */ void job_txn_unref(JobTxn *txn); +/* Same as job_txn_unref(), but called with job lock held. */ +void job_txn_unref_locked(JobTxn *txn); + /** * Create a new long-running job and return it. * @@ -380,12 +383,18 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, */ void job_ref(Job *job); +/* Same as job_ref(), but called with job lock held. */ +void job_ref_locked(Job *job); + /** * Release a reference that was previously acquired with job_ref() or * job_create(). If it's the last reference to the object, it will be freed. */ void job_unref(Job *job); +/* Same as job_unref(), but called with job lock held. */ +void job_unref_locked(Job *job); + /** * @job: The job that has made progress * @done: How much progress the job made since the last call @@ -426,6 +435,9 @@ void job_progress_increase_remaining(Job *job, uint64_t delta); */ void job_enter_cond(Job *job, bool(*fn)(Job *job)); +/* Same as job_enter_cond(), but called with job lock held. */ +void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)); + /** * @job: A job that has not yet been started. * @@ -478,6 +490,9 @@ bool job_is_internal(Job *job); /** Returns whether the job is being cancelled. */ bool job_is_cancelled(Job *job); +/* Same as job_is_cancelled(), but called with job lock held. */ +bool job_is_cancelled_locked(Job *job); + /** * Returns whether the job is scheduled for cancellation (at an * indefinite point). @@ -487,9 +502,15 @@ bool job_cancel_requested(Job *job); /** Returns whether the job is in a completed state. */ bool job_is_completed(Job *job); +/* Same as job_is_completed(), but called with job lock held. */ +bool job_is_completed_locked(Job *job); + /** Returns whether the job is ready to be completed. */ bool job_is_ready(Job *job); +/* Same as job_is_ready(), but called with job lock held. */ +bool job_is_ready_locked(Job *job); + /** * Request @job to pause at the next pause point. Must be paired with * job_resume(). If the job is supposed to be resumed by user action, call @@ -497,24 +518,39 @@ bool job_is_ready(Job *job); */ void job_pause(Job *job); +/* Same as job_pause(), but called with job lock held. */ +void job_pause_locked(Job *job); + /** Resumes a @job paused with job_pause. */ void job_resume(Job *job); +/* Same as job_resume(), but called with job lock held. */ +void job_resume_locked(Job *job); + /** * Asynchronously pause the specified @job. * Do not allow a resume until a matching call to job_user_resume. */ void job_user_pause(Job *job, Error **errp); +/* Same as job_user_pause(), but called with job lock held. */ +void job_user_pause_locked(Job *job, Error **errp); + /** Returns true if the job is user-paused. */ bool job_user_paused(Job *job); +/* Same as job_user_paused(), but called with job lock held. */ +bool job_user_paused_locked(Job *job); + /** * Resume the specified @job. * Must be paired with a preceding job_user_pause. */ void job_user_resume(Job *job, Error **errp); +/* Same as job_user_resume(), but called with job lock held. */ +void job_user_resume_locked(Job *job, Error **errp); + /** * Get the next element from the list of block jobs after @job, or the * first one if @job is %NULL. @@ -523,6 +559,9 @@ void job_user_resume(Job *job, Error **errp); */ Job *job_next(Job *job); +/* Same as job_next(), but called with job lock held. */ +Job *job_next_locked(Job *job); + /** * Get the job identified by @id (which must not be %NULL). * @@ -530,6 +569,9 @@ Job *job_next(Job *job); */ Job *job_get(const char *id); +/* Same as job_get(), but called with job lock held. */ +Job *job_get_locked(const char *id); + /** * Check whether the verb @verb can be applied to @job in its current state. * Returns 0 if the verb can be applied; otherwise errp is set and -EPERM @@ -537,6 +579,9 @@ Job *job_get(const char *id); */ int job_apply_verb(Job *job, JobVerb verb, Error **errp); +/* Same as job_apply_verb, but called with job lock held. */ +int job_apply_verb_locked(Job *job, JobVerb verb, Error **errp); + /** The @job could not be started, free it. */ void job_early_fail(Job *job); @@ -546,18 +591,27 @@ void job_transition_to_ready(Job *job); /** Asynchronously complete the specified @job. */ void job_complete(Job *job, Error **errp); +/* Same as job_complete(), but called with job lock held. */ +void job_complete_locked(Job *job, Error **errp); + /** * Asynchronously cancel the specified @job. If @force is true, the job should * be cancelled immediately without waiting for a consistent state. */ void job_cancel(Job *job, bool force); +/* Same as job_cancel(), but called with job lock held. */ +void job_cancel_locked(Job *job, bool force); + /** * Cancels the specified job like job_cancel(), but may refuse to do so if the * operation isn't meaningful in the current state of the job. */ void job_user_cancel(Job *job, bool force, Error **errp); +/* Same as job_user_cancel(), but called with job lock held. */ +void job_user_cancel_locked(Job *job, bool force, Error **errp); + /** * Synchronously cancel the @job. The completion callback is called * before the function returns. If @force is false, the job may @@ -571,6 +625,9 @@ void job_user_cancel(Job *job, bool force, Error **errp); */ int job_cancel_sync(Job *job, bool force); +/* Same as job_cancel_sync, but called with job lock held. */ +int job_cancel_sync_locked(Job *job, bool force); + /** Synchronously force-cancels all jobs using job_cancel_sync(). */ void job_cancel_sync_all(void); @@ -590,6 +647,9 @@ void job_cancel_sync_all(void); */ int job_complete_sync(Job *job, Error **errp); +/* Same as job_complete_sync, but called with job lock held. */ +int job_complete_sync_locked(Job *job, Error **errp); + /** * For a @job that has finished its work and is pending awaiting explicit * acknowledgement to commit its work, this will commit that work. @@ -600,12 +660,18 @@ int job_complete_sync(Job *job, Error **errp); */ void job_finalize(Job *job, Error **errp); +/* Same as job_finalize(), but called with job lock held. */ +void job_finalize_locked(Job *job, Error **errp); + /** * Remove the concluded @job from the query list and resets the passed pointer * to %NULL. Returns an error if the job is not actually concluded. */ void job_dismiss(Job **job, Error **errp); +/* Same as job_dismiss(), but called with job lock held. */ +void job_dismiss_locked(Job **job, Error **errp); + /** * Synchronously finishes the given @job. If @finish is given, it is called to * trigger completion or cancellation of the job. @@ -615,6 +681,11 @@ void job_dismiss(Job **job, Error **errp); * * Callers must hold the AioContext lock of job->aio_context. */ -int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp); +int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), + Error **errp); + +/* Same as job_finish_sync, but called with job lock held. */ +int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp), + Error **errp); #endif diff --git a/job.c b/job.c index cafd597ba4..dd44fac8dd 100644 --- a/job.c +++ b/job.c @@ -113,18 +113,25 @@ JobTxn *job_txn_new(void) return txn; } -static void job_txn_ref(JobTxn *txn) +/* Called with job_mutex held. */ +static void job_txn_ref_locked(JobTxn *txn) { txn->refcnt++; } -void job_txn_unref(JobTxn *txn) +void job_txn_unref_locked(JobTxn *txn) { if (txn && --txn->refcnt == 0) { g_free(txn); } } +void job_txn_unref(JobTxn *txn) +{ + JOB_LOCK_GUARD(); + job_txn_unref_locked(txn); +} + /** * @txn: The transaction (may be NULL) * @job: Job to add to the transaction @@ -134,8 +141,10 @@ void job_txn_unref(JobTxn *txn) * the reference that is automatically grabbed here. * * If @txn is NULL, the function does nothing. + * + * Called with job_mutex held. */ -static void job_txn_add_job(JobTxn *txn, Job *job) +static void job_txn_add_job_locked(JobTxn *txn, Job *job) { if (!txn) { return; @@ -145,19 +154,21 @@ static void job_txn_add_job(JobTxn *txn, Job *job) job->txn = txn; QLIST_INSERT_HEAD(&txn->jobs, job, txn_list); - job_txn_ref(txn); + job_txn_ref_locked(txn); } -static void job_txn_del_job(Job *job) +/* Called with job_mutex held. */ +static void job_txn_del_job_locked(Job *job) { if (job->txn) { QLIST_REMOVE(job, txn_list); - job_txn_unref(job->txn); + job_txn_unref_locked(job->txn); job->txn = NULL; } } -static int job_txn_apply(Job *job, int fn(Job *)) +/* Called with job_mutex held. */ +static int job_txn_apply_locked(Job *job, int fn(Job *)) { AioContext *inner_ctx; Job *other_job, *next; @@ -170,7 +181,7 @@ static int job_txn_apply(Job *job, int fn(Job *)) * we need to release it here to avoid holding the lock twice - which would * break AIO_WAIT_WHILE from within fn. */ - job_ref(job); + job_ref_locked(job); aio_context_release(job->aio_context); QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) { @@ -188,7 +199,7 @@ static int job_txn_apply(Job *job, int fn(Job *)) * can't use a local variable to cache it. */ aio_context_acquire(job->aio_context); - job_unref(job); + job_unref_locked(job); return rc; } @@ -197,7 +208,8 @@ bool job_is_internal(Job *job) return (job->id == NULL); } -static void job_state_transition(Job *job, JobStatus s1) +/* Called with job_mutex held. */ +static void job_state_transition_locked(Job *job, JobStatus s1) { JobStatus s0 = job->status; assert(s1 >= 0 && s1 < JOB_STATUS__MAX); @@ -212,7 +224,7 @@ static void job_state_transition(Job *job, JobStatus s1) } } -int job_apply_verb(Job *job, JobVerb verb, Error **errp) +int job_apply_verb_locked(Job *job, JobVerb verb, Error **errp) { JobStatus s0 = job->status; assert(verb >= 0 && verb < JOB_VERB__MAX); @@ -226,6 +238,12 @@ int job_apply_verb(Job *job, JobVerb verb, Error **errp) return -EPERM; } +int job_apply_verb(Job *job, JobVerb verb, Error **errp) +{ + JOB_LOCK_GUARD(); + return job_apply_verb_locked(job, verb, errp); +} + JobType job_type(const Job *job) { return job->driver->job_type; @@ -236,19 +254,32 @@ const char *job_type_str(const Job *job) return JobType_str(job_type(job)); } -bool job_is_cancelled(Job *job) +bool job_is_cancelled_locked(Job *job) { /* force_cancel may be true only if cancelled is true, too */ assert(job->cancelled || !job->force_cancel); return job->force_cancel; } -bool job_cancel_requested(Job *job) +bool job_is_cancelled(Job *job) +{ + JOB_LOCK_GUARD(); + return job_is_cancelled_locked(job); +} + +/* Called with job_mutex held. */ +static bool job_cancel_requested_locked(Job *job) { return job->cancelled; } -bool job_is_ready(Job *job) +bool job_cancel_requested(Job *job) +{ + JOB_LOCK_GUARD(); + return job_cancel_requested_locked(job); +} + +bool job_is_ready_locked(Job *job) { switch (job->status) { case JOB_STATUS_UNDEFINED: @@ -270,7 +301,13 @@ bool job_is_ready(Job *job) return false; } -bool job_is_completed(Job *job) +bool job_is_ready(Job *job) +{ + JOB_LOCK_GUARD(); + return job_is_ready_locked(job); +} + +bool job_is_completed_locked(Job *job) { switch (job->status) { case JOB_STATUS_UNDEFINED: @@ -292,17 +329,24 @@ bool job_is_completed(Job *job) return false; } +bool job_is_completed(Job *job) +{ + JOB_LOCK_GUARD(); + return job_is_completed_locked(job); +} + static bool job_started(Job *job) { return job->co; } -static bool job_should_pause(Job *job) +/* Called with job_mutex held. */ +static bool job_should_pause_locked(Job *job) { return job->pause_count > 0; } -Job *job_next(Job *job) +Job *job_next_locked(Job *job) { if (!job) { return QLIST_FIRST(&jobs); @@ -310,7 +354,13 @@ Job *job_next(Job *job) return QLIST_NEXT(job, job_list); } -Job *job_get(const char *id) +Job *job_next(Job *job) +{ + JOB_LOCK_GUARD(); + return job_next_locked(job); +} + +Job *job_get_locked(const char *id) { Job *job; @@ -323,6 +373,13 @@ Job *job_get(const char *id) return NULL; } +Job *job_get(const char *id) +{ + JOB_LOCK_GUARD(); + return job_get_locked(id); +} + +/* Called with job_mutex *not* held. */ static void job_sleep_timer_cb(void *opaque) { Job *job = opaque; @@ -336,6 +393,8 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, { Job *job; + JOB_LOCK_GUARD(); + if (job_id) { if (flags & JOB_INTERNAL) { error_setg(errp, "Cannot specify job ID for internal job"); @@ -345,7 +404,7 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, error_setg(errp, "Invalid job ID '%s'", job_id); return NULL; } - if (job_get(job_id)) { + if (job_get_locked(job_id)) { error_setg(errp, "Job ID '%s' already in use", job_id); return NULL; } @@ -375,7 +434,7 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, notifier_list_init(&job->on_ready); notifier_list_init(&job->on_idle); - job_state_transition(job, JOB_STATUS_CREATED); + job_state_transition_locked(job, JOB_STATUS_CREATED); aio_timer_init(qemu_get_aio_context(), &job->sleep_timer, QEMU_CLOCK_REALTIME, SCALE_NS, job_sleep_timer_cb, job); @@ -386,21 +445,27 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, * consolidating the job management logic */ if (!txn) { txn = job_txn_new(); - job_txn_add_job(txn, job); - job_txn_unref(txn); + job_txn_add_job_locked(txn, job); + job_txn_unref_locked(txn); } else { - job_txn_add_job(txn, job); + job_txn_add_job_locked(txn, job); } return job; } -void job_ref(Job *job) +void job_ref_locked(Job *job) { ++job->refcnt; } -void job_unref(Job *job) +void job_ref(Job *job) +{ + JOB_LOCK_GUARD(); + job_ref_locked(job); +} + +void job_unref_locked(Job *job) { GLOBAL_STATE_CODE(); @@ -410,7 +475,9 @@ void job_unref(Job *job) assert(!job->txn); if (job->driver->free) { + job_unlock(); job->driver->free(job); + job_lock(); } QLIST_REMOVE(job, job_list); @@ -422,6 +489,12 @@ void job_unref(Job *job) } } +void job_unref(Job *job) +{ + JOB_LOCK_GUARD(); + job_unref_locked(job); +} + void job_progress_update(Job *job, uint64_t done) { progress_work_done(&job->progress, done); @@ -439,36 +512,41 @@ void job_progress_increase_remaining(Job *job, uint64_t delta) /** * To be called when a cancelled job is finalised. + * Called with job_mutex held. */ -static void job_event_cancelled(Job *job) +static void job_event_cancelled_locked(Job *job) { notifier_list_notify(&job->on_finalize_cancelled, job); } /** * To be called when a successfully completed job is finalised. + * Called with job_mutex held. */ -static void job_event_completed(Job *job) +static void job_event_completed_locked(Job *job) { notifier_list_notify(&job->on_finalize_completed, job); } -static void job_event_pending(Job *job) +/* Called with job_mutex held. */ +static void job_event_pending_locked(Job *job) { notifier_list_notify(&job->on_pending, job); } -static void job_event_ready(Job *job) +/* Called with job_mutex held. */ +static void job_event_ready_locked(Job *job) { notifier_list_notify(&job->on_ready, job); } -static void job_event_idle(Job *job) +/* Called with job_mutex held. */ +static void job_event_idle_locked(Job *job) { notifier_list_notify(&job->on_idle, job); } -void job_enter_cond(Job *job, bool(*fn)(Job *job)) +void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)) { if (!job_started(job)) { return; @@ -495,9 +573,16 @@ void job_enter_cond(Job *job, bool(*fn)(Job *job)) aio_co_enter(job->aio_context, job->co); } +void job_enter_cond(Job *job, bool(*fn)(Job *job)) +{ + JOB_LOCK_GUARD(); + job_enter_cond_locked(job, fn); +} + void job_enter(Job *job) { - job_enter_cond(job, NULL); + JOB_LOCK_GUARD(); + job_enter_cond_locked(job, NULL); } /* Yield, and schedule a timer to reenter the coroutine after @ns nanoseconds. @@ -505,100 +590,128 @@ void job_enter(Job *job) * is allowed and cancels the timer. * * If @ns is (uint64_t) -1, no timer is scheduled and job_enter() must be - * called explicitly. */ -static void coroutine_fn job_do_yield(Job *job, uint64_t ns) + * called explicitly. + * + * Called with job_mutex held, but releases it temporarily. + */ +static void coroutine_fn job_do_yield_locked(Job *job, uint64_t ns) { real_job_lock(); if (ns != -1) { timer_mod(&job->sleep_timer, ns); } job->busy = false; - job_event_idle(job); + job_event_idle_locked(job); real_job_unlock(); + job_unlock(); qemu_coroutine_yield(); + job_lock(); /* Set by job_enter_cond() before re-entering the coroutine. */ assert(job->busy); } -void coroutine_fn job_pause_point(Job *job) +void coroutine_fn job_pause_point_locked(Job *job) { assert(job && job_started(job)); - if (!job_should_pause(job)) { + if (!job_should_pause_locked(job)) { return; } - if (job_is_cancelled(job)) { + if (job_is_cancelled_locked(job)) { return; } if (job->driver->pause) { + job_unlock(); job->driver->pause(job); + job_lock(); } - if (job_should_pause(job) && !job_is_cancelled(job)) { + if (job_should_pause_locked(job) && !job_is_cancelled_locked(job)) { JobStatus status = job->status; - job_state_transition(job, status == JOB_STATUS_READY - ? JOB_STATUS_STANDBY - : JOB_STATUS_PAUSED); + job_state_transition_locked(job, status == JOB_STATUS_READY + ? JOB_STATUS_STANDBY + : JOB_STATUS_PAUSED); job->paused = true; - job_do_yield(job, -1); + job_do_yield_locked(job, -1); job->paused = false; - job_state_transition(job, status); + job_state_transition_locked(job, status); } if (job->driver->resume) { + job_unlock(); job->driver->resume(job); + job_lock(); } } -void job_yield(Job *job) +void coroutine_fn job_pause_point(Job *job) +{ + JOB_LOCK_GUARD(); + job_pause_point_locked(job); +} + +void job_yield_locked(Job *job) { assert(job->busy); /* Check cancellation *before* setting busy = false, too! */ - if (job_is_cancelled(job)) { + if (job_is_cancelled_locked(job)) { return; } - if (!job_should_pause(job)) { - job_do_yield(job, -1); + if (!job_should_pause_locked(job)) { + job_do_yield_locked(job, -1); } - job_pause_point(job); + job_pause_point_locked(job); +} + +void job_yield(Job *job) +{ + JOB_LOCK_GUARD(); + job_yield_locked(job); } void coroutine_fn job_sleep_ns(Job *job, int64_t ns) { + JOB_LOCK_GUARD(); assert(job->busy); /* Check cancellation *before* setting busy = false, too! */ - if (job_is_cancelled(job)) { + if (job_is_cancelled_locked(job)) { return; } - if (!job_should_pause(job)) { - job_do_yield(job, qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + ns); + if (!job_should_pause_locked(job)) { + job_do_yield_locked(job, qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + ns); } - job_pause_point(job); + job_pause_point_locked(job); } -/* Assumes the block_job_mutex is held */ -static bool job_timer_not_pending(Job *job) +/* Assumes the job_mutex is held */ +static bool job_timer_not_pending_locked(Job *job) { return !timer_pending(&job->sleep_timer); } -void job_pause(Job *job) +void job_pause_locked(Job *job) { job->pause_count++; if (!job->paused) { - job_enter(job); + job_enter_cond_locked(job, NULL); } } -void job_resume(Job *job) +void job_pause(Job *job) +{ + JOB_LOCK_GUARD(); + job_pause_locked(job); +} + +void job_resume_locked(Job *job) { assert(job->pause_count > 0); job->pause_count--; @@ -607,12 +720,18 @@ void job_resume(Job *job) } /* kick only if no timer is pending */ - job_enter_cond(job, job_timer_not_pending); + job_enter_cond_locked(job, job_timer_not_pending_locked); } -void job_user_pause(Job *job, Error **errp) +void job_resume(Job *job) +{ + JOB_LOCK_GUARD(); + job_resume_locked(job); +} + +void job_user_pause_locked(Job *job, Error **errp) { - if (job_apply_verb(job, JOB_VERB_PAUSE, errp)) { + if (job_apply_verb_locked(job, JOB_VERB_PAUSE, errp)) { return; } if (job->user_paused) { @@ -620,15 +739,27 @@ void job_user_pause(Job *job, Error **errp) return; } job->user_paused = true; - job_pause(job); + job_pause_locked(job); } -bool job_user_paused(Job *job) +void job_user_pause(Job *job, Error **errp) +{ + JOB_LOCK_GUARD(); + job_user_pause_locked(job, errp); +} + +bool job_user_paused_locked(Job *job) { return job->user_paused; } -void job_user_resume(Job *job, Error **errp) +bool job_user_paused(Job *job) +{ + JOB_LOCK_GUARD(); + return job_user_paused_locked(job); +} + +void job_user_resume_locked(Job *job, Error **errp) { assert(job); GLOBAL_STATE_CODE(); @@ -636,132 +767,168 @@ void job_user_resume(Job *job, Error **errp) error_setg(errp, "Can't resume a job that was not paused"); return; } - if (job_apply_verb(job, JOB_VERB_RESUME, errp)) { + if (job_apply_verb_locked(job, JOB_VERB_RESUME, errp)) { return; } if (job->driver->user_resume) { + job_unlock(); job->driver->user_resume(job); + job_lock(); } job->user_paused = false; - job_resume(job); + job_resume_locked(job); } -static void job_do_dismiss(Job *job) +void job_user_resume(Job *job, Error **errp) +{ + JOB_LOCK_GUARD(); + job_user_resume_locked(job, errp); +} + +/* Called with job_mutex held. */ +static void job_do_dismiss_locked(Job *job) { assert(job); job->busy = false; job->paused = false; job->deferred_to_main_loop = true; - job_txn_del_job(job); + job_txn_del_job_locked(job); - job_state_transition(job, JOB_STATUS_NULL); - job_unref(job); + job_state_transition_locked(job, JOB_STATUS_NULL); + job_unref_locked(job); } -void job_dismiss(Job **jobptr, Error **errp) +void job_dismiss_locked(Job **jobptr, Error **errp) { Job *job = *jobptr; /* similarly to _complete, this is QMP-interface only. */ assert(job->id); - if (job_apply_verb(job, JOB_VERB_DISMISS, errp)) { + if (job_apply_verb_locked(job, JOB_VERB_DISMISS, errp)) { return; } - job_do_dismiss(job); + job_do_dismiss_locked(job); *jobptr = NULL; } +void job_dismiss(Job **jobptr, Error **errp) +{ + JOB_LOCK_GUARD(); + job_dismiss_locked(jobptr, errp); +} + void job_early_fail(Job *job) { + JOB_LOCK_GUARD(); assert(job->status == JOB_STATUS_CREATED); - job_do_dismiss(job); + job_do_dismiss_locked(job); } -static void job_conclude(Job *job) +/* Called with job_mutex held. */ +static void job_conclude_locked(Job *job) { - job_state_transition(job, JOB_STATUS_CONCLUDED); + job_state_transition_locked(job, JOB_STATUS_CONCLUDED); if (job->auto_dismiss || !job_started(job)) { - job_do_dismiss(job); + job_do_dismiss_locked(job); } } -static void job_update_rc(Job *job) +/* Called with job_mutex held. */ +static void job_update_rc_locked(Job *job) { - if (!job->ret && job_is_cancelled(job)) { + if (!job->ret && job_is_cancelled_locked(job)) { job->ret = -ECANCELED; } if (job->ret) { if (!job->err) { error_setg(&job->err, "%s", strerror(-job->ret)); } - job_state_transition(job, JOB_STATUS_ABORTING); + job_state_transition_locked(job, JOB_STATUS_ABORTING); } } -static void job_commit(Job *job) +/* Called with job_mutex held, but releases it temporarily */ +static void job_commit_locked(Job *job) { assert(!job->ret); GLOBAL_STATE_CODE(); if (job->driver->commit) { + job_unlock(); job->driver->commit(job); + job_lock(); } } -static void job_abort(Job *job) +/* Called with job_mutex held, but releases it temporarily */ +static void job_abort_locked(Job *job) { assert(job->ret); GLOBAL_STATE_CODE(); if (job->driver->abort) { + job_unlock(); job->driver->abort(job); + job_lock(); } } -static void job_clean(Job *job) +/* Called with job_mutex held, but releases it temporarily */ +static void job_clean_locked(Job *job) { GLOBAL_STATE_CODE(); if (job->driver->clean) { + job_unlock(); job->driver->clean(job); + job_lock(); } } -static int job_finalize_single(Job *job) +/* Called with job_mutex held, but releases it temporarily */ +static int job_finalize_single_locked(Job *job) { - assert(job_is_completed(job)); + int job_ret; + + assert(job_is_completed_locked(job)); /* Ensure abort is called for late-transactional failures */ - job_update_rc(job); + job_update_rc_locked(job); if (!job->ret) { - job_commit(job); + job_commit_locked(job); } else { - job_abort(job); + job_abort_locked(job); } - job_clean(job); + job_clean_locked(job); if (job->cb) { - job->cb(job->opaque, job->ret); + job_ret = job->ret; + job_unlock(); + job->cb(job->opaque, job_ret); + job_lock(); } /* Emit events only if we actually started */ if (job_started(job)) { - if (job_is_cancelled(job)) { - job_event_cancelled(job); + if (job_is_cancelled_locked(job)) { + job_event_cancelled_locked(job); } else { - job_event_completed(job); + job_event_completed_locked(job); } } - job_txn_del_job(job); - job_conclude(job); + job_txn_del_job_locked(job); + job_conclude_locked(job); return 0; } -static void job_cancel_async(Job *job, bool force) +/* Called with job_mutex held, but releases it temporarily */ +static void job_cancel_async_locked(Job *job, bool force) { GLOBAL_STATE_CODE(); if (job->driver->cancel) { + job_unlock(); force = job->driver->cancel(job, force); + job_lock(); } else { /* No .cancel() means the job will behave as if force-cancelled */ force = true; @@ -770,7 +937,9 @@ static void job_cancel_async(Job *job, bool force) if (job->user_paused) { /* Do not call job_enter here, the caller will handle it. */ if (job->driver->user_resume) { + job_unlock(); job->driver->user_resume(job); + job_lock(); } job->user_paused = false; assert(job->pause_count > 0); @@ -791,7 +960,8 @@ static void job_cancel_async(Job *job, bool force) } } -static void job_completed_txn_abort(Job *job) +/* Called with job_mutex held. */ +static void job_completed_txn_abort_locked(Job *job) { AioContext *ctx; JobTxn *txn = job->txn; @@ -804,7 +974,7 @@ static void job_completed_txn_abort(Job *job) return; } txn->aborting = true; - job_txn_ref(txn); + job_txn_ref_locked(txn); /* * We can only hold the single job's AioContext lock while calling @@ -812,7 +982,7 @@ static void job_completed_txn_abort(Job *job) * calls of AIO_WAIT_WHILE(), which could deadlock otherwise. * Note that the job's AioContext may change when it is finalized. */ - job_ref(job); + job_ref_locked(job); aio_context_release(job->aio_context); /* Other jobs are effectively cancelled by us, set the status for @@ -827,7 +997,7 @@ static void job_completed_txn_abort(Job *job) * Therefore, pass force=true to terminate all other jobs as quickly * as possible. */ - job_cancel_async(other_job, true); + job_cancel_async_locked(other_job, true); aio_context_release(ctx); } } @@ -839,11 +1009,11 @@ static void job_completed_txn_abort(Job *job) */ ctx = other_job->aio_context; aio_context_acquire(ctx); - if (!job_is_completed(other_job)) { - assert(job_cancel_requested(other_job)); - job_finish_sync(other_job, NULL, NULL); + if (!job_is_completed_locked(other_job)) { + assert(job_cancel_requested_locked(other_job)); + job_finish_sync_locked(other_job, NULL, NULL); } - job_finalize_single(other_job); + job_finalize_single_locked(other_job); aio_context_release(ctx); } @@ -852,110 +1022,129 @@ static void job_completed_txn_abort(Job *job) * even if the job went away during job_finalize_single(). */ aio_context_acquire(job->aio_context); - job_unref(job); + job_unref_locked(job); - job_txn_unref(txn); + job_txn_unref_locked(txn); } -static int job_prepare(Job *job) +/* Called with job_mutex held, but releases it temporarily */ +static int job_prepare_locked(Job *job) { GLOBAL_STATE_CODE(); if (job->ret == 0 && job->driver->prepare) { + job_unlock(); job->ret = job->driver->prepare(job); - job_update_rc(job); + job_lock(); + job_update_rc_locked(job); } return job->ret; } -static int job_needs_finalize(Job *job) +/* Called with job_mutex held */ +static int job_needs_finalize_locked(Job *job) { return !job->auto_finalize; } -static void job_do_finalize(Job *job) +/* Called with job_mutex held */ +static void job_do_finalize_locked(Job *job) { int rc; assert(job && job->txn); /* prepare the transaction to complete */ - rc = job_txn_apply(job, job_prepare); + rc = job_txn_apply_locked(job, job_prepare_locked); if (rc) { - job_completed_txn_abort(job); + job_completed_txn_abort_locked(job); } else { - job_txn_apply(job, job_finalize_single); + job_txn_apply_locked(job, job_finalize_single_locked); } } -void job_finalize(Job *job, Error **errp) +void job_finalize_locked(Job *job, Error **errp) { assert(job && job->id); - if (job_apply_verb(job, JOB_VERB_FINALIZE, errp)) { + if (job_apply_verb_locked(job, JOB_VERB_FINALIZE, errp)) { return; } - job_do_finalize(job); + job_do_finalize_locked(job); +} + +void job_finalize(Job *job, Error **errp) +{ + JOB_LOCK_GUARD(); + job_finalize_locked(job, errp); } -static int job_transition_to_pending(Job *job) +/* Called with job_mutex held. */ +static int job_transition_to_pending_locked(Job *job) { - job_state_transition(job, JOB_STATUS_PENDING); + job_state_transition_locked(job, JOB_STATUS_PENDING); if (!job->auto_finalize) { - job_event_pending(job); + job_event_pending_locked(job); } return 0; } void job_transition_to_ready(Job *job) { - job_state_transition(job, JOB_STATUS_READY); - job_event_ready(job); + JOB_LOCK_GUARD(); + job_state_transition_locked(job, JOB_STATUS_READY); + job_event_ready_locked(job); } -static void job_completed_txn_success(Job *job) +/* Called with job_mutex held. */ +static void job_completed_txn_success_locked(Job *job) { JobTxn *txn = job->txn; Job *other_job; - job_state_transition(job, JOB_STATUS_WAITING); + job_state_transition_locked(job, JOB_STATUS_WAITING); /* * Successful completion, see if there are other running jobs in this * txn. */ QLIST_FOREACH(other_job, &txn->jobs, txn_list) { - if (!job_is_completed(other_job)) { + if (!job_is_completed_locked(other_job)) { return; } assert(other_job->ret == 0); } - job_txn_apply(job, job_transition_to_pending); + job_txn_apply_locked(job, job_transition_to_pending_locked); /* If no jobs need manual finalization, automatically do so */ - if (job_txn_apply(job, job_needs_finalize) == 0) { - job_do_finalize(job); + if (job_txn_apply_locked(job, job_needs_finalize_locked) == 0) { + job_do_finalize_locked(job); } } -static void job_completed(Job *job) +/* Called with job_mutex held. */ +static void job_completed_locked(Job *job) { - assert(job && job->txn && !job_is_completed(job)); + assert(job && job->txn && !job_is_completed_locked(job)); - job_update_rc(job); + job_update_rc_locked(job); trace_job_completed(job, job->ret); if (job->ret) { - job_completed_txn_abort(job); + job_completed_txn_abort_locked(job); } else { - job_completed_txn_success(job); + job_completed_txn_success_locked(job); } } -/** Useful only as a type shim for aio_bh_schedule_oneshot. */ +/** + * Useful only as a type shim for aio_bh_schedule_oneshot. + * Called with job_mutex *not* held. + */ static void job_exit(void *opaque) { Job *job = (Job *)opaque; AioContext *ctx; + JOB_LOCK_GUARD(); - job_ref(job); + job_ref_locked(job); aio_context_acquire(job->aio_context); /* This is a lie, we're not quiescent, but still doing the completion @@ -963,9 +1152,9 @@ static void job_exit(void *opaque) * drain block nodes, and if .drained_poll still returned true, we would * deadlock. */ job->busy = false; - job_event_idle(job); + job_event_idle_locked(job); - job_completed(job); + job_completed_locked(job); /* * Note that calling job_completed can move the job to a different @@ -974,7 +1163,7 @@ static void job_exit(void *opaque) * the job underneath us. */ ctx = job->aio_context; - job_unref(job); + job_unref_locked(job); aio_context_release(ctx); } @@ -985,37 +1174,47 @@ static void job_exit(void *opaque) static void coroutine_fn job_co_entry(void *opaque) { Job *job = opaque; + int ret; assert(job && job->driver && job->driver->run); - assert(job->aio_context == qemu_get_current_aio_context()); - job_pause_point(job); - job->ret = job->driver->run(job, &job->err); - job->deferred_to_main_loop = true; - job->busy = true; + WITH_JOB_LOCK_GUARD() { + assert(job->aio_context == qemu_get_current_aio_context()); + job_pause_point_locked(job); + } + ret = job->driver->run(job, &job->err); + WITH_JOB_LOCK_GUARD() { + job->ret = ret; + job->deferred_to_main_loop = true; + job->busy = true; + } aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job); } void job_start(Job *job) { - assert(job && !job_started(job) && job->paused && - job->driver && job->driver->run); - job->co = qemu_coroutine_create(job_co_entry, job); - job->pause_count--; - job->busy = true; - job->paused = false; - job_state_transition(job, JOB_STATUS_RUNNING); + assert(qemu_in_main_thread()); + + WITH_JOB_LOCK_GUARD() { + assert(job && !job_started(job) && job->paused && + job->driver && job->driver->run); + job->co = qemu_coroutine_create(job_co_entry, job); + job->pause_count--; + job->busy = true; + job->paused = false; + job_state_transition_locked(job, JOB_STATUS_RUNNING); + } aio_co_enter(job->aio_context, job->co); } -void job_cancel(Job *job, bool force) +void job_cancel_locked(Job *job, bool force) { if (job->status == JOB_STATUS_CONCLUDED) { - job_do_dismiss(job); + job_do_dismiss_locked(job); return; } - job_cancel_async(job, force); + job_cancel_async_locked(job, force); if (!job_started(job)) { - job_completed(job); + job_completed_locked(job); } else if (job->deferred_to_main_loop) { /* * job_cancel_async() ignores soft-cancel requests for jobs @@ -1027,102 +1226,150 @@ void job_cancel(Job *job, bool force) * choose to call job_is_cancelled() to show that we invoke * job_completed_txn_abort() only for force-cancelled jobs.) */ - if (job_is_cancelled(job)) { - job_completed_txn_abort(job); + if (job_is_cancelled_locked(job)) { + job_completed_txn_abort_locked(job); } } else { - job_enter(job); + job_enter_cond_locked(job, NULL); } } -void job_user_cancel(Job *job, bool force, Error **errp) +void job_cancel(Job *job, bool force) { - if (job_apply_verb(job, JOB_VERB_CANCEL, errp)) { + JOB_LOCK_GUARD(); + job_cancel_locked(job, force); +} + +void job_user_cancel_locked(Job *job, bool force, Error **errp) +{ + if (job_apply_verb_locked(job, JOB_VERB_CANCEL, errp)) { return; } - job_cancel(job, force); + job_cancel_locked(job, force); +} + +void job_user_cancel(Job *job, bool force, Error **errp) +{ + JOB_LOCK_GUARD(); + job_user_cancel_locked(job, force, errp); } /* A wrapper around job_cancel() taking an Error ** parameter so it may be * used with job_finish_sync() without the need for (rather nasty) function - * pointer casts there. */ -static void job_cancel_err(Job *job, Error **errp) + * pointer casts there. + * + * Called with job_mutex held. + */ +static void job_cancel_err_locked(Job *job, Error **errp) { - job_cancel(job, false); + job_cancel_locked(job, false); } /** * Same as job_cancel_err(), but force-cancel. + * Called with job_mutex held. */ -static void job_force_cancel_err(Job *job, Error **errp) +static void job_force_cancel_err_locked(Job *job, Error **errp) { - job_cancel(job, true); + job_cancel_locked(job, true); } -int job_cancel_sync(Job *job, bool force) +int job_cancel_sync_locked(Job *job, bool force) { if (force) { - return job_finish_sync(job, &job_force_cancel_err, NULL); + return job_finish_sync_locked(job, &job_force_cancel_err_locked, NULL); } else { - return job_finish_sync(job, &job_cancel_err, NULL); + return job_finish_sync_locked(job, &job_cancel_err_locked, NULL); } } +int job_cancel_sync(Job *job, bool force) +{ + JOB_LOCK_GUARD(); + return job_cancel_sync_locked(job, force); +} + void job_cancel_sync_all(void) { Job *job; AioContext *aio_context; + JOB_LOCK_GUARD(); - while ((job = job_next(NULL))) { + while ((job = job_next_locked(NULL))) { aio_context = job->aio_context; aio_context_acquire(aio_context); - job_cancel_sync(job, true); + job_cancel_sync_locked(job, true); aio_context_release(aio_context); } } +int job_complete_sync_locked(Job *job, Error **errp) +{ + return job_finish_sync_locked(job, job_complete_locked, errp); +} + int job_complete_sync(Job *job, Error **errp) { - return job_finish_sync(job, job_complete, errp); + JOB_LOCK_GUARD(); + return job_complete_sync_locked(job, errp); } -void job_complete(Job *job, Error **errp) +void job_complete_locked(Job *job, Error **errp) { /* Should not be reachable via external interface for internal jobs */ assert(job->id); GLOBAL_STATE_CODE(); - if (job_apply_verb(job, JOB_VERB_COMPLETE, errp)) { + if (job_apply_verb_locked(job, JOB_VERB_COMPLETE, errp)) { return; } - if (job_cancel_requested(job) || !job->driver->complete) { + if (job_cancel_requested_locked(job) || !job->driver->complete) { error_setg(errp, "The active block job '%s' cannot be completed", job->id); return; } + job_unlock(); job->driver->complete(job, errp); + job_lock(); } -int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp) +void job_complete(Job *job, Error **errp) +{ + JOB_LOCK_GUARD(); + job_complete_locked(job, errp); +} + +int job_finish_sync_locked(Job *job, + void (*finish)(Job *, Error **errp), + Error **errp) { Error *local_err = NULL; int ret; - job_ref(job); + job_ref_locked(job); if (finish) { finish(job, &local_err); } if (local_err) { error_propagate(errp, local_err); - job_unref(job); + job_unref_locked(job); return -EBUSY; } - AIO_WAIT_WHILE(job->aio_context, - (job_enter(job), !job_is_completed(job))); + job_unlock(); + AIO_WAIT_WHILE_UNLOCKED(job->aio_context, + (job_enter(job), !job_is_completed(job))); + job_lock(); - ret = (job_is_cancelled(job) && job->ret == 0) ? -ECANCELED : job->ret; - job_unref(job); + ret = (job_is_cancelled_locked(job) && job->ret == 0) + ? -ECANCELED : job->ret; + job_unref_locked(job); return ret; } + +int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp) +{ + JOB_LOCK_GUARD(); + return job_finish_sync_locked(job, finish, errp); +} \ No newline at end of file From patchwork Wed Jun 29 14:15:24 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900218 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B8A37C43334 for ; Wed, 29 Jun 2022 14:37:32 +0000 (UTC) Received: from localhost ([::1]:33364 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6Yot-0001ty-Q9 for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:37:31 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36400) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTy-0007wi-D4 for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:54 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]:31480) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTt-00023J-RL for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:54 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512149; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=a5AaHjl5kBfuJCVPN9dewCN9kesbyjxpkbexHJDD7MY=; b=MeFUC2tvy1bvIyu8BmcAOfbOGuCowKcE4oXfOEzi4NvdZO444CD6rAjEUZM0YUQJsta0Tc 26E1VJMJSbXzkJPAwNZ7m/6e/a0/n9yQNa5WA4QD/4SkAP7qYoSDpKkwrrKqSf/IJyoV9p Wd4G127u5vro81SNrD+v+orFlnzq92U= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-523-9E_I11XmPcm5F6iU5WpbHg-1; Wed, 29 Jun 2022 10:15:45 -0400 X-MC-Unique: 9E_I11XmPcm5F6iU5WpbHg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 337FA801233; Wed, 29 Jun 2022 14:15:45 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id D81D918EAD; Wed, 29 Jun 2022 14:15:44 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 06/20] job.h: define functions called without job lock held Date: Wed, 29 Jun 2022 10:15:24 -0400 Message-Id: <20220629141538.3400679-7-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.129.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -28 X-Spam_score: -2.9 X-Spam_bar: -- X-Spam_report: (-2.9 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" These functions don't need a _locked() counterpart, since they are all called outside job.c and take the lock only internally. Update also the comments in blockjob.c (and move them in job.c). Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. No functional change intended. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- blockjob.c | 20 -------------------- include/qemu/job.h | 37 ++++++++++++++++++++++++++++++++++--- job.c | 15 +++++++++++++++ 3 files changed, 49 insertions(+), 23 deletions(-) diff --git a/blockjob.c b/blockjob.c index 4868453d74..7da59a1f1c 100644 --- a/blockjob.c +++ b/blockjob.c @@ -36,21 +36,6 @@ #include "qemu/main-loop.h" #include "qemu/timer.h" -/* - * The block job API is composed of two categories of functions. - * - * The first includes functions used by the monitor. The monitor is - * peculiar in that it accesses the block job list with block_job_get, and - * therefore needs consistency across block_job_get and the actual operation - * (e.g. block_job_set_speed). The consistency is achieved with - * aio_context_acquire/release. These functions are declared in blockjob.h. - * - * The second includes functions used by the block job drivers and sometimes - * by the core block layer. These do not care about locking, because the - * whole coroutine runs under the AioContext lock, and are declared in - * blockjob_int.h. - */ - static bool is_block_job(Job *job) { return job_type(job) == JOB_TYPE_BACKUP || @@ -433,11 +418,6 @@ static void block_job_event_ready(Notifier *n, void *opaque) } -/* - * API for block job drivers and the block layer. These functions are - * declared in blockjob_int.h. - */ - void *block_job_create(const char *job_id, const BlockJobDriver *driver, JobTxn *txn, BlockDriverState *bs, uint64_t perm, uint64_t shared_perm, int64_t speed, int flags, diff --git a/include/qemu/job.h b/include/qemu/job.h index 99960cc9a3..b714236c1a 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -363,6 +363,7 @@ void job_txn_unref_locked(JobTxn *txn); /** * Create a new long-running job and return it. + * Called with job_mutex *not* held. * * @job_id: The id of the newly-created job, or %NULL for internal jobs * @driver: The class object for the newly-created job. @@ -400,6 +401,8 @@ void job_unref_locked(Job *job); * @done: How much progress the job made since the last call * * Updates the progress counter of the job. + * + * Progress API is thread safe. */ void job_progress_update(Job *job, uint64_t done); @@ -410,6 +413,8 @@ void job_progress_update(Job *job, uint64_t done); * * Sets the expected end value of the progress counter of a job so that a * completion percentage can be calculated when the progress is updated. + * + * Progress API is thread safe. */ void job_progress_set_remaining(Job *job, uint64_t remaining); @@ -425,6 +430,8 @@ void job_progress_set_remaining(Job *job, uint64_t remaining); * length before, and job_progress_update() afterwards. * (So the operation acts as a parenthesis in regards to the main job * operation running in background.) + * + * Progress API is thread safe. */ void job_progress_increase_remaining(Job *job, uint64_t delta); @@ -443,6 +450,8 @@ void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)); * * Begins execution of a job. * Takes ownership of one reference to the job object. + * + * Called with job_mutex *not* held. */ void job_start(Job *job); @@ -450,6 +459,7 @@ void job_start(Job *job); * @job: The job to enter. * * Continue the specified job by entering the coroutine. + * Called with job_mutex *not* held. */ void job_enter(Job *job); @@ -458,6 +468,9 @@ void job_enter(Job *job); * * Pause now if job_pause() has been called. Jobs that perform lots of I/O * must call this between requests so that the job can be paused. + * + * Called with job_mutex *not* held (we don't want the coroutine + * to yield with the lock held!). */ void coroutine_fn job_pause_point(Job *job); @@ -465,6 +478,8 @@ void coroutine_fn job_pause_point(Job *job); * @job: The job that calls the function. * * Yield the job coroutine. + * Called with job_mutex *not* held (we don't want the coroutine + * to yield with the lock held!). */ void job_yield(Job *job); @@ -475,6 +490,9 @@ void job_yield(Job *job); * Put the job to sleep (assuming that it wasn't canceled) for @ns * %QEMU_CLOCK_REALTIME nanoseconds. Canceling the job will immediately * interrupt the wait. + * + * Called with job_mutex *not* held (we don't want the coroutine + * to yield with the lock held!). */ void coroutine_fn job_sleep_ns(Job *job, int64_t ns); @@ -496,6 +514,7 @@ bool job_is_cancelled_locked(Job *job); /** * Returns whether the job is scheduled for cancellation (at an * indefinite point). + * Called with job_mutex *not* held. */ bool job_cancel_requested(Job *job); @@ -582,10 +601,16 @@ int job_apply_verb(Job *job, JobVerb verb, Error **errp); /* Same as job_apply_verb, but called with job lock held. */ int job_apply_verb_locked(Job *job, JobVerb verb, Error **errp); -/** The @job could not be started, free it. */ +/** + * The @job could not be started, free it. + * Called with job_mutex *not* held. + */ void job_early_fail(Job *job); -/** Moves the @job from RUNNING to READY */ +/** + * Moves the @job from RUNNING to READY. + * Called with job_mutex *not* held. + */ void job_transition_to_ready(Job *job); /** Asynchronously complete the specified @job. */ @@ -628,7 +653,13 @@ int job_cancel_sync(Job *job, bool force); /* Same as job_cancel_sync, but called with job lock held. */ int job_cancel_sync_locked(Job *job, bool force); -/** Synchronously force-cancels all jobs using job_cancel_sync(). */ +/** + * Synchronously force-cancels all jobs using job_cancel_sync_locked(). + * + * Called with job_lock *not* held, unlike most other APIs consumed + * by the monitor! This is primarly to avoid adding unnecessary lock-unlock + * patterns in the caller. + */ void job_cancel_sync_all(void); /** diff --git a/job.c b/job.c index dd44fac8dd..7a3cc93f66 100644 --- a/job.c +++ b/job.c @@ -32,12 +32,27 @@ #include "trace/trace-root.h" #include "qapi/qapi-events-job.h" +/* + * The job API is composed of two categories of functions. + * + * The first includes functions used by the monitor. The monitor is + * peculiar in that it accesses the block job list with job_get, and + * therefore needs consistency across job_get and the actual operation + * (e.g. job_user_cancel). To achieve this consistency, the caller + * calls job_lock/job_unlock itself around the whole operation. + * + * + * The second includes functions used by the block job drivers and sometimes + * by the core block layer. These delegate the locking to the callee instead. + */ + /* * job_mutex protects the jobs list, but also makes the * struct job fields thread-safe. */ QemuMutex job_mutex; +/* Protected by job_mutex */ static QLIST_HEAD(, Job) jobs = QLIST_HEAD_INITIALIZER(jobs); /* Job State Transition Table */ From patchwork Wed Jun 29 14:15:25 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900172 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 7CFC1C433EF for ; Wed, 29 Jun 2022 14:30:39 +0000 (UTC) Received: from localhost ([::1]:44514 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6YiE-0005Fa-FL for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:30:38 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36304) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTw-0007un-9Q for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:52 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]:46633) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTs-00020e-35 for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:51 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512147; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=qRiGxbMhY5zNyj0sPP/NQI3ZH/xWGMdWTyXSXJweSG4=; b=QiGqRTceIHOWT1OupPf1GxYCYebnkUuDhw2AaKOEQAnp6yw0+rCK57MgHAz4+CUCLiLPQm tEP0zNYlsdWVxF8NEp/ZUNwQQyADihg3ru1cgTXoEE75RSlcTJrGZBLd/jluXR36dLztBm EsW2WpKdfaouT/VqtT4knfamjt46zJI= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-581-8vgD3S0SN3mXwlwjZirBFw-1; Wed, 29 Jun 2022 10:15:46 -0400 X-MC-Unique: 8vgD3S0SN3mXwlwjZirBFw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 8C519185A79C; Wed, 29 Jun 2022 14:15:45 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3B91D9D63; Wed, 29 Jun 2022 14:15:45 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 07/20] job.h: add _locked public functions Date: Wed, 29 Jun 2022 10:15:25 -0400 Message-Id: <20220629141538.3400679-8-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.129.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -28 X-Spam_score: -2.9 X-Spam_bar: -- X-Spam_report: (-2.9 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" These functions will be used later when we use the job lock. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- include/qemu/job.h | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index b714236c1a..e887f88cb2 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -505,7 +505,10 @@ const char *job_type_str(const Job *job); /** Returns true if the job should not be visible to the management layer. */ bool job_is_internal(Job *job); -/** Returns whether the job is being cancelled. */ +/** + * Returns whether the job is being cancelled. + * Called with job_mutex *not* held. + */ bool job_is_cancelled(Job *job); /* Same as job_is_cancelled(), but called with job lock held. */ @@ -518,13 +521,19 @@ bool job_is_cancelled_locked(Job *job); */ bool job_cancel_requested(Job *job); -/** Returns whether the job is in a completed state. */ +/** + * Returns whether the job is in a completed state. + * Called with job_mutex *not* held. + */ bool job_is_completed(Job *job); /* Same as job_is_completed(), but called with job lock held. */ bool job_is_completed_locked(Job *job); -/** Returns whether the job is ready to be completed. */ +/** + * Returns whether the job is ready to be completed. + * Called with job_mutex *not* held. + */ bool job_is_ready(Job *job); /* Same as job_is_ready(), but called with job lock held. */ From patchwork Wed Jun 29 14:15:26 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900215 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 79F52C43334 for ; Wed, 29 Jun 2022 14:35:15 +0000 (UTC) Received: from localhost ([::1]:56398 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6Ymg-000633-FU for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:35:14 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36392) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTy-0007wf-9p for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:54 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:47561) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTt-00022q-O9 for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:53 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512148; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=+B22VuMqef3uTddmFmvYC+qaug2bOt+ysY22FYnYlAU=; b=dYWmyKoPRTfmulkTQ84kkPEBUG1epgvyXssCLPVGBhNvRY0FZkvpDRmIt2SvjBmxhj5Ba8 oga7ZbLl6uawol27W6pQ6PtSISVpCtoD4kkPxO6dSYv56qhnoAO/0MkoeSfR2aca9IGZ3n cm4ktwFiM8Aqx93RdPG8euFkpoQWrm4= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-616-_rSl2kxEOoiUQklUFk6Ziw-1; Wed, 29 Jun 2022 10:15:46 -0400 X-MC-Unique: _rSl2kxEOoiUQklUFk6Ziw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E469480029D; Wed, 29 Jun 2022 14:15:45 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id 941999D63; Wed, 29 Jun 2022 14:15:45 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 08/20] blockjob.h: introduce block_job _locked() APIs Date: Wed, 29 Jun 2022 10:15:26 -0400 Message-Id: <20220629141538.3400679-9-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Just as done with job.h, create _locked() functions in blockjob.h These functions will be later useful when caller has already taken the lock. All blockjob _locked functions call job _locked functions. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito --- blockjob.c | 52 ++++++++++++++++++++++++++++++++-------- include/block/blockjob.h | 15 ++++++++++++ 2 files changed, 57 insertions(+), 10 deletions(-) diff --git a/blockjob.c b/blockjob.c index 7da59a1f1c..0d59aba439 100644 --- a/blockjob.c +++ b/blockjob.c @@ -44,21 +44,27 @@ static bool is_block_job(Job *job) job_type(job) == JOB_TYPE_STREAM; } -BlockJob *block_job_next(BlockJob *bjob) +BlockJob *block_job_next_locked(BlockJob *bjob) { Job *job = bjob ? &bjob->job : NULL; GLOBAL_STATE_CODE(); do { - job = job_next(job); + job = job_next_locked(job); } while (job && !is_block_job(job)); return job ? container_of(job, BlockJob, job) : NULL; } -BlockJob *block_job_get(const char *id) +BlockJob *block_job_next(BlockJob *bjob) { - Job *job = job_get(id); + JOB_LOCK_GUARD(); + return block_job_next_locked(bjob); +} + +BlockJob *block_job_get_locked(const char *id) +{ + Job *job = job_get_locked(id); GLOBAL_STATE_CODE(); if (job && is_block_job(job)) { @@ -68,6 +74,12 @@ BlockJob *block_job_get(const char *id) } } +BlockJob *block_job_get(const char *id) +{ + JOB_LOCK_GUARD(); + return block_job_get_locked(id); +} + void block_job_free(Job *job) { BlockJob *bjob = container_of(job, BlockJob, job); @@ -256,14 +268,14 @@ static bool job_timer_pending(Job *job) return timer_pending(&job->sleep_timer); } -bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp) { const BlockJobDriver *drv = block_job_driver(job); int64_t old_speed = job->speed; GLOBAL_STATE_CODE(); - if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) { + if (job_apply_verb_locked(&job->job, JOB_VERB_SET_SPEED, errp) < 0) { return false; } if (speed < 0) { @@ -277,7 +289,9 @@ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) job->speed = speed; if (drv->set_speed) { + job_unlock(); drv->set_speed(job, speed); + job_lock(); } if (speed && speed <= old_speed) { @@ -285,18 +299,24 @@ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) } /* kick only if a timer is pending */ - job_enter_cond(&job->job, job_timer_pending); + job_enter_cond_locked(&job->job, job_timer_pending); return true; } +bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) +{ + JOB_LOCK_GUARD(); + return block_job_set_speed_locked(job, speed, errp); +} + int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n) { IO_CODE(); return ratelimit_calculate_delay(&job->limit, n); } -BlockJobInfo *block_job_query(BlockJob *job, Error **errp) +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp) { BlockJobInfo *info; uint64_t progress_current, progress_total; @@ -320,7 +340,7 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) info->len = progress_total; info->speed = job->speed; info->io_status = job->iostatus; - info->ready = job_is_ready(&job->job), + info->ready = job_is_ready_locked(&job->job), info->status = job->job.status; info->auto_finalize = job->job.auto_finalize; info->auto_dismiss = job->job.auto_dismiss; @@ -333,6 +353,12 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) return info; } +BlockJobInfo *block_job_query(BlockJob *job, Error **errp) +{ + JOB_LOCK_GUARD(); + return block_job_query_locked(job, errp); +} + static void block_job_iostatus_set_err(BlockJob *job, int error) { if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) { @@ -478,7 +504,7 @@ fail: return NULL; } -void block_job_iostatus_reset(BlockJob *job) +void block_job_iostatus_reset_locked(BlockJob *job) { GLOBAL_STATE_CODE(); if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) { @@ -488,6 +514,12 @@ void block_job_iostatus_reset(BlockJob *job) job->iostatus = BLOCK_DEVICE_IO_STATUS_OK; } +void block_job_iostatus_reset(BlockJob *job) +{ + JOB_LOCK_GUARD(); + block_job_iostatus_reset_locked(job); +} + void block_job_user_resume(Job *job) { BlockJob *bjob = container_of(job, BlockJob, job); diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 6525e16fd5..3959a98612 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -92,6 +92,9 @@ typedef struct BlockJob { */ BlockJob *block_job_next(BlockJob *job); +/* Same as block_job_next(), but called with job lock held. */ +BlockJob *block_job_next_locked(BlockJob *job); + /** * block_job_get: * @id: The id of the block job. @@ -102,6 +105,9 @@ BlockJob *block_job_next(BlockJob *job); */ BlockJob *block_job_get(const char *id); +/* Same as block_job_get(), but called with job lock held. */ +BlockJob *block_job_get_locked(const char *id); + /** * block_job_add_bdrv: * @job: A block job @@ -145,6 +151,9 @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs); */ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); +/* Same as block_job_set_speed(), but called with job lock held. */ +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp); + /** * block_job_query: * @job: The job to get information about. @@ -153,6 +162,9 @@ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); */ BlockJobInfo *block_job_query(BlockJob *job, Error **errp); +/* Same as block_job_query(), but called with job lock held. */ +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp); + /** * block_job_iostatus_reset: * @job: The job whose I/O status should be reset. @@ -162,6 +174,9 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp); */ void block_job_iostatus_reset(BlockJob *job); +/* Same as block_job_iostatus_reset(), but called with job lock held. */ +void block_job_iostatus_reset_locked(BlockJob *job); + /* * block_job_get_aio_context: * From patchwork Wed Jun 29 14:15:27 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900228 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id BBC6DC433EF for ; Wed, 29 Jun 2022 14:38:56 +0000 (UTC) Received: from localhost ([::1]:37208 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6YqF-0004iA-If for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:38:55 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36566) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YU4-0007xT-0v for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:03 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]:32989) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTv-000249-KQ for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:59 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512150; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=fZLIi4ztmhGQMSsiZRqJz/mEYK9KxeBAMKyqhVKObsw=; b=a/NyyWckQtfHB1kpYk1IOyaWK+4199g1IniOtNsW0v3fivDvMtQcC0RkwYaMSqlNNBU7MY oUCCuw/6U5n0U/4JycQVD8Koc3Vhm/6SiQUkSMRUsBZYZm+mUJO5w84nHuOY3Rwwxsr/AD FJUTcRrDcnZocvZh8NkiuWMVja6AH7A= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-249-AoxynU6EPsS_VUZVdCVdzg-1; Wed, 29 Jun 2022 10:15:46 -0400 X-MC-Unique: AoxynU6EPsS_VUZVdCVdzg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 46E21384F806; Wed, 29 Jun 2022 14:15:46 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id EB60C9D63; Wed, 29 Jun 2022 14:15:45 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 09/20] blockjob: rename notifier callbacks as _locked Date: Wed, 29 Jun 2022 10:15:27 -0400 Message-Id: <20220629141538.3400679-10-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.129.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -28 X-Spam_score: -2.9 X-Spam_bar: -- X-Spam_report: (-2.9 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" They all are called with job_lock held, in job_event_*_locked() Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- blockjob.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/blockjob.c b/blockjob.c index 0d59aba439..70952879d8 100644 --- a/blockjob.c +++ b/blockjob.c @@ -247,7 +247,8 @@ int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs, return 0; } -static void block_job_on_idle(Notifier *n, void *opaque) +/* Called with job_mutex lock held. */ +static void block_job_on_idle_locked(Notifier *n, void *opaque) { aio_wait_kick(); } @@ -367,7 +368,8 @@ static void block_job_iostatus_set_err(BlockJob *job, int error) } } -static void block_job_event_cancelled(Notifier *n, void *opaque) +/* Called with job_mutex lock held. */ +static void block_job_event_cancelled_locked(Notifier *n, void *opaque) { BlockJob *job = opaque; uint64_t progress_current, progress_total; @@ -386,7 +388,8 @@ static void block_job_event_cancelled(Notifier *n, void *opaque) job->speed); } -static void block_job_event_completed(Notifier *n, void *opaque) +/* Called with job_mutex lock held. */ +static void block_job_event_completed_locked(Notifier *n, void *opaque) { BlockJob *job = opaque; const char *msg = NULL; @@ -412,7 +415,8 @@ static void block_job_event_completed(Notifier *n, void *opaque) msg); } -static void block_job_event_pending(Notifier *n, void *opaque) +/* Called with job_mutex lock held. */ +static void block_job_event_pending_locked(Notifier *n, void *opaque) { BlockJob *job = opaque; @@ -424,7 +428,8 @@ static void block_job_event_pending(Notifier *n, void *opaque) job->job.id); } -static void block_job_event_ready(Notifier *n, void *opaque) +/* Called with job_mutex lock held. */ +static void block_job_event_ready_locked(Notifier *n, void *opaque) { BlockJob *job = opaque; uint64_t progress_current, progress_total; @@ -469,11 +474,11 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, ratelimit_init(&job->limit); - job->finalize_cancelled_notifier.notify = block_job_event_cancelled; - job->finalize_completed_notifier.notify = block_job_event_completed; - job->pending_notifier.notify = block_job_event_pending; - job->ready_notifier.notify = block_job_event_ready; - job->idle_notifier.notify = block_job_on_idle; + job->finalize_cancelled_notifier.notify = block_job_event_cancelled_locked; + job->finalize_completed_notifier.notify = block_job_event_completed_locked; + job->pending_notifier.notify = block_job_event_pending_locked; + job->ready_notifier.notify = block_job_event_ready_locked; + job->idle_notifier.notify = block_job_on_idle_locked; notifier_list_add(&job->job.on_finalize_cancelled, &job->finalize_cancelled_notifier); From patchwork Wed Jun 29 14:15:28 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900164 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id EAC0DC43334 for ; Wed, 29 Jun 2022 14:23:18 +0000 (UTC) Received: from localhost ([::1]:54042 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6Yb7-0000hB-RO for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:23:17 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36560) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YU3-0007xR-Uu for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:03 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:57385) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTu-00024H-VB for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:15:57 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512150; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4mn4Lh1I0BcuecCjotWgmFnAKxitFsXmLMGOlBw/Ouo=; b=VXBdiw9ptaEPedTV4pKCTwrEbx8nGA9jz4IamLhiLv33AhrRzNs+XCnZTOuGvbRLU56K+G hd9yb573fZsYRI1YiWz/lgm9wdIHTDx1UqaVjfTdt9chR9i7ngxEszp207Jz4pkuw+bGBv ckReg6PQ61SLLHidy2uOIbOpH+HaGT8= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-150--ejrOfEAN-WGIXr5KFdBnQ-1; Wed, 29 Jun 2022 10:15:47 -0400 X-MC-Unique: -ejrOfEAN-WGIXr5KFdBnQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 9E979181E061; Wed, 29 Jun 2022 14:15:46 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4E1719D63; Wed, 29 Jun 2022 14:15:46 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 10/20] jobs: add job lock in find_* functions Date: Wed, 29 Jun 2022 10:15:28 -0400 Message-Id: <20220629141538.3400679-11-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Both blockdev.c and job-qmp.c have TOC/TOU conditions, because they first search for the job and then perform an action on it. Therefore, we need to do the search + action under the same job mutex critical section. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- blockdev.c | 67 +++++++++++++++++++++++++++++++++++++----------------- job-qmp.c | 55 ++++++++++++++++++++++++++++++-------------- 2 files changed, 84 insertions(+), 38 deletions(-) diff --git a/blockdev.c b/blockdev.c index 9230888e34..71f793c4ab 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3302,9 +3302,13 @@ out: aio_context_release(aio_context); } -/* Get a block job using its ID and acquire its AioContext */ -static BlockJob *find_block_job(const char *id, AioContext **aio_context, - Error **errp) +/* + * Get a block job using its ID and acquire its AioContext. + * Called with job_mutex held. + */ +static BlockJob *find_block_job_locked(const char *id, + AioContext **aio_context, + Error **errp) { BlockJob *job; @@ -3312,7 +3316,7 @@ static BlockJob *find_block_job(const char *id, AioContext **aio_context, *aio_context = NULL; - job = block_job_get(id); + job = block_job_get_locked(id); if (!job) { error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE, @@ -3329,13 +3333,16 @@ static BlockJob *find_block_job(const char *id, AioContext **aio_context, void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job; + + JOB_LOCK_GUARD(); + job = find_block_job_locked(device, &aio_context, errp); if (!job) { return; } - block_job_set_speed(job, speed, errp); + block_job_set_speed_locked(job, speed, errp); aio_context_release(aio_context); } @@ -3343,7 +3350,10 @@ void qmp_block_job_cancel(const char *device, bool has_force, bool force, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job; + + JOB_LOCK_GUARD(); + job = find_block_job_locked(device, &aio_context, errp); if (!job) { return; @@ -3353,14 +3363,14 @@ void qmp_block_job_cancel(const char *device, force = false; } - if (job_user_paused(&job->job) && !force) { + if (job_user_paused_locked(&job->job) && !force) { error_setg(errp, "The block job for device '%s' is currently paused", device); goto out; } trace_qmp_block_job_cancel(job); - job_user_cancel(&job->job, force, errp); + job_user_cancel_locked(&job->job, force, errp); out: aio_context_release(aio_context); } @@ -3368,57 +3378,69 @@ out: void qmp_block_job_pause(const char *device, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job; + + JOB_LOCK_GUARD(); + job = find_block_job_locked(device, &aio_context, errp); if (!job) { return; } trace_qmp_block_job_pause(job); - job_user_pause(&job->job, errp); + job_user_pause_locked(&job->job, errp); aio_context_release(aio_context); } void qmp_block_job_resume(const char *device, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job; + + JOB_LOCK_GUARD(); + job = find_block_job_locked(device, &aio_context, errp); if (!job) { return; } trace_qmp_block_job_resume(job); - job_user_resume(&job->job, errp); + job_user_resume_locked(&job->job, errp); aio_context_release(aio_context); } void qmp_block_job_complete(const char *device, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(device, &aio_context, errp); + BlockJob *job; + + JOB_LOCK_GUARD(); + job = find_block_job_locked(device, &aio_context, errp); if (!job) { return; } trace_qmp_block_job_complete(job); - job_complete(&job->job, errp); + job_complete_locked(&job->job, errp); aio_context_release(aio_context); } void qmp_block_job_finalize(const char *id, Error **errp) { AioContext *aio_context; - BlockJob *job = find_block_job(id, &aio_context, errp); + BlockJob *job; + + JOB_LOCK_GUARD(); + job = find_block_job_locked(id, &aio_context, errp); if (!job) { return; } trace_qmp_block_job_finalize(job); - job_ref(&job->job); - job_finalize(&job->job, errp); + job_ref_locked(&job->job); + job_finalize_locked(&job->job, errp); /* * Job's context might have changed via job_finalize (and job_txn_apply @@ -3426,23 +3448,26 @@ void qmp_block_job_finalize(const char *id, Error **errp) * one. */ aio_context = block_job_get_aio_context(job); - job_unref(&job->job); + job_unref_locked(&job->job); aio_context_release(aio_context); } void qmp_block_job_dismiss(const char *id, Error **errp) { AioContext *aio_context; - BlockJob *bjob = find_block_job(id, &aio_context, errp); + BlockJob *bjob; Job *job; + JOB_LOCK_GUARD(); + bjob = find_block_job_locked(id, &aio_context, errp); + if (!bjob) { return; } trace_qmp_block_job_dismiss(bjob); job = &bjob->job; - job_dismiss(&job, errp); + job_dismiss_locked(&job, errp); aio_context_release(aio_context); } diff --git a/job-qmp.c b/job-qmp.c index 829a28aa70..8ce3b7965e 100644 --- a/job-qmp.c +++ b/job-qmp.c @@ -29,14 +29,17 @@ #include "qapi/error.h" #include "trace/trace-root.h" -/* Get a job using its ID and acquire its AioContext */ -static Job *find_job(const char *id, AioContext **aio_context, Error **errp) +/* + * Get a block job using its ID and acquire its AioContext. + * Called with job_mutex held. + */ +static Job *find_job_locked(const char *id, AioContext **aio_context, Error **errp) { Job *job; *aio_context = NULL; - job = job_get(id); + job = job_get_locked(id); if (!job) { error_setg(errp, "Job not found"); return NULL; @@ -51,71 +54,86 @@ static Job *find_job(const char *id, AioContext **aio_context, Error **errp) void qmp_job_cancel(const char *id, Error **errp) { AioContext *aio_context; - Job *job = find_job(id, &aio_context, errp); + Job *job; + + JOB_LOCK_GUARD(); + job = find_job_locked(id, &aio_context, errp); if (!job) { return; } trace_qmp_job_cancel(job); - job_user_cancel(job, true, errp); + job_user_cancel_locked(job, true, errp); aio_context_release(aio_context); } void qmp_job_pause(const char *id, Error **errp) { AioContext *aio_context; - Job *job = find_job(id, &aio_context, errp); + Job *job; + + JOB_LOCK_GUARD(); + job = find_job_locked(id, &aio_context, errp); if (!job) { return; } trace_qmp_job_pause(job); - job_user_pause(job, errp); + job_user_pause_locked(job, errp); aio_context_release(aio_context); } void qmp_job_resume(const char *id, Error **errp) { AioContext *aio_context; - Job *job = find_job(id, &aio_context, errp); + Job *job; + + JOB_LOCK_GUARD(); + job = find_job_locked(id, &aio_context, errp); if (!job) { return; } trace_qmp_job_resume(job); - job_user_resume(job, errp); + job_user_resume_locked(job, errp); aio_context_release(aio_context); } void qmp_job_complete(const char *id, Error **errp) { AioContext *aio_context; - Job *job = find_job(id, &aio_context, errp); + Job *job; + + JOB_LOCK_GUARD(); + job = find_job_locked(id, &aio_context, errp); if (!job) { return; } trace_qmp_job_complete(job); - job_complete(job, errp); + job_complete_locked(job, errp); aio_context_release(aio_context); } void qmp_job_finalize(const char *id, Error **errp) { AioContext *aio_context; - Job *job = find_job(id, &aio_context, errp); + Job *job; + + JOB_LOCK_GUARD(); + job = find_job_locked(id, &aio_context, errp); if (!job) { return; } trace_qmp_job_finalize(job); - job_ref(job); - job_finalize(job, errp); + job_ref_locked(job); + job_finalize_locked(job, errp); /* * Job's context might have changed via job_finalize (and job_txn_apply @@ -123,21 +141,24 @@ void qmp_job_finalize(const char *id, Error **errp) * one. */ aio_context = job->aio_context; - job_unref(job); + job_unref_locked(job); aio_context_release(aio_context); } void qmp_job_dismiss(const char *id, Error **errp) { AioContext *aio_context; - Job *job = find_job(id, &aio_context, errp); + Job *job; + + JOB_LOCK_GUARD(); + job = find_job_locked(id, &aio_context, errp); if (!job) { return; } trace_qmp_job_dismiss(job); - job_dismiss(&job, errp); + job_dismiss_locked(&job, errp); aio_context_release(aio_context); } From patchwork Wed Jun 29 14:15:29 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900174 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A5189C43334 for ; Wed, 29 Jun 2022 14:32:41 +0000 (UTC) Received: from localhost ([::1]:47420 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6YkB-0007cP-H9 for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:32:40 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36622) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YU5-0007xe-Oo for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:03 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:38862) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTv-00024d-PV for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:00 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512151; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=XhXsmXCD29sFoDbXBXW3FWZFBZjNdWfPuRMqM0NHbsc=; b=dsItVE/xp7cNHBa8U6hfWrKXiMyJDwSNVdA7XFn5S2C+CAHbaxSrVT91ccPBZle4DaSCwC 0oNXH1XqINYcOgOCGnt192NI9FMZrexahdrlplG29MbaeGoLpXctcP++bRFrqO7HxbajJv dJJgjf7o7ZqxkQLCg0LTZrPEHtJTgik= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-640-fcWSfjKrMyOPvZmG-ejQKA-1; Wed, 29 Jun 2022 10:15:47 -0400 X-MC-Unique: fcWSfjKrMyOPvZmG-ejQKA-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 016691019C88; Wed, 29 Jun 2022 14:15:47 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id A62CD9D63; Wed, 29 Jun 2022 14:15:46 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 11/20] jobs: use job locks also in the unit tests Date: Wed, 29 Jun 2022 10:15:29 -0400 Message-Id: <20220629141538.3400679-12-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Add missing job synchronization in the unit tests, with explicit locks. We are deliberately using _locked functions wrapped by a guard instead of a normal call because the normal call will be removed in future, as the only usage is limited to the tests. In other words, if a function like job_pause() is/will be only used in tests to avoid: WITH_JOB_LOCK_GUARD(){ job_pause_locked(); } then it is not worth keeping job_pause(), and just use the guard. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- tests/unit/test-bdrv-drain.c | 76 +++++++++++++++++--------- tests/unit/test-block-iothread.c | 8 ++- tests/unit/test-blockjob-txn.c | 24 +++++--- tests/unit/test-blockjob.c | 94 ++++++++++++++++++++++++-------- 4 files changed, 141 insertions(+), 61 deletions(-) diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index 36be84ae55..0db056ea63 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -943,61 +943,83 @@ static void test_blockjob_common_drain_node(enum drain_type drain_type, } } - g_assert_cmpint(job->job.pause_count, ==, 0); - g_assert_false(job->job.paused); - g_assert_true(tjob->running); - g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */ + WITH_JOB_LOCK_GUARD() { + g_assert_cmpint(job->job.pause_count, ==, 0); + g_assert_false(job->job.paused); + g_assert_true(tjob->running); + g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */ + } do_drain_begin_unlocked(drain_type, drain_bs); - if (drain_type == BDRV_DRAIN_ALL) { - /* bdrv_drain_all() drains both src and target */ - g_assert_cmpint(job->job.pause_count, ==, 2); - } else { - g_assert_cmpint(job->job.pause_count, ==, 1); + WITH_JOB_LOCK_GUARD() { + if (drain_type == BDRV_DRAIN_ALL) { + /* bdrv_drain_all() drains both src and target */ + g_assert_cmpint(job->job.pause_count, ==, 2); + } else { + g_assert_cmpint(job->job.pause_count, ==, 1); + } + g_assert_true(job->job.paused); + g_assert_false(job->job.busy); /* The job is paused */ } - g_assert_true(job->job.paused); - g_assert_false(job->job.busy); /* The job is paused */ do_drain_end_unlocked(drain_type, drain_bs); if (use_iothread) { - /* paused is reset in the I/O thread, wait for it */ + /* + * Here we are waiting for the paused status to change, + * so don't bother protecting the read every time. + * + * paused is reset in the I/O thread, wait for it + */ while (job->job.paused) { aio_poll(qemu_get_aio_context(), false); } } - g_assert_cmpint(job->job.pause_count, ==, 0); - g_assert_false(job->job.paused); - g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */ + WITH_JOB_LOCK_GUARD() { + g_assert_cmpint(job->job.pause_count, ==, 0); + g_assert_false(job->job.paused); + g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */ + } do_drain_begin_unlocked(drain_type, target); - if (drain_type == BDRV_DRAIN_ALL) { - /* bdrv_drain_all() drains both src and target */ - g_assert_cmpint(job->job.pause_count, ==, 2); - } else { - g_assert_cmpint(job->job.pause_count, ==, 1); + WITH_JOB_LOCK_GUARD() { + if (drain_type == BDRV_DRAIN_ALL) { + /* bdrv_drain_all() drains both src and target */ + g_assert_cmpint(job->job.pause_count, ==, 2); + } else { + g_assert_cmpint(job->job.pause_count, ==, 1); + } + g_assert_true(job->job.paused); + g_assert_false(job->job.busy); /* The job is paused */ } - g_assert_true(job->job.paused); - g_assert_false(job->job.busy); /* The job is paused */ do_drain_end_unlocked(drain_type, target); if (use_iothread) { - /* paused is reset in the I/O thread, wait for it */ + /* + * Here we are waiting for the paused status to change, + * so don't bother protecting the read every time. + * + * paused is reset in the I/O thread, wait for it + */ while (job->job.paused) { aio_poll(qemu_get_aio_context(), false); } } - g_assert_cmpint(job->job.pause_count, ==, 0); - g_assert_false(job->job.paused); - g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */ + WITH_JOB_LOCK_GUARD() { + g_assert_cmpint(job->job.pause_count, ==, 0); + g_assert_false(job->job.paused); + g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */ + } aio_context_acquire(ctx); - ret = job_complete_sync(&job->job, &error_abort); + WITH_JOB_LOCK_GUARD() { + ret = job_complete_sync_locked(&job->job, &error_abort); + } g_assert_cmpint(ret, ==, (result == TEST_JOB_SUCCESS ? 0 : -EIO)); if (use_iothread) { diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c index 94718c9319..89e7f0fffb 100644 --- a/tests/unit/test-block-iothread.c +++ b/tests/unit/test-block-iothread.c @@ -456,7 +456,9 @@ static void test_attach_blockjob(void) } aio_context_acquire(ctx); - job_complete_sync(&tjob->common.job, &error_abort); + WITH_JOB_LOCK_GUARD() { + job_complete_sync_locked(&tjob->common.job, &error_abort); + } blk_set_aio_context(blk, qemu_get_aio_context(), &error_abort); aio_context_release(ctx); @@ -630,7 +632,9 @@ static void test_propagate_mirror(void) BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT, false, "filter_node", MIRROR_COPY_MODE_BACKGROUND, &error_abort); - job = job_get("job0"); + WITH_JOB_LOCK_GUARD() { + job = job_get_locked("job0"); + } filter = bdrv_find_node("filter_node"); /* Change the AioContext of src */ diff --git a/tests/unit/test-blockjob-txn.c b/tests/unit/test-blockjob-txn.c index c69028b450..d3b0bb24be 100644 --- a/tests/unit/test-blockjob-txn.c +++ b/tests/unit/test-blockjob-txn.c @@ -116,8 +116,10 @@ static void test_single_job(int expected) job = test_block_job_start(1, true, expected, &result, txn); job_start(&job->job); - if (expected == -ECANCELED) { - job_cancel(&job->job, false); + WITH_JOB_LOCK_GUARD() { + if (expected == -ECANCELED) { + job_cancel_locked(&job->job, false); + } } while (result == -EINPROGRESS) { @@ -160,13 +162,15 @@ static void test_pair_jobs(int expected1, int expected2) /* Release our reference now to trigger as many nice * use-after-free bugs as possible. */ - job_txn_unref(txn); + WITH_JOB_LOCK_GUARD() { + job_txn_unref_locked(txn); - if (expected1 == -ECANCELED) { - job_cancel(&job1->job, false); - } - if (expected2 == -ECANCELED) { - job_cancel(&job2->job, false); + if (expected1 == -ECANCELED) { + job_cancel_locked(&job1->job, false); + } + if (expected2 == -ECANCELED) { + job_cancel_locked(&job2->job, false); + } } while (result1 == -EINPROGRESS || result2 == -EINPROGRESS) { @@ -219,7 +223,9 @@ static void test_pair_jobs_fail_cancel_race(void) job_start(&job1->job); job_start(&job2->job); - job_cancel(&job1->job, false); + WITH_JOB_LOCK_GUARD() { + job_cancel_locked(&job1->job, false); + } /* Now make job2 finish before the main loop kicks jobs. This simulates * the race between a pending kick and another job completing. diff --git a/tests/unit/test-blockjob.c b/tests/unit/test-blockjob.c index 4c9e1bf1e5..17755e58db 100644 --- a/tests/unit/test-blockjob.c +++ b/tests/unit/test-blockjob.c @@ -211,8 +211,11 @@ static CancelJob *create_common(Job **pjob) bjob = mk_job(blk, "Steve", &test_cancel_driver, true, JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS); job = &bjob->job; - job_ref(job); - assert(job->status == JOB_STATUS_CREATED); + WITH_JOB_LOCK_GUARD() { + job_ref_locked(job); + assert(job->status == JOB_STATUS_CREATED); + } + s = container_of(bjob, CancelJob, common); s->blk = blk; @@ -231,12 +234,14 @@ static void cancel_common(CancelJob *s) aio_context_acquire(ctx); job_cancel_sync(&job->job, true); - if (sts != JOB_STATUS_CREATED && sts != JOB_STATUS_CONCLUDED) { - Job *dummy = &job->job; - job_dismiss(&dummy, &error_abort); + WITH_JOB_LOCK_GUARD() { + if (sts != JOB_STATUS_CREATED && sts != JOB_STATUS_CONCLUDED) { + Job *dummy = &job->job; + job_dismiss_locked(&dummy, &error_abort); + } + assert(job->job.status == JOB_STATUS_NULL); + job_unref_locked(&job->job); } - assert(job->job.status == JOB_STATUS_NULL); - job_unref(&job->job); destroy_blk(blk); aio_context_release(ctx); @@ -251,6 +256,10 @@ static void test_cancel_created(void) cancel_common(s); } +/* + * This test always runs in the main loop, so there is no + * need to protect job->status. + */ static void test_cancel_running(void) { Job *job; @@ -264,6 +273,10 @@ static void test_cancel_running(void) cancel_common(s); } +/* + * This test always runs in the main loop, so there is no + * need to protect job->status. + */ static void test_cancel_paused(void) { Job *job; @@ -274,13 +287,19 @@ static void test_cancel_paused(void) job_start(job); assert(job->status == JOB_STATUS_RUNNING); - job_user_pause(job, &error_abort); + WITH_JOB_LOCK_GUARD() { + job_user_pause_locked(job, &error_abort); + } job_enter(job); assert(job->status == JOB_STATUS_PAUSED); cancel_common(s); } +/* + * This test always runs in the main loop, so there is no + * need to protect job->status. + */ static void test_cancel_ready(void) { Job *job; @@ -298,6 +317,10 @@ static void test_cancel_ready(void) cancel_common(s); } +/* + * This test always runs in the main loop, so there is no + * need to protect job->status. + */ static void test_cancel_standby(void) { Job *job; @@ -312,13 +335,19 @@ static void test_cancel_standby(void) job_enter(job); assert(job->status == JOB_STATUS_READY); - job_user_pause(job, &error_abort); + WITH_JOB_LOCK_GUARD() { + job_user_pause_locked(job, &error_abort); + } job_enter(job); assert(job->status == JOB_STATUS_STANDBY); cancel_common(s); } +/* + * This test always runs in the main loop, so there is no + * need to protect job->status. + */ static void test_cancel_pending(void) { Job *job; @@ -333,7 +362,9 @@ static void test_cancel_pending(void) job_enter(job); assert(job->status == JOB_STATUS_READY); - job_complete(job, &error_abort); + WITH_JOB_LOCK_GUARD() { + job_complete_locked(job, &error_abort); + } job_enter(job); while (!job->deferred_to_main_loop) { aio_poll(qemu_get_aio_context(), true); @@ -345,6 +376,10 @@ static void test_cancel_pending(void) cancel_common(s); } +/* + * This test always runs in the main loop, so there is no + * need to protect job->status. + */ static void test_cancel_concluded(void) { Job *job; @@ -359,7 +394,9 @@ static void test_cancel_concluded(void) job_enter(job); assert(job->status == JOB_STATUS_READY); - job_complete(job, &error_abort); + WITH_JOB_LOCK_GUARD() { + job_complete_locked(job, &error_abort); + } job_enter(job); while (!job->deferred_to_main_loop) { aio_poll(qemu_get_aio_context(), true); @@ -369,7 +406,9 @@ static void test_cancel_concluded(void) assert(job->status == JOB_STATUS_PENDING); aio_context_acquire(job->aio_context); - job_finalize(job, &error_abort); + WITH_JOB_LOCK_GUARD() { + job_finalize_locked(job, &error_abort); + } aio_context_release(job->aio_context); assert(job->status == JOB_STATUS_CONCLUDED); @@ -459,36 +498,45 @@ static void test_complete_in_standby(void) bjob = mk_job(blk, "job", &test_yielding_driver, true, JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS); job = &bjob->job; + /* Job did not start, so status is safe to read*/ assert(job->status == JOB_STATUS_CREATED); /* Wait for the job to become READY */ job_start(job); aio_context_acquire(ctx); + /* + * Here we are waiting for the status to change, so don't bother + * protecting the read every time. + */ AIO_WAIT_WHILE(ctx, job->status != JOB_STATUS_READY); aio_context_release(ctx); /* Begin the drained section, pausing the job */ bdrv_drain_all_begin(); - assert(job->status == JOB_STATUS_STANDBY); + WITH_JOB_LOCK_GUARD() { + assert(job->status == JOB_STATUS_STANDBY); + } /* Lock the IO thread to prevent the job from being run */ aio_context_acquire(ctx); /* This will schedule the job to resume it */ bdrv_drain_all_end(); - /* But the job cannot run, so it will remain on standby */ - assert(job->status == JOB_STATUS_STANDBY); + WITH_JOB_LOCK_GUARD() { + /* But the job cannot run, so it will remain on standby */ + assert(job->status == JOB_STATUS_STANDBY); - /* Even though the job is on standby, this should work */ - job_complete(job, &error_abort); + /* Even though the job is on standby, this should work */ + job_complete_locked(job, &error_abort); - /* The test is done now, clean up. */ - job_finish_sync(job, NULL, &error_abort); - assert(job->status == JOB_STATUS_PENDING); + /* The test is done now, clean up. */ + job_finish_sync_locked(job, NULL, &error_abort); + assert(job->status == JOB_STATUS_PENDING); - job_finalize(job, &error_abort); - assert(job->status == JOB_STATUS_CONCLUDED); + job_finalize_locked(job, &error_abort); + assert(job->status == JOB_STATUS_CONCLUDED); - job_dismiss(&job, &error_abort); + job_dismiss_locked(&job, &error_abort); + } destroy_blk(blk); aio_context_release(ctx); From patchwork Wed Jun 29 14:15:30 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900253 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9E1A3C43334 for ; Wed, 29 Jun 2022 14:50:27 +0000 (UTC) Received: from localhost ([::1]:52908 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6Z1O-0007AW-N0 for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:50:26 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36702) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YU8-000814-8V for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:04 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]:36267) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTx-00026A-O5 for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:03 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512153; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=onSpRKVFNRnC1visFTu1hPTv/cFs0iPy6zmldJBqPI8=; b=TwQAwmZg/pYq01g0+hYHEDLgtG+g+eDEnoQg2jotieyVyknykcC35LdEFT704ii6bZT8XL 05AT2lZI+Kx20pR1V8vE+muz0e5VKYpYdJWhcX45HMI//i5KbKtV4wAFEdAWXhV95hSHfv r53lYPMxEntl6J5auXKWxROUgtP9sD4= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-339-hpXgYvdyMXOBlU-kV7Sz5w-1; Wed, 29 Jun 2022 10:15:47 -0400 X-MC-Unique: hpXgYvdyMXOBlU-kV7Sz5w-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 58FC51C006BE; Wed, 29 Jun 2022 14:15:47 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0966E18EAA; Wed, 29 Jun 2022 14:15:47 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 12/20] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU Date: Wed, 29 Jun 2022 10:15:30 -0400 Message-Id: <20220629141538.3400679-13-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.129.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -28 X-Spam_score: -2.9 X-Spam_bar: -- X-Spam_report: (-2.9 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Once job lock is used and aiocontext is removed, mirror has to perform job operations under the same critical section, using the helpers prepared in previous commit. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- block/mirror.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/block/mirror.c b/block/mirror.c index d8ecb9efa2..b38676e19d 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -654,9 +654,13 @@ static int mirror_exit_common(Job *job) BlockDriverState *target_bs; BlockDriverState *mirror_top_bs; Error *local_err = NULL; - bool abort = job->ret < 0; + bool abort; int ret = 0; + WITH_JOB_LOCK_GUARD() { + abort = job->ret < 0; + } + if (s->prepared) { return 0; } @@ -1152,8 +1156,10 @@ static void mirror_complete(Job *job, Error **errp) s->should_complete = true; /* If the job is paused, it will be re-entered when it is resumed */ - if (!job->paused) { - job_enter(job); + WITH_JOB_LOCK_GUARD() { + if (!job->paused) { + job_enter_cond_locked(job, NULL); + } } } @@ -1173,8 +1179,11 @@ static bool mirror_drained_poll(BlockJob *job) * from one of our own drain sections, to avoid a deadlock waiting for * ourselves. */ - if (!s->common.job.paused && !job_is_cancelled(&job->job) && !s->in_drain) { - return true; + WITH_JOB_LOCK_GUARD() { + if (!s->common.job.paused && !job_is_cancelled_locked(&job->job) + && !s->in_drain) { + return true; + } } return !!s->in_flight; From patchwork Wed Jun 29 14:15:31 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900252 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id ECD11C433EF for ; Wed, 29 Jun 2022 14:48:42 +0000 (UTC) Received: from localhost ([::1]:50368 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6Yzh-0005Si-TE for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:48:41 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36662) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YU7-0007xl-2x for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:03 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:21379) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTx-00025o-25 for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:02 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512152; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=WNYq0gSSV9cw0Omn6iltZyOpbe1HatzX1nkQ2ELMevI=; b=b77/KZz/EQGuaUxRsdYIGDBG9liTNypNr53TukNs903qUxb5JmPgUgbN+QkuGKUFR5VS7I 02uFDPiss4mHg3GP1I0lTIBpbSpp/qoHmVz3dW/aZtCrL8VIZpuE9+Hv7mtdii0PBQEZoA chmsL1BfbDctH+e2HquJlY8VjYKGPPo= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-668-BbXQYX8RMju_DCwze9Grdg-1; Wed, 29 Jun 2022 10:15:49 -0400 X-MC-Unique: BbXQYX8RMju_DCwze9Grdg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B13BF3C0F38D; Wed, 29 Jun 2022 14:15:47 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6105D18EAB; Wed, 29 Jun 2022 14:15:47 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 13/20] jobs: group together API calls under the same job lock Date: Wed, 29 Jun 2022 10:15:31 -0400 Message-Id: <20220629141538.3400679-14-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Now that the API offers also _locked() functions, take advantage of it and give also the caller control to take the lock and call _locked functions. This makes sense especially when we have for loops, because it makes no sense to have: for(job = job_next(); ...) where each job_next() takes the lock internally. Instead we want JOB_LOCK_GUARD(); for(job = job_next_locked(); ...) Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito --- block.c | 20 +++++++++++------- blockdev.c | 12 ++++++++--- blockjob.c | 52 +++++++++++++++++++++++++++++++--------------- job-qmp.c | 4 +++- job.c | 13 +++++++----- monitor/qmp-cmds.c | 7 +++++-- qemu-img.c | 41 +++++++++++++++++++++--------------- 7 files changed, 97 insertions(+), 52 deletions(-) diff --git a/block.c b/block.c index 2c00dddd80..d0db104d71 100644 --- a/block.c +++ b/block.c @@ -4978,9 +4978,12 @@ static void bdrv_close(BlockDriverState *bs) void bdrv_close_all(void) { - assert(job_next(NULL) == NULL); GLOBAL_STATE_CODE(); + WITH_JOB_LOCK_GUARD() { + assert(job_next_locked(NULL) == NULL); + } + /* Drop references from requests still in flight, such as canceled block * jobs whose AIO context has not been polled yet */ bdrv_drain_all(); @@ -6165,13 +6168,16 @@ XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp) } } - for (job = block_job_next(NULL); job; job = block_job_next(job)) { - GSList *el; + WITH_JOB_LOCK_GUARD() { + for (job = block_job_next_locked(NULL); job; + job = block_job_next_locked(job)) { + GSList *el; - xdbg_graph_add_node(gr, job, X_DBG_BLOCK_GRAPH_NODE_TYPE_BLOCK_JOB, - job->job.id); - for (el = job->nodes; el; el = el->next) { - xdbg_graph_add_edge(gr, job, (BdrvChild *)el->data); + xdbg_graph_add_node(gr, job, X_DBG_BLOCK_GRAPH_NODE_TYPE_BLOCK_JOB, + job->job.id); + for (el = job->nodes; el; el = el->next) { + xdbg_graph_add_edge(gr, job, (BdrvChild *)el->data); + } } } diff --git a/blockdev.c b/blockdev.c index 71f793c4ab..5b79093155 100644 --- a/blockdev.c +++ b/blockdev.c @@ -150,12 +150,15 @@ void blockdev_mark_auto_del(BlockBackend *blk) return; } - for (job = block_job_next(NULL); job; job = block_job_next(job)) { + JOB_LOCK_GUARD(); + + for (job = block_job_next_locked(NULL); job; + job = block_job_next_locked(job)) { if (block_job_has_bdrv(job, blk_bs(blk))) { AioContext *aio_context = job->job.aio_context; aio_context_acquire(aio_context); - job_cancel(&job->job, false); + job_cancel_locked(&job->job, false); aio_context_release(aio_context); } @@ -3745,7 +3748,10 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp) BlockJobInfoList *head = NULL, **tail = &head; BlockJob *job; - for (job = block_job_next(NULL); job; job = block_job_next(job)) { + JOB_LOCK_GUARD(); + + for (job = block_job_next_locked(NULL); job; + job = block_job_next_locked(job)) { BlockJobInfo *value; AioContext *aio_context; diff --git a/blockjob.c b/blockjob.c index 70952879d8..1075def475 100644 --- a/blockjob.c +++ b/blockjob.c @@ -99,7 +99,9 @@ static char *child_job_get_parent_desc(BdrvChild *c) static void child_job_drained_begin(BdrvChild *c) { BlockJob *job = c->opaque; - job_pause(&job->job); + WITH_JOB_LOCK_GUARD() { + job_pause_locked(&job->job); + } } static bool child_job_drained_poll(BdrvChild *c) @@ -111,8 +113,10 @@ static bool child_job_drained_poll(BdrvChild *c) /* An inactive or completed job doesn't have any pending requests. Jobs * with !job->busy are either already paused or have a pause point after * being reentered, so no job driver code will run before they pause. */ - if (!job->busy || job_is_completed(job)) { - return false; + WITH_JOB_LOCK_GUARD() { + if (!job->busy || job_is_completed_locked(job)) { + return false; + } } /* Otherwise, assume that it isn't fully stopped yet, but allow the job to @@ -127,7 +131,9 @@ static bool child_job_drained_poll(BdrvChild *c) static void child_job_drained_end(BdrvChild *c, int *drained_end_counter) { BlockJob *job = c->opaque; - job_resume(&job->job); + WITH_JOB_LOCK_GUARD() { + job_resume_locked(&job->job); + } } static bool child_job_can_set_aio_ctx(BdrvChild *c, AioContext *ctx, @@ -480,13 +486,15 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, job->ready_notifier.notify = block_job_event_ready_locked; job->idle_notifier.notify = block_job_on_idle_locked; - notifier_list_add(&job->job.on_finalize_cancelled, - &job->finalize_cancelled_notifier); - notifier_list_add(&job->job.on_finalize_completed, - &job->finalize_completed_notifier); - notifier_list_add(&job->job.on_pending, &job->pending_notifier); - notifier_list_add(&job->job.on_ready, &job->ready_notifier); - notifier_list_add(&job->job.on_idle, &job->idle_notifier); + WITH_JOB_LOCK_GUARD() { + notifier_list_add(&job->job.on_finalize_cancelled, + &job->finalize_cancelled_notifier); + notifier_list_add(&job->job.on_finalize_completed, + &job->finalize_completed_notifier); + notifier_list_add(&job->job.on_pending, &job->pending_notifier); + notifier_list_add(&job->job.on_ready, &job->ready_notifier); + notifier_list_add(&job->job.on_idle, &job->idle_notifier); + } error_setg(&job->blocker, "block device is in use by block job: %s", job_type_str(&job->job)); @@ -498,7 +506,10 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver, bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker); - if (!block_job_set_speed(job, speed, errp)) { + WITH_JOB_LOCK_GUARD() { + ret = block_job_set_speed_locked(job, speed, errp); + } + if (!ret) { goto fail; } @@ -529,7 +540,9 @@ void block_job_user_resume(Job *job) { BlockJob *bjob = container_of(job, BlockJob, job); GLOBAL_STATE_CODE(); - block_job_iostatus_reset(bjob); + WITH_JOB_LOCK_GUARD() { + block_job_iostatus_reset_locked(bjob); + } } BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err, @@ -563,10 +576,15 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err, action); } if (action == BLOCK_ERROR_ACTION_STOP) { - if (!job->job.user_paused) { - job_pause(&job->job); - /* make the pause user visible, which will be resumed from QMP. */ - job->job.user_paused = true; + WITH_JOB_LOCK_GUARD() { + if (!job->job.user_paused) { + job_pause_locked(&job->job); + /* + * make the pause user visible, which will be + * resumed from QMP. + */ + job->job.user_paused = true; + } } block_job_iostatus_set_err(job, error); } diff --git a/job-qmp.c b/job-qmp.c index 8ce3b7965e..6eff7016b2 100644 --- a/job-qmp.c +++ b/job-qmp.c @@ -192,7 +192,9 @@ JobInfoList *qmp_query_jobs(Error **errp) JobInfoList *head = NULL, **tail = &head; Job *job; - for (job = job_next(NULL); job; job = job_next(job)) { + JOB_LOCK_GUARD(); + + for (job = job_next_locked(NULL); job; job = job_next_locked(job)) { JobInfo *value; AioContext *aio_context; diff --git a/job.c b/job.c index 7a3cc93f66..19d711dc73 100644 --- a/job.c +++ b/job.c @@ -1045,11 +1045,14 @@ static void job_completed_txn_abort_locked(Job *job) /* Called with job_mutex held, but releases it temporarily */ static int job_prepare_locked(Job *job) { + int ret; + GLOBAL_STATE_CODE(); if (job->ret == 0 && job->driver->prepare) { job_unlock(); - job->ret = job->driver->prepare(job); + ret = job->driver->prepare(job); job_lock(); + job->ret = ret; job_update_rc_locked(job); } return job->ret; @@ -1235,10 +1238,10 @@ void job_cancel_locked(Job *job, bool force) * job_cancel_async() ignores soft-cancel requests for jobs * that are already done (i.e. deferred to the main loop). We * have to check again whether the job is really cancelled. - * (job_cancel_requested() and job_is_cancelled() are equivalent - * here, because job_cancel_async() will make soft-cancel - * requests no-ops when deferred_to_main_loop is true. We - * choose to call job_is_cancelled() to show that we invoke + * (job_cancel_requested_locked() and job_is_cancelled_locked() + * are equivalent here, because job_cancel_async() will + * make soft-cancel requests no-ops when deferred_to_main_loop is true. + * We choose to call job_is_cancelled_locked() to show that we invoke * job_completed_txn_abort() only for force-cancelled jobs.) */ if (job_is_cancelled_locked(job)) { diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c index 1ebb89f46c..1897ed7a13 100644 --- a/monitor/qmp-cmds.c +++ b/monitor/qmp-cmds.c @@ -133,8 +133,11 @@ void qmp_cont(Error **errp) blk_iostatus_reset(blk); } - for (job = block_job_next(NULL); job; job = block_job_next(job)) { - block_job_iostatus_reset(job); + WITH_JOB_LOCK_GUARD() { + for (job = block_job_next_locked(NULL); job; + job = block_job_next_locked(job)) { + block_job_iostatus_reset_locked(job); + } } /* Continuing after completed migration. Images have been inactivated to diff --git a/qemu-img.c b/qemu-img.c index 4cf4d2423d..289d88a156 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -912,25 +912,30 @@ static void run_block_job(BlockJob *job, Error **errp) int ret = 0; aio_context_acquire(aio_context); - job_ref(&job->job); - do { - float progress = 0.0f; - aio_poll(aio_context, true); + WITH_JOB_LOCK_GUARD() { + job_ref_locked(&job->job); + do { + float progress = 0.0f; + job_unlock(); + aio_poll(aio_context, true); + + progress_get_snapshot(&job->job.progress, &progress_current, + &progress_total); + if (progress_total) { + progress = (float)progress_current / progress_total * 100.f; + } + qemu_progress_print(progress, 0); + job_lock(); + } while (!job_is_ready_locked(&job->job) && + !job_is_completed_locked(&job->job)); - progress_get_snapshot(&job->job.progress, &progress_current, - &progress_total); - if (progress_total) { - progress = (float)progress_current / progress_total * 100.f; + if (!job_is_completed_locked(&job->job)) { + ret = job_complete_sync_locked(&job->job, errp); + } else { + ret = job->job.ret; } - qemu_progress_print(progress, 0); - } while (!job_is_ready(&job->job) && !job_is_completed(&job->job)); - - if (!job_is_completed(&job->job)) { - ret = job_complete_sync(&job->job, errp); - } else { - ret = job->job.ret; + job_unref_locked(&job->job); } - job_unref(&job->job); aio_context_release(aio_context); /* publish completion progress only when success */ @@ -1083,7 +1088,9 @@ static int img_commit(int argc, char **argv) bdrv_ref(bs); } - job = block_job_get("commit"); + WITH_JOB_LOCK_GUARD() { + job = block_job_get_locked("commit"); + } assert(job); run_block_job(job, &local_err); if (local_err) { From patchwork Wed Jun 29 14:15:32 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900229 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A8081C433EF for ; Wed, 29 Jun 2022 14:38:59 +0000 (UTC) Received: from localhost ([::1]:37362 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6YqI-0004oP-Ob for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:38:58 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36766) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YUA-00085x-RG for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:06 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:38413) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTy-00026x-Kr for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:06 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512154; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=8omTvCcE+op9RCmyy1eWl115+wbqnGF5hVRzvVlR+TM=; b=MFTAXtxH6DaUTii7/KoQJ2S8e3QK8t/FeS8SKZF5Xt34F4YwqfXNqY+9730CKzGXLN8r08 uGLECL/yIRz+q5HWqGFXwYjE4SPDoNyIigzNSJHkgIRCiOyJUYOeXtTVqXbg2KdzYBFQR+ BDSDRBU0HTvOz1w2/0UgU5fHNsi2FJk= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-632-tsvQ3Jf7PJ-e4FES2k5Baw-1; Wed, 29 Jun 2022 10:15:49 -0400 X-MC-Unique: tsvQ3Jf7PJ-e4FES2k5Baw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 14AE3181E061; Wed, 29 Jun 2022 14:15:48 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id B85C19D7F; Wed, 29 Jun 2022 14:15:47 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 14/20] commit and mirror: create new nodes using bdrv_get_aio_context, and not the job aiocontext Date: Wed, 29 Jun 2022 10:15:32 -0400 Message-Id: <20220629141538.3400679-15-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" We are always using the given bs AioContext, so there is no need to take the job ones (which is identical anyways). This also reduces the point we need to check when protecting job.aio_context field. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- block/commit.c | 4 ++-- block/mirror.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/block/commit.c b/block/commit.c index 851d1c557a..336f799172 100644 --- a/block/commit.c +++ b/block/commit.c @@ -370,7 +370,7 @@ void commit_start(const char *job_id, BlockDriverState *bs, goto fail; } - s->base = blk_new(s->common.job.aio_context, + s->base = blk_new(bdrv_get_aio_context(bs), base_perms, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED); @@ -382,7 +382,7 @@ void commit_start(const char *job_id, BlockDriverState *bs, s->base_bs = base; /* Required permissions are already taken with block_job_add_bdrv() */ - s->top = blk_new(s->common.job.aio_context, 0, BLK_PERM_ALL); + s->top = blk_new(bdrv_get_aio_context(bs), 0, BLK_PERM_ALL); ret = blk_insert_bs(s->top, top, errp); if (ret < 0) { goto fail; diff --git a/block/mirror.c b/block/mirror.c index b38676e19d..1977e25171 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1728,7 +1728,7 @@ static BlockJob *mirror_start_job( goto fail; } - s->target = blk_new(s->common.job.aio_context, + s->target = blk_new(bdrv_get_aio_context(bs), target_perms, target_shared_perms); ret = blk_insert_bs(s->target, target, errp); if (ret < 0) { From patchwork Wed Jun 29 14:15:33 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900254 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 69069C43334 for ; Wed, 29 Jun 2022 14:52:24 +0000 (UTC) Received: from localhost ([::1]:55118 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6Z3G-0000KB-HK for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:52:23 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36748) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YU9-00083V-Tp for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:05 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]:40230) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTx-00026G-Vu for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:05 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512153; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=H0esYiaCJfle0JOcYYQjJhmHNoft98dR0V0qIu4mS28=; b=hb0vlSs/WJAxdptu4tI94U2r3TNTr5aqoE7iVuWWI+Q5D2sZ4gdtY5aCZIVZvRyaOVgYax grzjZ3By58GXU31S4ZPQTjFcy0zyu0Fu2jJFObG59cKIBQj30pAUnvx2KNXO6Wn3qbGCbZ RqO36UkndK2pI+7bwYBWviqXFacIgME= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-663-cwDLRmbUNUm8ndkw1w4REQ-1; Wed, 29 Jun 2022 10:15:48 -0400 X-MC-Unique: cwDLRmbUNUm8ndkw1w4REQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 67AB41C006CA; Wed, 29 Jun 2022 14:15:48 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1CF969D63; Wed, 29 Jun 2022 14:15:48 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org Subject: [PATCH v8 15/20] job: detect change of aiocontext within job coroutine Date: Wed, 29 Jun 2022 10:15:33 -0400 Message-Id: <20220629141538.3400679-16-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.129.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -28 X-Spam_score: -2.9 X-Spam_bar: -- X-Spam_report: (-2.9 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" From: Paolo Bonzini We want to make sure access of job->aio_context is always done under either BQL or job_mutex. The problem is that using aio_co_enter(job->aiocontext, job->co) in job_start and job_enter_cond makes the coroutine immediately resume, so we can't hold the job lock. And caching it is not safe either, as it might change. job_start is under BQL, so it can freely read job->aiocontext, but job_enter_cond is not. In order to fix this, use aio_co_wake(): the advantage is that it won't use job->aiocontext, but the main disadvantage is that it won't be able to detect a change of job AioContext. Calling bdrv_try_set_aio_context() will issue the following calls (simplified): * in terms of bdrv callbacks: .drained_begin -> .set_aio_context -> .drained_end * in terms of child_job functions: child_job_drained_begin -> child_job_set_aio_context -> child_job_drained_end * in terms of job functions: job_pause_locked -> job_set_aio_context -> job_resume_locked We can see that after setting the new aio_context, job_resume_locked calls again job_enter_cond, which then invokes aio_co_wake(). But while job->aiocontext has been set in job_set_aio_context, job->co->ctx has not changed, so the coroutine would be entering in the wrong aiocontext. Using aio_co_schedule in job_resume_locked() might seem as a valid alternative, but the problem is that the bh resuming the coroutine is not scheduled immediately, and if in the meanwhile another bdrv_try_set_aio_context() is run (see test_propagate_mirror() in test-block-iothread.c), we would have the first schedule in the wrong aiocontext, and the second set of drains won't even manage to schedule the coroutine, as job->busy would still be true from the previous job_resume_locked(). The solution is to stick with aio_co_wake(), but then detect every time the coroutine resumes back from yielding if job->aio_context has changed. If so, we can reschedule it to the new context. Check for the aiocontext change in job_do_yield_locked because: 1) aio_co_reschedule_self requires to be in the running coroutine 2) since child_job_set_aio_context allows changing the aiocontext only while the job is paused, this is the exact place where the coroutine resumes, before running JobDriver's code. Signed-off-by: Paolo Bonzini Reviewed-by: Stefan Hajnoczi --- job.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/job.c b/job.c index 19d711dc73..8db80b8086 100644 --- a/job.c +++ b/job.c @@ -581,11 +581,12 @@ void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)) return; } - assert(!job->deferred_to_main_loop); timer_del(&job->sleep_timer); job->busy = true; real_job_unlock(); - aio_co_enter(job->aio_context, job->co); + job_unlock(); + aio_co_wake(job->co); + job_lock(); } void job_enter_cond(Job *job, bool(*fn)(Job *job)) @@ -611,6 +612,8 @@ void job_enter(Job *job) */ static void coroutine_fn job_do_yield_locked(Job *job, uint64_t ns) { + AioContext *next_aio_context; + real_job_lock(); if (ns != -1) { timer_mod(&job->sleep_timer, ns); @@ -622,7 +625,20 @@ static void coroutine_fn job_do_yield_locked(Job *job, uint64_t ns) qemu_coroutine_yield(); job_lock(); - /* Set by job_enter_cond() before re-entering the coroutine. */ + next_aio_context = job->aio_context; + /* + * Coroutine has resumed, but in the meanwhile the job AioContext + * might have changed via bdrv_try_set_aio_context(), so we need to move + * the coroutine too in the new aiocontext. + */ + while (qemu_get_current_aio_context() != next_aio_context) { + job_unlock(); + aio_co_reschedule_self(next_aio_context); + job_lock(); + next_aio_context = job->aio_context; + } + + /* Set by job_enter_cond_locked() before re-entering the coroutine. */ assert(job->busy); } From patchwork Wed Jun 29 14:15:34 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900216 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3056CC43334 for ; Wed, 29 Jun 2022 14:35:47 +0000 (UTC) Received: from localhost ([::1]:57212 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6YnC-0006fX-9Y for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:35:46 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36756) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YU9-00083a-VQ for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:06 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:40339) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTy-00026X-6A for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:05 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512153; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=8oGklbGMcXuEKkCX2fd3FQ7B2T97dW+S6k0jrBlY1FI=; b=NUQGIlT8NOgz5jdK0XrNS2tBbE5E7jCtSMh4952lx4A7Y8rJIKE5Y+dzsx2M3bAfxl6Bp4 aloxLhUwrNI/pTkXECdAd6B+aiAwsxcYpvnHWls/65V9QrCwA7NzOhivoHP7P82I53ueSY fYVsggPrQa+EPfjQdeMA/vr2MGSXl94= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-612-xm2NyXVnPJq7wUCnWoPYtg-1; Wed, 29 Jun 2022 10:15:49 -0400 X-MC-Unique: xm2NyXVnPJq7wUCnWoPYtg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id BEE2A936EB1; Wed, 29 Jun 2022 14:15:48 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6F23C9D63; Wed, 29 Jun 2022 14:15:48 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 16/20] jobs: protect job.aio_context with BQL and job_mutex Date: Wed, 29 Jun 2022 10:15:34 -0400 Message-Id: <20220629141538.3400679-17-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" In order to make it thread safe, implement a "fake rwlock", where we allow reads under BQL *or* job_mutex held, but writes only under BQL *and* job_mutex. The only write we have is in child_job_set_aio_ctx, which always happens under drain (so the job is paused). For this reason, introduce job_set_aio_context and make sure that the context is set under BQL, job_mutex and drain. Also make sure all other places where the aiocontext is read are protected. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Suggested-by: Paolo Bonzini Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- block/replication.c | 6 ++++-- blockjob.c | 3 ++- include/qemu/job.h | 19 ++++++++++++++++++- job.c | 12 ++++++++++++ 4 files changed, 36 insertions(+), 4 deletions(-) diff --git a/block/replication.c b/block/replication.c index 55c8f894aa..2189863df1 100644 --- a/block/replication.c +++ b/block/replication.c @@ -148,8 +148,10 @@ static void replication_close(BlockDriverState *bs) } if (s->stage == BLOCK_REPLICATION_FAILOVER) { commit_job = &s->commit_job->job; - assert(commit_job->aio_context == qemu_get_current_aio_context()); - job_cancel_sync(commit_job, false); + WITH_JOB_LOCK_GUARD() { + assert(commit_job->aio_context == qemu_get_current_aio_context()); + job_cancel_sync_locked(commit_job, false); + } } if (s->mode == REPLICATION_MODE_SECONDARY) { diff --git a/blockjob.c b/blockjob.c index 1075def475..2293a00b4a 100644 --- a/blockjob.c +++ b/blockjob.c @@ -166,12 +166,13 @@ static void child_job_set_aio_ctx(BdrvChild *c, AioContext *ctx, bdrv_set_aio_context_ignore(sibling->bs, ctx, ignore); } - job->job.aio_context = ctx; + job_set_aio_context(&job->job, ctx); } static AioContext *child_job_get_parent_aio_context(BdrvChild *c) { BlockJob *job = c->opaque; + assert(qemu_in_main_thread()); return job->job.aio_context; } diff --git a/include/qemu/job.h b/include/qemu/job.h index e887f88cb2..8f13c3de61 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -77,7 +77,12 @@ typedef struct Job { /** Protected by AioContext lock */ - /** AioContext to run the job coroutine in */ + /** + * AioContext to run the job coroutine in. + * This field can be read when holding either the BQL (so we are in + * the main loop) or the job_mutex. + * It can be only written when we hold *both* BQL and job_mutex. + */ AioContext *aio_context; /** Reference count of the block job */ @@ -728,4 +733,16 @@ int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp), Error **errp); +/** + * Sets the @job->aio_context. + * Called with job_mutex *not* held. + * + * This function must run in the main thread to protect against + * concurrent read in job_finish_sync_locked(), + * takes the job_mutex lock to protect against the read in + * job_do_yield_locked(), and must be called when the coroutine + * is quiescent. + */ +void job_set_aio_context(Job *job, AioContext *ctx); + #endif diff --git a/job.c b/job.c index 8db80b8086..ad2badd107 100644 --- a/job.c +++ b/job.c @@ -394,6 +394,17 @@ Job *job_get(const char *id) return job_get_locked(id); } +void job_set_aio_context(Job *job, AioContext *ctx) +{ + /* protect against read in job_finish_sync_locked and job_start */ + assert(qemu_in_main_thread()); + /* protect against read in job_do_yield_locked */ + JOB_LOCK_GUARD(); + /* ensure the coroutine is quiescent while the AioContext is changed */ + assert(job->pause_count > 0); + job->aio_context = ctx; +} + /* Called with job_mutex *not* held. */ static void job_sleep_timer_cb(void *opaque) { @@ -1379,6 +1390,7 @@ int job_finish_sync_locked(Job *job, { Error *local_err = NULL; int ret; + assert(qemu_in_main_thread()); job_ref_locked(job); From patchwork Wed Jun 29 14:15:35 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900255 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2A679C43334 for ; Wed, 29 Jun 2022 14:53:27 +0000 (UTC) Received: from localhost ([::1]:57710 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6Z4I-00022e-1S for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:53:26 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36810) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YUC-000879-DS for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:11 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:38506) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YU3-00027Z-8U for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:08 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512156; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=GrGK4BguaFwqa5t62MCQOoEHFkmo2AcZHS/STn3FA4o=; b=XjT7Kkj5kgenD2naW+d8qmIbRrxlASIYmuYEIDehuQmCCU3YB3Au+eBWSvu4IVpZEeeN6s 73Un/5T37+CG+g0kjEOR/crWFf/LUflRNDkT+ZbLPmQClZz6dUKkPJ5GmUVFlpgPY3E3YP SitmURgu9uiC54ElTT2Zyg3GNnNGM5U= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-654-OWL2myhWPzCR97tI8bX59w-1; Wed, 29 Jun 2022 10:15:50 -0400 X-MC-Unique: OWL2myhWPzCR97tI8bX59w-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 27F83293248F; Wed, 29 Jun 2022 14:15:49 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id C6F5318EAC; Wed, 29 Jun 2022 14:15:48 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 17/20] job.c: enable job lock/unlock and remove Aiocontext locks Date: Wed, 29 Jun 2022 10:15:35 -0400 Message-Id: <20220629141538.3400679-18-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Change the job_{lock/unlock} and macros to use job_mutex. Now that they are not nop anymore, remove the aiocontext to avoid deadlocks. Therefore: - when possible, remove completely the aiocontext lock/unlock pair - if it is used by some other function too, reduce the locking section as much as possible, leaving the job API outside. There is only one JobDriver callback, ->free() that assumes that the aiocontext lock is held (because it calls bdrv_unref), so for now keep that under aiocontext lock. Also remove real_job_{lock/unlock}, as they are replaced by the public functions. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- blockdev.c | 74 +++++----------------------- include/qemu/job.h | 22 ++++----- job-qmp.c | 44 ++++------------- job.c | 82 ++++++-------------------------- tests/unit/test-bdrv-drain.c | 4 +- tests/unit/test-block-iothread.c | 2 +- tests/unit/test-blockjob.c | 13 ++--- 7 files changed, 51 insertions(+), 190 deletions(-) diff --git a/blockdev.c b/blockdev.c index 5b79093155..2cd84d206c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -155,12 +155,7 @@ void blockdev_mark_auto_del(BlockBackend *blk) for (job = block_job_next_locked(NULL); job; job = block_job_next_locked(job)) { if (block_job_has_bdrv(job, blk_bs(blk))) { - AioContext *aio_context = job->job.aio_context; - aio_context_acquire(aio_context); - job_cancel_locked(&job->job, false); - - aio_context_release(aio_context); } } @@ -1836,14 +1831,7 @@ static void drive_backup_abort(BlkActionState *common) DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); if (state->job) { - AioContext *aio_context; - - aio_context = bdrv_get_aio_context(state->bs); - aio_context_acquire(aio_context); - job_cancel_sync(&state->job->job, true); - - aio_context_release(aio_context); } } @@ -1937,14 +1925,7 @@ static void blockdev_backup_abort(BlkActionState *common) BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); if (state->job) { - AioContext *aio_context; - - aio_context = bdrv_get_aio_context(state->bs); - aio_context_acquire(aio_context); - job_cancel_sync(&state->job->job, true); - - aio_context_release(aio_context); } } @@ -3306,19 +3287,14 @@ out: } /* - * Get a block job using its ID and acquire its AioContext. - * Called with job_mutex held. + * Get a block job using its ID. Called with job_mutex held. */ -static BlockJob *find_block_job_locked(const char *id, - AioContext **aio_context, - Error **errp) +static BlockJob *find_block_job_locked(const char *id, Error **errp) { BlockJob *job; assert(id != NULL); - *aio_context = NULL; - job = block_job_get_locked(id); if (!job) { @@ -3327,36 +3303,30 @@ static BlockJob *find_block_job_locked(const char *id, return NULL; } - *aio_context = block_job_get_aio_context(job); - aio_context_acquire(*aio_context); - return job; } void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp) { - AioContext *aio_context; BlockJob *job; JOB_LOCK_GUARD(); - job = find_block_job_locked(device, &aio_context, errp); + job = find_block_job_locked(device, errp); if (!job) { return; } block_job_set_speed_locked(job, speed, errp); - aio_context_release(aio_context); } void qmp_block_job_cancel(const char *device, bool has_force, bool force, Error **errp) { - AioContext *aio_context; BlockJob *job; JOB_LOCK_GUARD(); - job = find_block_job_locked(device, &aio_context, errp); + job = find_block_job_locked(device, errp); if (!job) { return; @@ -3369,22 +3339,19 @@ void qmp_block_job_cancel(const char *device, if (job_user_paused_locked(&job->job) && !force) { error_setg(errp, "The block job for device '%s' is currently paused", device); - goto out; + return; } trace_qmp_block_job_cancel(job); job_user_cancel_locked(&job->job, force, errp); -out: - aio_context_release(aio_context); } void qmp_block_job_pause(const char *device, Error **errp) { - AioContext *aio_context; BlockJob *job; JOB_LOCK_GUARD(); - job = find_block_job_locked(device, &aio_context, errp); + job = find_block_job_locked(device, errp); if (!job) { return; @@ -3392,16 +3359,14 @@ void qmp_block_job_pause(const char *device, Error **errp) trace_qmp_block_job_pause(job); job_user_pause_locked(&job->job, errp); - aio_context_release(aio_context); } void qmp_block_job_resume(const char *device, Error **errp) { - AioContext *aio_context; BlockJob *job; JOB_LOCK_GUARD(); - job = find_block_job_locked(device, &aio_context, errp); + job = find_block_job_locked(device, errp); if (!job) { return; @@ -3409,16 +3374,14 @@ void qmp_block_job_resume(const char *device, Error **errp) trace_qmp_block_job_resume(job); job_user_resume_locked(&job->job, errp); - aio_context_release(aio_context); } void qmp_block_job_complete(const char *device, Error **errp) { - AioContext *aio_context; BlockJob *job; JOB_LOCK_GUARD(); - job = find_block_job_locked(device, &aio_context, errp); + job = find_block_job_locked(device, errp); if (!job) { return; @@ -3426,16 +3389,14 @@ void qmp_block_job_complete(const char *device, Error **errp) trace_qmp_block_job_complete(job); job_complete_locked(&job->job, errp); - aio_context_release(aio_context); } void qmp_block_job_finalize(const char *id, Error **errp) { - AioContext *aio_context; BlockJob *job; JOB_LOCK_GUARD(); - job = find_block_job_locked(id, &aio_context, errp); + job = find_block_job_locked(id, errp); if (!job) { return; @@ -3445,24 +3406,16 @@ void qmp_block_job_finalize(const char *id, Error **errp) job_ref_locked(&job->job); job_finalize_locked(&job->job, errp); - /* - * Job's context might have changed via job_finalize (and job_txn_apply - * automatically acquires the new one), so make sure we release the correct - * one. - */ - aio_context = block_job_get_aio_context(job); job_unref_locked(&job->job); - aio_context_release(aio_context); } void qmp_block_job_dismiss(const char *id, Error **errp) { - AioContext *aio_context; BlockJob *bjob; Job *job; JOB_LOCK_GUARD(); - bjob = find_block_job_locked(id, &aio_context, errp); + bjob = find_block_job_locked(id, errp); if (!bjob) { return; @@ -3471,7 +3424,6 @@ void qmp_block_job_dismiss(const char *id, Error **errp) trace_qmp_block_job_dismiss(bjob); job = &bjob->job; job_dismiss_locked(&job, errp); - aio_context_release(aio_context); } void qmp_change_backing_file(const char *device, @@ -3753,15 +3705,11 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp) for (job = block_job_next_locked(NULL); job; job = block_job_next_locked(job)) { BlockJobInfo *value; - AioContext *aio_context; if (block_job_is_internal(job)) { continue; } - aio_context = block_job_get_aio_context(job); - aio_context_acquire(aio_context); - value = block_job_query(job, errp); - aio_context_release(aio_context); + value = block_job_query_locked(job, errp); if (!value) { qapi_free_BlockJobInfoList(head); return NULL; diff --git a/include/qemu/job.h b/include/qemu/job.h index 8f13c3de61..5db35f765c 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -75,13 +75,14 @@ typedef struct Job { ProgressMeter progress; - /** Protected by AioContext lock */ + /** Protected by job_mutex */ /** * AioContext to run the job coroutine in. - * This field can be read when holding either the BQL (so we are in - * the main loop) or the job_mutex. - * It can be only written when we hold *both* BQL and job_mutex. + * The job Aiocontext can be read when holding *either* + * the BQL (so we are in the main loop) or the job_mutex. + * It can only be written when we hold *both* BQL + * and the job_mutex. */ AioContext *aio_context; @@ -106,7 +107,7 @@ typedef struct Job { /** * Set to false by the job while the coroutine has yielded and may be * re-entered by job_enter(). There may still be I/O or event loop activity - * pending. Accessed under block_job_mutex (in blockjob.c). + * pending. Accessed under job_mutex. * * When the job is deferred to the main loop, busy is true as long as the * bottom half is still pending. @@ -322,9 +323,9 @@ typedef enum JobCreateFlags { extern QemuMutex job_mutex; -#define JOB_LOCK_GUARD() /* QEMU_LOCK_GUARD(&job_mutex) */ +#define JOB_LOCK_GUARD() QEMU_LOCK_GUARD(&job_mutex) -#define WITH_JOB_LOCK_GUARD() /* WITH_QEMU_LOCK_GUARD(&job_mutex) */ +#define WITH_JOB_LOCK_GUARD() WITH_QEMU_LOCK_GUARD(&job_mutex) /** * job_lock: @@ -660,7 +661,7 @@ void job_user_cancel_locked(Job *job, bool force, Error **errp); * Returns the return value from the job if the job actually completed * during the call, or -ECANCELED if it was canceled. * - * Callers must hold the AioContext lock of job->aio_context. + * Called with job_lock held. */ int job_cancel_sync(Job *job, bool force); @@ -687,8 +688,7 @@ void job_cancel_sync_all(void); * function). * * Returns the return value from the job. - * - * Callers must hold the AioContext lock of job->aio_context. + * Called with job_lock held. */ int job_complete_sync(Job *job, Error **errp); @@ -724,7 +724,7 @@ void job_dismiss_locked(Job **job, Error **errp); * Returns 0 if the job is successfully completed, -ECANCELED if the job was * cancelled before completing, and -errno in other error cases. * - * Callers must hold the AioContext lock of job->aio_context. + * Called with job_lock held. */ int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp); diff --git a/job-qmp.c b/job-qmp.c index 6eff7016b2..96d67246d2 100644 --- a/job-qmp.c +++ b/job-qmp.c @@ -30,34 +30,27 @@ #include "trace/trace-root.h" /* - * Get a block job using its ID and acquire its AioContext. - * Called with job_mutex held. + * Get a block job using its ID. Called with job_mutex held. */ -static Job *find_job_locked(const char *id, AioContext **aio_context, Error **errp) +static Job *find_job_locked(const char *id, Error **errp) { Job *job; - *aio_context = NULL; - job = job_get_locked(id); if (!job) { error_setg(errp, "Job not found"); return NULL; } - *aio_context = job->aio_context; - aio_context_acquire(*aio_context); - return job; } void qmp_job_cancel(const char *id, Error **errp) { - AioContext *aio_context; Job *job; JOB_LOCK_GUARD(); - job = find_job_locked(id, &aio_context, errp); + job = find_job_locked(id, errp); if (!job) { return; @@ -65,16 +58,14 @@ void qmp_job_cancel(const char *id, Error **errp) trace_qmp_job_cancel(job); job_user_cancel_locked(job, true, errp); - aio_context_release(aio_context); } void qmp_job_pause(const char *id, Error **errp) { - AioContext *aio_context; Job *job; JOB_LOCK_GUARD(); - job = find_job_locked(id, &aio_context, errp); + job = find_job_locked(id, errp); if (!job) { return; @@ -82,16 +73,14 @@ void qmp_job_pause(const char *id, Error **errp) trace_qmp_job_pause(job); job_user_pause_locked(job, errp); - aio_context_release(aio_context); } void qmp_job_resume(const char *id, Error **errp) { - AioContext *aio_context; Job *job; JOB_LOCK_GUARD(); - job = find_job_locked(id, &aio_context, errp); + job = find_job_locked(id, errp); if (!job) { return; @@ -99,16 +88,14 @@ void qmp_job_resume(const char *id, Error **errp) trace_qmp_job_resume(job); job_user_resume_locked(job, errp); - aio_context_release(aio_context); } void qmp_job_complete(const char *id, Error **errp) { - AioContext *aio_context; Job *job; JOB_LOCK_GUARD(); - job = find_job_locked(id, &aio_context, errp); + job = find_job_locked(id, errp); if (!job) { return; @@ -116,16 +103,14 @@ void qmp_job_complete(const char *id, Error **errp) trace_qmp_job_complete(job); job_complete_locked(job, errp); - aio_context_release(aio_context); } void qmp_job_finalize(const char *id, Error **errp) { - AioContext *aio_context; Job *job; JOB_LOCK_GUARD(); - job = find_job_locked(id, &aio_context, errp); + job = find_job_locked(id, errp); if (!job) { return; @@ -135,23 +120,15 @@ void qmp_job_finalize(const char *id, Error **errp) job_ref_locked(job); job_finalize_locked(job, errp); - /* - * Job's context might have changed via job_finalize (and job_txn_apply - * automatically acquires the new one), so make sure we release the correct - * one. - */ - aio_context = job->aio_context; job_unref_locked(job); - aio_context_release(aio_context); } void qmp_job_dismiss(const char *id, Error **errp) { - AioContext *aio_context; Job *job; JOB_LOCK_GUARD(); - job = find_job_locked(id, &aio_context, errp); + job = find_job_locked(id, errp); if (!job) { return; @@ -159,7 +136,6 @@ void qmp_job_dismiss(const char *id, Error **errp) trace_qmp_job_dismiss(job); job_dismiss_locked(&job, errp); - aio_context_release(aio_context); } static JobInfo *job_query_single(Job *job, Error **errp) @@ -196,15 +172,11 @@ JobInfoList *qmp_query_jobs(Error **errp) for (job = job_next_locked(NULL); job; job = job_next_locked(job)) { JobInfo *value; - AioContext *aio_context; if (job_is_internal(job)) { continue; } - aio_context = job->aio_context; - aio_context_acquire(aio_context); value = job_query_single(job, errp); - aio_context_release(aio_context); if (!value) { qapi_free_JobInfoList(head); return NULL; diff --git a/job.c b/job.c index ad2badd107..d0ba463803 100644 --- a/job.c +++ b/job.c @@ -96,21 +96,11 @@ struct JobTxn { }; void job_lock(void) -{ - /* nop */ -} - -void job_unlock(void) -{ - /* nop */ -} - -static void real_job_lock(void) { qemu_mutex_lock(&job_mutex); } -static void real_job_unlock(void) +void job_unlock(void) { qemu_mutex_unlock(&job_mutex); } @@ -185,7 +175,6 @@ static void job_txn_del_job_locked(Job *job) /* Called with job_mutex held. */ static int job_txn_apply_locked(Job *job, int fn(Job *)) { - AioContext *inner_ctx; Job *other_job, *next; JobTxn *txn = job->txn; int rc = 0; @@ -197,23 +186,14 @@ static int job_txn_apply_locked(Job *job, int fn(Job *)) * break AIO_WAIT_WHILE from within fn. */ job_ref_locked(job); - aio_context_release(job->aio_context); QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) { - inner_ctx = other_job->aio_context; - aio_context_acquire(inner_ctx); rc = fn(other_job); - aio_context_release(inner_ctx); if (rc) { break; } } - /* - * Note that job->aio_context might have been changed by calling fn, so we - * can't use a local variable to cache it. - */ - aio_context_acquire(job->aio_context); job_unref_locked(job); return rc; } @@ -501,8 +481,12 @@ void job_unref_locked(Job *job) assert(!job->txn); if (job->driver->free) { + AioContext *aio_context = job->aio_context; job_unlock(); + /* FIXME: aiocontext lock is required because cb calls blk_unref */ + aio_context_acquire(aio_context); job->driver->free(job); + aio_context_release(aio_context); job_lock(); } @@ -581,20 +565,16 @@ void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)) return; } - real_job_lock(); if (job->busy) { - real_job_unlock(); return; } if (fn && !fn(job)) { - real_job_unlock(); return; } timer_del(&job->sleep_timer); job->busy = true; - real_job_unlock(); job_unlock(); aio_co_wake(job->co); job_lock(); @@ -625,13 +605,11 @@ static void coroutine_fn job_do_yield_locked(Job *job, uint64_t ns) { AioContext *next_aio_context; - real_job_lock(); if (ns != -1) { timer_mod(&job->sleep_timer, ns); } job->busy = false; job_event_idle_locked(job); - real_job_unlock(); job_unlock(); qemu_coroutine_yield(); job_lock(); @@ -929,12 +907,15 @@ static void job_clean_locked(Job *job) static int job_finalize_single_locked(Job *job) { int job_ret; + AioContext *ctx = job->aio_context; assert(job_is_completed_locked(job)); /* Ensure abort is called for late-transactional failures */ job_update_rc_locked(job); + aio_context_acquire(ctx); + if (!job->ret) { job_commit_locked(job); } else { @@ -942,6 +923,8 @@ static int job_finalize_single_locked(Job *job) } job_clean_locked(job); + aio_context_release(ctx); + if (job->cb) { job_ret = job->ret; job_unlock(); @@ -1005,7 +988,6 @@ static void job_cancel_async_locked(Job *job, bool force) /* Called with job_mutex held. */ static void job_completed_txn_abort_locked(Job *job) { - AioContext *ctx; JobTxn *txn = job->txn; Job *other_job; @@ -1018,54 +1000,31 @@ static void job_completed_txn_abort_locked(Job *job) txn->aborting = true; job_txn_ref_locked(txn); - /* - * We can only hold the single job's AioContext lock while calling - * job_finalize_single() because the finalization callbacks can involve - * calls of AIO_WAIT_WHILE(), which could deadlock otherwise. - * Note that the job's AioContext may change when it is finalized. - */ job_ref_locked(job); - aio_context_release(job->aio_context); /* Other jobs are effectively cancelled by us, set the status for * them; this job, however, may or may not be cancelled, depending * on the caller, so leave it. */ QLIST_FOREACH(other_job, &txn->jobs, txn_list) { if (other_job != job) { - ctx = other_job->aio_context; - aio_context_acquire(ctx); /* * This is a transaction: If one job failed, no result will matter. * Therefore, pass force=true to terminate all other jobs as quickly * as possible. */ job_cancel_async_locked(other_job, true); - aio_context_release(ctx); } } while (!QLIST_EMPTY(&txn->jobs)) { other_job = QLIST_FIRST(&txn->jobs); - /* - * The job's AioContext may change, so store it in @ctx so we - * release the same context that we have acquired before. - */ - ctx = other_job->aio_context; - aio_context_acquire(ctx); if (!job_is_completed_locked(other_job)) { assert(job_cancel_requested_locked(other_job)); job_finish_sync_locked(other_job, NULL, NULL); } job_finalize_single_locked(other_job); - aio_context_release(ctx); } - /* - * Use job_ref()/job_unref() so we can read the AioContext here - * even if the job went away during job_finalize_single(). - */ - aio_context_acquire(job->aio_context); job_unref_locked(job); - job_txn_unref_locked(txn); } @@ -1073,15 +1032,20 @@ static void job_completed_txn_abort_locked(Job *job) static int job_prepare_locked(Job *job) { int ret; + AioContext *ctx = job->aio_context; GLOBAL_STATE_CODE(); + if (job->ret == 0 && job->driver->prepare) { job_unlock(); + aio_context_acquire(ctx); ret = job->driver->prepare(job); + aio_context_release(ctx); job_lock(); job->ret = ret; job_update_rc_locked(job); } + return job->ret; } @@ -1186,11 +1150,8 @@ static void job_completed_locked(Job *job) static void job_exit(void *opaque) { Job *job = (Job *)opaque; - AioContext *ctx; JOB_LOCK_GUARD(); - job_ref_locked(job); - aio_context_acquire(job->aio_context); /* This is a lie, we're not quiescent, but still doing the completion * callbacks. However, completion callbacks tend to involve operations that @@ -1200,16 +1161,7 @@ static void job_exit(void *opaque) job_event_idle_locked(job); job_completed_locked(job); - - /* - * Note that calling job_completed can move the job to a different - * aio_context, so we cannot cache from above. job_txn_apply takes care of - * acquiring the new lock, and we ref/unref to avoid job_completed freeing - * the job underneath us. - */ - ctx = job->aio_context; job_unref_locked(job); - aio_context_release(ctx); } /** @@ -1337,14 +1289,10 @@ int job_cancel_sync(Job *job, bool force) void job_cancel_sync_all(void) { Job *job; - AioContext *aio_context; JOB_LOCK_GUARD(); while ((job = job_next_locked(NULL))) { - aio_context = job->aio_context; - aio_context_acquire(aio_context); job_cancel_sync_locked(job, true); - aio_context_release(aio_context); } } diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index 0db056ea63..4924ceb562 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -930,9 +930,9 @@ static void test_blockjob_common_drain_node(enum drain_type drain_type, tjob->prepare_ret = -EIO; break; } + aio_context_release(ctx); job_start(&job->job); - aio_context_release(ctx); if (use_iothread) { /* job_co_entry() is run in the I/O thread, wait for the actual job @@ -1016,12 +1016,12 @@ static void test_blockjob_common_drain_node(enum drain_type drain_type, g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */ } - aio_context_acquire(ctx); WITH_JOB_LOCK_GUARD() { ret = job_complete_sync_locked(&job->job, &error_abort); } g_assert_cmpint(ret, ==, (result == TEST_JOB_SUCCESS ? 0 : -EIO)); + aio_context_acquire(ctx); if (use_iothread) { blk_set_aio_context(blk_src, qemu_get_aio_context(), &error_abort); assert(blk_get_aio_context(blk_target) == qemu_get_aio_context()); diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c index 89e7f0fffb..9d7c8be00f 100644 --- a/tests/unit/test-block-iothread.c +++ b/tests/unit/test-block-iothread.c @@ -455,10 +455,10 @@ static void test_attach_blockjob(void) aio_poll(qemu_get_aio_context(), false); } - aio_context_acquire(ctx); WITH_JOB_LOCK_GUARD() { job_complete_sync_locked(&tjob->common.job, &error_abort); } + aio_context_acquire(ctx); blk_set_aio_context(blk, qemu_get_aio_context(), &error_abort); aio_context_release(ctx); diff --git a/tests/unit/test-blockjob.c b/tests/unit/test-blockjob.c index 17755e58db..535827ffd9 100644 --- a/tests/unit/test-blockjob.c +++ b/tests/unit/test-blockjob.c @@ -228,10 +228,6 @@ static void cancel_common(CancelJob *s) BlockJob *job = &s->common; BlockBackend *blk = s->blk; JobStatus sts = job->job.status; - AioContext *ctx; - - ctx = job->job.aio_context; - aio_context_acquire(ctx); job_cancel_sync(&job->job, true); WITH_JOB_LOCK_GUARD() { @@ -244,7 +240,6 @@ static void cancel_common(CancelJob *s) } destroy_blk(blk); - aio_context_release(ctx); } static void test_cancel_created(void) @@ -405,11 +400,9 @@ static void test_cancel_concluded(void) aio_poll(qemu_get_aio_context(), true); assert(job->status == JOB_STATUS_PENDING); - aio_context_acquire(job->aio_context); WITH_JOB_LOCK_GUARD() { job_finalize_locked(job, &error_abort); } - aio_context_release(job->aio_context); assert(job->status == JOB_STATUS_CONCLUDED); cancel_common(s); @@ -503,13 +496,11 @@ static void test_complete_in_standby(void) /* Wait for the job to become READY */ job_start(job); - aio_context_acquire(ctx); /* * Here we are waiting for the status to change, so don't bother * protecting the read every time. */ - AIO_WAIT_WHILE(ctx, job->status != JOB_STATUS_READY); - aio_context_release(ctx); + AIO_WAIT_WHILE_UNLOCKED(ctx, job->status != JOB_STATUS_READY); /* Begin the drained section, pausing the job */ bdrv_drain_all_begin(); @@ -529,6 +520,7 @@ static void test_complete_in_standby(void) job_complete_locked(job, &error_abort); /* The test is done now, clean up. */ + aio_context_release(ctx); job_finish_sync_locked(job, NULL, &error_abort); assert(job->status == JOB_STATUS_PENDING); @@ -538,6 +530,7 @@ static void test_complete_in_standby(void) job_dismiss_locked(&job, &error_abort); } + aio_context_acquire(ctx); destroy_blk(blk); aio_context_release(ctx); iothread_join(iothread); From patchwork Wed Jun 29 14:15:36 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900171 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 839F7C43334 for ; Wed, 29 Jun 2022 14:28:47 +0000 (UTC) Received: from localhost ([::1]:40114 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6YgQ-0002Ah-B1 for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:28:46 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36612) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YU4-0007xa-T0 for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:03 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:54512) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTv-00024q-VK for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:00 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512151; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=g5dGX64717kVBV41u1440TWGOsgmzecmMm+F9leTYkI=; b=i3gKB7QsdPpqrNx7yEub9DX9aUTxSaEGsOnxmTeFoNJlj8fr1uG76h8/zkZf3IEovPPxdz 8B6Ilx8+HEu9hJmLxd62hqzy9fcfbRjN5y2xwYgAbvFPAWpgvpUigvIemzSjDdg4/gE/xF uZZoNPcpPtklZxOd5AjmLuWMcYJ7b10= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-528-xriAgFNTM22ByCEoMr1Czw-1; Wed, 29 Jun 2022 10:15:49 -0400 X-MC-Unique: xriAgFNTM22ByCEoMr1Czw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 7EC12811E75; Wed, 29 Jun 2022 14:15:49 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2F2B717452; Wed, 29 Jun 2022 14:15:49 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 18/20] block_job_query: remove atomic read Date: Wed, 29 Jun 2022 10:15:36 -0400 Message-Id: <20220629141538.3400679-19-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Not sure what the atomic here was supposed to do, since job.busy is protected by the job lock. Since the whole function is called under job_mutex, just remove the atomic. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- blockjob.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blockjob.c b/blockjob.c index 2293a00b4a..a2559b97a7 100644 --- a/blockjob.c +++ b/blockjob.c @@ -342,7 +342,7 @@ BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp) info = g_new0(BlockJobInfo, 1); info->type = g_strdup(job_type_str(&job->job)); info->device = g_strdup(job->job.id); - info->busy = qatomic_read(&job->job.busy); + info->busy = job->job.busy; info->paused = job->job.pause_count > 0; info->offset = progress_current; info->len = progress_total; From patchwork Wed Jun 29 14:15:37 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900175 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8E791C433EF for ; Wed, 29 Jun 2022 14:32:56 +0000 (UTC) Received: from localhost ([::1]:48838 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6YkR-0000UF-MC for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:32:55 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36626) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YU5-0007xg-Pp for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:03 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:33980) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTw-000259-AS for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:01 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512151; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=IpFOFAodRdV6ZW9euMo79zd87Rm/RdhPVYWqSzG4mu0=; b=NaZA/AhOZiXuSrbbihFGAumeks0gluio0HIOiRTJGjPwV3rL1JFbd+3/pCcWAXYgZVIDrb s9DdUrd/kuervTXrS6H701hZPiZwiZw4FKb50ZuwriNnN/z9BrBP3j8MK3jzhoxIi3rQk6 BCwogoQlHzo01906dIzuvvFuFpsawSo= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-104-PlnoVxnVMoe-IZDtcLJZ4Q-1; Wed, 29 Jun 2022 10:15:50 -0400 X-MC-Unique: PlnoVxnVMoe-IZDtcLJZ4Q-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id D6753811E76; Wed, 29 Jun 2022 14:15:49 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id 86AF99D7F; Wed, 29 Jun 2022 14:15:49 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 19/20] blockjob: remove unused functions Date: Wed, 29 Jun 2022 10:15:37 -0400 Message-Id: <20220629141538.3400679-20-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" These public functions are not used anywhere, thus can be dropped. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- blockjob.c | 30 ------------------------------ include/block/blockjob.h | 33 +++++++++++++-------------------- 2 files changed, 13 insertions(+), 50 deletions(-) diff --git a/blockjob.c b/blockjob.c index a2559b97a7..d84ddca363 100644 --- a/blockjob.c +++ b/blockjob.c @@ -56,12 +56,6 @@ BlockJob *block_job_next_locked(BlockJob *bjob) return job ? container_of(job, BlockJob, job) : NULL; } -BlockJob *block_job_next(BlockJob *bjob) -{ - JOB_LOCK_GUARD(); - return block_job_next_locked(bjob); -} - BlockJob *block_job_get_locked(const char *id) { Job *job = job_get_locked(id); @@ -74,12 +68,6 @@ BlockJob *block_job_get_locked(const char *id) } } -BlockJob *block_job_get(const char *id) -{ - JOB_LOCK_GUARD(); - return block_job_get_locked(id); -} - void block_job_free(Job *job) { BlockJob *bjob = container_of(job, BlockJob, job); @@ -312,12 +300,6 @@ bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp) return true; } -bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) -{ - JOB_LOCK_GUARD(); - return block_job_set_speed_locked(job, speed, errp); -} - int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n) { IO_CODE(); @@ -361,12 +343,6 @@ BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp) return info; } -BlockJobInfo *block_job_query(BlockJob *job, Error **errp) -{ - JOB_LOCK_GUARD(); - return block_job_query_locked(job, errp); -} - static void block_job_iostatus_set_err(BlockJob *job, int error) { if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) { @@ -531,12 +507,6 @@ void block_job_iostatus_reset_locked(BlockJob *job) job->iostatus = BLOCK_DEVICE_IO_STATUS_OK; } -void block_job_iostatus_reset(BlockJob *job) -{ - JOB_LOCK_GUARD(); - block_job_iostatus_reset_locked(job); -} - void block_job_user_resume(Job *job) { BlockJob *bjob = container_of(job, BlockJob, job); diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 3959a98612..753862e1f2 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -82,30 +82,26 @@ typedef struct BlockJob { */ /** - * block_job_next: + * block_job_next_locked: * @job: A block job, or %NULL. * * Get the next element from the list of block jobs after @job, or the * first one if @job is %NULL. * * Returns the requested job, or %NULL if there are no more jobs left. + * Called with job lock held. */ -BlockJob *block_job_next(BlockJob *job); - -/* Same as block_job_next(), but called with job lock held. */ BlockJob *block_job_next_locked(BlockJob *job); /** - * block_job_get: + * block_job_get_locked: * @id: The id of the block job. * * Get the block job identified by @id (which must not be %NULL). * * Returns the requested job, or %NULL if it doesn't exist. + * Called with job lock held. */ -BlockJob *block_job_get(const char *id); - -/* Same as block_job_get(), but called with job lock held. */ BlockJob *block_job_get_locked(const char *id); /** @@ -141,40 +137,37 @@ void block_job_remove_all_bdrv(BlockJob *job); bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs); /** - * block_job_set_speed: + * block_job_set_speed_locked: * @job: The job to set the speed for. * @speed: The new value * @errp: Error object. * * Set a rate-limiting parameter for the job; the actual meaning may * vary depending on the job type. + * + * Called with job lock held. */ -bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); - -/* Same as block_job_set_speed(), but called with job lock held. */ bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp); /** - * block_job_query: + * block_job_query_locked: * @job: The job to get information about. * * Return information about a job. + * + * Called with job lock held. */ -BlockJobInfo *block_job_query(BlockJob *job, Error **errp); - -/* Same as block_job_query(), but called with job lock held. */ BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp); /** - * block_job_iostatus_reset: + * block_job_iostatus_reset_locked: * @job: The job whose I/O status should be reset. * * Reset I/O status on @job and on BlockDriverState objects it uses, * other than job->blk. + * + * Called with job lock held. */ -void block_job_iostatus_reset(BlockJob *job); - -/* Same as block_job_iostatus_reset(), but called with job lock held. */ void block_job_iostatus_reset_locked(BlockJob *job); /* From patchwork Wed Jun 29 14:15:38 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emanuele Giuseppe Esposito X-Patchwork-Id: 12900217 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 28EACC433EF for ; Wed, 29 Jun 2022 14:36:30 +0000 (UTC) Received: from localhost ([::1]:58396 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1o6Ynt-0007at-7W for qemu-devel@archiver.kernel.org; Wed, 29 Jun 2022 10:36:29 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36768) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YUA-000860-S9 for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:06 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:60860) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1o6YTy-00026j-GQ for qemu-devel@nongnu.org; Wed, 29 Jun 2022 10:16:06 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656512153; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5LJtLOeA5IpoZfV5f8K5QpRhGcgY/yrV/pnNN4d2Dvw=; b=hGCQauUNF2s5lQtNenqKhxipqYhRdszOGkJy8vQ0OQObDEtnEuVobSPa21RkCjEa4eitsv SrmqwKOmVHb+zVlWkfPNkpQY1fPFSxArHOOH8ZNNbS3n9Kx33TE4tvJeQyLcJhodgT/Zzg 43U0j4GfzG2DW6X0C8uBHrZKvPD9W/w= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-536-olBxTylzNoSZ0IDAIcF4aw-1; Wed, 29 Jun 2022 10:15:50 -0400 X-MC-Unique: olBxTylzNoSZ0IDAIcF4aw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 39A381019C88; Wed, 29 Jun 2022 14:15:50 +0000 (UTC) Received: from virtlab701.virt.lab.eng.bos.redhat.com (virtlab701.virt.lab.eng.bos.redhat.com [10.19.152.228]) by smtp.corp.redhat.com (Postfix) with ESMTP id DE5AA9D63; Wed, 29 Jun 2022 14:15:49 +0000 (UTC) From: Emanuele Giuseppe Esposito To: qemu-block@nongnu.org Cc: Kevin Wolf , Hanna Reitz , Paolo Bonzini , John Snow , Vladimir Sementsov-Ogievskiy , Wen Congyang , Xie Changlong , Markus Armbruster , Stefan Hajnoczi , Fam Zheng , qemu-devel@nongnu.org, Emanuele Giuseppe Esposito Subject: [PATCH v8 20/20] job: remove unused functions Date: Wed, 29 Jun 2022 10:15:38 -0400 Message-Id: <20220629141538.3400679-21-eesposit@redhat.com> In-Reply-To: <20220629141538.3400679-1-eesposit@redhat.com> References: <20220629141538.3400679-1-eesposit@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=eesposit@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -21 X-Spam_score: -2.2 X-Spam_bar: -- X-Spam_report: (-2.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.082, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" These public functions are not used anywhere, thus can be dropped. Also, since this is the final job API that doesn't use AioContext lock and replaces it with job_lock, adjust all remaining function documentation to clearly specify if the job lock is taken or not. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- include/qemu/job.h | 97 +++++++++++++---------------------- job.c | 122 ++------------------------------------------- 2 files changed, 40 insertions(+), 179 deletions(-) diff --git a/include/qemu/job.h b/include/qemu/job.h index 5db35f765c..f3d0694512 100644 --- a/include/qemu/job.h +++ b/include/qemu/job.h @@ -361,6 +361,8 @@ JobTxn *job_txn_new(void); /** * Release a reference that was previously acquired with job_txn_add_job or * job_txn_new. If it's the last reference to the object, it will be freed. + * + * Called with job lock *not* held. */ void job_txn_unref(JobTxn *txn); @@ -387,19 +389,17 @@ void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, /** * Add a reference to Job refcnt, it will be decreased with job_unref, and then * be freed if it comes to be the last reference. + * + * Called with job lock held. */ -void job_ref(Job *job); - -/* Same as job_ref(), but called with job lock held. */ void job_ref_locked(Job *job); /** * Release a reference that was previously acquired with job_ref() or * job_create(). If it's the last reference to the object, it will be freed. + * + * Called with job lock held. */ -void job_unref(Job *job); - -/* Same as job_unref(), but called with job lock held. */ void job_unref_locked(Job *job); /** @@ -445,10 +445,9 @@ void job_progress_increase_remaining(Job *job, uint64_t delta); * Conditionally enter the job coroutine if the job is ready to run, not * already busy and fn() returns true. fn() is called while under the job_lock * critical section. + * + * Called with job lock held, but might release it temporarily. */ -void job_enter_cond(Job *job, bool(*fn)(Job *job)); - -/* Same as job_enter_cond(), but called with job lock held. */ void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)); /** @@ -529,11 +528,8 @@ bool job_cancel_requested(Job *job); /** * Returns whether the job is in a completed state. - * Called with job_mutex *not* held. + * Called with job lock held. */ -bool job_is_completed(Job *job); - -/* Same as job_is_completed(), but called with job lock held. */ bool job_is_completed_locked(Job *job); /** @@ -549,40 +545,35 @@ bool job_is_ready_locked(Job *job); * Request @job to pause at the next pause point. Must be paired with * job_resume(). If the job is supposed to be resumed by user action, call * job_user_pause() instead. + * + * Called with job lock held. */ -void job_pause(Job *job); - -/* Same as job_pause(), but called with job lock held. */ void job_pause_locked(Job *job); -/** Resumes a @job paused with job_pause. */ -void job_resume(Job *job); - -/* Same as job_resume(), but called with job lock held. */ +/** + * Resumes a @job paused with job_pause. + * Called with job lock held, but might release it temporarily. + */ void job_resume_locked(Job *job); /** * Asynchronously pause the specified @job. * Do not allow a resume until a matching call to job_user_resume. + * Called with job lock held. */ -void job_user_pause(Job *job, Error **errp); - -/* Same as job_user_pause(), but called with job lock held. */ void job_user_pause_locked(Job *job, Error **errp); -/** Returns true if the job is user-paused. */ -bool job_user_paused(Job *job); - -/* Same as job_user_paused(), but called with job lock held. */ +/** + * Returns true if the job is user-paused. + * Called with job lock held. + */ bool job_user_paused_locked(Job *job); /** * Resume the specified @job. * Must be paired with a preceding job_user_pause. + * Called with job lock held. */ -void job_user_resume(Job *job, Error **errp); - -/* Same as job_user_resume(), but called with job lock held. */ void job_user_resume_locked(Job *job, Error **errp); /** @@ -590,30 +581,25 @@ void job_user_resume_locked(Job *job, Error **errp); * first one if @job is %NULL. * * Returns the requested job, or %NULL if there are no more jobs left. + * Called with job lock held. */ -Job *job_next(Job *job); - -/* Same as job_next(), but called with job lock held. */ Job *job_next_locked(Job *job); /** * Get the job identified by @id (which must not be %NULL). * * Returns the requested job, or %NULL if it doesn't exist. + * Called with job lock held. */ -Job *job_get(const char *id); - -/* Same as job_get(), but called with job lock held. */ Job *job_get_locked(const char *id); /** * Check whether the verb @verb can be applied to @job in its current state. * Returns 0 if the verb can be applied; otherwise errp is set and -EPERM * returned. + * + * Called with job lock held. */ -int job_apply_verb(Job *job, JobVerb verb, Error **errp); - -/* Same as job_apply_verb, but called with job lock held. */ int job_apply_verb_locked(Job *job, JobVerb verb, Error **errp); /** @@ -628,28 +614,24 @@ void job_early_fail(Job *job); */ void job_transition_to_ready(Job *job); -/** Asynchronously complete the specified @job. */ -void job_complete(Job *job, Error **errp); - -/* Same as job_complete(), but called with job lock held. */ +/** + * Asynchronously complete the specified @job. + * Called with job lock held, but might release it temporarily. + */ void job_complete_locked(Job *job, Error **errp); /** * Asynchronously cancel the specified @job. If @force is true, the job should * be cancelled immediately without waiting for a consistent state. + * Called with job lock held. */ -void job_cancel(Job *job, bool force); - -/* Same as job_cancel(), but called with job lock held. */ void job_cancel_locked(Job *job, bool force); /** * Cancels the specified job like job_cancel(), but may refuse to do so if the * operation isn't meaningful in the current state of the job. + * Called with job lock held. */ -void job_user_cancel(Job *job, bool force, Error **errp); - -/* Same as job_user_cancel(), but called with job lock held. */ void job_user_cancel_locked(Job *job, bool force, Error **errp); /** @@ -690,9 +672,6 @@ void job_cancel_sync_all(void); * Returns the return value from the job. * Called with job_lock held. */ -int job_complete_sync(Job *job, Error **errp); - -/* Same as job_complete_sync, but called with job lock held. */ int job_complete_sync_locked(Job *job, Error **errp); /** @@ -702,19 +681,17 @@ int job_complete_sync_locked(Job *job, Error **errp); * FIXME: Make the below statement universally true: * For jobs that support the manual workflow mode, all graph changes that occur * as a result will occur after this command and before a successful reply. + * + * Called with job lock held. */ -void job_finalize(Job *job, Error **errp); - -/* Same as job_finalize(), but called with job lock held. */ void job_finalize_locked(Job *job, Error **errp); /** * Remove the concluded @job from the query list and resets the passed pointer * to %NULL. Returns an error if the job is not actually concluded. + * + * Called with job lock held. */ -void job_dismiss(Job **job, Error **errp); - -/* Same as job_dismiss(), but called with job lock held. */ void job_dismiss_locked(Job **job, Error **errp); /** @@ -726,10 +703,6 @@ void job_dismiss_locked(Job **job, Error **errp); * * Called with job_lock held. */ -int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), - Error **errp); - -/* Same as job_finish_sync, but called with job lock held. */ int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp), Error **errp); diff --git a/job.c b/job.c index d0ba463803..8fd915ee68 100644 --- a/job.c +++ b/job.c @@ -233,12 +233,6 @@ int job_apply_verb_locked(Job *job, JobVerb verb, Error **errp) return -EPERM; } -int job_apply_verb(Job *job, JobVerb verb, Error **errp) -{ - JOB_LOCK_GUARD(); - return job_apply_verb_locked(job, verb, errp); -} - JobType job_type(const Job *job) { return job->driver->job_type; @@ -324,7 +318,7 @@ bool job_is_completed_locked(Job *job) return false; } -bool job_is_completed(Job *job) +static bool job_is_completed(Job *job) { JOB_LOCK_GUARD(); return job_is_completed_locked(job); @@ -349,12 +343,6 @@ Job *job_next_locked(Job *job) return QLIST_NEXT(job, job_list); } -Job *job_next(Job *job) -{ - JOB_LOCK_GUARD(); - return job_next_locked(job); -} - Job *job_get_locked(const char *id) { Job *job; @@ -368,12 +356,6 @@ Job *job_get_locked(const char *id) return NULL; } -Job *job_get(const char *id) -{ - JOB_LOCK_GUARD(); - return job_get_locked(id); -} - void job_set_aio_context(Job *job, AioContext *ctx) { /* protect against read in job_finish_sync_locked and job_start */ @@ -465,12 +447,6 @@ void job_ref_locked(Job *job) ++job->refcnt; } -void job_ref(Job *job) -{ - JOB_LOCK_GUARD(); - job_ref_locked(job); -} - void job_unref_locked(Job *job) { GLOBAL_STATE_CODE(); @@ -499,12 +475,6 @@ void job_unref_locked(Job *job) } } -void job_unref(Job *job) -{ - JOB_LOCK_GUARD(); - job_unref_locked(job); -} - void job_progress_update(Job *job, uint64_t done) { progress_work_done(&job->progress, done); @@ -580,12 +550,6 @@ void job_enter_cond_locked(Job *job, bool(*fn)(Job *job)) job_lock(); } -void job_enter_cond(Job *job, bool(*fn)(Job *job)) -{ - JOB_LOCK_GUARD(); - job_enter_cond_locked(job, fn); -} - void job_enter(Job *job) { JOB_LOCK_GUARD(); @@ -631,7 +595,8 @@ static void coroutine_fn job_do_yield_locked(Job *job, uint64_t ns) assert(job->busy); } -void coroutine_fn job_pause_point_locked(Job *job) +/* Called with job lock held */ +static void coroutine_fn job_pause_point_locked(Job *job) { assert(job && job_started(job)); @@ -672,8 +637,9 @@ void coroutine_fn job_pause_point(Job *job) job_pause_point_locked(job); } -void job_yield_locked(Job *job) +void job_yield(Job *job) { + JOB_LOCK_GUARD(); assert(job->busy); /* Check cancellation *before* setting busy = false, too! */ @@ -688,12 +654,6 @@ void job_yield_locked(Job *job) job_pause_point_locked(job); } -void job_yield(Job *job) -{ - JOB_LOCK_GUARD(); - job_yield_locked(job); -} - void coroutine_fn job_sleep_ns(Job *job, int64_t ns) { JOB_LOCK_GUARD(); @@ -725,12 +685,6 @@ void job_pause_locked(Job *job) } } -void job_pause(Job *job) -{ - JOB_LOCK_GUARD(); - job_pause_locked(job); -} - void job_resume_locked(Job *job) { assert(job->pause_count > 0); @@ -743,12 +697,6 @@ void job_resume_locked(Job *job) job_enter_cond_locked(job, job_timer_not_pending_locked); } -void job_resume(Job *job) -{ - JOB_LOCK_GUARD(); - job_resume_locked(job); -} - void job_user_pause_locked(Job *job, Error **errp) { if (job_apply_verb_locked(job, JOB_VERB_PAUSE, errp)) { @@ -762,23 +710,11 @@ void job_user_pause_locked(Job *job, Error **errp) job_pause_locked(job); } -void job_user_pause(Job *job, Error **errp) -{ - JOB_LOCK_GUARD(); - job_user_pause_locked(job, errp); -} - bool job_user_paused_locked(Job *job) { return job->user_paused; } -bool job_user_paused(Job *job) -{ - JOB_LOCK_GUARD(); - return job_user_paused_locked(job); -} - void job_user_resume_locked(Job *job, Error **errp) { assert(job); @@ -799,12 +735,6 @@ void job_user_resume_locked(Job *job, Error **errp) job_resume_locked(job); } -void job_user_resume(Job *job, Error **errp) -{ - JOB_LOCK_GUARD(); - job_user_resume_locked(job, errp); -} - /* Called with job_mutex held. */ static void job_do_dismiss_locked(Job *job) { @@ -832,12 +762,6 @@ void job_dismiss_locked(Job **jobptr, Error **errp) *jobptr = NULL; } -void job_dismiss(Job **jobptr, Error **errp) -{ - JOB_LOCK_GUARD(); - job_dismiss_locked(jobptr, errp); -} - void job_early_fail(Job *job) { JOB_LOCK_GUARD(); @@ -1079,12 +1003,6 @@ void job_finalize_locked(Job *job, Error **errp) job_do_finalize_locked(job); } -void job_finalize(Job *job, Error **errp) -{ - JOB_LOCK_GUARD(); - job_finalize_locked(job, errp); -} - /* Called with job_mutex held. */ static int job_transition_to_pending_locked(Job *job) { @@ -1231,12 +1149,6 @@ void job_cancel_locked(Job *job, bool force) } } -void job_cancel(Job *job, bool force) -{ - JOB_LOCK_GUARD(); - job_cancel_locked(job, force); -} - void job_user_cancel_locked(Job *job, bool force, Error **errp) { if (job_apply_verb_locked(job, JOB_VERB_CANCEL, errp)) { @@ -1245,12 +1157,6 @@ void job_user_cancel_locked(Job *job, bool force, Error **errp) job_cancel_locked(job, force); } -void job_user_cancel(Job *job, bool force, Error **errp) -{ - JOB_LOCK_GUARD(); - job_user_cancel_locked(job, force, errp); -} - /* A wrapper around job_cancel() taking an Error ** parameter so it may be * used with job_finish_sync() without the need for (rather nasty) function * pointer casts there. @@ -1301,12 +1207,6 @@ int job_complete_sync_locked(Job *job, Error **errp) return job_finish_sync_locked(job, job_complete_locked, errp); } -int job_complete_sync(Job *job, Error **errp) -{ - JOB_LOCK_GUARD(); - return job_complete_sync_locked(job, errp); -} - void job_complete_locked(Job *job, Error **errp) { /* Should not be reachable via external interface for internal jobs */ @@ -1326,12 +1226,6 @@ void job_complete_locked(Job *job, Error **errp) job_lock(); } -void job_complete(Job *job, Error **errp) -{ - JOB_LOCK_GUARD(); - job_complete_locked(job, errp); -} - int job_finish_sync_locked(Job *job, void (*finish)(Job *, Error **errp), Error **errp) @@ -1361,9 +1255,3 @@ int job_finish_sync_locked(Job *job, job_unref_locked(job); return ret; } - -int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp) -{ - JOB_LOCK_GUARD(); - return job_finish_sync_locked(job, finish, errp); -} \ No newline at end of file