Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753409AbaBDIQU (ORCPT ); Tue, 4 Feb 2014 03:16:20 -0500 Received: from smtp.citrix.com ([66.165.176.89]:56269 "EHLO SMTP.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752042AbaBDIQO (ORCPT ); Tue, 4 Feb 2014 03:16:14 -0500 X-IronPort-AV: E=Sophos;i="4.95,778,1384300800"; d="scan'208";a="99546463" Message-ID: <52F0A1C7.2010607@citrix.com> Date: Tue, 4 Feb 2014 09:16:07 +0100 From: =?UTF-8?B?Um9nZXIgUGF1IE1vbm7DqQ==?= User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Jan Beulich CC: Matt Rushton , Matt Wilson , DavidVrabel , Ian Campbell , , Boris Ostrovsky , Subject: Re: [Xen-devel] [PATCH 3/3] xen-blkback: fix shutdown race References: <1390931015-5490-1-git-send-email-roger.pau@citrix.com> <1390931015-5490-4-git-send-email-roger.pau@citrix.com> <52E8CF540200007800117D88@nat28.tlf.novell.com> <52EFCACF.6080604@citrix.com> <52F0AC990200007800118E09@nat28.tlf.novell.com> In-Reply-To: <52F0AC990200007800118E09@nat28.tlf.novell.com> X-Enigmail-Version: 1.6 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-DLP: MIA2 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/02/14 09:02, Jan Beulich wrote: >>>> On 03.02.14 at 17:58, Roger Pau Monné wrote: >> On 29/01/14 09:52, Jan Beulich wrote: >>>>>> On 28.01.14 at 18:43, Roger Pau Monne wrote: >>>> + free_req(blkif, pending_req); >>>> + /* >>>> + * Make sure the request is freed before releasing blkif, >>>> + * or there could be a race between free_req and the >>>> + * cleanup done in xen_blkif_free during shutdown. >>>> + * >>>> + * NB: The fact that we might try to wake up pending_free_wq >>>> + * before drain_complete (in case there's a drain going on) >>>> + * it's not a problem with our current implementation >>>> + * because we can assure there's no thread waiting on >>>> + * pending_free_wq if there's a drain going on, but it has >>>> + * to be taken into account if the current model is changed. >>>> + */ >>>> + xen_blkif_put(blkif); >>>> + if (atomic_read(&blkif->refcnt) <= 2) { >>>> + if (atomic_read(&blkif->drain)) >>>> + complete(&blkif->drain_complete); >>>> } >>>> - free_req(pending_req->blkif, pending_req); >>>> } >>>> } >>> >>> The put is still too early imo - you're explicitly accessing field in the >>> structure immediately afterwards. This may not be an issue at >>> present, but I think it's at least a latent one. >>> >>> Apart from that, the two if()s would - at least to me - be more >>> clear if combined into one. >> >> In order to get rid of the race I had to introduce yet another atomic_t >> in xen_blkif struct, which is something I don't really like, but I >> could not see any other way to solve this. If that's fine I will resend >> the series, here is the reworked patch: > > Mind explaining why you can't simply move the xen_blkif_put() > down between the if() and the free_ref(). You mean doing something like: if (atomic_read(&blkif->refcnt) <= 3) { if (atomic_read(&blkif->drain)) complete(&blkif->drain_complete); } xen_blkif_put(blkif); free_req(blkif, pending_req); This would not be safe, because we are comparing refcnt before decrementing it, and also we are not doing it atomically (the dec and compare). If for example we have two inflight requests, both could perform the atomic_read of refcnt, see there's still another inflight request, and then both decrement, without anyone calling complete. There's also the issue that with this approach as we are freeing the request after we have put blkif, which is a race with the cleanup being done in xen_blkif_free. Roger. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/