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:     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, Qu Wenruo <quwenruo.btrfs@gmx.com>
Subject: Re: [RFC PATCH v2 2/3] btrfs: Use kmap_local_page() on "out_page" in zlib_compress_pages()
Date:   Fri, 17 Jun 2022 19:46:21 +0200
Message-ID: <3674366.kQq0lBPeGt@opensuse>
In-Reply-To: <e2376a4f-2c9b-9aa6-4358-513fa6a30e67@gmx.com>
References: <20220617120538.18091-1-fmdefrancesco@gmail.com> <20220617120538.18091-3-fmdefrancesco@gmail.com> <e2376a4f-2c9b-9aa6-4358-513fa6a30e67@gmx.com>
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 14:54:22 CEST Qu Wenruo wrote:
>=20
> On 2022/6/17 20:05, Fabio M. De Francesco wrote:
> > The use of kmap() is being deprecated in favor of kmap_local_page().=20
With
> > kmap_local_page(), the mapping is per thread, CPU local and not=20
globally
> > visible.
> >
> > Therefore, use kmap_local_page() / kunmap_local() for "out_page" in
> > zlib_compress_pages() because in this function the mappings are per=20
thread
> > and are not visible in other contexts.
> >
> > Tested with xfstests on QEMU + KVM 32 bits VM with 4GB of RAM and
> > HIGHMEM64G enabled. This patch passes 26/26 tests of group "compress".
> >
> > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
>=20
> Looks good to me.
>=20
> Reviewed-by: Qu Wenruo <wqu@suse.com>
>=20

Again, thanks.

> The change is just to use @cpage_out to indicate if it's mapped (NULL =3D
> not mapped).
>=20

Most of the conversions are quite easy, I would say "mechanical".
As you already noted in 3/3, there are cases where it's not that simple :-(

> Just a small nit inlined below.
>=20
> > ---
> >   fs/btrfs/zlib.c | 20 +++++++++++---------
> >   1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
> > index 770c4c6bbaef..c7c69ce4a1a9 100644
> > --- a/fs/btrfs/zlib.c
> > +++ b/fs/btrfs/zlib.c
> > @@ -97,8 +97,8 @@ int zlib_compress_pages(struct list_head *ws, struct=
=20
address_space *mapping,
> >   {
> >   	struct workspace *workspace =3D list_entry(ws, struct workspace,=20
list);
> >   	int ret;
> > -	char *data_in;
> > -	char *cpage_out;
> > +	char *data_in =3D NULL;
>=20
> I didn't see any diff touching @data_in, any idea why it's initialized
> to NULL?
>=20

I suppose it's a relic of RFC v1 that I overlooked when I split it into=20
three patches. I will remember to avoid initialization in the "real" patch.

=46abio

>
> Thanks,
> Qu
>=20
> > +	char *cpage_out =3D NULL;
> >   	int nr_pages =3D 0;
> >   	struct page *in_page =3D NULL;
> >   	struct page *out_page =3D NULL;
> > @@ -126,7 +126,7 @@ int zlib_compress_pages(struct list_head *ws,=20
struct address_space *mapping,
> >   		ret =3D -ENOMEM;
> >   		goto out;
> >   	}
> > -	cpage_out =3D kmap(out_page);
> > +	cpage_out =3D kmap_local_page(out_page);
> >   	pages[0] =3D out_page;
> >   	nr_pages =3D 1;
> >
> > @@ -196,7 +196,8 @@ int zlib_compress_pages(struct list_head *ws,=20
struct address_space *mapping,
> >   		 * the stream end if required
> >   		 */
> >   		if (workspace->strm.avail_out =3D=3D 0) {
> > -			kunmap(out_page);
> > +			kunmap_local(cpage_out);
> > +			cpage_out =3D NULL;
> >   			if (nr_pages =3D=3D nr_dest_pages) {
> >   				out_page =3D NULL;
> >   				ret =3D -E2BIG;
> > @@ -207,7 +208,7 @@ int zlib_compress_pages(struct list_head *ws,=20
struct address_space *mapping,
> >   				ret =3D -ENOMEM;
> >   				goto out;
> >   			}
> > -			cpage_out =3D kmap(out_page);
> > +			cpage_out =3D kmap_local_page(out_page);
> >   			pages[nr_pages] =3D out_page;
> >   			nr_pages++;
> >   			workspace->strm.avail_out =3D PAGE_SIZE;
> > @@ -234,7 +235,8 @@ int zlib_compress_pages(struct list_head *ws,=20
struct address_space *mapping,
> >   			goto out;
> >   		} else if (workspace->strm.avail_out =3D=3D 0) {
> >   			/* get another page for the stream end */
> > -			kunmap(out_page);
> > +			kunmap_local(cpage_out);
> > +			cpage_out =3D NULL;
> >   			if (nr_pages =3D=3D nr_dest_pages) {
> >   				out_page =3D NULL;
> >   				ret =3D -E2BIG;
> > @@ -245,7 +247,7 @@ int zlib_compress_pages(struct list_head *ws,=20
struct address_space *mapping,
> >   				ret =3D -ENOMEM;
> >   				goto out;
> >   			}
> > -			cpage_out =3D kmap(out_page);
> > +			cpage_out =3D kmap_local_page(out_page);
> >   			pages[nr_pages] =3D out_page;
> >   			nr_pages++;
> >   			workspace->strm.avail_out =3D PAGE_SIZE;
> > @@ -264,8 +266,8 @@ int zlib_compress_pages(struct list_head *ws,=20
struct address_space *mapping,
> >   	*total_in =3D workspace->strm.total_in;
> >   out:
> >   	*out_pages =3D nr_pages;
> > -	if (out_page)
> > -		kunmap(out_page);
> > +	if (cpage_out)
> > +		kunmap_local(cpage_out);
> >
> >   	if (in_page) {
> >   		kunmap(in_page);
>=20