[PATCH 0/4] btrfs: Convert more kmaps to kmap_local_page()

[PATCH 0/4] btrfs: Convert more kmaps to kmap_local_page()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

I am submitting these for 5.13.

Further work to remove more kmap() calls in favor of the kmap_local_page() this
series converts those calls which required more than a common pattern which
were covered in my previous series[1].  This is the second of what I hope to be
3 series to fully convert btrfs.  However, the 3rd series is going to be an RFC
because I need to have more eyes on it before I'm sure about what to do.  For
now this series should be good to go for 5.13.

Also this series converts the kmaps in the raid5/6 code which required a fix to
the kmap'ings which was submitted in [2].

Thanks,
Ira

[1] https://lore.kernel.org/lkml/20210210062221.3023586-1-ira.weiny@intel.com/
[2] https://lore.kernel.org/lkml/20210205163943.GD5033@iweiny-DESK2.sc.intel.com/


Ira Weiny (4):
  fs/btrfs: Convert kmap to kmap_local_page() using coccinelle
  fs/btrfs: Convert raid5/6 kmaps to kmap_local_page()
  fs/btrfs: Use kmap_local_page() in __btrfsic_submit_bio()
  fs/btrfs: Convert block context kmap's to kmap_local_page()

 fs/btrfs/check-integrity.c | 12 ++++----
 fs/btrfs/compression.c     |  4 +--
 fs/btrfs/inode.c           |  4 +--
 fs/btrfs/lzo.c             |  9 +++---
 fs/btrfs/raid56.c          | 61 +++++++++++++++++++-------------------
 5 files changed, 44 insertions(+), 46 deletions(-)

-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 1/4] fs/btrfs: Convert kmap to kmap_local_page() using coccinelle

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Use a simple coccinelle script to help convert the most common
kmap()/kunmap() patterns to kmap_local_page()/kunmap_local().

Note that some kmaps which were caught by this script needed to be
handled by hand because of the strict unmapping order of kunmap_local()
so they are not included in this patch.  But this script got us started.

The development of this patch was aided by the follow script:

// <smpl>
// SPDX-License-Identifier: GPL-2.0-only
// Find kmap and replace with kmap_local_page then mark kunmap
//
// Confidence: Low
// Copyright: (C) 2021 Intel Corporation
// URL: http://coccinelle.lip6.fr/

@ catch_all @
expression e, e2;
@@

(
-kmap(e)
+kmap_local_page(e)
)
...
(
-kunmap(...)
+kunmap_local()
)

// </smpl>

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 fs/btrfs/compression.c | 4 ++--
 fs/btrfs/inode.c       | 4 ++--
 fs/btrfs/lzo.c         | 9 ++++-----
 fs/btrfs/raid56.c      | 4 ++--
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index a219dcdb749e..21833a2fe8c6 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -1578,7 +1578,7 @@ static void heuristic_collect_sample(struct inode *inode, u64 start, u64 end,
 	curr_sample_pos = 0;
 	while (index < index_end) {
 		page = find_get_page(inode->i_mapping, index);
-		in_data = kmap(page);
+		in_data = kmap_local_page(page);
 		/* Handle case where the start is not aligned to PAGE_SIZE */
 		i = start % PAGE_SIZE;
 		while (i < PAGE_SIZE - SAMPLING_READ_SIZE) {
@@ -1591,7 +1591,7 @@ static void heuristic_collect_sample(struct inode *inode, u64 start, u64 end,
 			start += SAMPLING_INTERVAL;
 			curr_sample_pos += SAMPLING_READ_SIZE;
 		}
-		kunmap(page);
+		kunmap_local(in_data);
 		put_page(page);
 
 		index++;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 641f21a11722..66c6f1185de2 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6845,7 +6845,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 				if (ret)
 					goto out;
 			} else {
-				map = kmap(page);
+				map = kmap_local_page(page);
 				read_extent_buffer(leaf, map + pg_offset, ptr,
 						   copy_size);
 				if (pg_offset + copy_size < PAGE_SIZE) {
@@ -6853,7 +6853,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 					       PAGE_SIZE - pg_offset -
 					       copy_size);
 				}
-				kunmap(page);
+				kunmap_local(map);
 			}
 			flush_dcache_page(page);
 		}
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index 9084a950dc09..cd042c7567a4 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -118,7 +118,7 @@ int lzo_compress_pages(struct list_head *ws, struct address_space *mapping,
 	struct workspace *workspace = list_entry(ws, struct workspace, list);
 	int ret = 0;
 	char *data_in;
-	char *cpage_out;
+	char *cpage_out, *sizes_ptr;
 	int nr_pages = 0;
 	struct page *in_page = NULL;
 	struct page *out_page = NULL;
@@ -258,10 +258,9 @@ int lzo_compress_pages(struct list_head *ws, struct address_space *mapping,
 	}
 
 	/* store the size of all chunks of compressed data */
-	cpage_out = kmap(pages[0]);
-	write_compress_length(cpage_out, tot_out);
-
-	kunmap(pages[0]);
+	sizes_ptr = kmap_local_page(pages[0]);
+	write_compress_length(sizes_ptr, tot_out);
+	kunmap_local(sizes_ptr);
 
 	ret = 0;
 	*total_out = tot_out;
diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 7c3f6dc918c1..9759fb31b73e 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2391,13 +2391,13 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
 
 		/* Check scrubbing parity and repair it */
 		p = rbio_stripe_page(rbio, rbio->scrubp, pagenr);
-		parity = kmap(p);
+		parity = kmap_local_page(p);
 		if (memcmp(parity, pointers[rbio->scrubp], PAGE_SIZE))
 			copy_page(parity, pointers[rbio->scrubp]);
 		else
 			/* Parity is right, needn't writeback */
 			bitmap_clear(rbio->dbitmap, pagenr, 1);
-		kunmap(p);
+		kunmap_local(parity);
 
 		for (stripe = 0; stripe < nr_data; stripe++)
 			kunmap(page_in_rbio(rbio, stripe, pagenr, 0));
-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 2/4] fs/btrfs: Convert raid5/6 kmaps to kmap_local_page()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

These kmaps are thread local and don't need to be atomic.  So they can use
the more efficient kmap_local_page().  However, the mapping of pages in
the stripes and the additional parity and qstripe pages are a bit
trickier because the unmapping must occur in the opposite order from the
mapping.  Furthermore, the pointer array in __raid_recover_end_io() may
get reordered.

Convert these calls to kmap_local_page() taking care to reverse the
unmappings of any page arrays as well as being careful with the mappings
of any special pages such as the parity and qstripe pages.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

---
This patch depends on the fix to raid5/6 kmapping sent previously

https://lore.kernel.org/lkml/20210205163943.GD5033@iweiny-DESK2.sc.intel.com/#t
---
 fs/btrfs/raid56.c | 57 +++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 9759fb31b73e..04abae305582 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -1233,13 +1233,13 @@ static noinline void finish_rmw(struct btrfs_raid_bio *rbio)
 		/* first collect one page from each data stripe */
 		for (stripe = 0; stripe < nr_data; stripe++) {
 			p = page_in_rbio(rbio, stripe, pagenr, 0);
-			pointers[stripe] = kmap(p);
+			pointers[stripe] = kmap_local_page(p);
 		}
 
 		/* then add the parity stripe */
 		p = rbio_pstripe_page(rbio, pagenr);
 		SetPageUptodate(p);
-		pointers[stripe++] = kmap(p);
+		pointers[stripe++] = kmap_local_page(p);
 
 		if (has_qstripe) {
 
@@ -1249,7 +1249,7 @@ static noinline void finish_rmw(struct btrfs_raid_bio *rbio)
 			 */
 			p = rbio_qstripe_page(rbio, pagenr);
 			SetPageUptodate(p);
-			pointers[stripe++] = kmap(p);
+			pointers[stripe++] = kmap_local_page(p);
 
 			raid6_call.gen_syndrome(rbio->real_stripes, PAGE_SIZE,
 						pointers);
@@ -1258,10 +1258,8 @@ static noinline void finish_rmw(struct btrfs_raid_bio *rbio)
 			copy_page(pointers[nr_data], pointers[0]);
 			run_xor(pointers + 1, nr_data - 1, PAGE_SIZE);
 		}
-
-
-		for (stripe = 0; stripe < rbio->real_stripes; stripe++)
-			kunmap(page_in_rbio(rbio, stripe, pagenr, 0));
+		for (stripe = stripe - 1; stripe >= 0; stripe--)
+			kunmap_local(pointers[stripe]);
 	}
 
 	/*
@@ -1780,6 +1778,7 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
 {
 	int pagenr, stripe;
 	void **pointers;
+	void **unmap_array;
 	int faila = -1, failb = -1;
 	struct page *page;
 	blk_status_t err;
@@ -1791,6 +1790,12 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
 		goto cleanup_io;
 	}
 
+	unmap_array = kcalloc(rbio->real_stripes, sizeof(void *), GFP_NOFS);
+	if (!unmap_array) {
+		err = BLK_STS_RESOURCE;
+		goto cleanup_pointers;
+	}
+
 	faila = rbio->faila;
 	failb = rbio->failb;
 
@@ -1814,6 +1819,9 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
 
 		/* setup our array of pointers with pages
 		 * from each stripe
+		 *
+		 * NOTE Store a duplicate array of pointers to preserve the
+		 * pointer order.
 		 */
 		for (stripe = 0; stripe < rbio->real_stripes; stripe++) {
 			/*
@@ -1827,7 +1835,8 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
 			} else {
 				page = rbio_stripe_page(rbio, stripe, pagenr);
 			}
-			pointers[stripe] = kmap(page);
+			pointers[stripe] = kmap_local_page(page);
+			unmap_array[stripe] = pointers[stripe];
 		}
 
 		/* all raid6 handling here */
@@ -1920,24 +1929,14 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
 				}
 			}
 		}
-		for (stripe = 0; stripe < rbio->real_stripes; stripe++) {
-			/*
-			 * if we're rebuilding a read, we have to use
-			 * pages from the bio list
-			 */
-			if ((rbio->operation == BTRFS_RBIO_READ_REBUILD ||
-			     rbio->operation == BTRFS_RBIO_REBUILD_MISSING) &&
-			    (stripe == faila || stripe == failb)) {
-				page = page_in_rbio(rbio, stripe, pagenr, 0);
-			} else {
-				page = rbio_stripe_page(rbio, stripe, pagenr);
-			}
-			kunmap(page);
-		}
+		for (stripe = rbio->real_stripes - 1; stripe >= 0; stripe--)
+			kunmap_local(unmap_array[stripe]);
 	}
 
 	err = BLK_STS_OK;
 cleanup:
+	kfree(unmap_array);
+cleanup_pointers:
 	kfree(pointers);
 
 cleanup_io:
@@ -2362,13 +2361,13 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
 			goto cleanup;
 		}
 		SetPageUptodate(q_page);
-		pointers[rbio->real_stripes - 1] = kmap(q_page);
+		pointers[rbio->real_stripes - 1] = kmap_local_page(q_page);
 	}
 
 	atomic_set(&rbio->error, 0);
 
 	/* map the parity stripe just once */
-	pointers[nr_data] = kmap(p_page);
+	pointers[nr_data] = kmap_local_page(p_page);
 
 	for_each_set_bit(pagenr, rbio->dbitmap, rbio->stripe_npages) {
 		struct page *p;
@@ -2376,7 +2375,7 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
 		/* first collect one page from each data stripe */
 		for (stripe = 0; stripe < nr_data; stripe++) {
 			p = page_in_rbio(rbio, stripe, pagenr, 0);
-			pointers[stripe] = kmap(p);
+			pointers[stripe] = kmap_local_page(p);
 		}
 
 		if (has_qstripe) {
@@ -2399,14 +2398,14 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
 			bitmap_clear(rbio->dbitmap, pagenr, 1);
 		kunmap_local(parity);
 
-		for (stripe = 0; stripe < nr_data; stripe++)
-			kunmap(page_in_rbio(rbio, stripe, pagenr, 0));
+		for (stripe = nr_data - 1; stripe >= 0; stripe--)
+			kunmap_local(pointers[stripe]);
 	}
 
-	kunmap(p_page);
+	kunmap_local(pointers[nr_data]);
 	__free_page(p_page);
 	if (q_page) {
-		kunmap(q_page);
+		kunmap_local(pointers[rbio->real_stripes - 1]);
 		__free_page(q_page);
 	}
 
-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 3/4] fs/btrfs: Use kmap_local_page() in __btrfsic_submit_bio()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

Again there is an array of pointers which must be unmapped in the correct
order.

Convert the kmap()'s to kmap_local_page() and adjust the unmapping
to work backwards through the unmapping loop.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 fs/btrfs/check-integrity.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
index 6ff44e53814c..726eb894be8b 100644
--- a/fs/btrfs/check-integrity.c
+++ b/fs/btrfs/check-integrity.c
@@ -2677,7 +2677,7 @@ static void __btrfsic_submit_bio(struct bio *bio)
 	dev_state = btrfsic_dev_state_lookup(bio_dev(bio) + bio->bi_partno);
 	if (NULL != dev_state &&
 	    (bio_op(bio) == REQ_OP_WRITE) && bio_has_data(bio)) {
-		unsigned int i = 0;
+		int i = 0;
 		u64 dev_bytenr;
 		u64 cur_bytenr;
 		struct bio_vec bvec;
@@ -2702,7 +2702,7 @@ static void __btrfsic_submit_bio(struct bio *bio)
 
 		bio_for_each_segment(bvec, bio, iter) {
 			BUG_ON(bvec.bv_len != PAGE_SIZE);
-			mapped_datav[i] = kmap(bvec.bv_page);
+			mapped_datav[i] = kmap_local_page(bvec.bv_page);
 			i++;
 
 			if (dev_state->state->print_mask &
@@ -2715,8 +2715,8 @@ static void __btrfsic_submit_bio(struct bio *bio)
 					      mapped_datav, segs,
 					      bio, &bio_is_patched,
 					      bio->bi_opf);
-		bio_for_each_segment(bvec, bio, iter)
-			kunmap(bvec.bv_page);
+		for (--i; i >= 0; i--)
+			kunmap_local(mapped_datav[i]);
 		kfree(mapped_datav);
 	} else if (NULL != dev_state && (bio->bi_opf & REQ_PREFLUSH)) {
 		if (dev_state->state->print_mask &
-- 
2.28.0.rc0.12.gb6a658bd00c9

[PATCH 4/4] fs/btrfs: Convert block context kmap's to kmap_local_page()

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

btrfsic_read_block() (which calls kmap()) and
btrfsic_release_block_ctx() (which calls kunmap()) are always called
within a single thread of execution.

Therefore the mappings created within these calls can be a thread local
mapping.

Convert the kmap() of bloc_ctx->pagev to kmap_local_page().  Luckily the
unmap loops backwards through the array pointer so no adjustment needs
to be made to the unmapping order.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
 fs/btrfs/check-integrity.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
index 726eb894be8b..1ff9e19508a7 100644
--- a/fs/btrfs/check-integrity.c
+++ b/fs/btrfs/check-integrity.c
@@ -1558,7 +1558,7 @@ static void btrfsic_release_block_ctx(struct btrfsic_block_data_ctx *block_ctx)
 		while (num_pages > 0) {
 			num_pages--;
 			if (block_ctx->datav[num_pages]) {
-				kunmap(block_ctx->pagev[num_pages]);
+				kunmap_local(block_ctx->datav[num_pages]);
 				block_ctx->datav[num_pages] = NULL;
 			}
 			if (block_ctx->pagev[num_pages]) {
@@ -1637,7 +1637,7 @@ static int btrfsic_read_block(struct btrfsic_state *state,
 		i = j;
 	}
 	for (i = 0; i < num_pages; i++)
-		block_ctx->datav[i] = kmap(block_ctx->pagev[i]);
+		block_ctx->datav[i] = kmap_local_page(block_ctx->pagev[i]);
 
 	return block_ctx->len;
 }
-- 
2.28.0.rc0.12.gb6a658bd00c9

Re: [PATCH 0/4] btrfs: Convert more kmaps to kmap_local_page()

[Ira Weiny]

On Tue, Feb 16, 2021 at 06:48:22PM -0800, 'Ira Weiny' wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> I am submitting these for 5.13.

Just a friendly ping on this set.

Ira

> 
> Further work to remove more kmap() calls in favor of the kmap_local_page() this
> series converts those calls which required more than a common pattern which
> were covered in my previous series[1].  This is the second of what I hope to be
> 3 series to fully convert btrfs.  However, the 3rd series is going to be an RFC
> because I need to have more eyes on it before I'm sure about what to do.  For
> now this series should be good to go for 5.13.
> 
> Also this series converts the kmaps in the raid5/6 code which required a fix to
> the kmap'ings which was submitted in [2].
> 
> Thanks,
> Ira
> 
> [1] https://lore.kernel.org/lkml/20210210062221.3023586-1-ira.weiny@intel.com/
> [2] https://lore.kernel.org/lkml/20210205163943.GD5033@iweiny-DESK2.sc.intel.com/
> 
> 
> Ira Weiny (4):
>   fs/btrfs: Convert kmap to kmap_local_page() using coccinelle
>   fs/btrfs: Convert raid5/6 kmaps to kmap_local_page()
>   fs/btrfs: Use kmap_local_page() in __btrfsic_submit_bio()
>   fs/btrfs: Convert block context kmap's to kmap_local_page()
> 
>  fs/btrfs/check-integrity.c | 12 ++++----
>  fs/btrfs/compression.c     |  4 +--
>  fs/btrfs/inode.c           |  4 +--
>  fs/btrfs/lzo.c             |  9 +++---
>  fs/btrfs/raid56.c          | 61 +++++++++++++++++++-------------------
>  5 files changed, 44 insertions(+), 46 deletions(-)
> 
> -- 
> 2.28.0.rc0.12.gb6a658bd00c9
> 

Re: [PATCH 1/4] fs/btrfs: Convert kmap to kmap_local_page() using coccinelle

[David Sterba]

On Tue, Feb 16, 2021 at 06:48:23PM -0800, ira.weiny@intel.com wrote:
> --- a/fs/btrfs/lzo.c
> +++ b/fs/btrfs/lzo.c
> @@ -118,7 +118,7 @@ int lzo_compress_pages(struct list_head *ws, struct address_space *mapping,
>  	struct workspace *workspace = list_entry(ws, struct workspace, list);
>  	int ret = 0;
>  	char *data_in;
> -	char *cpage_out;
> +	char *cpage_out, *sizes_ptr;
>  	int nr_pages = 0;
>  	struct page *in_page = NULL;
>  	struct page *out_page = NULL;
> @@ -258,10 +258,9 @@ int lzo_compress_pages(struct list_head *ws, struct address_space *mapping,
>  	}
>  
>  	/* store the size of all chunks of compressed data */
> -	cpage_out = kmap(pages[0]);
> -	write_compress_length(cpage_out, tot_out);
> -
> -	kunmap(pages[0]);
> +	sizes_ptr = kmap_local_page(pages[0]);
> +	write_compress_length(sizes_ptr, tot_out);
> +	kunmap_local(sizes_ptr);

Why is not cpage_out reused for this mapping? I don't see any reason for
another temporary variable, cpage_out is not used at this point.

Re: [PATCH 2/4] fs/btrfs: Convert raid5/6 kmaps to kmap_local_page()

[David Sterba]

On Tue, Feb 16, 2021 at 06:48:24PM -0800, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> These kmaps are thread local and don't need to be atomic.  So they can use
> the more efficient kmap_local_page().  However, the mapping of pages in
> the stripes and the additional parity and qstripe pages are a bit
> trickier because the unmapping must occur in the opposite order from the
> mapping.  Furthermore, the pointer array in __raid_recover_end_io() may
> get reordered.
> 
> Convert these calls to kmap_local_page() taking care to reverse the
> unmappings of any page arrays as well as being careful with the mappings
> of any special pages such as the parity and qstripe pages.

Though there's one more allocation for the additional array, I don't see
a simpler way to avoid it and track the array reordering at a lower
memory cost. At least it's not in a performance critical code and the
array size is reasonably small.

Re: [PATCH 0/4] btrfs: Convert more kmaps to kmap_local_page()

[David Sterba]

On Tue, Feb 16, 2021 at 06:48:22PM -0800, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> I am submitting these for 5.13.
> 
> Further work to remove more kmap() calls in favor of the kmap_local_page() this
> series converts those calls which required more than a common pattern which
> were covered in my previous series[1].  This is the second of what I hope to be
> 3 series to fully convert btrfs.  However, the 3rd series is going to be an RFC
> because I need to have more eyes on it before I'm sure about what to do.  For
> now this series should be good to go for 5.13.
> 
> Also this series converts the kmaps in the raid5/6 code which required a fix to
> the kmap'ings which was submitted in [2].

Branch added to for-next and will be moved to the devel queue next week.
I've added some comments about the ordering requirement, that's
something not obvious. There's a comment under 1st patch but that's
trivial to fix if needed. Thanks.

Re: [PATCH 1/4] fs/btrfs: Convert kmap to kmap_local_page() using coccinelle

[Ira Weiny]

On Fri, Mar 12, 2021 at 07:58:39PM +0100, David Sterba wrote:
> On Tue, Feb 16, 2021 at 06:48:23PM -0800, ira.weiny@intel.com wrote:
> > --- a/fs/btrfs/lzo.c
> > +++ b/fs/btrfs/lzo.c
> > @@ -118,7 +118,7 @@ int lzo_compress_pages(struct list_head *ws, struct address_space *mapping,
> >  	struct workspace *workspace = list_entry(ws, struct workspace, list);
> >  	int ret = 0;
> >  	char *data_in;
> > -	char *cpage_out;
> > +	char *cpage_out, *sizes_ptr;
> >  	int nr_pages = 0;
> >  	struct page *in_page = NULL;
> >  	struct page *out_page = NULL;
> > @@ -258,10 +258,9 @@ int lzo_compress_pages(struct list_head *ws, struct address_space *mapping,
> >  	}
> >  
> >  	/* store the size of all chunks of compressed data */
> > -	cpage_out = kmap(pages[0]);
> > -	write_compress_length(cpage_out, tot_out);
> > -
> > -	kunmap(pages[0]);
> > +	sizes_ptr = kmap_local_page(pages[0]);
> > +	write_compress_length(sizes_ptr, tot_out);
> > +	kunmap_local(sizes_ptr);
> 
> Why is not cpage_out reused for this mapping? I don't see any reason for
> another temporary variable, cpage_out is not used at this point.

For this patch that is true.  However, I'm trying to convert the other kmaps as
well.  To do that I'll need cpage_out preserved for the final kunmap_local().

Unfortunately, the required nesting ordering of kmap_local_page() makes
converting the other calls hacky at best.  I'm not sure what to do about them.
The best I've come up with is doing a hacky extra unmap/remap to preserve the
nesting.

Anyway, I'd prefer to leave this additional temp variable but I can certainly
change if it you want.  The other conversions may never work/land.  :-/

Ira

Re: [PATCH 0/4] btrfs: Convert more kmaps to kmap_local_page()

[Ira Weiny]

On Fri, Mar 12, 2021 at 08:41:41PM +0100, David Sterba wrote:
> On Tue, Feb 16, 2021 at 06:48:22PM -0800, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > I am submitting these for 5.13.
> > 
> > Further work to remove more kmap() calls in favor of the kmap_local_page() this
> > series converts those calls which required more than a common pattern which
> > were covered in my previous series[1].  This is the second of what I hope to be
> > 3 series to fully convert btrfs.  However, the 3rd series is going to be an RFC
> > because I need to have more eyes on it before I'm sure about what to do.  For
> > now this series should be good to go for 5.13.
> > 
> > Also this series converts the kmaps in the raid5/6 code which required a fix to
> > the kmap'ings which was submitted in [2].
> 
> Branch added to for-next and will be moved to the devel queue next week.
> I've added some comments about the ordering requirement, that's
> something not obvious. There's a comment under 1st patch but that's
> trivial to fix if needed. Thanks.

I've replied to the first patch.  LMK if you want me to respin it.

Thanks!
Ira

Re: [PATCH 1/4] fs/btrfs: Convert kmap to kmap_local_page() using coccinelle

[David Sterba]

On Fri, Mar 12, 2021 at 12:03:14PM -0800, Ira Weiny wrote:
> On Fri, Mar 12, 2021 at 07:58:39PM +0100, David Sterba wrote:
> > On Tue, Feb 16, 2021 at 06:48:23PM -0800, ira.weiny@intel.com wrote:
> > > --- a/fs/btrfs/lzo.c
> > > +++ b/fs/btrfs/lzo.c
> > > @@ -118,7 +118,7 @@ int lzo_compress_pages(struct list_head *ws, struct address_space *mapping,
> > >  	struct workspace *workspace = list_entry(ws, struct workspace, list);
> > >  	int ret = 0;
> > >  	char *data_in;
> > > -	char *cpage_out;
> > > +	char *cpage_out, *sizes_ptr;
> > >  	int nr_pages = 0;
> > >  	struct page *in_page = NULL;
> > >  	struct page *out_page = NULL;
> > > @@ -258,10 +258,9 @@ int lzo_compress_pages(struct list_head *ws, struct address_space *mapping,
> > >  	}
> > >  
> > >  	/* store the size of all chunks of compressed data */
> > > -	cpage_out = kmap(pages[0]);
> > > -	write_compress_length(cpage_out, tot_out);
> > > -
> > > -	kunmap(pages[0]);
> > > +	sizes_ptr = kmap_local_page(pages[0]);
> > > +	write_compress_length(sizes_ptr, tot_out);
> > > +	kunmap_local(sizes_ptr);
> > 
> > Why is not cpage_out reused for this mapping? I don't see any reason for
> > another temporary variable, cpage_out is not used at this point.
> 
> For this patch that is true.  However, I'm trying to convert the other kmaps as
> well.  To do that I'll need cpage_out preserved for the final kunmap_local().
> 
> Unfortunately, the required nesting ordering of kmap_local_page() makes
> converting the other calls hacky at best.  I'm not sure what to do about them.
> The best I've come up with is doing a hacky extra unmap/remap to preserve the
> nesting.
> 
> Anyway, I'd prefer to leave this additional temp variable but I can certainly
> change if it you want.  The other conversions may never work/land.  :-/

Ok, no problem keeping the variable then. I've added a note to changelog
why it's there. The whole conversion sounds tricky so adding trivial
helper code is no big deal.

Re: [PATCH 0/4] btrfs: Convert more kmaps to kmap_local_page()

[David Sterba]

On Fri, Mar 12, 2021 at 12:05:00PM -0800, Ira Weiny wrote:
> On Fri, Mar 12, 2021 at 08:41:41PM +0100, David Sterba wrote:
> > On Tue, Feb 16, 2021 at 06:48:22PM -0800, ira.weiny@intel.com wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > I am submitting these for 5.13.
> > > 
> > > Further work to remove more kmap() calls in favor of the kmap_local_page() this
> > > series converts those calls which required more than a common pattern which
> > > were covered in my previous series[1].  This is the second of what I hope to be
> > > 3 series to fully convert btrfs.  However, the 3rd series is going to be an RFC
> > > because I need to have more eyes on it before I'm sure about what to do.  For
> > > now this series should be good to go for 5.13.
> > > 
> > > Also this series converts the kmaps in the raid5/6 code which required a fix to
> > > the kmap'ings which was submitted in [2].
> > 
> > Branch added to for-next and will be moved to the devel queue next week.
> > I've added some comments about the ordering requirement, that's
> > something not obvious. There's a comment under 1st patch but that's
> > trivial to fix if needed. Thanks.
> 
> I've replied to the first patch.  LMK if you want me to respin it.

No need to respin, patchset now in misc-next. Thanks.

Re: [PATCH 0/4] btrfs: Convert more kmaps to kmap_local_page()

[Ira Weiny]

On Tue, Mar 16, 2021 at 12:07:24PM +0100, David Sterba wrote:
> On Fri, Mar 12, 2021 at 12:05:00PM -0800, Ira Weiny wrote:
> > On Fri, Mar 12, 2021 at 08:41:41PM +0100, David Sterba wrote:
> > > On Tue, Feb 16, 2021 at 06:48:22PM -0800, ira.weiny@intel.com wrote:
> > > > From: Ira Weiny <ira.weiny@intel.com>
> > > > 
> > > > I am submitting these for 5.13.
> > > > 
> > > > Further work to remove more kmap() calls in favor of the kmap_local_page() this
> > > > series converts those calls which required more than a common pattern which
> > > > were covered in my previous series[1].  This is the second of what I hope to be
> > > > 3 series to fully convert btrfs.  However, the 3rd series is going to be an RFC
> > > > because I need to have more eyes on it before I'm sure about what to do.  For
> > > > now this series should be good to go for 5.13.
> > > > 
> > > > Also this series converts the kmaps in the raid5/6 code which required a fix to
> > > > the kmap'ings which was submitted in [2].
> > > 
> > > Branch added to for-next and will be moved to the devel queue next week.
> > > I've added some comments about the ordering requirement, that's
> > > something not obvious. There's a comment under 1st patch but that's
> > > trivial to fix if needed. Thanks.
> > 
> > I've replied to the first patch.  LMK if you want me to respin it.
> 
> No need to respin, patchset now in misc-next. Thanks.

Sweet!  Thanks!
Ira