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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <2037432.KlZ2vcFHjT@opensuse>
Date:   Fri, 17 Jun 2022 20:25:04 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     dsterba@...e.cz, Qu Wenruo <quwenruo.btrfs@....com>,
        "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 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