From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from bombadil.infradead.org (bombadil.infradead.org [IPv6:2607:7c80:54:e::133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1EC5B2116527B for ; Tue, 4 Dec 2018 17:34:45 -0800 (PST) Date: Tue, 4 Dec 2018 17:34:44 -0800 From: Matthew Wilcox Subject: Re: [PATCH] dax: Fix Xarray conversion of dax_unlock_mapping_entry() Message-ID: <20181205013444.GX10377@bombadil.infradead.org> References: <154353682674.1676897.15440708268545845062.stgit@dwillia2-desk3.amr.corp.intel.com> <20181130154902.GL10377@bombadil.infradead.org> <20181130162435.GM10377@bombadil.infradead.org> <20181130195021.GN10377@bombadil.infradead.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: 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: Dan Williams Cc: linux-fsdevel , Jan Kara , Linux Kernel Mailing List , linux-nvdimm List-ID: On Mon, Dec 03, 2018 at 07:33:43PM -0800, Dan Williams wrote: > On Fri, Nov 30, 2018 at 12:05 PM Dan Williams wrote: > > > -void dax_unlock_mapping_entry(struct page *page) > > > +void dax_unlock_mapping_entry(struct page *page, dax_entry_t entry) > > > > Let's not require the page to be passed back, it can be derived: > > > > page = pfn_to_page(dax_to_pfn((void*) entry)); > > > > A bit more symmetric that way and canonical with other locking schemes > > that return a cookie. > > The patch does not apply on top of the pending fixes. could resend on > top of the current libnvdimm-fixes branch [1]? > > I think because we are changing the calling convention to take return > a locked dax_entry_t type, I feel like we should go back to the > originally proposed names of these interfaces. I.e. > > dax_entry_t dax_lock_page(struct page *page) > > void dax_unlock_page(dax_entry_t entry) > > Yes, it introduces an entry-to-pfn and pfn-to-page conversion. > However, If I can't convince you to drop the page argument, lets at > least do the name change. I.e. offload the responsibility of conveying > that this is not the traditional page lock to the fact that a > dax_entry is returned and passed back to the unlock routine. From: Matthew Wilcox Date: Fri, 30 Nov 2018 11:05:06 -0500 Subject: [PATCH] dax: Change lock/unlock API Return the unlock cookie from dax_lock_page() and pass it to dax_unlock_page(). This fixes a bug where dax_unlock_page() was assuming that the page was PMD-aligned if the entry was a PMD entry. Debugged-by: Dan Williams Fixes: 9f32d221301c ("dax: Convert dax_lock_mapping_entry to XArray") Signed-off-by: Matthew Wilcox --- diff --git a/fs/dax.c b/fs/dax.c index 3f592dc18d67..48132eca3761 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -379,20 +379,20 @@ static struct page *dax_busy_page(void *entry) * @page: The page whose entry we want to lock * * Context: Process context. - * Return: %true if the entry was locked or does not need to be locked. + * Return: A cookie to pass to dax_unlock_page() or 0 if the entry could + * not be locked. */ -bool dax_lock_mapping_entry(struct page *page) +dax_entry_t dax_lock_page(struct page *page) { XA_STATE(xas, NULL, 0); void *entry; - bool locked; /* Ensure page->mapping isn't freed while we look at it */ rcu_read_lock(); for (;;) { struct address_space *mapping = READ_ONCE(page->mapping); - locked = false; + entry = NULL; if (!mapping || !dax_mapping(mapping)) break; @@ -403,7 +403,7 @@ bool dax_lock_mapping_entry(struct page *page) * otherwise we would not have a valid pfn_to_page() * translation. */ - locked = true; + entry = (void *)~0UL; if (S_ISCHR(mapping->host->i_mode)) break; @@ -426,23 +426,18 @@ bool dax_lock_mapping_entry(struct page *page) break; } rcu_read_unlock(); - return locked; + return (dax_entry_t)entry; } -void dax_unlock_mapping_entry(struct page *page) +void dax_unlock_page(struct page *page, dax_entry_t cookie) { struct address_space *mapping = page->mapping; XA_STATE(xas, &mapping->i_pages, page->index); - void *entry; if (S_ISCHR(mapping->host->i_mode)) return; - rcu_read_lock(); - entry = xas_load(&xas); - rcu_read_unlock(); - entry = dax_make_entry(page_to_pfn_t(page), dax_is_pmd_entry(entry)); - dax_unlock_entry(&xas, entry); + dax_unlock_entry(&xas, (void *)cookie); } /* diff --git a/include/linux/dax.h b/include/linux/dax.h index 450b28db9533..0dd316a74a29 100644 --- a/include/linux/dax.h +++ b/include/linux/dax.h @@ -7,6 +7,8 @@ #include #include +typedef unsigned long dax_entry_t; + struct iomap_ops; struct dax_device; struct dax_operations { @@ -88,8 +90,8 @@ int dax_writeback_mapping_range(struct address_space *mapping, struct block_device *bdev, struct writeback_control *wbc); struct page *dax_layout_busy_page(struct address_space *mapping); -bool dax_lock_mapping_entry(struct page *page); -void dax_unlock_mapping_entry(struct page *page); +dax_entry_t dax_lock_page(struct page *page); +void dax_unlock_page(struct page *page, dax_entry_t cookie); #else static inline bool bdev_dax_supported(struct block_device *bdev, int blocksize) @@ -122,14 +124,14 @@ static inline int dax_writeback_mapping_range(struct address_space *mapping, return -EOPNOTSUPP; } -static inline bool dax_lock_mapping_entry(struct page *page) +static inline dax_entry_t dax_lock_page(struct page *page) { if (IS_DAX(page->mapping->host)) - return true; - return false; + return ~0UL; + return 0; } -static inline void dax_unlock_mapping_entry(struct page *page) +static inline void dax_unlock_page(struct page *page, dax_entry_t cookie) { } #endif diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 0cd3de3550f0..7c72f2a95785 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -1161,6 +1161,7 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags, LIST_HEAD(tokill); int rc = -EBUSY; loff_t start; + dax_entry_t cookie; /* * Prevent the inode from being freed while we are interrogating @@ -1169,7 +1170,8 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags, * also prevents changes to the mapping of this pfn until * poison signaling is complete. */ - if (!dax_lock_mapping_entry(page)) + cookie = dax_lock_page(page); + if (!cookie) goto out; if (hwpoison_filter(page)) { @@ -1220,7 +1222,7 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags, kill_procs(&tokill, flags & MF_MUST_KILL, !unmap_success, pfn, flags); rc = 0; unlock: - dax_unlock_mapping_entry(page); + dax_unlock_page(page, cookie); out: /* drop pgmap ref acquired in caller */ put_dev_pagemap(pgmap); _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm