From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752399AbcGYMZz (ORCPT ); Mon, 25 Jul 2016 08:25:55 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:32788 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752188AbcGYMZs (ORCPT ); Mon, 25 Jul 2016 08:25:48 -0400 Message-ID: <57960530.7090608@oracle.com> Date: Mon, 25 Jul 2016 20:25:20 +0800 From: Bob Liu User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130308 Thunderbird/17.0.4 MIME-Version: 1.0 To: =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= CC: linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org, konrad.wilk@oracle.com, jgross@suse.com Subject: Re: [PATCH 3/3] xen-blkfront: dynamic configuration of per-vbd resources References: <20160722074506.l5nfcmqg3jzsmxzi@mac> <5791D6AC.1070604@oracle.com> <20160722093409.iwcmlubhou4rjjop@mac> <5791EAC4.2030309@oracle.com> <20160722114538.7un36zw7mrvcueob@mac> <57929BAF.4030304@oracle.com> <20160725092002.madb7j6ryn4jcoho@mac> <5795EA02.2090200@oracle.com> <20160725105314.aatqpi4rxaxntt5b@mac> <5795F334.4040106@oracle.com> <20160725121121.6ym4wjjgqyp7akgx@mac> In-Reply-To: <20160725121121.6ym4wjjgqyp7akgx@mac> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8bit X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/25/2016 08:11 PM, Roger Pau Monné wrote: > On Mon, Jul 25, 2016 at 07:08:36PM +0800, Bob Liu wrote: >> >> On 07/25/2016 06:53 PM, Roger Pau Monné wrote: >> ..[snip..] >>>>>> * We get the device lock and blk_mq_freeze_queue() in dynamic_reconfig_device(), >>>>>> and then have to release in blkif_recover() asynchronously which makes the code more difficult to readable. >>>>> >>>>> I'm not sure I'm following here, do you mean that you will pick the lock in >>>>> dynamic_reconfig_device and release it in blkif_recover? Why wouldn't you >>>> >>>> Yes. >>>> >>>>> release the lock in dynamic_reconfig_device after doing whatever is needed? >>>>> >>>> >>>> Both 'dynamic configuration' and migration:xenbus_dev_resume() use { blkfront_resume(); blkif_recover() } and depends on the change of xbdev->state. >>>> If they happen simultaneously, the State machine of xbdev->state is going to be a mess and very difficult to track. >>>> >>>> The lock(actually a mutex) is like a big lock to make sure no race would happen at all. >>> >>> So the only thing that you should do is set the frontend state to closed and >>> wait for the backend to also switch to state closed, and then switch the >>> frontend state to init again in order to trigger a reconnection. >>> >> >> But if migration:xenbus_dev_resume() is triggered at the same time, any state be set might be changed. >> ===== >> E.g >> Dynamic_reconfig_device: Migration:xenbus_dev_resume() >> >> Set the frontend state to closed >> >> ^^^^ >> frontend state will be reset to XenbusStateInitialising by xenbus_dev_resume() >> >> Wait for the backend to also switch to state closed > > This is not really a race, the backend will not switch to state closed, and > instead will appear at state init again, which is what we wanted anyway in > order to reconnect, so I don't see an issue with this flow. > >> ===== >> Similar situation may happen at any other place regarding set the state. > > Right, but I don't see how your proposed patch also avoids this issues. I > see that you pick the device lock in dynamic_reconfig_device, but I don't > see xenbus_dev_resume picking the lock at all, and in any case I don't think The lock is picked from the power management framework. Anyway, I'm convinced and will follow your suggestion. Thank you! > you should prevent the VM from migrating. > > Depending on the toolstack it might decide to kill the VM because it has not > been able to migrate it, in which case the result is not better. >