gfs2: kmap{_atomic} conversion to kmap_local_{page/folio}

This patch series proposes to replace the kmap/kmap_atomic implementation to the preferred kmap_local_* APIs.

The code blocks for this module where kmap/kmap_atomic calls are implemented do not appear to depend on disabling page-faults or preemption. Hence such code blocks are safe for converting to improved kmap_local_{page,folio} APIs.

Note: The proposed patches are build tested only.

Initially, only a single patch was sent and now being converted into a patch series including the other files/functions of this module. Hence all patches, that are included for the first time in this series are also marked as v3.

Changes in v3:
- Patch set introduced to include all gfs2 kmap conversions
- Patches 3/6 through 6/6 are included to build the series
- Initial stand-alone patch split into 2 patches [1/6 and 2/6]

Changes in v2:
- 3/6 to 6/6: None.
- 1/6 + 2/6: Correct patch description for the replacement function name from kmap_local_folio to kmap_local_page

Deepak R Varma (6):
gfs2: Replace kmap_atomic() by kmap_local_page() in stuffed_readpage
gfs2: Replace kmap_atomic()+memcpy by memcpy_from_page()
gfs2: Replace kmap() by kmap_local_page() in gfs2_unstuffer_page
gfs2: Replace kmap_atomic() by kmap_local_page() in lops.c
gfs2: Replace kmap() by kmap_local_page() in gfs2_read_super
gfs2: Replace kmap_atomic() by kmap_local_page() in
gfs2_write_buf_to_page

 fs/gfs2/aops.c       | 13 ++++++-------
 fs/gfs2/bmap.c       |  4 ++--
 fs/gfs2/lops.c       | 12 ++++++------
 fs/gfs2/ops_fstype.c |  4 ++--
 fs/gfs2/quota.c      |  4 ++--
 5 files changed, 18 insertions(+), 19 deletions(-)

2.34.1

kmap_atomic() is deprecated in favor of kmap_local_{folio,page}().

Therefore, replace kmap_atomic() with kmap_local_page() in stuffed_readpage().

kmap_atomic() disables page-faults and preemption (the latter only for !PREEMPT_RT kernels), However, the code within the mapping/un-mapping in stuffed_readpage() does not depend on the above-mentioned side effects.

Therefore, a mere replacement of the old API with the new one is all that is required (i.e., there is no need to explicitly add any calls to pagefault_disable() and/or preempt_disable()).

---
Changes in v3:
- split into 2 patches
- included in the patch set. Was sent as standalone patch previously

Changes in v2:
- Update patch description to correct the replacement function name from kmap_local_page to kmap_local_folio

 fs/gfs2/aops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 3b41542d6697..3eac4f2f5c27 100644
@@ -432,10 +432,10 @@ static int stuffed_readpage(struct gfs2_inode *ip, struct page *page)
 	if (error)
 		return error;
 
-	kaddr = kmap_atomic(page);
+	kaddr = kmap_local_page(page);
 	memcpy(kaddr, dibh->b_data + sizeof(struct gfs2_dinode), dsize);
 	memset(kaddr + dsize, 0, PAGE_SIZE - dsize);
-	kunmap_atomic(kaddr);
+	kunmap_local(kaddr);
 	flush_dcache_page(page);
 	brelse(dibh);
 	SetPageUptodate(page);

2.34.1

It LGTM, therefore, it is...

NIT: I can't understand why you think the previous single patch needed to be split. Despite I can't understand why, I have nothing against it :-)

Thanks,

Fabio

P.S.: Next time please take note somewhere (maybe after the three dashes?) that you are re-using my commit message word by word. I'd appreciate it :-) However, it doesn't really matter much so please _don't_ send a newer patch only for this little request.

kmap_atomic() is deprecated in favor of kmap_local_{folio,page}().

kmap_atomic() disables page-faults and preemption (the latter only for !PREEMPT_RT kernels), However, the code within the mapping/un-mapping in gfs2_internal_read() does not depend on the above-mentioned side effects.

Further, memcpy_{from,to}_page() wrappers combine the
{kmap, unmap}_local_page() blocks when they are intended exclusively to copy contents from/to the temporary mapped page. So, replace the kmap_atomic()/kunmap_automic() block by the memcpy_from_page() API call. This change allows to tidy-up code and also eliminate unused variable p.

---
Changes in v3:
- Split as a separate patch for conversion in gfs2_internal_read()
- Use memcpy_from_page() as suggested by Fabio and Andreas G
- Included split version in patch set

Changes in v2:
- Update patch description to correct the replacement function name from kmap_local_folio to kmap_local _page

 fs/gfs2/aops.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

index 3eac4f2f5c27..f47fed657763 100644
@@ -489,7 +489,6 @@ int gfs2_internal_read(struct gfs2_inode *ip, char *buf, loff_t *pos,
 	unsigned copied = 0;
 	unsigned amt;
 	struct page *page;
-	void *p;
 
 	do {
 		page = read_cache_page(mapping, index, gfs2_read_folio, NULL);

@@ -498,12 +497,12 @@ int gfs2_internal_read(struct gfs2_inode *ip, char *buf, loff_t *pos,
 				continue;
 			return PTR_ERR(page);
 		}
-		p = kmap_atomic(page);
-		amt = size - copied;
 		if (offset + size > PAGE_SIZE)
 			amt = PAGE_SIZE - offset;
-		memcpy(buf + copied, p + offset, amt);
-		kunmap_atomic(p);
+		else
+			amt = size - copied;
+
+		memcpy_from_page(buf, page, offset, amt);
 		put_page(page);
 		copied += amt;
 		index++;

2.34.1

s/kunmap_automic/kunmap_atomic/

I don't like to read the list of the functions replaced by the memcpy_from_page() helper in the subject of the patch. These details are better suited in commit messages.

Furthermore, you have not been consistent: "kmap_atomic()+memcpy" contains parentheses only in one of the two functions. Both or none.

The thing that really matters here is that You are replacing the deprecated kmap_atomic() with kmap_local_page(). This is what the subject should show.

Please cite the use of the suited helper _only_ within the commit message and explain why it is preferred to open code kmap_local_page() + memcpy() + kunmap_local().

The use of kmap() is being deprecated in favor of kmap_local_page().

There are two main problems with kmap(): (1) It comes with an overhead as the mapping space is restricted and protected by a global lock for synchronization and (2) it also requires global TLB invalidation when the kmap?s pool wraps and it might block when the mapping space is fully utilized until a slot becomes available.

With kmap_local_page() the mappings are per thread, CPU local, can take page faults, and can be called from any context (including interrupts). It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, the tasks can be preempted and, when they are scheduled to run again, the kernel virtual addresses are restored and still valid.

Therefore, replace kmap() with kmap_local_page() in gfs2_unstuffer_page().

---
Changes in v3:
- Patch included in the patch series

Changes in v2:
- None

 fs/gfs2/bmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 8d611fbcf0bd..6b850e2ba5c8 100644
@@ -58,12 +58,12 @@ static int gfs2_unstuffer_page(struct gfs2_inode *ip, struct buffer_head *dibh,
 	struct inode *inode = &ip->i_inode;
 
 	if (!PageUptodate(page)) {
-		void *kaddr = kmap(page);
+		void *kaddr = kmap_local_page(page);
 		u64 dsize = i_size_read(inode);
  
 		memcpy(kaddr, dibh->b_data + sizeof(struct gfs2_dinode), dsize);
 		memset(kaddr + dsize, 0, PAGE_SIZE - dsize);
-		kunmap(page);
+		kunmap_local(kaddr);
 
 		SetPageUptodate(page);
 	}

2.34.1

Deepak,

Would you please cite the author of this boiler-plate commit message? I think that you are not required by any stated formal rule, however it would be much appreciated (by me, at least :-)).

kmap_atomic() is deprecated in favor of kmap_local_{folio,page}().

Therefore, replace kmap_atomic() with kmap_local_page() in following functions of lops.c:
- gfs2_jhead_pg_srch()
- gfs2_check_magic()
- gfs2_before_commit()

kmap_atomic() disables page-faults and preemption (the latter only for !PREEMPT_RT kernels), However, the code within the mapping/un-mapping in stuffed_readpage() does not depend on the above-mentioned side effects.

Therefore, a mere replacement of the old API with the new one is all that is required (i.e., there is no need to explicitly add any calls to pagefault_disable() and/or preempt_disable()).

---
Changes in v3:
- Patch included in patch series

Changes in v2:
- None

 fs/gfs2/lops.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

index 1902413d5d12..a7c2296cb3c6 100644
@@ -427,7 +427,7 @@ static bool gfs2_jhead_pg_srch(struct gfs2_jdesc *jd,
 {
 	struct gfs2_sbd *sdp = GFS2_SB(jd->jd_inode);
 	struct gfs2_log_header_host lh;
-	void *kaddr = kmap_atomic(page);
+	void *kaddr = kmap_local_page(page);
 	unsigned int offset;
 	bool ret = false;
 

@@ -441,7 +441,7 @@ static bool gfs2_jhead_pg_srch(struct gfs2_jdesc *jd,
 			}
 		}
 	}
-	kunmap_atomic(kaddr);
+	kunmap_local(kaddr);
 	return ret;
 }
 

@@ -626,11 +626,11 @@ static void gfs2_check_magic(struct buffer_head *bh)
 	__be32 *ptr;
 
 	clear_buffer_escaped(bh);
-	kaddr = kmap_atomic(bh->b_page);
+	kaddr = kmap_local_page(bh->b_page);
 	ptr = kaddr + bh_offset(bh);
 	if (*ptr == cpu_to_be32(GFS2_MAGIC))
 		set_buffer_escaped(bh);
-	kunmap_atomic(kaddr);
+	kunmap_local(kaddr);
 }
 
 static int blocknr_cmp(void *priv, const struct list_head *a,

@@ -699,10 +699,10 @@ static void gfs2_before_commit(struct gfs2_sbd *sdp, unsigned int limit,
 				void *kaddr;
 				page = mempool_alloc(gfs2_page_pool, GFP_NOIO);
 				ptr = page_address(page);
-				kaddr = kmap_atomic(bd2->bd_bh->b_page);
+				kaddr = kmap_local_page(bd2->bd_bh->b_page);
 				memcpy(ptr, kaddr + bh_offset(bd2->bd_bh),
 				       bd2->bd_bh->b_size);
-				kunmap_atomic(kaddr);
+				kunmap_local(kaddr);
 				*(__be32 *)ptr = 0;
 				clear_buffer_escaped(bd2->bd_bh);
 				unlock_buffer(bd2->bd_bh);

2.34.1

Deepak,

Can you please add a reference to the highmem documentation and to the patch from Ira that added a deprecation check for kmap() and kmap_atomic() in his commit regarding checkpatch.pl?

There may be maintainers / reviewers who are still unaware of this information. It would surely help them with reviewing. Furthermore it might suggest maintainers to convert their subsystem / driver to the new API or remove and use plain page_address() (if it is possible to prove that pages can't come from ZONE_HIGHMEM).

The use of kmap() is being deprecated in favor of kmap_local_page().

There are two main problems with kmap(): (1) It comes with an overhead as the mapping space is restricted and protected by a global lock for synchronization and (2) it also requires global TLB invalidation when the kmap?s pool wraps and it might block when the mapping space is fully utilized until a slot becomes available.

With kmap_local_page() the mappings are per thread, CPU local, can take page faults, and can be called from any context (including interrupts). It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore, the tasks can be preempted and, when they are scheduled to run again, the kernel virtual addresses are restored and still valid.

Therefore, replace kmap() with kmap_local_page() in gfs2_read_super().

---
Changes in v3:
- Patch included in patch set

Changes in v2:
- None

 fs/gfs2/ops_fstype.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 8a27957dbfee..80fe61662412 100644
@@ -264,9 +264,9 @@ static int gfs2_read_super(struct gfs2_sbd *sdp, sector_t sector, int silent)
 		__free_page(page);
 		return -EIO;
 	}
-	p = kmap(page);
+	p = kmap_local_page(page);
 	gfs2_sb_in(sdp, p);
-	kunmap(page);
+	kunmap_local(p);
 	__free_page(page);
 	return gfs2_check_sb(sdp, silent);
 }

2.34.1

kmap_atomic() is deprecated in favor of kmap_local_{folio,page}().

Therefore, replace kmap_atomic() with kmap_local_page() in gfs2_write_buf_to_page().

kmap_atomic() disables page-faults and preemption (the latter only for !PREEMPT_RT kernels), However, the code within the mapping/un-mapping in gfs2_write_buf_to_page() does not depend on the above-mentioned side effects.

Therefore, a mere replacement of the old API with the new one is all that is required (i.e., there is no need to explicitly add any calls to pagefault_disable() and/or preempt_disable()).

---
Changes in v3:
- Patch included in patch set

Changes in v2:
- None

 fs/gfs2/quota.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 386ca770ce2e..e5767133aeea 100644
@@ -764,10 +764,10 @@ static int gfs2_write_buf_to_page(struct gfs2_inode *ip, unsigned long index,
 	}
 
 	/* Write to the page, now that we have setup the buffer(s) */
-	kaddr = kmap_atomic(page);
+	kaddr = kmap_local_page(page);
 	memcpy(kaddr + off, buf, bytes);
 	flush_dcache_page(page);
-	kunmap_atomic(kaddr);
+	kunmap_local(kaddr);
 	unlock_page(page);
 	put_page(page);
 

2.34.1

Deepak,

Again please refer to documentation and/or Ira's deprecation patch. The reasons why are in one of my previous messages.

Hi Fabio,
This change was already added by Andreas. So my patchset can be dropped. However, your feedback on the individual patches is agreed to and accepted. I will keep your suggestions in mind when I submit next patches.

Thank you :)

Deepak.

Hi Deepak,

I have already merged version 2 of this patch series and I've fixed up the remaining issues in follow-up patches; see the cluster-devel mailing list:

https://listman.redhat.com/archives/cluster-devel/2023-June/024391.html
https://listman.redhat.com/archives/cluster-devel/2023-June/024392.html
https://listman.redhat.com/archives/cluster-devel/2023-June/024393.html

As well as our for-next branch:

https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/log/?h=for-next

As far as I can see, there is nothing in v3 of your patches that I haven't addressed already. Please speak out if I've missed anything.

Thanks,
Andreas