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 X-Spam-Level: X-Spam-Status: No, score=-16.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 30DE0C636CA for ; Thu, 15 Jul 2021 22:08:43 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id C239261289 for ; Thu, 15 Jul 2021 22:08:42 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C239261289 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 21B708D00F4; Thu, 15 Jul 2021 18:08:43 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1CB798D00EC; Thu, 15 Jul 2021 18:08:43 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 045F98D00F4; Thu, 15 Jul 2021 18:08:43 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0046.hostedemail.com [216.40.44.46]) by kanga.kvack.org (Postfix) with ESMTP id D70338D00EC for ; Thu, 15 Jul 2021 18:08:42 -0400 (EDT) Received: from smtpin05.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id AE5A3824556B for ; Thu, 15 Jul 2021 22:08:41 +0000 (UTC) X-FDA: 78366212442.05.B0A4B41 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by imf21.hostedemail.com (Postfix) with ESMTP id 4BAB7D0070E5 for ; Thu, 15 Jul 2021 22:08:41 +0000 (UTC) Received: by mail.kernel.org (Postfix) with ESMTPSA id 7BC8461289; Thu, 15 Jul 2021 22:08:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1626386920; bh=ALJUHE+C9LUrPDDnGfrCa1mLT5eVC712CTidE2c7xI8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=jroFG37bLH/2wJqwRblZtf2WXYIWF117yiK7yRaLE41bLF4snqJz4UH3r+2gVsVnr Hiemg0cRc2p6dmcBJNr+mtjHRKZKyqXdl8OlpsCFt9pNa8ZJGa4PWCfkXx5jik/2u9 8kkj0ebwqvIm5cB/ANkIUVbEvUpT/nt6YgDBuPPfe+cN5fdyTr6II681gGetcK2f59 mHOBROuy9zFgyYsQ9DYU4xLpYpKQlpi/A+Dhf7HPuQVSUMz01zfNiojtnK+X6DX3fn GUXJXDb0u4ANWkp3ZPkAniQxLYJ9SWbZgzlZlEgpdelfigthrLEunNtHYGHxpphS7I sSkdKx0e/oHjQ== Date: Thu, 15 Jul 2021 15:08:40 -0700 From: "Darrick J. Wong" To: "Matthew Wilcox (Oracle)" Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v14 124/138] fs: Convert vfs_dedupe_file_range_compare to folios Message-ID: <20210715220840.GS22357@magnolia> References: <20210715033704.692967-1-willy@infradead.org> <20210715033704.692967-125-willy@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210715033704.692967-125-willy@infradead.org> X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 4BAB7D0070E5 X-Stat-Signature: 67gkgx5iiqm17mbsyfri6qrdnjjbczzj Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=jroFG37b; spf=pass (imf21.hostedemail.com: domain of djwong@kernel.org designates 198.145.29.99 as permitted sender) smtp.mailfrom=djwong@kernel.org; dmarc=pass (policy=none) header.from=kernel.org X-HE-Tag: 1626386921-687574 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: On Thu, Jul 15, 2021 at 04:36:50AM +0100, Matthew Wilcox (Oracle) wrote: > We still only operate on a single page of data at a time due to using > kmap(). A more complex implementation would work on each page in a folio, > but it's not clear that such a complex implementation would be worthwhile. Does this break up a compound folio into smaller pages? > Signed-off-by: Matthew Wilcox (Oracle) > --- > fs/remap_range.c | 116 ++++++++++++++++++++++------------------------- > 1 file changed, 55 insertions(+), 61 deletions(-) > > diff --git a/fs/remap_range.c b/fs/remap_range.c > index e4a5fdd7ad7b..886e6ed2c6c2 100644 > --- a/fs/remap_range.c > +++ b/fs/remap_range.c > @@ -158,41 +158,41 @@ static int generic_remap_check_len(struct inode *inode_in, > } > > /* Read a page's worth of file data into the page cache. */ > -static struct page *vfs_dedupe_get_page(struct inode *inode, loff_t offset) > +static struct folio *vfs_dedupe_get_folio(struct inode *inode, loff_t pos) > { > - struct page *page; > + struct folio *folio; > > - page = read_mapping_page(inode->i_mapping, offset >> PAGE_SHIFT, NULL); > - if (IS_ERR(page)) > - return page; > - if (!PageUptodate(page)) { > - put_page(page); > + folio = read_mapping_folio(inode->i_mapping, pos >> PAGE_SHIFT, NULL); > + if (IS_ERR(folio)) > + return folio; > + if (!folio_test_uptodate(folio)) { > + folio_put(folio); > return ERR_PTR(-EIO); > } > - return page; > + return folio; > } > > /* > - * Lock two pages, ensuring that we lock in offset order if the pages are from > - * the same file. > + * Lock two folios, ensuring that we lock in offset order if the folios > + * are from the same file. > */ > -static void vfs_lock_two_pages(struct page *page1, struct page *page2) > +static void vfs_lock_two_folios(struct folio *folio1, struct folio *folio2) > { > /* Always lock in order of increasing index. */ > - if (page1->index > page2->index) > - swap(page1, page2); > + if (folio1->index > folio2->index) > + swap(folio1, folio2); > > - lock_page(page1); > - if (page1 != page2) > - lock_page(page2); > + folio_lock(folio1); > + if (folio1 != folio2) > + folio_lock(folio2); > } > > -/* Unlock two pages, being careful not to unlock the same page twice. */ > -static void vfs_unlock_two_pages(struct page *page1, struct page *page2) > +/* Unlock two folios, being careful not to unlock the same folio twice. */ > +static void vfs_unlock_two_folios(struct folio *folio1, struct folio *folio2) > { > - unlock_page(page1); > - if (page1 != page2) > - unlock_page(page2); > + folio_unlock(folio1); > + if (folio1 != folio2) > + folio_unlock(folio2); This could result in a lot of folio lock cycling. Do you think it's worth the effort to minimize this by keeping the folio locked if the next page is going to be from the same one? --D > } > > /* > @@ -200,77 +200,71 @@ static void vfs_unlock_two_pages(struct page *page1, struct page *page2) > * Caller must have locked both inodes to prevent write races. > */ > static int vfs_dedupe_file_range_compare(struct inode *src, loff_t srcoff, > - struct inode *dest, loff_t destoff, > + struct inode *dest, loff_t dstoff, > loff_t len, bool *is_same) > { > - loff_t src_poff; > - loff_t dest_poff; > - void *src_addr; > - void *dest_addr; > - struct page *src_page; > - struct page *dest_page; > - loff_t cmp_len; > - bool same; > - int error; > - > - error = -EINVAL; > - same = true; > + bool same = true; > + int error = -EINVAL; > + > while (len) { > - src_poff = srcoff & (PAGE_SIZE - 1); > - dest_poff = destoff & (PAGE_SIZE - 1); > - cmp_len = min(PAGE_SIZE - src_poff, > - PAGE_SIZE - dest_poff); > + struct folio *src_folio, *dst_folio; > + void *src_addr, *dst_addr; > + loff_t cmp_len = min(PAGE_SIZE - offset_in_page(srcoff), > + PAGE_SIZE - offset_in_page(dstoff)); > + > cmp_len = min(cmp_len, len); > if (cmp_len <= 0) > goto out_error; > > - src_page = vfs_dedupe_get_page(src, srcoff); > - if (IS_ERR(src_page)) { > - error = PTR_ERR(src_page); > + src_folio = vfs_dedupe_get_folio(src, srcoff); > + if (IS_ERR(src_folio)) { > + error = PTR_ERR(src_folio); > goto out_error; > } > - dest_page = vfs_dedupe_get_page(dest, destoff); > - if (IS_ERR(dest_page)) { > - error = PTR_ERR(dest_page); > - put_page(src_page); > + dst_folio = vfs_dedupe_get_folio(dest, dstoff); > + if (IS_ERR(dst_folio)) { > + error = PTR_ERR(dst_folio); > + folio_put(src_folio); > goto out_error; > } > > - vfs_lock_two_pages(src_page, dest_page); > + vfs_lock_two_folios(src_folio, dst_folio); > > /* > - * Now that we've locked both pages, make sure they're still > + * Now that we've locked both folios, make sure they're still > * mapped to the file data we're interested in. If not, > * someone is invalidating pages on us and we lose. > */ > - if (!PageUptodate(src_page) || !PageUptodate(dest_page) || > - src_page->mapping != src->i_mapping || > - dest_page->mapping != dest->i_mapping) { > + if (!folio_test_uptodate(src_folio) || !folio_test_uptodate(dst_folio) || > + src_folio->mapping != src->i_mapping || > + dst_folio->mapping != dest->i_mapping) { > same = false; > goto unlock; > } > > - src_addr = kmap_atomic(src_page); > - dest_addr = kmap_atomic(dest_page); > + src_addr = kmap_local_folio(src_folio, > + offset_in_folio(src_folio, srcoff)); > + dst_addr = kmap_local_folio(dst_folio, > + offset_in_folio(dst_folio, dstoff)); > > - flush_dcache_page(src_page); > - flush_dcache_page(dest_page); > + flush_dcache_folio(src_folio); > + flush_dcache_folio(dst_folio); > > - if (memcmp(src_addr + src_poff, dest_addr + dest_poff, cmp_len)) > + if (memcmp(src_addr, dst_addr, cmp_len)) > same = false; > > - kunmap_atomic(dest_addr); > - kunmap_atomic(src_addr); > + kunmap_local(dst_addr); > + kunmap_local(src_addr); > unlock: > - vfs_unlock_two_pages(src_page, dest_page); > - put_page(dest_page); > - put_page(src_page); > + vfs_unlock_two_folios(src_folio, dst_folio); > + folio_put(dst_folio); > + folio_put(src_folio); > > if (!same) > break; > > srcoff += cmp_len; > - destoff += cmp_len; > + dstoff += cmp_len; > len -= cmp_len; > } > > -- > 2.30.2 >