[RFC PATCH v2 0/3] btrfs: Convert zlib.c to use kmap_local_page()

[RFC PATCH v2 0/3] btrfs: Convert zlib.c to use kmap_local_page()

[Fabio M. De Francesco]

The use of kmap() is being deprecated in favor of kmap_local_page(). With
kmap_local_page(), the mapping is per thread, CPU local and not globally
visible.

Therefore, use kmap_local_page() / kunmap_local() in zlib.c because in
this file the mappings are per thread and are not visible in other
contexts.

This is an RFC because patch 3/3 uses an horrid hack. I'm using an array
based stack which tracks local mappings / un-mappings. I understand that
it is not the better solution, but it works and it is easy to implement :)

I've decided to decompose RFC v1 into a series of three patches in order to
make clear that I encountered problems with conversions in patch 3/3. I'm
pretty sure that people who are familiar with these functions could refactor
this code and provide much more elegant and efficient solutions.

Since this code currently uses kmap() / kunmap(), I think that the functions
I'm converting had been designed without taking into account the rules of
ordering of nesting local mappings / un-mappings.

I've been trying to refactor this code but it is something beyond my current
level of knowledge and skills...

Can anyone please provide any better suited solutions for patch 3/3?

Tested with xfstests on QEMU + KVM 32 bits VM with 4GB of RAM and
HIGHMEM64G enabled. Each patch of this series passes 26/26 tests of group
"compress".

tweed32:/usr/lib/xfstests # ./check -g compress
FSTYP         -- btrfs
PLATFORM      -- Linux/i686 tweed32 5.19.0-rc2-vanilla-debug+ #45 SMP PREEMPT_DYNAMIC Fri Jun 17 11:14:11 CEST 2022
MKFS_OPTIONS  -- /dev/loop1
MOUNT_OPTIONS -- /dev/loop1 /mnt/scratch

btrfs/024 2s ...  3s
btrfs/026 5s ...  5s
btrfs/037 3s ...  3s
btrfs/038 3s ...  2s
btrfs/041 3s ...  3s
btrfs/062 41s ...  41s
btrfs/063 22s ...  23s
btrfs/067 39s ...  40s
btrfs/068 14s ...  14s
btrfs/070	[not run] btrfs and this test needs 5 or more disks in SCRATCH_DEV_POOL
btrfs/071	[not run] btrfs and this test needs 5 or more disks in SCRATCH_DEV_POOL
btrfs/072 39s ...  41s
btrfs/073 20s ...  20s
btrfs/074 41s ...  41s
btrfs/076 3s ...  3s
btrfs/103 3s ...  3s
btrfs/106 3s ...  3s
btrfs/109 3s ...  3s
btrfs/113 3s ...  3s
btrfs/138 53s ...  54s
btrfs/149 3s ...  3s
btrfs/183 3s ...  2s
btrfs/205 4s ...  3s
btrfs/234 4s ...  4s
btrfs/246 2s ...  3s
btrfs/251 3s ...  2s
Ran: btrfs/024 btrfs/026 btrfs/037 btrfs/038 btrfs/041 btrfs/062 btrfs/063 btrfs/067 btrfs/068 btrfs/070 btrfs/071 btrfs/072 btrfs/073 btrfs/074 btrfs/076 btrfs/103 btrfs/106 btrfs/109 btrfs/113 btrfs/138 btrfs/149 btrfs/183 btrfs/205 btrfs/234 btrfs/246 btrfs/251
Not run: btrfs/070 btrfs/071
Passed all 26 tests

Fabio M. De Francesco (3):
  btrfs: Convert zlib_decompress_bio() to use kmap_local_page()
  btrfs: Use kmap_local_page() on "out_page" in zlib_compress_pages()
  btrfs: Use kmap_local_page() on "in_page" in zlib_compress_pages()

 fs/btrfs/zlib.c | 91 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 67 insertions(+), 24 deletions(-)

-- 
2.36.1

[RFC PATCH v2 1/3] btrfs: Convert zlib_decompress_bio() to use kmap_local_page()

[Fabio M. De Francesco]

The use of kmap() is being deprecated in favor of kmap_local_page(). With
kmap_local_page(), the mapping is per thread, CPU local and not globally
visible.

Therefore, use kmap_local_page() / kunmap_local() in zlib_decompress_bio()
because in this function the mappings are per 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>
---
 fs/btrfs/zlib.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index 767a0c6c9694..770c4c6bbaef 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -287,7 +287,7 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 	unsigned long buf_start;
 	struct page **pages_in = cb->compressed_pages;
 
-	data_in = kmap(pages_in[page_in_index]);
+	data_in = kmap_local_page(pages_in[page_in_index]);
 	workspace->strm.next_in = data_in;
 	workspace->strm.avail_in = min_t(size_t, srclen, PAGE_SIZE);
 	workspace->strm.total_in = 0;
@@ -309,7 +309,7 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 
 	if (Z_OK != zlib_inflateInit2(&workspace->strm, wbits)) {
 		pr_warn("BTRFS: inflateInit failed\n");
-		kunmap(pages_in[page_in_index]);
+		kunmap_local(data_in);
 		return -EIO;
 	}
 	while (workspace->strm.total_in < srclen) {
@@ -336,13 +336,13 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 
 		if (workspace->strm.avail_in == 0) {
 			unsigned long tmp;
-			kunmap(pages_in[page_in_index]);
+			kunmap_local(data_in);
 			page_in_index++;
 			if (page_in_index >= total_pages_in) {
 				data_in = NULL;
 				break;
 			}
-			data_in = kmap(pages_in[page_in_index]);
+			data_in = kmap_local_page(pages_in[page_in_index]);
 			workspace->strm.next_in = data_in;
 			tmp = srclen - workspace->strm.total_in;
 			workspace->strm.avail_in = min(tmp, PAGE_SIZE);
@@ -355,7 +355,7 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 done:
 	zlib_inflateEnd(&workspace->strm);
 	if (data_in)
-		kunmap(pages_in[page_in_index]);
+		kunmap_local(data_in);
 	if (!ret)
 		zero_fill_bio(cb->orig_bio);
 	return ret;
-- 
2.36.1

[RFC PATCH v2 2/3] btrfs: Use kmap_local_page() on "out_page" in zlib_compress_pages()

[Fabio M. De Francesco]

The use of kmap() is being deprecated in favor of kmap_local_page(). With
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 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>
---
 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 address_space *mapping,
 {
 	struct workspace *workspace = list_entry(ws, struct workspace, list);
 	int ret;
-	char *data_in;
-	char *cpage_out;
+	char *data_in = NULL;
+	char *cpage_out = NULL;
 	int nr_pages = 0;
 	struct page *in_page = NULL;
 	struct page *out_page = NULL;
@@ -126,7 +126,7 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping,
 		ret = -ENOMEM;
 		goto out;
 	}
-	cpage_out = kmap(out_page);
+	cpage_out = kmap_local_page(out_page);
 	pages[0] = out_page;
 	nr_pages = 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 == 0) {
-			kunmap(out_page);
+			kunmap_local(cpage_out);
+			cpage_out = NULL;
 			if (nr_pages == nr_dest_pages) {
 				out_page = NULL;
 				ret = -E2BIG;
@@ -207,7 +208,7 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping,
 				ret = -ENOMEM;
 				goto out;
 			}
-			cpage_out = kmap(out_page);
+			cpage_out = kmap_local_page(out_page);
 			pages[nr_pages] = out_page;
 			nr_pages++;
 			workspace->strm.avail_out = 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 == 0) {
 			/* get another page for the stream end */
-			kunmap(out_page);
+			kunmap_local(cpage_out);
+			cpage_out = NULL;
 			if (nr_pages == nr_dest_pages) {
 				out_page = NULL;
 				ret = -E2BIG;
@@ -245,7 +247,7 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping,
 				ret = -ENOMEM;
 				goto out;
 			}
-			cpage_out = kmap(out_page);
+			cpage_out = kmap_local_page(out_page);
 			pages[nr_pages] = out_page;
 			nr_pages++;
 			workspace->strm.avail_out = PAGE_SIZE;
@@ -264,8 +266,8 @@ int zlib_compress_pages(struct list_head *ws, struct address_space *mapping,
 	*total_in = workspace->strm.total_in;
 out:
 	*out_pages = nr_pages;
-	if (out_page)
-		kunmap(out_page);
+	if (cpage_out)
+		kunmap_local(cpage_out);
 
 	if (in_page) {
 		kunmap(in_page);
-- 
2.36.1

[RFC PATCH v2 3/3] btrfs: Use kmap_local_page() on "in_page" in zlib_compress_pages()

[Fabio M. De Francesco]

The use of kmap() is being deprecated in favor of kmap_local_page(). With
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 thread
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.

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

Re: [RFC PATCH v2 1/3] btrfs: Convert zlib_decompress_bio() to use kmap_local_page()

[Qu Wenruo]

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

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

Thanks,
Qu
> ---
>   fs/btrfs/zlib.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
> index 767a0c6c9694..770c4c6bbaef 100644
> --- a/fs/btrfs/zlib.c
> +++ b/fs/btrfs/zlib.c
> @@ -287,7 +287,7 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
>   	unsigned long buf_start;
>   	struct page **pages_in = cb->compressed_pages;
>
> -	data_in = kmap(pages_in[page_in_index]);
> +	data_in = kmap_local_page(pages_in[page_in_index]);
>   	workspace->strm.next_in = data_in;
>   	workspace->strm.avail_in = min_t(size_t, srclen, PAGE_SIZE);
>   	workspace->strm.total_in = 0;
> @@ -309,7 +309,7 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
>
>   	if (Z_OK != zlib_inflateInit2(&workspace->strm, wbits)) {
>   		pr_warn("BTRFS: inflateInit failed\n");
> -		kunmap(pages_in[page_in_index]);
> +		kunmap_local(data_in);
>   		return -EIO;
>   	}
>   	while (workspace->strm.total_in < srclen) {
> @@ -336,13 +336,13 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
>
>   		if (workspace->strm.avail_in == 0) {
>   			unsigned long tmp;
> -			kunmap(pages_in[page_in_index]);
> +			kunmap_local(data_in);
>   			page_in_index++;
>   			if (page_in_index >= total_pages_in) {
>   				data_in = NULL;
>   				break;
>   			}
> -			data_in = kmap(pages_in[page_in_index]);
> +			data_in = kmap_local_page(pages_in[page_in_index]);
>   			workspace->strm.next_in = data_in;
>   			tmp = srclen - workspace->strm.total_in;
>   			workspace->strm.avail_in = min(tmp, PAGE_SIZE);
> @@ -355,7 +355,7 @@ int zlib_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
>   done:
>   	zlib_inflateEnd(&workspace->strm);
>   	if (data_in)
> -		kunmap(pages_in[page_in_index]);
> +		kunmap_local(data_in);
>   	if (!ret)
>   		zero_fill_bio(cb->orig_bio);
>   	return ret;

Re: [RFC PATCH v2 2/3] btrfs: Use kmap_local_page() on "out_page" in zlib_compress_pages()

[Qu Wenruo]

On 2022/6/17 20:05, Fabio M. De Francesco wrote:
> The use of kmap() is being deprecated in favor of kmap_local_page(). With
> 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 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>

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

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

Thanks,
Qu

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

Re: [RFC PATCH v2 3/3] btrfs: Use kmap_local_page() on "in_page" in zlib_compress_pages()

[Qu Wenruo]

On 2022/6/17 20:05, Fabio M. De Francesco wrote:
> The use of kmap() is being deprecated in favor of kmap_local_page(). With
> 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 thread
> 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 address_space *mapping,
>   	int ret;
>   	char *data_in = NULL;
>   	char *cpage_out = NULL;
> +	char mstack[2];
> +	int sind = 0;
>   	int nr_pages = 0;
>   	struct page *in_page = NULL;
>   	struct page *out_page = NULL;
> @@ -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.

Thanks,
Qu

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

Re: [RFC PATCH v2 3/3] btrfs: Use kmap_local_page() on "in_page" in zlib_compress_pages()

[David Sterba]

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.

Re: [RFC PATCH v2 1/3] btrfs: Convert zlib_decompress_bio() to use kmap_local_page()

[Fabio M. De Francesco]

On venerdì 17 giugno 2022 14:51:03 CEST Qu Wenruo wrote:
> 
> On 2022/6/17 20:05, Fabio M. De Francesco wrote:
> > The use of kmap() is being deprecated in favor of kmap_local_page(). 
With
> > kmap_local_page(), the mapping is per thread, CPU local and not 
globally
> > visible.
> >
> > Therefore, use kmap_local_page() / kunmap_local() in 
zlib_decompress_bio()
> > because in this function the mappings are per 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>
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 

Thanks for your review.
I'll forward your tag on to the "real" patch.

Fabio

> Thanks,
> Qu
> > ---
> >   fs/btrfs/zlib.c | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
> > index 767a0c6c9694..770c4c6bbaef 100644
> > --- a/fs/btrfs/zlib.c
> > +++ b/fs/btrfs/zlib.c
> > @@ -287,7 +287,7 @@ int zlib_decompress_bio(struct list_head *ws, 
struct compressed_bio *cb)
> >   	unsigned long buf_start;
> >   	struct page **pages_in = cb->compressed_pages;
> >
> > -	data_in = kmap(pages_in[page_in_index]);
> > +	data_in = kmap_local_page(pages_in[page_in_index]);
> >   	workspace->strm.next_in = data_in;
> >   	workspace->strm.avail_in = min_t(size_t, srclen, PAGE_SIZE);
> >   	workspace->strm.total_in = 0;
> > @@ -309,7 +309,7 @@ int zlib_decompress_bio(struct list_head *ws, 
struct compressed_bio *cb)
> >
> >   	if (Z_OK != zlib_inflateInit2(&workspace->strm, wbits)) {
> >   		pr_warn("BTRFS: inflateInit failed\n");
> > -		kunmap(pages_in[page_in_index]);
> > +		kunmap_local(data_in);
> >   		return -EIO;
> >   	}
> >   	while (workspace->strm.total_in < srclen) {
> > @@ -336,13 +336,13 @@ int zlib_decompress_bio(struct list_head *ws, 
struct compressed_bio *cb)
> >
> >   		if (workspace->strm.avail_in == 0) {
> >   			unsigned long tmp;
> > -			kunmap(pages_in[page_in_index]);
> > +			kunmap_local(data_in);
> >   			page_in_index++;
> >   			if (page_in_index >= total_pages_in) {
> >   				data_in = NULL;
> >   				break;
> >   			}
> > -			data_in = kmap(pages_in[page_in_index]);
> > +			data_in = 
kmap_local_page(pages_in[page_in_index]);
> >   			workspace->strm.next_in = data_in;
> >   			tmp = srclen - workspace->strm.total_in;
> >   			workspace->strm.avail_in = min(tmp, 
PAGE_SIZE);
> > @@ -355,7 +355,7 @@ int zlib_decompress_bio(struct list_head *ws, 
struct compressed_bio *cb)
> >   done:
> >   	zlib_inflateEnd(&workspace->strm);
> >   	if (data_in)
> > -		kunmap(pages_in[page_in_index]);
> > +		kunmap_local(data_in);
> >   	if (!ret)
> >   		zero_fill_bio(cb->orig_bio);
> >   	return ret;
> 

Re: [RFC PATCH v2 2/3] btrfs: Use kmap_local_page() on "out_page" in zlib_compress_pages()

[Fabio M. De Francesco]

On venerdì 17 giugno 2022 14:54:22 CEST Qu Wenruo wrote:
> 
> On 2022/6/17 20:05, Fabio M. De Francesco wrote:
> > The use of kmap() is being deprecated in favor of kmap_local_page(). 
With
> > 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 
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>
> 
> Looks good to me.
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 

Again, thanks.

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

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.
> 
> > ---
> >   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 
address_space *mapping,
> >   {
> >   	struct workspace *workspace = list_entry(ws, struct workspace, 
list);
> >   	int ret;
> > -	char *data_in;
> > -	char *cpage_out;
> > +	char *data_in = NULL;
> 
> I didn't see any diff touching @data_in, any idea why it's initialized
> to NULL?
> 

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

Fabio

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

Re: [RFC PATCH v2 3/3] btrfs: Use kmap_local_page() on "in_page" in zlib_compress_pages()

[Fabio M. De Francesco]

On venerdì 17 giugno 2022 15:09:47 CEST Qu Wenruo wrote:
> 
> On 2022/6/17 20:05, Fabio M. De Francesco wrote:
> > The use of kmap() is being deprecated in favor of kmap_local_page(). 
With
> > 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 
thread
> > 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?
> 

Actually, I'm not sure about what to add. I thought that whoever need more 
information about LIFO mappings / un-mappings can look at highmem.rst. I've 
changed that document and now it contains information on why and how to use 
kmap_local_page() in place of kmap() and kmap_atomic().

Am I misunderstanding what you are trying to say? If so, any specific 
suggestions would be greatly appreciated.

> >
> > 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 
address_space *mapping,
> >   	int ret;
> >   	char *data_in = NULL;
> >   	char *cpage_out = NULL;
> > +	char mstack[2];
> > +	int sind = 0;
> >   	int nr_pages = 0;
> >   	struct page *in_page = NULL;
> >   	struct page *out_page = NULL;
> > @@ -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.
> 
As I said in a recent email, I'm relatively new to kernel development, 
especially to Btrfs and other filesystems.

However, I noted that this code does different handling depending on how 
many "in_page" is going to map. I am not able to say why...

>
> 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 for any help you can provide, but let me be clear about what my goal 
is. I've been assigned with the task to convert kmap() (and possibly also 
kmap_atomic()) to kmap_local_page() wherever it can be done across the 
entire kernel. 

Furthermore, wherever it cannot be done with the API we already have, 
changes to the API will be required. One small change has already been 
carried out upon David's suggestion to make kunmap_local() to take pointers 
to const void. However I'm also talking of much larger changes, if they are 
needed. 

This is to say that I cannot spend too much on Btrfs. There is lot of work 
to be done in other subsystems where I don't yet know which kinds of 
difficulties I'm going to find out.

Any help with clean-ups / refactoring of zlib_compress_pages() will be much 
appreciated for the reasons I've tried to convey in the paragraphs above.

Thank you so much,

Fabio 

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

Re: [RFC PATCH v2 3/3] btrfs: Use kmap_local_page() on "in_page" in zlib_compress_pages()

[Fabio M. De Francesco]

On venerdì 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, 
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.
> 
I'm afraid that cleaning up that loop won't be enough.

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

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

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

Thanks so much,

Fabio

Re: [RFC PATCH v2 3/3] btrfs: Use kmap_local_page() on "in_page" in zlib_compress_pages()

[Qu Wenruo]

On 2022/6/18 02:13, Fabio M. De Francesco wrote:
> On venerdì 17 giugno 2022 15:09:47 CEST Qu Wenruo wrote:
>>
>> On 2022/6/17 20:05, Fabio M. De Francesco wrote:
>>> The use of kmap() is being deprecated in favor of kmap_local_page().
> With
>>> 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
> thread
>>> 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?
>>
>
> Actually, I'm not sure about what to add. I thought that whoever need more
> information about LIFO mappings / un-mappings can look at highmem.rst. I've
> changed that document and now it contains information on why and how to use
> kmap_local_page() in place of kmap() and kmap_atomic().
>
> Am I misunderstanding what you are trying to say? If so, any specific
> suggestions would be greatly appreciated.

Thanks for pointing to the doc, and that doc is enough to answer my
question.

>
>>>
>>> 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
> address_space *mapping,
>>>    	int ret;
>>>    	char *data_in = NULL;
>>>    	char *cpage_out = NULL;
>>> +	char mstack[2];
>>> +	int sind = 0;
>>>    	int nr_pages = 0;
>>>    	struct page *in_page = NULL;
>>>    	struct page *out_page = NULL;
>>> @@ -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.
>>
> As I said in a recent email, I'm relatively new to kernel development,
> especially to Btrfs and other filesystems.

That's not a big deal, that's why we're here to provide help.

>
> However, I noted that this code does different handling depending on how
> many "in_page" is going to map. I am not able to say why...

AFAIK the reason is optimization.

The idea is like this, if there are multiple pages left as input, we
copy the pages from page cache into the workspace buffer.

If there is no more than one page left, we use that page from page cache
directly.

I believe that's the problem causing the difficult in converting to
kmap_local_page().

>
>>
>> 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 for any help you can provide, but let me be clear about what my goal
> is. I've been assigned with the task to convert kmap() (and possibly also
> kmap_atomic()) to kmap_local_page() wherever it can be done across the
> entire kernel.
>
> Furthermore, wherever it cannot be done with the API we already have,
> changes to the API will be required. One small change has already been
> carried out upon David's suggestion to make kunmap_local() to take pointers
> to const void. However I'm also talking of much larger changes, if they are
> needed.
>
> This is to say that I cannot spend too much on Btrfs. There is lot of work
> to be done in other subsystems where I don't yet know which kinds of
> difficulties I'm going to find out.
>
> Any help with clean-ups / refactoring of zlib_compress_pages() will be much
> appreciated for the reasons I've tried to convey in the paragraphs above.

I'll send out a cleanup for zlib_compress_pages(), mostly to make the
(strm.avail_in == 0) branch to call kmap() and kunmap() in pairs,
without holding @in_page mapped.

Would that make it easier?

Thanks,
Qu

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

Re: [RFC PATCH v2 3/3] btrfs: Use kmap_local_page() on "in_page" in zlib_compress_pages()

[Fabio M. De Francesco]

On sabato 18 giugno 2022 00:16:15 CEST Qu Wenruo wrote:
> 
> On 2022/6/18 02:13, Fabio M. De Francesco wrote:

[snip]

> 
> Thanks for pointing to the doc, and that doc is enough to answer my
> question.
> 

Well, this confirms that my changes were quite helpful :-)

[snip]

> > As I said in a recent email, I'm relatively new to kernel development,
> > especially to Btrfs and other filesystems.
> 
> That's not a big deal, that's why we're here to provide help.
> 
> >
> > However, I noted that this code does different handling depending 
> > on how many "in_page" is going to map. I am not able to say why...
> 
> AFAIK the reason is optimization.
> 
> The idea is like this, if there are multiple pages left as input, we
> copy the pages from page cache into the workspace buffer.
> 
> If there is no more than one page left, we use that page from page cache
> directly.
> 
> I believe that's the problem causing the difficult in converting to
> kmap_local_page().
> 

[snip]

> 
> I'll send out a cleanup for zlib_compress_pages(), mostly to make the
> (strm.avail_in == 0) branch to call kmap() and kunmap() in pairs,
> without holding @in_page mapped.
> 
> Would that make it easier?
> 

I was doubtful when you asked this question. However, when this morning I 
saw your patch, I soon understood that it would make that task so easy that 
a silly script could do a mechanical conversion.

Thanks so much,

Fabio