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
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 <[email protected]>
Signed-off-by: Deepak R Varma <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Deepak R Varma <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Deepak R Varma <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Deepak R Varma <[email protected]>
---
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
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 <[email protected]>
It LGTM, therefore, it is...
Reviewed-by: Fabio M. De Francesco <[email protected]>
> Signed-off-by: Deepak R Varma <[email protected]>
> ---
> 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
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 <[email protected]>
> Signed-off-by: Deepak R Varma <[email protected]>
> ---
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
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 <[email protected]>
> Signed-off-by: Deepak R Varma <[email protected]>
> ---
> 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
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 <[email protected]>
> Signed-off-by: Deepak R Varma <[email protected]>
> ---
> 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
Hi Deepak,
On Thu, Jun 29, 2023 at 11:48 PM Deepak R Varma <[email protected]> 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
>
>
>
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
>
>
>
>