Re: [RFC PATCH v2 3/3] btrfs: Use kmap_local_page() on "in_page" in zlib_compress_pages()

[David Sterba]

On Fri, Jun 17, 2022 at 09:09:47PM +0800, Qu Wenruo wrote:
> On 2022/6/17 20:05, Fabio M. De Francesco wrote:
> > @@ -126,6 +128,8 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping,
> >   		ret = -ENOMEM;
> >   		goto out;
> >   	}
> > +	mstack[sind] = 'A';
> > +	sind++;
> >   	cpage_out = kmap_local_page(out_page);
> >   	pages[0] = out_page;
> >   	nr_pages = 1;
> > @@ -148,26 +152,32 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping,
> >   				int i;
> >
> >   				for (i = 0; i < in_buf_pages; i++) {
> > -					if (in_page) {
> > -						kunmap(in_page);
> 
> I don't think we really need to keep @in_page mapped for that long.
> 
> We only need the input pages (pages from inode page cache) when we run
> out of input.
> 
> So what we really need is just to map the input, copy the data to
> buffer, unmap the page.
> 
> > +					if (data_in) {
> > +						sind--;
> > +						kunmap_local(data_in);
> >   						put_page(in_page);
> >   					}
> >   					in_page = find_get_page(mapping,
> >   								start >> PAGE_SHIFT);
> > -					data_in = kmap(in_page);
> > +					mstack[sind] = 'B';
> > +					sind++;
> > +					data_in = kmap_local_page(in_page);
> >   					memcpy(workspace->buf + i * PAGE_SIZE,
> >   					       data_in, PAGE_SIZE);
> >   					start += PAGE_SIZE;
> >   				}
> >   				workspace->strm.next_in = workspace->buf;
> >   			} else {
> 
> I think we can clean up the code.
> 
> In fact the for loop can handle both case, I didn't see any special
> reason to do different handling, we can always use workspace->buf,
> instead of manually dancing using different paths.
> 
> I believe with all these cleanup, it should be much simpler to convert
> to kmap_local_page().
> 
> I'm pretty happy to provide help on this refactor if you don't feel
> confident enough on this part of btrfs.

My first thought was "why to clean up zlib loop if we want to just
replace kmap" but after seeing the whole stack and fiddling with the
indexes I agree that a simplification should be done first.