From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-x241.google.com (mail-oi0-x241.google.com [IPv6:2607:f8b0:4003:c06::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 930F520337376 for ; Wed, 4 Jul 2018 08:17:39 -0700 (PDT) Received: by mail-oi0-x241.google.com with SMTP id k81-v6so11351573oib.4 for ; Wed, 04 Jul 2018 08:17:39 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20180611154146.jc5xt4gyaihq64lm@quack2.suse.cz> References: <152850182079.38390.8280340535691965744.stgit@dwillia2-desk3.amr.corp.intel.com> <152850187437.38390.2257981090761438811.stgit@dwillia2-desk3.amr.corp.intel.com> <20180611154146.jc5xt4gyaihq64lm@quack2.suse.cz> From: Dan Williams Date: Wed, 4 Jul 2018 08:17:38 -0700 Message-ID: Subject: Re: [PATCH v4 10/12] filesystem-dax: Introduce dax_lock_page() List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: linux-nvdimm-bounces@lists.01.org Sender: "Linux-nvdimm" To: Jan Kara Cc: linux-fsdevel , Linux MM , Christoph Hellwig , linux-nvdimm List-ID: On Mon, Jun 11, 2018 at 8:41 AM, Jan Kara wrote: > On Fri 08-06-18 16:51:14, Dan Williams wrote: >> In preparation for implementing support for memory poison (media error) >> handling via dax mappings, implement a lock_page() equivalent. Poison >> error handling requires rmap and needs guarantees that the page->mapping >> association is maintained / valid (inode not freed) for the duration of >> the lookup. >> >> In the device-dax case it is sufficient to simply hold a dev_pagemap >> reference. In the filesystem-dax case we need to use the entry lock. >> >> Export the entry lock via dax_lock_page() that uses rcu_read_lock() to >> protect against the inode being freed, and revalidates the page->mapping >> association under xa_lock(). >> >> Signed-off-by: Dan Williams > > Some comments below... > >> diff --git a/fs/dax.c b/fs/dax.c >> index cccf6cad1a7a..b7e71b108fcf 100644 >> --- a/fs/dax.c >> +++ b/fs/dax.c >> @@ -361,6 +361,82 @@ static void dax_disassociate_entry(void *entry, struct address_space *mapping, >> } >> } >> >> +struct page *dax_lock_page(unsigned long pfn) >> +{ > > Why do you return struct page here? Any reason behind that? Unlike lock_page() there is no guarantee that we can lock a mapping entry given a pfn. There is a chance that we lose a race and can't validate the pfn to take the lock. So returning 'struct page *' was there to indicate that we successfully validated the pfn and were able to take the lock. I'll rework it to just return bool. > Because struct > page exists and can be accessed through pfn_to_page() regardless of result > of this function so it looks a bit confusing. Also dax_lock_page() name > seems a bit confusing. Maybe dax_lock_pfn_mapping_entry()? Ok. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm