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

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

On venerdì 17 giugno 2022 16:20:24 CEST David Sterba wrote:
> 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.
> 
I'm afraid that cleaning up that loop won't be enough.

My major problem with this conversion were about the ordering of nested 
mappings / un-mappings (especially when the code jumps to the "out" label 
from different paths. Different paths may require different un-mappings 
orders to not break the stack (LIFO) nesting rules: map(A), map(B), 
unmap(B), unmap(A), and further variations on this fundamental requirement. 

OK, I might very well be wrong... but I think that what led to the 
introduction of that horrid hack, I mean that array based stack, seem to 
have very little to do with the different handling of assignments of 
kaddr's in that loop.

If I'm missing the point, I'd appreciate clarifications and suggestions 
about how to work out this task with a much more elegant strategy.

Thanks so much,

Fabio