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;
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
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
Subject: Re: [RFC PATCH v2 3/3] btrfs: Use kmap_local_page() on "in_page" in zlib_compress_pages()
Date:   Fri, 17 Jun 2022 20:25:04 +0200
Message-ID: <2037432.KlZ2vcFHjT@opensuse>
In-Reply-To: <20220617142024.GL20633@twin.jikos.cz>
References: <20220617120538.18091-1-fmdefrancesco@gmail.com> <8cbfc1ff-f86d-f2cc-d37e-ef874f4600bc@gmx.com> <20220617142024.GL20633@twin.jikos.cz>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"
Precedence: bulk

On venerd=C3=AC 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,=20
struct address_space *mapping,
> > >   		ret =3D -ENOMEM;
> > >   		goto out;
> > >   	}
> > > +	mstack[sind] =3D 'A';
> > > +	sind++;
> > >   	cpage_out =3D kmap_local_page(out_page);
> > >   	pages[0] =3D out_page;
> > >   	nr_pages =3D 1;
> > > @@ -148,26 +152,32 @@ int zlib_compress_pages(struct list_head *ws,=20
struct address_space *mapping,
> > >   				int i;
> > >
> > >   				for (i =3D 0; i < in_buf_pages; i++)=20
{
> > > -					if (in_page) {
> > > -					=09
kunmap(in_page);
> >=20
> > I don't think we really need to keep @in_page mapped for that long.
> >=20
> > We only need the input pages (pages from inode page cache) when we run
> > out of input.
> >=20
> > So what we really need is just to map the input, copy the data to
> > buffer, unmap the page.
> >=20
> > > +					if (data_in) {
> > > +						sind--;
> > > +					=09
kunmap_local(data_in);
> > >   					=09
put_page(in_page);
> > >   					}
> > >   					in_page =3D=20
find_get_page(mapping,
> > >   							=09
start >> PAGE_SHIFT);
> > > -					data_in =3D kmap(in_page);
> > > +					mstack[sind] =3D 'B';
> > > +					sind++;
> > > +					data_in =3D=20
kmap_local_page(in_page);
> > >   					memcpy(workspace->buf + i=20
* PAGE_SIZE,
> > >   					       data_in,=20
PAGE_SIZE);
> > >   					start +=3D PAGE_SIZE;
> > >   				}
> > >   				workspace->strm.next_in =3D=20
workspace->buf;
> > >   			} else {
> >=20
> > I think we can clean up the code.
> >=20
> > 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.
> >=20
> > I believe with all these cleanup, it should be much simpler to convert
> > to kmap_local_page().
> >=20
> > I'm pretty happy to provide help on this refactor if you don't feel
> > confident enough on this part of btrfs.
>=20
> 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.
>=20
I'm afraid that cleaning up that loop won't be enough.

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

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

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

Thanks so much,

=46abio