Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753502AbaBDIcL (ORCPT ); Tue, 4 Feb 2014 03:32:11 -0500 Received: from nat28.tlf.novell.com ([130.57.49.28]:43381 "EHLO nat28.tlf.novell.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753063AbaBDIb7 (ORCPT ); Tue, 4 Feb 2014 03:31:59 -0500 Message-Id: <52F0B38A0200007800118E62@nat28.tlf.novell.com> X-Mailer: Novell GroupWise Internet Agent 12.0.2 Date: Tue, 04 Feb 2014 08:31:54 +0000 From: "Jan Beulich" To: "=?UTF-8?Q?Roger=20Pau=20Monn=C3=A9?=" 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> <52F0A1C7.2010607@citrix.com> In-Reply-To: <52F0A1C7.2010607@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org >>> On 04.02.14 at 09:16, Roger Pau Monné wrote: > 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); Actually, I got the description wrong. I really meant free_req(); if (atomic_read ...) complete(); xen_blkif_put(); Jan -- 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/