Re: [PATCH v2] btrfs: zlib: refactor how we prepare the buffers

[Ira Weiny <ira.weiny@intel.com>]

On Tue, Jun 21, 2022 at 03:15:21PM +0200, David Sterba wrote:
> On Tue, Jun 21, 2022 at 01:59:46PM +0800, Qu Wenruo wrote:
> > Inspired by recent kmap() change from Fabio M. De Francesco.
> > 
> > There are some weird behavior in zlib_compress_pages(), mostly around how
> > we prepare the input and output buffers.
> > 
> > [BEFORE]
> > - We hold a page mapped for a long time
> >   This is making it much harder to convert kmap() to kmap_local_page(),
> >   as such long mapped page can lead to nested mapped page.
> > 
> > - Different paths in the name of "optimization"
> >   When we ran out of input buffer, we will grab the new input with two
> >   different paths:
> > 
> >   * If there are more than one pages left, we copy the content into the
> >     input buffer.
> >     This behavior is introduced mostly for S390, as that arch needs
> >     multiple pages as input buffer for hardware decompression.
> > 
> >   * If there is only one page left, we use that page from page cache
> >     directly without copying the content.
> > 
> >   This is making page map/unmap much harder, especially due the latter
> >   case.
> > 
> > - Input and output pages can be unmapped at different timing
> >   This make it almost impossible to convert the kmap() into
> >   kmap_local_page().
> > 
> >   As kmap_local_page() have strict requirement on the sequence of nested
> >   kmap:
> >                    OK              |            BAD
> >   ---------------------------------+---------------------------------
> >   in = kmap_local_page(in_page);   | in = kmap_local_page(in_page);
> >   out = kmap_local_page(out_page); | out = kmap_local_page(out_page);
> 
> The input pages come from page cache and could be allocated from
> highmem but the output pages are allocated by us and only with GFP_NOFS
> so they don't need to be kmapped at all, right?

How important is it to optimize the HIGHMEM systems?

On !HIGHMEM systems the kmap_local_page() calls fall out anyway.

Ira

> 
> I sent the patches to remove kmap from the compression code but it was
> buggy as it removed it from the input pages too. The revert was complete
> so we get back to a known state but the output pages do not have to be
> mapped. With that removed it should be straightforward to use kmap_local
> without any nesting.