Re: [PATCH v2] erofs: use kmap_local_page() only for erofs_bread()

[Gao Xiang]

On Wed, Oct 19, 2022 at 08:17:05PM +0200, Fabio M. De Francesco wrote:
> On Wednesday, October 19, 2022 1:36:55 AM CEST Gao Xiang wrote:
> > On Wed, Oct 19, 2022 at 01:21:27AM +0200, Fabio M. De Francesco wrote:
> > > On Tuesday, October 18, 2022 11:29:21 PM CEST Gao Xiang wrote:
> > 
> > ...
> > 
> > > 
> > > > One of what I need to care is nested kmap() usage,
> > > > some unmap/remap order cannot be simply converted to kmap_local()
> > > 
> > > Correct about nesting. If we map A and then B, we must unmap B and then A.
> > > 
> > > However, as you seem to convey, not always unmappings in right order 
> (stack 
> > > based) is possible, sometimes because very long functions have many loop's 
> > > breaks and many goto exit labels.
> > > 
> > > > but I think
> > > > it's not the case for erofs_bread().  Actually EROFS has one of such 
> nested
> > > > kmap() usage, but I don't really care its performance on HIGHMEM 
> platforms,
> > > > so I think kmap() is still somewhat useful compared to kmap_local() from 
> > > this
> > > > point of view],
> > > 
> 
> fs/erofs conversions are in our (Ira's and my) list. So I'm am happy to see 
> that we can delete some entries because of your changes. :-)
> 
> > > In Btrfs I solved (thanks to David S.' advice) by mapping only one of two 
> > > pages, only the one coming from the page cache. 
> > > 
> > > The other page didn't need the use of kmap_local_page() because it was 
> > > allocated in the filesystem with "alloc_page(GFP_NOFS)". GFP_NOFS won't 
> ever 
> > > allocate from ZONE_HIGHMEM, therefore a direct page_address() could avoid 
> the 
> > > mapping and the nesting issues.
> > > 
> > > Did you check if you may solve the same way? 
> > 
> > That is not simple.  Currently we have compressed pages and decompressed
> > pages (page cache or others), and they can be unmapped when either data
> > is all consumed, so compressed pages can be unmapped first, or
> > decompressed pages can be unmapped first.  That quite depends on which
> > pages goes first.
> > 
> > I think such usage is a quite common pattern for decoder or encoder,
> > you could take a look at z_erofs_lzma_decompress() in
> > fs/erofs/decompressor_lzma.c.  
> 
> I haven't yet read that code, however I may attempt to propose a pattern for 
> solving this kinds of issue, I mean where you don't know which page got mapped 
> last...
> 
> It's not elegant but it may work. You have compressed and decompressed pages 
> and you can't know in advance what page should be unmapped first because you 
> can't know in which order they where mapped, right?
> 

Not really.

> I'd use a variable to save two different values, each representing the last 
> page mapped. When the code gets to the unmapping block (perhaps in an "out" 
> label), just check what that variable contains. Depending on that value, say 
> 'c' or 'd', you will be able to know what must be unmapped for first. An "if / 
> else" can do the work.

That is not the simple nested unmapped case as you said above, I could take
a very brief example:

1. map a decompresed page
2. map a compressed page
3. working
4. decompressed page is all consumed, unmap the current decompressed page
5. map the next decompressed page
6. working
7. decompressed page is all consumed, unmap the current decompressed page
8. map the next decompressed page
9. working
10. compressed page is all consumed, unmap the current compressed page
11. map the next compressed page
12. working
13. ... (anyway, unmap and remap a compressed page or a decompressed page
         in any order.)

until all process is finished.  by using kmap(), it's much simple to
implement this, but kmap_local(), it only complexes the code.

Thanks,
Gao Xiang

> 
> What do you think of this?
>