From: Jan Kara <jack@suse.cz>
To: Matthew Wilcox <willy@infradead.org>
Cc: Ira Weiny <ira.weiny@intel.com>, Jan Kara <jack@suse.cz>,
reiserfs-devel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
"Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Subject: Re: [PATCH 5/8] reiserfs: Convert do_journal_end() to use kmap_local_folio()
Date: Thu, 22 Dec 2022 11:37:05 +0100 [thread overview]
Message-ID: <20221222103705.f2s7bpwv2g7x2bwt@quack3> (raw)
In-Reply-To: <Y6NYonXNGL58+rV8@casper.infradead.org>
On Wed 21-12-22 19:04:02, Matthew Wilcox wrote:
> On Tue, Dec 20, 2022 at 03:59:39PM -0800, Ira Weiny wrote:
> > On Tue, Dec 20, 2022 at 06:34:57PM +0000, Matthew Wilcox wrote:
> > > On Tue, Dec 20, 2022 at 08:58:52AM -0800, Ira Weiny wrote:
> > > > On Tue, Dec 20, 2022 at 12:18:01PM +0100, Jan Kara wrote:
> > > > > On Tue 20-12-22 09:35:43, Matthew Wilcox wrote:
> > > > > > But that doesn't solve the "What about fs block size > PAGE_SIZE"
> > > > > > problem that we also want to solve. Here's a concrete example:
> > > > > >
> > > > > > static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh)
> > > > > > {
> > > > > > - struct page *page = bh->b_page;
> > > > > > + struct folio *folio = bh->b_folio;
> > > > > > char *addr;
> > > > > > __u32 checksum;
> > > > > >
> > > > > > - addr = kmap_atomic(page);
> > > > > > - checksum = crc32_be(crc32_sum,
> > > > > > - (void *)(addr + offset_in_page(bh->b_data)), bh->b_size);
> > > > > > - kunmap_atomic(addr);
> > > > > > + BUG_ON(IS_ENABLED(CONFIG_HIGHMEM) && bh->b_size > PAGE_SIZE);
> > > > > > +
> > > > > > + addr = kmap_local_folio(folio, offset_in_folio(folio, bh->b_data));
> > > > > > + checksum = crc32_be(crc32_sum, addr, bh->b_size);
> > > > > > + kunmap_local(addr);
> > > > > >
> > > > > > return checksum;
> > > > > > }
> > > > > >
> > > > > > I don't want to add a lot of complexity to handle the case of b_size >
> > > > > > PAGE_SIZE on a HIGHMEM machine since that's not going to benefit terribly
> > > > > > many people. I'd rather have the assertion that we don't support it.
> > > > > > But if there's a good higher-level abstraction I'm missing here ...
> > > > >
> > > > > Just out of curiosity: So far I was thinking folio is physically contiguous
> > > > > chunk of memory. And if it is, then it does not seem as a huge overkill if
> > > > > kmap_local_folio() just maps the whole folio?
> > > >
> > > > Willy proposed that previously but we could not come to a consensus on how to
> > > > do it.
> > > >
> > > > https://lore.kernel.org/all/Yv2VouJb2pNbP59m@iweiny-desk3/
> > > >
> > > > FWIW I still think increasing the entries to cover any foreseeable need would
> > > > be sufficient because HIGHMEM does not need to be optimized. Couldn't we hide
> > > > the entry count into some config option which is only set if a FS needs a
> > > > larger block size on a HIGHMEM system?
> > >
> > > "any foreseeable need"? I mean ... I'd like to support 2MB folios,
> > > even on HIGHMEM machines, and that's 512 entries. If we're doing
> > > memcpy_to_folio(), we know that's only one mapping, but still, 512
> > > entries is _a lot_ of address space to be reserving on a 32-bit machine.
> >
> > I'm confused. A memcpy_to_folio() could loop to map the pages as needed
> > depending on the amount of data to copy. Or just map/unmap in a loop.
> >
> > This seems like an argument to have a memcpy_to_folio() to hide such nastiness
> > on HIGHMEM from the user.
>
> I see that you are confused. What I'm not quite sure of is how I confused
> you, so I'm just going to try again in different words.
>
> Given the desire to support 2MB folios on x86/ARM PAE systems, we can't
> have a kmap_local_entire_folio() because that would take up too much
> address space.
Is that really a problem? I mean sure 2MB is noticeable in 32-bit address
space but these mappings are very shortlived due to their nature (and the
API kind of enforces that) so there'd hardly be more than a handful of them
existing in parallel on a system. Or is my expectation wrong?
But I agree the solution with memcpy_to/from_folio() works as well.
> > [*] I only play a file system developer on TV. ;-)
>
> That's OK, I'm only pretending to be an MM developer. Keep quiet, and
> I think we can get away with this.
"All the world's a stage, and all the men and women merely players." :)
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
next prev parent reply other threads:[~2022-12-22 10:37 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-16 20:53 [PATCH 0/8] Convert reiserfs from b_page to b_folio Matthew Wilcox (Oracle)
2022-12-16 20:53 ` [PATCH 1/8] reiserfs: use b_folio instead of b_page in some obvious cases Matthew Wilcox (Oracle)
2022-12-16 20:53 ` [PATCH 2/8] reiserfs: use kmap_local_folio() in _get_block_create_0() Matthew Wilcox (Oracle)
2022-12-17 17:14 ` Ira Weiny
2022-12-17 19:07 ` Matthew Wilcox
2022-12-17 23:33 ` Ira Weiny
2022-12-19 10:42 ` Jan Kara
2022-12-16 20:53 ` [PATCH 3/8] reiserfs: Convert direct2indirect() to call folio_zero_range() Matthew Wilcox (Oracle)
2022-12-17 21:08 ` Ira Weiny
2022-12-16 20:53 ` [PATCH 4/8] reiserfs: Convert reiserfs_delete_item() to use kmap_local_folio() Matthew Wilcox (Oracle)
2022-12-17 23:44 ` Ira Weiny
2022-12-16 20:53 ` [PATCH 5/8] reiserfs: Convert do_journal_end() " Matthew Wilcox (Oracle)
2022-12-17 23:52 ` Ira Weiny
2022-12-20 9:35 ` Matthew Wilcox
2022-12-20 11:18 ` Jan Kara
2022-12-20 16:58 ` Ira Weiny
2022-12-20 18:34 ` Matthew Wilcox
2022-12-20 23:59 ` Ira Weiny
2022-12-21 19:04 ` Matthew Wilcox
2022-12-22 10:37 ` Jan Kara [this message]
2022-12-16 20:53 ` [PATCH 6/8] reiserfs: Convert map_block_for_writepage() " Matthew Wilcox (Oracle)
2022-12-18 0:02 ` Ira Weiny
2022-12-16 20:53 ` [PATCH 7/8] reiserfs: Convert convert_tail_for_hole() to use folios Matthew Wilcox (Oracle)
2022-12-16 20:53 ` [PATCH 8/8] reiserfs: Use flush_dcache_folio() in reiserfs_quota_write() Matthew Wilcox (Oracle)
2022-12-17 20:43 ` [PATCH 0/8] Convert reiserfs from b_page to b_folio Fabio M. De Francesco
2022-12-17 23:39 ` Ira Weiny
2022-12-18 8:09 ` Fabio M. De Francesco
2022-12-18 17:59 ` Matthew Wilcox
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221222103705.f2s7bpwv2g7x2bwt@quack3 \
--to=jack@suse.cz \
--cc=fmdefrancesco@gmail.com \
--cc=ira.weiny@intel.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=reiserfs-devel@vger.kernel.org \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).