From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4BFF8E7719C for ; Fri, 10 Jan 2025 16:44:43 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D6F966B0093; Fri, 10 Jan 2025 11:44:42 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D1FB66B00C1; Fri, 10 Jan 2025 11:44:42 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C0EB26B00C2; Fri, 10 Jan 2025 11:44:42 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 683AC6B0093 for ; Fri, 10 Jan 2025 11:44:42 -0500 (EST) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id DBCB943DDA for ; Fri, 10 Jan 2025 16:44:41 +0000 (UTC) X-FDA: 82992115962.15.A30A737 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf12.hostedemail.com (Postfix) with ESMTP id 3086F40003 for ; Fri, 10 Jan 2025 16:44:39 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=O5UR+WQT; spf=pass (imf12.hostedemail.com: domain of djwong@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=djwong@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1736527480; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=eDYFOGueTkvy+TGXt7yHjBF4hDSaK2IB0+UVMd/GAfc=; b=2/s3VZdUT0OyI++DM0VATSzuyNLCaTwxccChJO8ERNKni4YEC7BRahQ9B/m3Q1oDSK+GL0 t2r740pJjKqdjVvN/G8zYAVe58bswMtG3XuiwhThmKuDTD38445oahhoksgwPteGsoIroE 0KIXvOEQTpjXXyF+H1Zq+NRrZZALVZs= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736527480; a=rsa-sha256; cv=none; b=aUBVrPuVBMee5Qug7Bm36Lp00uttB1XXZyGyOD2f00ZyOymu0PPfqA8+4nqiFfcQmftGhh ZJ2MVkb9hs2zEAC5xyhkF50cS2IpzNFRf3GDfgLJY3fSQMwqf7s5oKfzTFPI5AFoZKXYQo JKmD/t5KDXRm8wHvj0CplrCtbJxyydA= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=O5UR+WQT; spf=pass (imf12.hostedemail.com: domain of djwong@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=djwong@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 15F5B5C4363; Fri, 10 Jan 2025 16:43:58 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id ADD1CC4CED6; Fri, 10 Jan 2025 16:44:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1736527478; bh=C4CeHm6Or5VLGEzPlFMQwU1Lqi8VzWhjclCv/3py67E=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=O5UR+WQTuOopW+m3Pi1zmwj3sDf93V+XjOFJYBM1RzPO3sDZ1K+ormfip92T4e79w U7XfSgY+NLYzXu3FSqu6NPO6Duv551jmp1bCI/QrpFZEE2pZI1sr9JEOmCb6BY1OnU ygd6hC3MnYQ2N+RhjQSfldq11cHroGlaZKiItHVNmIb3DkLT0NVgh6huf2W9+DLo9X YeINgKpATzcjR+uCpeX3HIxD3GxiQDjsZM7pKUDW6Vf7Go2Sgai6ua3WPKo0H4Ox5B AqV/p/rvC1rT0uQQGOw0QzXTSq+PszsSgUN9oJlsXQxouGCrMbf3PP8PksxH6qiYUq YoNvaeUovuyGw== Date: Fri, 10 Jan 2025 08:44:38 -0800 From: "Darrick J. Wong" To: Alistair Popple Cc: akpm@linux-foundation.org, dan.j.williams@intel.com, linux-mm@kvack.org, alison.schofield@intel.com, lina@asahilina.net, zhang.lyra@gmail.com, gerald.schaefer@linux.ibm.com, vishal.l.verma@intel.com, dave.jiang@intel.com, logang@deltatee.com, bhelgaas@google.com, jack@suse.cz, jgg@ziepe.ca, catalin.marinas@arm.com, will@kernel.org, mpe@ellerman.id.au, npiggin@gmail.com, dave.hansen@linux.intel.com, ira.weiny@intel.com, willy@infradead.org, tytso@mit.edu, linmiaohe@huawei.com, david@redhat.com, peterx@redhat.com, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, nvdimm@lists.linux.dev, linux-cxl@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org, jhubbard@nvidia.com, hch@lst.de, david@fromorbit.com, chenhuacai@kernel.org, kernel@xen0n.name, loongarch@lists.linux.dev Subject: Re: [PATCH v6 05/26] fs/dax: Create a common implementation to break DAX layouts Message-ID: <20250110164438.GJ6156@frogsfrogsfrogs> References: <79936ac15c917f4004397027f648d4fc9c092424.1736488799.git-series.apopple@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <79936ac15c917f4004397027f648d4fc9c092424.1736488799.git-series.apopple@nvidia.com> X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 3086F40003 X-Stat-Signature: u7ge6juh97h5po71c8w4rg1deiwey4ob X-Rspam-User: X-HE-Tag: 1736527479-882734 X-HE-Meta: U2FsdGVkX1+sBEwFyRZwJve/5nnpqG/b03bKMJLJUXWmQuvb6KVK/BnbzsRYEHU+zrfQPm6u5y0my69OUvv2gMjmaIOStPLZVkXSDJ5ldEzShF2MNomVJ5e4c7LEdCbbGn4VHSZePK96rumdqZXCiLMTtLSAtaO8aQHaqYmOcBMwrlnCQqXym5oXtf89T5yn/EOZfdUjzoK6ghsJNLrTDX/oAzj3nHILiY9T3C6oDlzXlbwCQ9dEgN1/feMoUMXO2IEUwwnl/NLs4R9UWjN3J4W/HH49F1Zvy0+Zdb7c6q8SgCw8QJVKm86fklZjylbvd9Yh9IN0d5AgBCP46h+hL35dR3qSQKbMTqpJrkNyp+SdpJzG2CWzojoTDVi0t+oPYAzOubt50qwnoFlUrXEMuy5lHOyuZXD7SvXAxS0cLWAhWMEdO4m4Orv+OigyNYT/XuW5/Jv7pJTF9O5I9vcQEHdXCfCGKmll38v/ckbUB8nDrSFIVlk9NuD/3hJYmZwm7LP9KR6nTmGD0E0JzAwuPSuOqR9zMzGK2oLs9N2rCycidiOFJ7rMSBK4zb5yND2JecZTi7ywkoY+0NY6Q880m9+8L21dkkREoyEhQJJlcM9yCIphyJUPZol35/cUNSatnCWYwT0aGtYwPF8MObOWehlR4G2roi3XIATfaCElOAa1Av5r7KHwATu1le1i2Exz5nE+IB/Rb+DpyWa1HtVrb6bpLYvwwhjprjwWRfUfFRr+5sicU84zx6l0z6oFOVtVMIW2yRbItinaKTbqEOOT8ygx3xQlD9V2qqYyStUYSoLGgbDz0E53daz1Ndux/McvZScGOssuSFLcXQekWdZzdxcv13QcHia0hxb0ZI2ZFckrjie3/bvdR709XlYKSkhP2LMwkMqju9V8jM5hyShKJeKoOGH7Pp6bAl6vXJlZLngsuOi5Mq9mowx1tPQrzD28hucdHRdbncEit+piGvc wZ7JLRxQ z+QFGV8My69HDkjiZ7mN/cofqvzmVp3UCfK/eF9LpsxTUsJ6xaxuXkhzB86QOhcirKkoHne7reWPQ3EqmpzOF6NkYgQVsB3tgksvk2Tjnz4yMp8eP0petpogNhzNSiOUe56L0rbPIyS13SN/GhoHNUqmmlc7JEoKv2fa/p59jsojcqM2Ri8Nn8lp8yCy+oYcXLVr5DL+28Zmt3vDayk0SnndvJbuEcq9hTZoOp9KWbqTIMUJWUT12eXpK067PdOuMmDmhqI4jUkrnC/CV4SocVctPQ6c4pChRW3WX9WgtbQpFnIAs0Pe6pXwdgFPpo/GpI3PWEszooMpECh+r2H2Gzl5zWDFo/5YCYkRV X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Fri, Jan 10, 2025 at 05:00:33PM +1100, Alistair Popple wrote: > Prior to freeing a block file systems supporting FS DAX must check > that the associated pages are both unmapped from user-space and not > undergoing DMA or other access from eg. get_user_pages(). This is > achieved by unmapping the file range and scanning the FS DAX > page-cache to see if any pages within the mapping have an elevated > refcount. > > This is done using two functions - dax_layout_busy_page_range() which > returns a page to wait for the refcount to become idle on. Rather than > open-code this introduce a common implementation to both unmap and > wait for the page to become idle. > > Signed-off-by: Alistair Popple So now that Dan Carpenter has complained, I guess I should look at this... > --- > > Changes for v5: > > - Don't wait for idle pages on non-DAX mappings > > Changes for v4: > > - Fixed some build breakage due to missing symbol exports reported by > John Hubbard (thanks!). > --- > fs/dax.c | 33 +++++++++++++++++++++++++++++++++ > fs/ext4/inode.c | 10 +--------- > fs/fuse/dax.c | 27 +++------------------------ > fs/xfs/xfs_inode.c | 23 +++++------------------ > fs/xfs/xfs_inode.h | 2 +- > include/linux/dax.h | 21 +++++++++++++++++++++ > mm/madvise.c | 8 ++++---- > 7 files changed, 68 insertions(+), 56 deletions(-) > > diff --git a/fs/dax.c b/fs/dax.c > index d010c10..9c3bd07 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -845,6 +845,39 @@ int dax_delete_mapping_entry(struct address_space *mapping, pgoff_t index) > return ret; > } > > +static int wait_page_idle(struct page *page, > + void (cb)(struct inode *), > + struct inode *inode) > +{ > + return ___wait_var_event(page, page_ref_count(page) == 1, > + TASK_INTERRUPTIBLE, 0, 0, cb(inode)); > +} > + > +/* > + * Unmaps the inode and waits for any DMA to complete prior to deleting the > + * DAX mapping entries for the range. > + */ > +int dax_break_mapping(struct inode *inode, loff_t start, loff_t end, > + void (cb)(struct inode *)) > +{ > + struct page *page; > + int error; > + > + if (!dax_mapping(inode->i_mapping)) > + return 0; > + > + do { > + page = dax_layout_busy_page_range(inode->i_mapping, start, end); > + if (!page) > + break; > + > + error = wait_page_idle(page, cb, inode); > + } while (error == 0); You didn't initialize error to 0, so it could be any value. What if dax_layout_busy_page_range returns null the first time through the loop? > + > + return error; > +} > +EXPORT_SYMBOL_GPL(dax_break_mapping); > + > /* > * Invalidate DAX entry if it is clean. > */ > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index 42ea203..295730a 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -2715,21 +2715,17 @@ xfs_mmaplock_two_inodes_and_break_dax_layout( > struct xfs_inode *ip2) > { > int error; > - bool retry; > struct page *page; > > if (ip1->i_ino > ip2->i_ino) > swap(ip1, ip2); > > again: > - retry = false; > /* Lock the first inode */ > xfs_ilock(ip1, XFS_MMAPLOCK_EXCL); > - error = xfs_break_dax_layouts(VFS_I(ip1), &retry); > - if (error || retry) { > + error = xfs_break_dax_layouts(VFS_I(ip1)); > + if (error) { > xfs_iunlock(ip1, XFS_MMAPLOCK_EXCL); > - if (error == 0 && retry) > - goto again; Hmm, so the retry loop has moved into xfs_break_dax_layouts, which means that we no longer cycle the MMAPLOCK. Why was the lock cycling unnecessary? > return error; > } > > @@ -2988,19 +2984,11 @@ xfs_wait_dax_page( > > int > xfs_break_dax_layouts( > - struct inode *inode, > - bool *retry) > + struct inode *inode) > { > - struct page *page; > - > xfs_assert_ilocked(XFS_I(inode), XFS_MMAPLOCK_EXCL); > > - page = dax_layout_busy_page(inode->i_mapping); > - if (!page) > - return 0; > - > - *retry = true; > - return dax_wait_page_idle(page, xfs_wait_dax_page, inode); > + return dax_break_mapping_inode(inode, xfs_wait_dax_page); > } > > int > @@ -3018,8 +3006,7 @@ xfs_break_layouts( > retry = false; > switch (reason) { > case BREAK_UNMAP: > - error = xfs_break_dax_layouts(inode, &retry); > - if (error || retry) > + if (xfs_break_dax_layouts(inode)) dax_break_mapping can return -ERESTARTSYS, right? So doesn't this need to be: error = xfs_break_dax_layouts(inode); if (error) break; Hm? --D > break; > fallthrough; > case BREAK_WRITE: > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h > index 1648dc5..c4f03f6 100644 > --- a/fs/xfs/xfs_inode.h > +++ b/fs/xfs/xfs_inode.h > @@ -593,7 +593,7 @@ xfs_itruncate_extents( > return xfs_itruncate_extents_flags(tpp, ip, whichfork, new_size, 0); > } > > -int xfs_break_dax_layouts(struct inode *inode, bool *retry); > +int xfs_break_dax_layouts(struct inode *inode); > int xfs_break_layouts(struct inode *inode, uint *iolock, > enum layout_break_reason reason); > > diff --git a/include/linux/dax.h b/include/linux/dax.h > index 9b1ce98..f6583d3 100644 > --- a/include/linux/dax.h > +++ b/include/linux/dax.h > @@ -228,6 +228,20 @@ static inline void dax_read_unlock(int id) > { > } > #endif /* CONFIG_DAX */ > + > +#if !IS_ENABLED(CONFIG_FS_DAX) > +static inline int __must_check dax_break_mapping(struct inode *inode, > + loff_t start, loff_t end, void (cb)(struct inode *)) > +{ > + return 0; > +} > + > +static inline void dax_break_mapping_uninterruptible(struct inode *inode, > + void (cb)(struct inode *)) > +{ > +} > +#endif > + > bool dax_alive(struct dax_device *dax_dev); > void *dax_get_private(struct dax_device *dax_dev); > long dax_direct_access(struct dax_device *dax_dev, pgoff_t pgoff, long nr_pages, > @@ -251,6 +265,13 @@ vm_fault_t dax_finish_sync_fault(struct vm_fault *vmf, > int dax_delete_mapping_entry(struct address_space *mapping, pgoff_t index); > int dax_invalidate_mapping_entry_sync(struct address_space *mapping, > pgoff_t index); > +int __must_check dax_break_mapping(struct inode *inode, loff_t start, > + loff_t end, void (cb)(struct inode *)); > +static inline int __must_check dax_break_mapping_inode(struct inode *inode, > + void (cb)(struct inode *)) > +{ > + return dax_break_mapping(inode, 0, LLONG_MAX, cb); > +} > int dax_dedupe_file_range_compare(struct inode *src, loff_t srcoff, > struct inode *dest, loff_t destoff, > loff_t len, bool *is_same, > diff --git a/mm/madvise.c b/mm/madvise.c > index 49f3a75..1f4c99e 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -1063,7 +1063,7 @@ static int guard_install_pud_entry(pud_t *pud, unsigned long addr, > pud_t pudval = pudp_get(pud); > > /* If huge return >0 so we abort the operation + zap. */ > - return pud_trans_huge(pudval) || pud_devmap(pudval); > + return pud_trans_huge(pudval); > } > > static int guard_install_pmd_entry(pmd_t *pmd, unsigned long addr, > @@ -1072,7 +1072,7 @@ static int guard_install_pmd_entry(pmd_t *pmd, unsigned long addr, > pmd_t pmdval = pmdp_get(pmd); > > /* If huge return >0 so we abort the operation + zap. */ > - return pmd_trans_huge(pmdval) || pmd_devmap(pmdval); > + return pmd_trans_huge(pmdval); > } > > static int guard_install_pte_entry(pte_t *pte, unsigned long addr, > @@ -1183,7 +1183,7 @@ static int guard_remove_pud_entry(pud_t *pud, unsigned long addr, > pud_t pudval = pudp_get(pud); > > /* If huge, cannot have guard pages present, so no-op - skip. */ > - if (pud_trans_huge(pudval) || pud_devmap(pudval)) > + if (pud_trans_huge(pudval)) > walk->action = ACTION_CONTINUE; > > return 0; > @@ -1195,7 +1195,7 @@ static int guard_remove_pmd_entry(pmd_t *pmd, unsigned long addr, > pmd_t pmdval = pmdp_get(pmd); > > /* If huge, cannot have guard pages present, so no-op - skip. */ > - if (pmd_trans_huge(pmdval) || pmd_devmap(pmdval)) > + if (pmd_trans_huge(pmdval)) > walk->action = ACTION_CONTINUE; > > return 0; > -- > git-series 0.9.1 >