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 D478FE77188 for ; Mon, 13 Jan 2025 02:47:54 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 50B846B007B; Sun, 12 Jan 2025 21:47:54 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 4947B6B0083; Sun, 12 Jan 2025 21:47:54 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 3349C6B0085; Sun, 12 Jan 2025 21:47:54 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 124666B007B for ; Sun, 12 Jan 2025 21:47:54 -0500 (EST) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id ACC29B09CB for ; Mon, 13 Jan 2025 02:47:53 +0000 (UTC) X-FDA: 83000893626.19.E40F408 Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf15.hostedemail.com (Postfix) with ESMTP id 0F77FA0006 for ; Mon, 13 Jan 2025 02:47:51 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=FOKokPMs; spf=pass (imf15.hostedemail.com: domain of djwong@kernel.org designates 147.75.193.91 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=1736736472; 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=6n6sKsBDmiVoYxYrY4SkA6YOxkGOYZnDd+ueER1GTOs=; b=b/weKOI671kV5YWg1nj2BMxZhAXAI/IeBz4p+htXdMBm3GaHx6M9NhrzqJxrTiQODIifWG kZFXr+KmAOqEj+95I+eXz4P/LoUx0RKtNBKCcIQ5/k7NlhnefglYQ5Ge6oDxLUT+3A9hnM FCppVCY7lOAXoPFAcbja1Lz/VX6kqmw= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=FOKokPMs; spf=pass (imf15.hostedemail.com: domain of djwong@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=djwong@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736736472; a=rsa-sha256; cv=none; b=723NY23a+0O8nm0rOXanYSWklUQYxc9DM6CPPMbgD5wsram2sfJkaTWQFGH0c5zm8sz42y GOrfe74FViyFzPaqhnLutzK3bD6SgSzhDhivQy960meBiEBkwtT2LfAoNnhlQB8PIKRz7c OzMH+9QDcM0FWIC8BqdXXQqeOrUqoXY= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id F3828A400AC; Mon, 13 Jan 2025 02:46:02 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D5855C4CEDF; Mon, 13 Jan 2025 02:47:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1736736470; bh=gP6Hf8DV4w/UTePON+8BvVG7kYjOiT0KO8ZCUbBNisg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=FOKokPMsFfajbfEYx/DPjYFLRqlx6tsU9JAJwyO9Yl6QzrD5hN/rzZz0rR61g8F+p KNgYIBGaE9Cza208rccMBtJQo3udGHEdyJYl35QvZkUQfQIBnGDYxMTBjxQSBdNl6W rfvJbZkYu0OkLsuu05877EMrvG5LshP8SSGSkn/pXipItuycmvNk+AaXR6HCH+Bo8m nY+0jB3niQ+2CZZoz8UHNbwN+St82Zpzws8K8kQIloUL2RirNCeodtrow9nIeTn0Wq Z3ienwoVjtHr8LM4rdPu+pT3S7nV6JPjXLrrZcEdMLRIgiTR8CX82mo5IJU29NHsuu 0XlGEOGFxd/+w== Date: Sun, 12 Jan 2025 18:47:50 -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: <20250113024750.GV1306365@frogsfrogsfrogs> References: <79936ac15c917f4004397027f648d4fc9c092424.1736488799.git-series.apopple@nvidia.com> <20250110164438.GJ6156@frogsfrogsfrogs> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Queue-Id: 0F77FA0006 X-Stat-Signature: be7nfiq81np9k7666uqd985u377gjkc6 X-Rspam-User: X-Rspamd-Server: rspam11 X-HE-Tag: 1736736471-959667 X-HE-Meta: U2FsdGVkX1/SclbzTuZh7nefNnIrAV1hzXYOWH6pMDO4M9rDSw+j3G7V7xZNDmqS0wj842/DV4x6ok0LnmnfD+aZ3DBYy7KI6JWVEuMQ2L9gX4ARfmmwZ3hQ8wfHn19/F1pCmrfUqboxT1C6cSMxdFBF0ranN0F9JLKTYH9mqsBdBZ+HWeWhFmUKop00NQcryiRLlRljqmqoRrTi67jgQhP+pEecLxKKBvtOhwsvbZhsqQFkP8q9RQrZa5E0LJ3PRUX1TroZUx2NL2pt1wBR+JKEmbnQHuEhCSMS++jDTYrceWFNGJ6EBUXCQEwYKvL40sGnyKKjXgKl/vgXpDMAdyED5g+gWnMeCfeCvowMPkk7nk+BYSS1VsbqCsAEF/Wcou0VtLRzlL/8aPnc0LUB+AfGnu+3vZ/YMk4q1FjtvznEOxaQ9Q0Ey85hMhydlZbAI2YaqcxbFKmVLcpRQzt95OU2jkdHmNb4DHUxELttI6PgzAgSAjlLwbS/FbscCkhwysaD32s82bUdPLyJDeGaB7q02GTNgMmuIsghbZ6/qK1JmBSlRundubr/okUWuth75SNqEq/ZF61WufLOeH6DiAe5eq047zrq6GC+ngyPg3i9f5lcNPivBIbklo6+97P/DNZ57Mg/koLwYyfoHriLCACSN8etGeRUroPy2C84uuzyAjWhVP7Pw1zij0+Ag70gckT8KoNVJrPShkPfvydxY5bnDH2FQ5onLrjwXl6zMPc5y9kvFFe1VRZZ6OHbt7l6DCbzWxjPFEPz9TOmXdga9UERUDF19t8KF7M2sAoQXbGHL+6Uz9calnN6PvyE0yFm7mU7v7d2Hn/R1XY+0WV+/UCxVD9+HpcgCAxxpYaELbvP/mmeBwIpI1IhYsV4pK8niv+JeILji5B7jRFYarryd9Whn+w7bpcWvWpjyebCx/+9AC61xQEHrpjBkZsvTWFYNOutQSlFKWWDvOQ+bfq wVEvZ9pR m4bWYwfk2PDRs4lPb05+Aj86ueEUd4f2ilGUZo23/mUc8GLkHSYzP3XAehg3FDznrgVRJqHPFIT4HKuLl73/Mx6ZmSYLZVWaNHZrkYp0hfshfOYr8dL3ffH9dAIst2+Mua+7x4w6ggqIZIb+gcDt1EM+iN35tgutRaZVZxGE2MYsH8ZqjAwNQFffitCEqNjmB3lrBKL/C8+l5dqdNlcm9I55AjRsbcStvOoRJtlkjhbIPehusLD8YO3p/mzp/NGsytZpfS6kPK6SjszKaQjuLX1mdbv7TjyXccnMKnZfGFuKT3i06GxhGZO6ZeTZNKzbRUZ0F5tt0qgIX5rsH76qy7tissZFWyIHdivvE 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 Mon, Jan 13, 2025 at 11:47:41AM +1100, Alistair Popple wrote: > On Fri, Jan 10, 2025 at 08:44:38AM -0800, Darrick J. Wong wrote: > > 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? > > Yes. I went down the rabbit hole of figuring out why this didn't produce a > compiler warning and forgot to go back and fix it. Thanks. > > > > + > > > + 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? > > Because the lock cycling is already happening in the xfs_wait_dax_page() > callback which is called as part of the retry loop in dax_break_mapping(). Aha, good point. --D > > > 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? > > Right. Thanks for the review, have fixed for the next respin. > > - Alistair > > > --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 > > > >