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;
Message-ID: <e2376a4f-2c9b-9aa6-4358-513fa6a30e67@gmx.com>
Date:   Fri, 17 Jun 2022 20:54:22 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [RFC PATCH v2 2/3] btrfs: Use kmap_local_page() on "out_page" in
 zlib_compress_pages()
Content-Language: en-US
To:     "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-3-fmdefrancesco@gmail.com>
From:   Qu Wenruo <quwenruo.btrfs@gmx.com>
In-Reply-To: <20220617120538.18091-3-fmdefrancesco@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On 2022/6/17 20:05, Fabio M. De Francesco wrote:
> The use of kmap() is being deprecated in favor of kmap_local_page(). Wit=
h
> kmap_local_page(), the mapping is per thread, CPU local and not globally
> visible.
>
> Therefore, use kmap_local_page() / kunmap_local() for "out_page" in
> zlib_compress_pages() because in this function the mappings are per thre=
ad
> 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>

Looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

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

Just a small nit inlined below.

> ---
>   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 a=
ddress_space *mapping,
>   {
>   	struct workspace *workspace =3D list_entry(ws, struct workspace, list=
);
>   	int ret;
> -	char *data_in;
> -	char *cpage_out;
> +	char *data_in =3D NULL;

I didn't see any diff touching @data_in, any idea why it's initialized
to NULL?

Thanks,
Qu

> +	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, 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, 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, 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, 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, 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, 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);