[Cluster-devel] [PATCH v3 0/6] gfs2: kmap{_atomic} conversion to kmap_local_{page/folio}

[Cluster-devel] [PATCH v3 0/6] gfs2: kmap{_atomic} conversion to kmap_local_{page/folio}

[Deepak R Varma]

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

[Cluster-devel] [PATCH v3 1/6] gfs2: Replace kmap_atomic() by kmap_local_page() in stuffed_readpage

[Deepak R Varma]

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()).

Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Deepak R Varma <drv@mailo.com>
---
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(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 3b41542d6697..3eac4f2f5c27 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -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

[Cluster-devel] [PATCH v3 2/6] gfs2: Replace kmap_atomic()+memcpy by memcpy_from_page()

[Deepak R Varma]

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.

Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Deepak R Varma <drv@mailo.com>
---
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(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 3eac4f2f5c27..f47fed657763 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -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

[Cluster-devel] [PATCH v3 3/6] gfs2: Replace kmap() by kmap_local_page() in gfs2_unstuffer_page

[Deepak R Varma]

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().

Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Deepak R Varma <drv@mailo.com>
---
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(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 8d611fbcf0bd..6b850e2ba5c8 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -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

[Cluster-devel] [PATCH v3 4/6] gfs2: Replace kmap_atomic() by kmap_local_page() in lops.c

[Deepak R Varma]

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()).

Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Deepak R Varma <drv@mailo.com>
---
Changes in v3:
   - Patch included in patch series

Changes in v2:
   - None


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

diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
index 1902413d5d12..a7c2296cb3c6 100644
--- a/fs/gfs2/lops.c
+++ b/fs/gfs2/lops.c
@@ -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

[Cluster-devel] [PATCH v3 5/6] gfs2: Replace kmap() by kmap_local_page() in gfs2_read_super

[Deepak R Varma]

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().

Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Deepak R Varma <drv@mailo.com>
---
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(-)

diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 8a27957dbfee..80fe61662412 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -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

[Cluster-devel] [PATCH v3 6/6] gfs2: Replace kmap_atomic() by kmap_local_page() in gfs2_write_buf_to_page

[Deepak R Varma]

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()).

Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Signed-off-by: Deepak R Varma <drv@mailo.com>
---
Changes in v3:
   - Patch included in patch set

Changes in v2:
   - None


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

diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index 386ca770ce2e..e5767133aeea 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -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

[Cluster-devel] [PATCH v3 1/6] gfs2: Replace kmap_atomic() by kmap_local_page() in stuffed_readpage

[Fabio M. De Francesco]

On gioved? 29 giugno 2023 23:49:29 CEST Deepak R Varma wrote:
> 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()).
> 
> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

It LGTM, therefore, it is...

Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
> Changes in v3:
>    - split into 2 patches

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.

>    - 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(-)
> 
> diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
> index 3b41542d6697..3eac4f2f5c27 100644
> --- a/fs/gfs2/aops.c
> +++ b/fs/gfs2/aops.c
> @@ -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

[Cluster-devel] [PATCH v3 2/6] gfs2: Replace kmap_atomic()+memcpy by memcpy_from_page()

[Fabio M. De Francesco]

On gioved? 29 giugno 2023 23:50:07 CEST Deepak R Varma wrote:
> 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()

s/kunmap_automic/kunmap_atomic/

> block by the memcpy_from_page() API call.
> This change allows to tidy-up code and also eliminate unused variable p.
> 
> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

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(). 

> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
> 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(-)
> 
> diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
> index 3eac4f2f5c27..f47fed657763 100644
> --- a/fs/gfs2/aops.c
> +++ b/fs/gfs2/aops.c
> @@ -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;

This is a different logical change. Please put it in a different patch with 
proper subject and commit message.

Thanks,

Fabio 

> +
> +		memcpy_from_page(buf, page, offset, amt);
>  		put_page(page);
>  		copied += amt;
>  		index++;
> --
> 2.34.1

[Cluster-devel] [PATCH v3 3/6] gfs2: Replace kmap() by kmap_local_page() in gfs2_unstuffer_page

[Fabio M. De Francesco]

On gioved? 29 giugno 2023 23:50:43 CEST Deepak R Varma wrote:
> 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().
> 
> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---

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 :-)).

> 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(-)
> 
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index 8d611fbcf0bd..6b850e2ba5c8 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -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);

As a general rule, we should take the mappings the shorter time it is possible 
(to avoid to disable migration for too long). I'm not sure why the "dsize" 
assignment is made between mapping and un-mapping. Can you please explain why?

Thanks,

Fabio

>  		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

[Cluster-devel] [PATCH v3 4/6] gfs2: Replace kmap_atomic() by kmap_local_page() in lops.c

[Fabio M. De Francesco]

On gioved? 29 giugno 2023 23:51:17 CEST Deepak R Varma wrote:
> kmap_atomic() is deprecated in favor of kmap_local_{folio,page}().

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).

> 
> 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()).
> 
> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
> Changes in v3:
>    - Patch included in patch series
> 
> Changes in v2:
>    - None
> 
> 
>  fs/gfs2/lops.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index 1902413d5d12..a7c2296cb3c6 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -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;
>
Deepak,

Are we mixing declarations with functions calls? Is it good practice? If not, 
I'd suggest to move the mapping to a better suited place.
> 
> @@ -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);
>
Deepak,

How about memcpy_from_page()?

Thanks,

Fabio
>
> -				kunmap_atomic(kaddr);
> +				kunmap_local(kaddr);
>  				*(__be32 *)ptr = 0;
>  				clear_buffer_escaped(bd2->bd_bh);
>  				unlock_buffer(bd2->bd_bh);
> --
> 2.34.1

[Cluster-devel] [PATCH v3 5/6] gfs2: Replace kmap() by kmap_local_page() in gfs2_read_super

[Fabio M. De Francesco]

On gioved? 29 giugno 2023 23:51:53 CEST Deepak R Varma wrote:
> 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().
> 
> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
> 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(-)
> 
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index 8a27957dbfee..80fe61662412 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -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);
>
Deepak,

Could this page ever come from ZONE_HIGHMEM? Can you please check whether we 
really need to kmap*() it or not? 

Fabio
>
>  	gfs2_sb_in(sdp, p);
> -	kunmap(page);
> +	kunmap_local(p);
>  	__free_page(page);
>  	return gfs2_check_sb(sdp, silent);
>  }
> --
> 2.34.1

[Cluster-devel] [PATCH v3 6/6] gfs2: Replace kmap_atomic() by kmap_local_page() in gfs2_write_buf_to_page

[Fabio M. De Francesco]

On gioved? 29 giugno 2023 23:52:27 CEST Deepak R Varma wrote:
> kmap_atomic() is deprecated in favor of kmap_local_{folio,page}().

Deepak,

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

> 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()).
>
> Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> Signed-off-by: Deepak R Varma <drv@mailo.com>
> ---
> Changes in v3:
>    - Patch included in patch set
> 
> Changes in v2:
>    - None
> 
> 
>  fs/gfs2/quota.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> index 386ca770ce2e..e5767133aeea 100644
> --- a/fs/gfs2/quota.c
> +++ b/fs/gfs2/quota.c
> @@ -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);
>
Well, if this page could come from HIGHMEM, how about memcpy_to_page()? 
Otherwise, (if it cannot come from HIGHMEM) we don't need to kmap*() it. 

Can you please take a look at the allocation's flags?

Thanks,

Fabio
>
>  	memcpy(kaddr + off, buf, bytes);
>  	flush_dcache_page(page);
> -	kunmap_atomic(kaddr);
> +	kunmap_local(kaddr);
>  	unlock_page(page);
>  	put_page(page);
> 
> --
> 2.34.1

[Cluster-devel] [PATCH v3 0/6] gfs2: kmap{_atomic} conversion to kmap_local_{page/folio}

[Andreas Gruenbacher]

Hi Deepak,

On Thu, Jun 29, 2023 at 11:48?PM Deepak R Varma <drv@mailo.com> wrote:
> 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]

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


>
> 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
>
>
>

Re: [Cluster-devel] [PATCH v3 6/6] gfs2: Replace kmap_atomic() by kmap_local_page() in gfs2_write_buf_to_page

[Deepak R Varma]

On Sat, Jul 01, 2023 at 03:54:06PM +0200, Fabio M. De Francesco wrote:
> On giovedì 29 giugno 2023 23:52:27 CEST Deepak R Varma wrote:
> > kmap_atomic() is deprecated in favor of kmap_local_{folio,page}().
>
> 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.

>
> > Therefore, replace kmap_atomic() with kmap_local_page() in
> > --
> > 2.34.1
>
>
>
>