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: <8cbfc1ff-f86d-f2cc-d37e-ef874f4600bc@gmx.com>
Date:   Fri, 17 Jun 2022 21:09:47 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
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-4-fmdefrancesco@gmail.com>
From:   Qu Wenruo <quwenruo.btrfs@gmx.com>
Subject: Re: [RFC PATCH v2 3/3] btrfs: Use kmap_local_page() on "in_page" in
 zlib_compress_pages()
In-Reply-To: <20220617120538.18091-4-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() on "in_page" in
> zlib_compress_pages() because in this function the mappings are per thre=
ad
> and are not visible in other contexts.
>
> Use an array based stack in order to take note of the order of mappings
> and un-mappings to comply to the rules of nesting local mappings.

Any extra comment on the "rules of nesting local mappings" part?

>
> 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>
> ---
>   fs/btrfs/zlib.c | 65 ++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 53 insertions(+), 12 deletions(-)
>
> diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
> index c7c69ce4a1a9..1f16014e8ff3 100644
> --- a/fs/btrfs/zlib.c
> +++ b/fs/btrfs/zlib.c
> @@ -99,6 +99,8 @@ int zlib_compress_pages(struct list_head *ws, struct a=
ddress_space *mapping,
>   	int ret;
>   	char *data_in =3D NULL;
>   	char *cpage_out =3D NULL;
> +	char mstack[2];
> +	int sind =3D 0;
>   	int nr_pages =3D 0;
>   	struct page *in_page =3D NULL;
>   	struct page *out_page =3D NULL;
> @@ -126,6 +128,8 @@ int zlib_compress_pages(struct list_head *ws, 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, stru=
ct address_space *mapping,
>   				int i;
>
>   				for (i =3D 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 =3D find_get_page(mapping,
>   								start >> PAGE_SHIFT);
> -					data_in =3D kmap(in_page);
> +					mstack[sind] =3D 'B';
> +					sind++;
> +					data_in =3D kmap_local_page(in_page);
>   					memcpy(workspace->buf + i * PAGE_SIZE,
>   					       data_in, PAGE_SIZE);
>   					start +=3D PAGE_SIZE;
>   				}
>   				workspace->strm.next_in =3D 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.

Thanks,
Qu

> -				if (in_page) {
> -					kunmap(in_page);
> +				if (data_in) {
> +					sind--;
> +					kunmap_local(data_in);
>   					put_page(in_page);
>   				}
>   				in_page =3D find_get_page(mapping,
>   							start >> PAGE_SHIFT);
> -				data_in =3D kmap(in_page);
> +				mstack[sind] =3D 'B';
> +				sind++;
> +				data_in =3D kmap_local_page(in_page);
>   				start +=3D PAGE_SIZE;
>   				workspace->strm.next_in =3D data_in;
>   			}
> @@ -196,23 +206,39 @@ int zlib_compress_pages(struct list_head *ws, stru=
ct address_space *mapping,
>   		 * the stream end if required
>   		 */
>   		if (workspace->strm.avail_out =3D=3D 0) {
> +			sind--;
> +			kunmap_local(data_in);
> +			data_in =3D NULL;
> +
> +			sind--;
>   			kunmap_local(cpage_out);
>   			cpage_out =3D NULL;
> +
>   			if (nr_pages =3D=3D nr_dest_pages) {
>   				out_page =3D NULL;
> +				put_page(in_page);
>   				ret =3D -E2BIG;
>   				goto out;
>   			}
> +
>   			out_page =3D alloc_page(GFP_NOFS);
>   			if (out_page =3D=3D NULL) {
> +				put_page(in_page);
>   				ret =3D -ENOMEM;
>   				goto out;
>   			}
> +
> +			mstack[sind] =3D 'A';
> +			sind++;
>   			cpage_out =3D kmap_local_page(out_page);
>   			pages[nr_pages] =3D out_page;
>   			nr_pages++;
>   			workspace->strm.avail_out =3D PAGE_SIZE;
>   			workspace->strm.next_out =3D cpage_out;
> +
> +			mstack[sind] =3D 'B';
> +			sind++;
> +			data_in =3D kmap_local_page(in_page);
>   		}
>   		/* we're all done */
>   		if (workspace->strm.total_in >=3D len)
> @@ -235,10 +261,16 @@ int zlib_compress_pages(struct list_head *ws, stru=
ct address_space *mapping,
>   			goto out;
>   		} else if (workspace->strm.avail_out =3D=3D 0) {
>   			/* get another page for the stream end */
> +			sind--;
> +			kunmap_local(data_in);
> +			data_in =3D NULL;
> +
> +			sind--;
>   			kunmap_local(cpage_out);
>   			cpage_out =3D NULL;
>   			if (nr_pages =3D=3D nr_dest_pages) {
>   				out_page =3D NULL;
> +				put_page(in_page);
>   				ret =3D -E2BIG;
>   				goto out;
>   			}
> @@ -247,11 +279,18 @@ int zlib_compress_pages(struct list_head *ws, stru=
ct address_space *mapping,
>   				ret =3D -ENOMEM;
>   				goto out;
>   			}
> +
> +			mstack[sind] =3D 'A';
> +			sind++;
>   			cpage_out =3D kmap_local_page(out_page);
>   			pages[nr_pages] =3D out_page;
>   			nr_pages++;
>   			workspace->strm.avail_out =3D PAGE_SIZE;
>   			workspace->strm.next_out =3D cpage_out;
> +
> +			mstack[sind] =3D 'B';
> +			sind++;
> +			data_in =3D kmap_local_page(in_page);
>   		}
>   	}
>   	zlib_deflateEnd(&workspace->strm);
> @@ -266,13 +305,15 @@ int zlib_compress_pages(struct list_head *ws, stru=
ct address_space *mapping,
>   	*total_in =3D workspace->strm.total_in;
>   out:
>   	*out_pages =3D nr_pages;
> -	if (cpage_out)
> -		kunmap_local(cpage_out);
> -
> -	if (in_page) {
> -		kunmap(in_page);
> -		put_page(in_page);
> +	while (--sind >=3D 0) {
> +		if (mstack[sind] =3D=3D 'B') {
> +			kunmap_local(data_in);
> +			put_page(in_page);
> +		} else {
> +			kunmap_local(cpage_out);
> +		}
>   	}
> +
>   	return ret;
>   }
>