linux-kernel - Re: [RFC PATCH v2 3/3] btrfs: Use kmap_local_page() on "in_page" in zlib_compress

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 17 Jun 2022 16:20:24 +0200
From:   David Sterba <dsterba@...e.cz>
To:     Qu Wenruo <quwenruo.btrfs@....com>
Cc:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>,
        Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>,
        Nick Terrell <terrelln@...com>,
        Chris Down <chris@...isdown.name>,
        Filipe Manana <fdmanana@...e.com>, Qu Wenruo <wqu@...e.com>,
        Nikolay Borisov <nborisov@...e.com>,
        Gabriel Niebler <gniebler@...e.com>,
        Ira Weiny <ira.weiny@...el.com>, linux-btrfs@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2 3/3] btrfs: Use kmap_local_page() on "in_page" in
 zlib_compress_pages()

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.