From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from bombadil.infradead.org ([198.137.202.133]:34036 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730777AbeK1F56 (ORCPT ); Wed, 28 Nov 2018 00:57:58 -0500 Date: Tue, 27 Nov 2018 10:59:06 -0800 From: Matthew Wilcox To: Dan Williams Cc: Jan Kara , linux-fsdevel , linux-nvdimm , Dave Jiang Subject: Re: dax_lock_mapping_entry was never safe Message-ID: <20181127185906.GE10377@bombadil.infradead.org> References: <20181126161240.GH3065@bombadil.infradead.org> <20181126171137.GD25835@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, Nov 26, 2018 at 12:36:26PM -0800, Dan Williams wrote: > On Mon, Nov 26, 2018 at 9:11 AM Jan Kara wrote: > > The code looks good. Maybe can we call this wait_entry_unlocked() to stress > > that entry is not really usable after this function returns? And comment > > before the function that this is safe to call even if we don't have a > > reference keeping mapping alive? > > Yes, maybe even something more ambiguous like "wait_entry_event()", > because there's no guarantee the entry is unlocked just that now is a > good time to try to interrogate the entry again. It _became_ unlocked ... it might be locked again, or have disappeared by the time we get to it, but somebody called dax_wake_entry() for this exact entry. I mean, we could call it wait_entry_wake(), but I think that's a little generic. I'm going with Jan's version ;-) > > The continue here actually is not safe either because if the mapping got > > freed, page->mapping will be NULL and we oops at the beginning of the loop. > > So that !dax_mapping() check should also check for mapping != NULL. > > Yes. Sigh. I'll make that a separate patch so it applies cleanly to 4.19.