From patchwork Wed May 8 10:28:07 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ross Lagerwall X-Patchwork-Id: 10934913 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 4C73692A for ; Wed, 8 May 2019 10:29:52 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3BF1C1FFC8 for ; Wed, 8 May 2019 10:29:52 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2F81427CAF; Wed, 8 May 2019 10:29:52 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 0887F1FFC8 for ; Wed, 8 May 2019 10:29:51 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1hOJoG-0000Xx-6S; Wed, 08 May 2019 10:28:24 +0000 Received: from us1-rack-dfw2.inumbo.com ([104.130.134.6]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1hOJoF-0000Xs-Je for xen-devel@lists.xenproject.org; Wed, 08 May 2019 10:28:23 +0000 X-Inumbo-ID: 00a5f9b8-717c-11e9-843c-bc764e045a96 Received: from SMTP03.CITRIX.COM (unknown [162.221.156.55]) by us1-rack-dfw2.inumbo.com (Halon) with ESMTPS id 00a5f9b8-717c-11e9-843c-bc764e045a96; Wed, 08 May 2019 10:28:21 +0000 (UTC) X-IronPort-AV: E=Sophos;i="5.60,445,1549929600"; d="scan'208";a="85259235" From: Ross Lagerwall To: Boris Ostrovsky , Juergen Gross Date: Wed, 8 May 2019 11:28:07 +0100 Message-ID: <20190508102807.7096-1-ross.lagerwall@citrix.com> X-Mailer: git-send-email 2.17.2 MIME-Version: 1.0 Subject: [Xen-devel] [PATCH] xenbus: Avoid deadlock during suspend due to open transactions X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: xen-devel@lists.xenproject.org, Ross Lagerwall Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP During a suspend/resume, the xenwatch thread waits for all outstanding xenstore requests and transactions to complete. This does not work correctly for transactions started by userspace because it waits for them to complete after freezing userspace threads which means the transactions has no way of completing, resulting in a deadlock. This is trivial to reproduce by running this script and then suspending the VM: import pyxs, time c = pyxs.client.Client(xen_bus_path="/dev/xen/xenbus") c.connect() c.transaction() time.sleep(3600) Even if this deadlock were resolved, misbehaving userspace should not prevent a VM from being migrated. So, instead of waiting for these transactions to complete, ignore them during suspend and mark them as aborted during the return path. If the caller commits the transaction, return EAGAIN so that they try again. If the caller discards the transaction, return OK since no changes were made anyway. This only affects users of the xenbus file interface. In-kernel users of xenbus are assumed to be well-behaved and complete all transactions before freezing. Signed-off-by: Ross Lagerwall --- drivers/xen/xenbus/xenbus.h | 2 + drivers/xen/xenbus/xenbus_dev_frontend.c | 60 ++++++++++++++++++++++++ drivers/xen/xenbus/xenbus_xs.c | 16 ++++++- 3 files changed, 77 insertions(+), 1 deletion(-) diff --git a/drivers/xen/xenbus/xenbus.h b/drivers/xen/xenbus/xenbus.h index 092981171df1..a977e1396149 100644 --- a/drivers/xen/xenbus/xenbus.h +++ b/drivers/xen/xenbus/xenbus.h @@ -133,4 +133,6 @@ void xenbus_ring_ops_init(void); int xenbus_dev_request_and_reply(struct xsd_sockmsg *msg, void *par); void xenbus_dev_queue_reply(struct xb_req_data *req); +unsigned int xenbus_file_abort_trans(bool abort); + #endif diff --git a/drivers/xen/xenbus/xenbus_dev_frontend.c b/drivers/xen/xenbus/xenbus_dev_frontend.c index 0782ff3c2273..623218a2a165 100644 --- a/drivers/xen/xenbus/xenbus_dev_frontend.c +++ b/drivers/xen/xenbus/xenbus_dev_frontend.c @@ -69,6 +69,7 @@ struct xenbus_transaction_holder { struct list_head list; struct xenbus_transaction handle; + bool aborted; }; /* @@ -113,8 +114,49 @@ struct xenbus_file_priv { wait_queue_head_t read_waitq; struct kref kref; + + struct list_head file_list; }; +static DEFINE_SPINLOCK(file_list_lock); +static LIST_HEAD(file_list); + +static void register_xenbus_file(struct xenbus_file_priv *u) +{ + spin_lock(&file_list_lock); + list_add(&u->file_list, &file_list); + spin_unlock(&file_list_lock); +} + +static void unregister_xenbus_file(struct xenbus_file_priv *u) +{ + spin_lock(&file_list_lock); + list_del(&u->file_list); + spin_unlock(&file_list_lock); +} + +unsigned int xenbus_file_abort_trans(bool abort) +{ + struct xenbus_file_priv *u; + struct xenbus_transaction_holder *trans; + unsigned int count = 0; + + spin_lock(&file_list_lock); + list_for_each_entry(u, &file_list, file_list) { + mutex_lock(&u->msgbuffer_mutex); + list_for_each_entry(trans, &u->transactions, list) { + if (!trans->aborted) { + count++; + trans->aborted = abort; + } + } + mutex_unlock(&u->msgbuffer_mutex); + } + spin_unlock(&file_list_lock); + + return count; +} + /* Read out any raw xenbus messages queued up. */ static ssize_t xenbus_file_read(struct file *filp, char __user *ubuf, @@ -306,6 +348,8 @@ static void xenbus_file_free(struct kref *kref) u = container_of(kref, struct xenbus_file_priv, kref); + unregister_xenbus_file(u); + /* * No need for locking here because there are no other users, * by definition. @@ -449,6 +493,20 @@ static int xenbus_write_transaction(unsigned msg_type, !(msg->hdr.len == 2 && (!strcmp(msg->body, "T") || !strcmp(msg->body, "F")))) return xenbus_command_reply(u, XS_ERROR, "EINVAL"); + else if (msg_type == XS_TRANSACTION_END) { + trans = xenbus_get_transaction(u, msg->hdr.tx_id); + if (trans && trans->aborted) { + list_del(&trans->list); + kfree(trans); + if (!strcmp(msg->body, "T")) + return xenbus_command_reply(u, XS_ERROR, + "EAGAIN"); + else + return xenbus_command_reply(u, + XS_TRANSACTION_END, + "OK"); + } + } rc = xenbus_dev_request_and_reply(&msg->hdr, u); if (rc && trans) { @@ -640,6 +698,8 @@ static int xenbus_file_open(struct inode *inode, struct file *filp) filp->private_data = u; + register_xenbus_file(u); + return 0; } diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c index 49a3874ae6bb..9abff635fc20 100644 --- a/drivers/xen/xenbus/xenbus_xs.c +++ b/drivers/xen/xenbus/xenbus_xs.c @@ -95,12 +95,23 @@ static pid_t xenwatch_pid; static DEFINE_MUTEX(xenwatch_mutex); static DECLARE_WAIT_QUEUE_HEAD(watch_events_waitq); +static unsigned int xs_state_count_users(void) +{ + unsigned int count; + + spin_lock(&xs_state_lock); + count = xs_state_users - xenbus_file_abort_trans(false); + spin_unlock(&xs_state_lock); + + return count; +} + static void xs_suspend_enter(void) { spin_lock(&xs_state_lock); xs_suspend_active++; spin_unlock(&xs_state_lock); - wait_event(xs_state_exit_wq, xs_state_users == 0); + wait_event(xs_state_exit_wq, xs_state_count_users() == 0); } static void xs_suspend_exit(void) @@ -838,6 +849,9 @@ void xs_resume(void) mutex_unlock(&xs_response_mutex); + spin_lock(&xs_state_lock); + xs_state_users -= xenbus_file_abort_trans(true); + spin_unlock(&xs_state_lock); xs_suspend_exit(); /* No need for watches_lock: the xs_watch_rwsem is sufficient. */