diff mbox series

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

Message ID 20210217024826.3466046-2-ira.weiny@intel.com (mailing list archive)
State New, archived
Headers show
Series btrfs: Convert more kmaps to kmap_local_page() | expand

Commit Message

Ira Weiny Feb. 17, 2021, 2:48 a.m. UTC 
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(-)

Comments

David Sterba March 12, 2021, 6:58 p.m. UTC | #1 
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.
Ira Weiny March 12, 2021, 8:03 p.m. UTC | #2 
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
David Sterba March 16, 2021, 11:04 a.m. UTC | #3 
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.
diff mbox series

Patch

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