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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/6/22 02:19, David Sterba wrote:
> On Tue, Jun 21, 2022 at 10:18:12AM -0700, Ira Weiny wrote:
>> 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:
>>>>
>>>>    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?
>
> We optimize for 64bit architectures and add maybe cheap fallbacks or
> warnings but specifically high memory is not supposed to be anywhere if
> possible, all highmem allocations have been dropped. The kmap
> requirement comes only when pages are from page cache and filesystem
> can't affect that.
>
>> On !HIGHMEM systems the kmap_local_page() calls fall out anyway.
>
> Right, but as long as it's kmap_local it must follow the nesting rules
> and this complicates the code or requires restructuring. My point is
> that we can drop kmap for the output page thus getting rid of the
> nesting. Then the mapping of input pages should be straightforward and
> working on 32bit architectures.

I got your point, we can get rid of the kmap_local/kunmap_local for the
output pages.

But since this patch completely refactor how we map pages (hides behind
the new helpers), it can handle output pages in highmem or not.

And to switch to non-highmem for output pages, it's really simple based
on this patch (just change map_output_buffer() and unmap_output_buffer()).

As you mentioned, previously we had problems related to kmap() changes,
so for now I want to be extra safe on both input and output pages.

As long as the patch is fine after enough tests, we can do any output
page mapping change super easily.


Another thing is, isn't this patch itself cleaning up the already
complex mapping scheme?
Yes, kmap_local_page() has extra requirement, but the requirement is
also pretty sane to me, and we should follow that requirement as our
basic scheme.

How we handling the original mapping code is so ugly, thus I believe
even we don't need to handle output page mapping, it's still a worthy
cleanup/refactor.

Thanks,
Qu