Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 17 Jun 2022 16:20:24 +0200
From:   David Sterba <dsterba@suse.cz>
To:     Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>,
        Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>,
        David Sterba <dsterba@suse.com>,
        Nick Terrell <terrelln@fb.com>,
        Chris Down <chris@chrisdown.name>,
        Filipe Manana <fdmanana@suse.com>, Qu Wenruo <wqu@suse.com>,
        Nikolay Borisov <nborisov@suse.com>,
        Gabriel Niebler <gniebler@suse.com>,
        Ira Weiny <ira.weiny@intel.com>, linux-btrfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v2 3/3] btrfs: Use kmap_local_page() on "in_page" in
 zlib_compress_pages()
Message-ID: <20220617142024.GL20633@twin.jikos.cz>
Reply-To: dsterba@suse.cz
Mail-Followup-To: dsterba@suse.cz, Qu Wenruo <quwenruo.btrfs@gmx.com>,
        "Fabio M. De Francesco" <fmdefrancesco@gmail.com>,
        Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>,
        David Sterba <dsterba@suse.com>, Nick Terrell <terrelln@fb.com>,
        Chris Down <chris@chrisdown.name>,
        Filipe Manana <fdmanana@suse.com>, Qu Wenruo <wqu@suse.com>,
        Nikolay Borisov <nborisov@suse.com>,
        Gabriel Niebler <gniebler@suse.com>,
        Ira Weiny <ira.weiny@intel.com>, linux-btrfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220617120538.18091-1-fmdefrancesco@gmail.com>
 <20220617120538.18091-4-fmdefrancesco@gmail.com>
 <8cbfc1ff-f86d-f2cc-d37e-ef874f4600bc@gmx.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8cbfc1ff-f86d-f2cc-d37e-ef874f4600bc@gmx.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Precedence: bulk

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.