[v4,3/3] fs/ufs: Replace kmap() with kmap_local_page()

  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.

Since its use in fs/ufs is safe everywhere, it should be preferred.

Therefore, replace kmap() with kmap_local_page() in fs/ufs. kunmap_local()
requires the mapping address, so return that address from ufs_get_page()
to be used in ufs_put_page().

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 fs/ufs/dir.c | 75 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 46 insertions(+), 29 deletions(-)
  

On Wed, Dec 21, 2022 at 06:28:02PM +0100, Fabio M. De Francesco wrote:
> 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.
> 
> Since its use in fs/ufs is safe everywhere, it should be preferred.
> 
> Therefore, replace kmap() with kmap_local_page() in fs/ufs. kunmap_local()
> requires the mapping address, so return that address from ufs_get_page()
> to be used in ufs_put_page().

I don't see the calls to kunmap() in ufs_rename converted here?

Did I miss them?

I think those calls need to be changed to ufs_put_page() calls in a precursor
patch to this one unless I'm missing something.

> 
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  fs/ufs/dir.c | 75 ++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 46 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c
> index 9fa86614d2d1..ed3568da29a8 100644
> --- a/fs/ufs/dir.c
> +++ b/fs/ufs/dir.c
> @@ -61,9 +61,9 @@ static int ufs_commit_chunk(struct page *page, loff_t pos, unsigned len)
>  	return err;
>  }
>  
> -static inline void ufs_put_page(struct page *page)
> +static inline void ufs_put_page(struct page *page, void *page_addr)
>  {
> -	kunmap(page);
> +	kunmap_local((void *)((unsigned long)page_addr & PAGE_MASK));

Any address in the page can be passed to kunmap_local() as this mask is done
internally.

>  	put_page(page);
>  }
>  
> @@ -76,7 +76,7 @@ ino_t ufs_inode_by_name(struct inode *dir, const struct qstr *qstr)
>  	de = ufs_find_entry(dir, qstr, &page);
>  	if (de) {
>  		res = fs32_to_cpu(dir->i_sb, de->d_ino);
> -		ufs_put_page(page);
> +		ufs_put_page(page, de);
>  	}
>  	return res;
>  }
> @@ -99,18 +99,17 @@ void ufs_set_link(struct inode *dir, struct ufs_dir_entry *de,
>  	ufs_set_de_type(dir->i_sb, de, inode->i_mode);
>  
>  	err = ufs_commit_chunk(page, pos, len);
> -	ufs_put_page(page);
> +	ufs_put_page(page, de);
>  	if (update_times)
>  		dir->i_mtime = dir->i_ctime = current_time(dir);
>  	mark_inode_dirty(dir);
>  }
>  
>  
> -static bool ufs_check_page(struct page *page)
> +static bool ufs_check_page(struct page *page, char *kaddr)
>  {
>  	struct inode *dir = page->mapping->host;
>  	struct super_block *sb = dir->i_sb;
> -	char *kaddr = page_address(page);
>  	unsigned offs, rec_len;
>  	unsigned limit = PAGE_SIZE;
>  	const unsigned chunk_mask = UFS_SB(sb)->s_uspi->s_dirblksize - 1;
> @@ -185,23 +184,32 @@ static bool ufs_check_page(struct page *page)
>  	return false;
>  }
>  
> +/*
> + * Calls to ufs_get_page()/ufs_put_page() must be nested according to the
> + * rules documented in kmap_local_page()/kunmap_local().
> + *
> + * NOTE: ufs_find_entry() and ufs_dotdot() act as calls to ufs_get_page()
> + * and must be treated accordingly for nesting purposes.
> + */
>  static void *ufs_get_page(struct inode *dir, unsigned long n, struct page **p)
>  {
> +	char *kaddr;
> +
>  	struct address_space *mapping = dir->i_mapping;
>  	struct page *page = read_mapping_page(mapping, n, NULL);
>  	if (!IS_ERR(page)) {
> -		kmap(page);
> +		kaddr = kmap_local_page(page);
>  		if (unlikely(!PageChecked(page))) {
> -			if (!ufs_check_page(page))
> +			if (!ufs_check_page(page, kaddr))
>  				goto fail;
>  		}
>  		*p = page;
> -		return page_address(page);
> +		return kaddr;
>  	}
>  	return ERR_CAST(page);
>  
>  fail:
> -	ufs_put_page(page);
> +	ufs_put_page(page, kaddr);
>  	return ERR_PTR(-EIO);
>  }
>  
> @@ -227,6 +235,13 @@ ufs_next_entry(struct super_block *sb, struct ufs_dir_entry *p)
>  					fs16_to_cpu(sb, p->d_reclen));
>  }
>  
> +/*
> + * Calls to ufs_get_page()/ufs_put_page() must be nested according to the
> + * rules documented in kmap_local_page()/kunmap_local().
> + *
> + * ufs_dotdot() acts as a call to ufs_get_page() and must be treated
> + * accordingly for nesting purposes.
> + */
>  struct ufs_dir_entry *ufs_dotdot(struct inode *dir, struct page **p)
>  {
>  	struct ufs_dir_entry *de = ufs_get_page(dir, 0, p);
> @@ -238,12 +253,15 @@ struct ufs_dir_entry *ufs_dotdot(struct inode *dir, struct page **p)
>  }
>  
>  /*
> - *	ufs_find_entry()
> + * Finds an entry in the specified directory with the wanted name. It returns a
> + * pointer to the directory's entry. The page in which the entry was found is
> + * in the res_page out parameter. The page is returned mapped and unlocked.
> + * The entry is guaranteed to be valid.
>   *
> - * finds an entry in the specified directory with the wanted name. It
> - * returns the page in which the entry was found, and the entry itself
> - * (as a parameter - res_dir). Page is returned mapped and unlocked.
> - * Entry is guaranteed to be valid.

I don't follow why this comment needed changing for this patch.  It probably
warrants it's own patch.

> + * On Success ufs_put_page() should be called on *res_page.
> + *
> + * ufs_find_entry() acts as a call to ufs_get_page() and must be treated
> + * accordingly for nesting purposes.
>   */
>  struct ufs_dir_entry *ufs_find_entry(struct inode *dir, const struct qstr *qstr,
>  				     struct page **res_page)
> @@ -282,7 +300,7 @@ struct ufs_dir_entry *ufs_find_entry(struct inode *dir, const struct qstr *qstr,
>  					goto found;
>  				de = ufs_next_entry(sb, de);
>  			}
> -			ufs_put_page(page);
> +			ufs_put_page(page, kaddr);
>  		}
>  		if (++n >= npages)
>  			n = 0;
> @@ -360,7 +378,7 @@ int ufs_add_link(struct dentry *dentry, struct inode *inode)
>  			de = (struct ufs_dir_entry *) ((char *) de + rec_len);
>  		}
>  		unlock_page(page);
> -		ufs_put_page(page);
> +		ufs_put_page(page, kaddr);
>  	}
>  	BUG();
>  	return -EINVAL;
> @@ -390,7 +408,7 @@ int ufs_add_link(struct dentry *dentry, struct inode *inode)
>  	mark_inode_dirty(dir);
>  	/* OFFSET_CACHE */
>  out_put:
> -	ufs_put_page(page);
> +	ufs_put_page(page, kaddr);
>  	return err;
>  out_unlock:
>  	unlock_page(page);
> @@ -468,13 +486,13 @@ ufs_readdir(struct file *file, struct dir_context *ctx)
>  					       ufs_get_de_namlen(sb, de),
>  					       fs32_to_cpu(sb, de->d_ino),
>  					       d_type)) {
> -					ufs_put_page(page);
> +					ufs_put_page(page, kaddr);
>  					return 0;
>  				}
>  			}
>  			ctx->pos += fs16_to_cpu(sb, de->d_reclen);
>  		}
> -		ufs_put_page(page);
> +		ufs_put_page(page, kaddr);
>  	}
>  	return 0;
>  }
> @@ -485,10 +503,10 @@ ufs_readdir(struct file *file, struct dir_context *ctx)
>   * previous entry.
>   */
>  int ufs_delete_entry(struct inode *inode, struct ufs_dir_entry *dir,
> -		     struct page * page)
> +		     struct page *page)
>  {
>  	struct super_block *sb = inode->i_sb;
> -	char *kaddr = page_address(page);
> +	char *kaddr = (char *)((unsigned long)dir & PAGE_MASK);

I feel like this deserves a comment to clarify that dir points somewhere in the
page we need the base address of.

>  	unsigned int from = offset_in_page(dir) & ~(UFS_SB(sb)->s_uspi->s_dirblksize - 1);
>  	unsigned int to = offset_in_page(dir) + fs16_to_cpu(sb, dir->d_reclen);
>  	loff_t pos;
> @@ -527,7 +545,7 @@ int ufs_delete_entry(struct inode *inode, struct ufs_dir_entry *dir,
>  	inode->i_ctime = inode->i_mtime = current_time(inode);
>  	mark_inode_dirty(inode);
>  out:
> -	ufs_put_page(page);
> +	ufs_put_page(page, kaddr);
>  	UFSD("EXIT\n");
>  	return err;
>  }
> @@ -551,8 +569,7 @@ int ufs_make_empty(struct inode * inode, struct inode *dir)
>  		goto fail;
>  	}
>  
> -	kmap(page);
> -	base = (char*)page_address(page);
> +	base = kmap_local_page(page);

NIT: I'd make this conversion a separate patch.

Ira

>  	memset(base, 0, PAGE_SIZE);
>  
>  	de = (struct ufs_dir_entry *) base;
> @@ -569,7 +586,7 @@ int ufs_make_empty(struct inode * inode, struct inode *dir)
>  	de->d_reclen = cpu_to_fs16(sb, chunk_size - UFS_DIR_REC_LEN(1));
>  	ufs_set_de_namlen(sb, de, 2);
>  	strcpy (de->d_name, "..");
> -	kunmap(page);
> +	kunmap_local(base);
>  
>  	err = ufs_commit_chunk(page, 0, chunk_size);
>  fail:
> @@ -585,9 +602,9 @@ int ufs_empty_dir(struct inode * inode)
>  	struct super_block *sb = inode->i_sb;
>  	struct page *page = NULL;
>  	unsigned long i, npages = dir_pages(inode);
> +	char *kaddr;
>  
>  	for (i = 0; i < npages; i++) {
> -		char *kaddr;
>  		struct ufs_dir_entry *de;
>  
>  		kaddr = ufs_get_page(inode, i, &page);
> @@ -620,12 +637,12 @@ int ufs_empty_dir(struct inode * inode)
>  			}
>  			de = ufs_next_entry(sb, de);
>  		}
> -		ufs_put_page(page);
> +		ufs_put_page(page, kaddr);
>  	}
>  	return 1;
>  
>  not_empty:
> -	ufs_put_page(page);
> +	ufs_put_page(page, kaddr);
>  	return 0;
>  }
>  
> -- 
> 2.39.0
>
  

On giovedì 22 dicembre 2022 06:41:01 CET Ira Weiny wrote:
> On Wed, Dec 21, 2022 at 06:28:02PM +0100, Fabio M. De Francesco wrote:
> > 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.
> > 
> > Since its use in fs/ufs is safe everywhere, it should be preferred.
> > 
> > Therefore, replace kmap() with kmap_local_page() in fs/ufs. kunmap_local()
> > requires the mapping address, so return that address from ufs_get_page()
> > to be used in ufs_put_page().
> 
> I don't see the calls to kunmap() in ufs_rename converted here?
> 
> Did I miss them?
>

No, it's my fault.
I must have used "grep" on all files in fs/ufs, but I forgot to run it :-(

While at this... I'm wondering whether or not we could benefit from a WARNING 
about the use of kunmap(). I'm talking about adding this too to checkpatch.pl, 
exactly as we already have it for catching the deprecated use of kmap(). 
 
>
> I think those calls need to be changed to ufs_put_page() calls in a 
precursor
> patch to this one unless I'm missing something.
> 

Again I think that you are not missing anything and that your suggestion 
sounds good.

I'll replace the three kunmap() + put_page() with three calls to 
ufs_put_page() in ufs_rename(). I'll do these changes in patch 3/4. Instead 
the current 3/4 patch will move ahead and become 4/4.

>
> > Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> > 
> >  fs/ufs/dir.c | 75 ++++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 46 insertions(+), 29 deletions(-)
> > 
> > diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c
> > index 9fa86614d2d1..ed3568da29a8 100644
> > --- a/fs/ufs/dir.c
> > +++ b/fs/ufs/dir.c
> > @@ -61,9 +61,9 @@ static int ufs_commit_chunk(struct page *page, loff_t 
pos,
> > unsigned len)> 
> >  	return err;
> >  
> >  }
> > 
> > -static inline void ufs_put_page(struct page *page)
> > +static inline void ufs_put_page(struct page *page, void *page_addr)
> > 
> >  {
> > 
> > -	kunmap(page);
> > +	kunmap_local((void *)((unsigned long)page_addr & PAGE_MASK));
> 
> Any address in the page can be passed to kunmap_local() as this mask is done
> internally.
> 

I know that any address can be passed and that the bitwise and is performed 
internally in kunmap_local_indexed(). This is why I've never done something 
like this in any other of my precedent conversions. 

However, I thought that Al should have had reasons to suggest to call 
kunmap_local() this way. Copy-pasted from one of his message (https://
lore.kernel.org/lkml/Y4E++JERgUMoqfjG@ZenIV/) while commenting the one patch 
old conversions:

--- begin ---

 -static inline void ufs_put_page(struct page *page)
> +inline void ufs_put_page(struct page *page, void *page_addr)
>  {
> -	kunmap(page);
> +	kunmap_local(page_addr);

Make that
	kunmap_local((void *)((unsigned long)page_addr & PAGE_MASK));
and things become much easier.

>  	put_page(page);
>  }

--- end ---

Did I misinterpret his words?
However, it's my fault again because I should have asked why :-(

> >  	put_page(page);
> >  
> >  }
> > 
> > @@ -76,7 +76,7 @@ ino_t ufs_inode_by_name(struct inode *dir, const struct
> > qstr *qstr)> 
> >  	de = ufs_find_entry(dir, qstr, &page);
> >  	if (de) {
> >  	
> >  		res = fs32_to_cpu(dir->i_sb, de->d_ino);
> > 
> > -		ufs_put_page(page);
> > +		ufs_put_page(page, de);
> > 
> >  	}
> >  	return res;
> >  
> >  }
> > 
> > @@ -99,18 +99,17 @@ void ufs_set_link(struct inode *dir, struct
> > ufs_dir_entry *de,> 
> >  	ufs_set_de_type(dir->i_sb, de, inode->i_mode);
> >  	
> >  	err = ufs_commit_chunk(page, pos, len);
> > 
> > -	ufs_put_page(page);
> > +	ufs_put_page(page, de);
> > 
> >  	if (update_times)
> >  	
> >  		dir->i_mtime = dir->i_ctime = current_time(dir);
> >  	
> >  	mark_inode_dirty(dir);
> >  
> >  }
> > 
> > -static bool ufs_check_page(struct page *page)
> > +static bool ufs_check_page(struct page *page, char *kaddr)
> > 
> >  {
> >  
> >  	struct inode *dir = page->mapping->host;
> >  	struct super_block *sb = dir->i_sb;
> > 
> > -	char *kaddr = page_address(page);
> > 
> >  	unsigned offs, rec_len;
> >  	unsigned limit = PAGE_SIZE;
> >  	const unsigned chunk_mask = UFS_SB(sb)->s_uspi->s_dirblksize - 1;
> > 
> > @@ -185,23 +184,32 @@ static bool ufs_check_page(struct page *page)
> > 
> >  	return false;
> >  
> >  }
> > 
> > +/*
> > + * Calls to ufs_get_page()/ufs_put_page() must be nested according to the
> > + * rules documented in kmap_local_page()/kunmap_local().
> > + *
> > + * NOTE: ufs_find_entry() and ufs_dotdot() act as calls to ufs_get_page()
> > + * and must be treated accordingly for nesting purposes.
> > + */
> > 
> >  static void *ufs_get_page(struct inode *dir, unsigned long n, struct page
> >  **p) {
> > 
> > +	char *kaddr;
> > +
> > 
> >  	struct address_space *mapping = dir->i_mapping;
> >  	struct page *page = read_mapping_page(mapping, n, NULL);
> >  	if (!IS_ERR(page)) {
> > 
> > -		kmap(page);
> > +		kaddr = kmap_local_page(page);
> > 
> >  		if (unlikely(!PageChecked(page))) {
> > 
> > -			if (!ufs_check_page(page))
> > +			if (!ufs_check_page(page, kaddr))
> > 
> >  				goto fail;
> >  		
> >  		}
> >  		*p = page;
> > 
> > -		return page_address(page);
> > +		return kaddr;
> > 
> >  	}
> >  	return ERR_CAST(page);
> >  
> >  fail:
> > -	ufs_put_page(page);
> > +	ufs_put_page(page, kaddr);
> > 
> >  	return ERR_PTR(-EIO);
> >  
> >  }
> > 
> > @@ -227,6 +235,13 @@ ufs_next_entry(struct super_block *sb, struct
> > ufs_dir_entry *p)> 
> >  					fs16_to_cpu(sb, p-
>d_reclen));
> >  
> >  }
> > 
> > +/*
> > + * Calls to ufs_get_page()/ufs_put_page() must be nested according to the
> > + * rules documented in kmap_local_page()/kunmap_local().
> > + *
> > + * ufs_dotdot() acts as a call to ufs_get_page() and must be treated
> > + * accordingly for nesting purposes.
> > + */
> > 
> >  struct ufs_dir_entry *ufs_dotdot(struct inode *dir, struct page **p)
> >  {
> >  
> >  	struct ufs_dir_entry *de = ufs_get_page(dir, 0, p);
> > 
> > @@ -238,12 +253,15 @@ struct ufs_dir_entry *ufs_dotdot(struct inode *dir,
> > struct page **p)> 
> >  }
> >  
> >  /*
> > 
> > - *	ufs_find_entry()
> > + * Finds an entry in the specified directory with the wanted name. It
> > returns a + * pointer to the directory's entry. The page in which the 
entry
> > was found is + * in the res_page out parameter. The page is returned 
mapped
> > and unlocked. + * The entry is guaranteed to be valid.
> > 
> >   *
> > 
> > - * finds an entry in the specified directory with the wanted name. It
> > - * returns the page in which the entry was found, and the entry itself
> > - * (as a parameter - res_dir). Page is returned mapped and unlocked.
> > - * Entry is guaranteed to be valid.
> 
> I don't follow why this comment needed changing for this patch.  It probably
> warrants it's own patch.
> 

Sure, the removal of the name of function is a different logical change, so 
I'll probably leave it as it was. 

> > + * On Success ufs_put_page() should be called on *res_page.
> > + *
> > + * ufs_find_entry() acts as a call to ufs_get_page() and must be treated
> > + * accordingly for nesting purposes.
> > 
> >   */

But this last part should be still added. Am I wrong?

> >  struct ufs_dir_entry *ufs_find_entry(struct inode *dir, const struct qstr 
*qstr,
> >  				struct page **res_page)
> > 
> > @@ -282,7 +300,7 @@ struct ufs_dir_entry *ufs_find_entry(struct inode 
*dir,
> > const struct qstr *qstr,> 
> >  					goto found;
> >  				
> >  				de = ufs_next_entry(sb, de);
> >  			
> >  			}
> > 
> > -			ufs_put_page(page);
> > +			ufs_put_page(page, kaddr);
> > 
> >  		}
> >  		if (++n >= npages)
> >  		
> >  			n = 0;
> > 
> > @@ -360,7 +378,7 @@ int ufs_add_link(struct dentry *dentry, struct inode
> > *inode)> 
> >  			de = (struct ufs_dir_entry *) ((char *) de + 
rec_len);
> >  		
> >  		}
> >  		unlock_page(page);
> > 
> > -		ufs_put_page(page);
> > +		ufs_put_page(page, kaddr);
> > 
> >  	}
> >  	BUG();
> >  	return -EINVAL;
> > 
> > @@ -390,7 +408,7 @@ int ufs_add_link(struct dentry *dentry, struct inode
> > *inode)> 
> >  	mark_inode_dirty(dir);
> >  	/* OFFSET_CACHE */
> >  
> >  out_put:
> > -	ufs_put_page(page);
> > +	ufs_put_page(page, kaddr);
> > 
> >  	return err;
> >  
> >  out_unlock:
> >  	unlock_page(page);
> > 
> > @@ -468,13 +486,13 @@ ufs_readdir(struct file *file, struct dir_context
> > *ctx)
> > 
> >  					       ufs_get_de_namlen(sb, 
de),
> >  					       fs32_to_cpu(sb, de-
>d_ino),
> >  					       d_type)) {
> > 
> > -					ufs_put_page(page);
> > +					ufs_put_page(page, kaddr);
> > 
> >  					return 0;
> >  				
> >  				}
> >  			
> >  			}
> >  			ctx->pos += fs16_to_cpu(sb, de->d_reclen);
> >  		
> >  		}
> > 
> > -		ufs_put_page(page);
> > +		ufs_put_page(page, kaddr);
> > 
> >  	}
> >  	return 0;
> >  
> >  }
> > 
> > @@ -485,10 +503,10 @@ ufs_readdir(struct file *file, struct dir_context
> > *ctx)
> > 
> >   * previous entry.
> >   */
> >  
> >  int ufs_delete_entry(struct inode *inode, struct ufs_dir_entry *dir,
> > 
> > -		     struct page * page)
> > +		     struct page *page)
> > 
> >  {
> >  
> >  	struct super_block *sb = inode->i_sb;
> > 
> > -	char *kaddr = page_address(page);
> > +	char *kaddr = (char *)((unsigned long)dir & PAGE_MASK);
> 
> I feel like this deserves a comment to clarify that dir points somewhere in
> the page we need the base address of.

OK, it sounds reasonable.

> >  	unsigned int from = offset_in_page(dir) &
> >  	~(UFS_SB(sb)->s_uspi->s_dirblksize - 1); unsigned int to =
> >  	offset_in_page(dir) + fs16_to_cpu(sb, dir->d_reclen); loff_t pos;
> > 
> > @@ -527,7 +545,7 @@ int ufs_delete_entry(struct inode *inode, struct
> > ufs_dir_entry *dir,> 
> >  	inode->i_ctime = inode->i_mtime = current_time(inode);
> >  	mark_inode_dirty(inode);
> >  
> >  out:
> > -	ufs_put_page(page);
> > +	ufs_put_page(page, kaddr);
> > 
> >  	UFSD("EXIT\n");
> >  	return err;
> >  
> >  }
> > 
> > @@ -551,8 +569,7 @@ int ufs_make_empty(struct inode * inode, struct inode
> > *dir)> 
> >  		goto fail;
> >  	
> >  	}
> > 
> > -	kmap(page);
> > -	base = (char*)page_address(page);
> > +	base = kmap_local_page(page);
> 
> NIT: I'd make this conversion a separate patch.
> 
> Ira
> 

We've always done multiple conversions at the same time if in the same file, 
even if they were unrelated.

I don't understand why we want to change the usual procedure. Can you please 
elaborate a bit more on this topic?

Thanks so much for finding the missing conversions and for your other comments 
and advice on this patch.

Fabio

> >  	memset(base, 0, PAGE_SIZE);
> >  	
> >  	de = (struct ufs_dir_entry *) base;
> > 
> > @@ -569,7 +586,7 @@ int ufs_make_empty(struct inode * inode, struct inode
> > *dir)> 
> >  	de->d_reclen = cpu_to_fs16(sb, chunk_size - UFS_DIR_REC_LEN(1));
> >  	ufs_set_de_namlen(sb, de, 2);
> >  	strcpy (de->d_name, "..");
> > 
> > -	kunmap(page);
> > +	kunmap_local(base);
> > 
> >  	err = ufs_commit_chunk(page, 0, chunk_size);
> >  
> >  fail:
> > @@ -585,9 +602,9 @@ int ufs_empty_dir(struct inode * inode)
> > 
> >  	struct super_block *sb = inode->i_sb;
> >  	struct page *page = NULL;
> >  	unsigned long i, npages = dir_pages(inode);
> > 
> > +	char *kaddr;
> > 
> >  	for (i = 0; i < npages; i++) {
> > 
> > -		char *kaddr;
> > 
> >  		struct ufs_dir_entry *de;
> >  		
> >  		kaddr = ufs_get_page(inode, i, &page);
> > 
> > @@ -620,12 +637,12 @@ int ufs_empty_dir(struct inode * inode)
> > 
> >  			}
> >  			de = ufs_next_entry(sb, de);
> >  		
> >  		}
> > 
> > -		ufs_put_page(page);
> > +		ufs_put_page(page, kaddr);
> > 
> >  	}
> >  	return 1;
> >  
> >  not_empty:
> > -	ufs_put_page(page);
> > +	ufs_put_page(page, kaddr);
> > 
> >  	return 0;
> >  
> >  }
> > 
> > --
> > 2.39.0
  

On Thu, Dec 22, 2022 at 03:27:08PM +0100, Fabio M. De Francesco wrote:
> On giovedì 22 dicembre 2022 06:41:01 CET Ira Weiny wrote:
> > On Wed, Dec 21, 2022 at 06:28:02PM +0100, Fabio M. De Francesco wrote:
> > > 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.
> > > 
> > > Since its use in fs/ufs is safe everywhere, it should be preferred.
> > > 
> > > Therefore, replace kmap() with kmap_local_page() in fs/ufs. kunmap_local()
> > > requires the mapping address, so return that address from ufs_get_page()
> > > to be used in ufs_put_page().
> > 
> > I don't see the calls to kunmap() in ufs_rename converted here?
> > 
> > Did I miss them?
> >
> 
> No, it's my fault.
> I must have used "grep" on all files in fs/ufs, but I forgot to run it :-(
> 
> While at this... I'm wondering whether or not we could benefit from a WARNING 
> about the use of kunmap(). I'm talking about adding this too to checkpatch.pl, 
> exactly as we already have it for catching the deprecated use of kmap(). 

That would not have caught this issue.  Any addition of kunmap() in a patch
would have to come with a call to kmap().  (Unless they are fixing some kmap
bug I suppose.)  I'm not sure how the checkpatch.pl maintainers would feel
about this.  You can always submit a patch and find out but I would not worry
about it.

>  
> >
> > I think those calls need to be changed to ufs_put_page() calls in a 
> precursor
> > patch to this one unless I'm missing something.
> > 
> 
> Again I think that you are not missing anything and that your suggestion 
> sounds good.
> 
> I'll replace the three kunmap() + put_page() with three calls to 
> ufs_put_page() in ufs_rename(). I'll do these changes in patch 3/4. Instead 
> the current 3/4 patch will move ahead and become 4/4.

Sounds good.

> 
> >
> > > Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> > > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > > 
> > >  fs/ufs/dir.c | 75 ++++++++++++++++++++++++++++++++--------------------
> > >  1 file changed, 46 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/fs/ufs/dir.c b/fs/ufs/dir.c
> > > index 9fa86614d2d1..ed3568da29a8 100644
> > > --- a/fs/ufs/dir.c
> > > +++ b/fs/ufs/dir.c
> > > @@ -61,9 +61,9 @@ static int ufs_commit_chunk(struct page *page, loff_t 
> pos,
> > > unsigned len)> 
> > >  	return err;
> > >  
> > >  }
> > > 
> > > -static inline void ufs_put_page(struct page *page)
> > > +static inline void ufs_put_page(struct page *page, void *page_addr)
> > > 
> > >  {
> > > 
> > > -	kunmap(page);
> > > +	kunmap_local((void *)((unsigned long)page_addr & PAGE_MASK));
> > 
> > Any address in the page can be passed to kunmap_local() as this mask is done
> > internally.
> > 
> 
> I know that any address can be passed and that the bitwise and is performed 
> internally in kunmap_local_indexed(). This is why I've never done something 
> like this in any other of my precedent conversions. 
> 
> However, I thought that Al should have had reasons to suggest to call 
> kunmap_local() this way. Copy-pasted from one of his message (https://
> lore.kernel.org/lkml/Y4E++JERgUMoqfjG@ZenIV/) while commenting the one patch 
> old conversions:
> 
> --- begin ---
> 
>  -static inline void ufs_put_page(struct page *page)
> > +inline void ufs_put_page(struct page *page, void *page_addr)
> >  {
> > -	kunmap(page);
> > +	kunmap_local(page_addr);
> 
> Make that
> 	kunmap_local((void *)((unsigned long)page_addr & PAGE_MASK));
> and things become much easier.
> 
> >  	put_page(page);
> >  }
> 
> --- end ---
> 
> Did I misinterpret his words?
> However, it's my fault again because I should have asked why :-(

Perhaps Al did not know that kunmap_local() would take care of this for you?

> 
> > >  	put_page(page);
> > >  
> > >  }
> > > 
> > > @@ -76,7 +76,7 @@ ino_t ufs_inode_by_name(struct inode *dir, const struct
> > > qstr *qstr)> 
> > >  	de = ufs_find_entry(dir, qstr, &page);
> > >  	if (de) {
> > >  	
> > >  		res = fs32_to_cpu(dir->i_sb, de->d_ino);
> > > 
> > > -		ufs_put_page(page);
> > > +		ufs_put_page(page, de);
> > > 
> > >  	}
> > >  	return res;
> > >  
> > >  }
> > > 
> > > @@ -99,18 +99,17 @@ void ufs_set_link(struct inode *dir, struct
> > > ufs_dir_entry *de,> 
> > >  	ufs_set_de_type(dir->i_sb, de, inode->i_mode);
> > >  	
> > >  	err = ufs_commit_chunk(page, pos, len);
> > > 
> > > -	ufs_put_page(page);
> > > +	ufs_put_page(page, de);
> > > 
> > >  	if (update_times)
> > >  	
> > >  		dir->i_mtime = dir->i_ctime = current_time(dir);
> > >  	
> > >  	mark_inode_dirty(dir);
> > >  
> > >  }
> > > 
> > > -static bool ufs_check_page(struct page *page)
> > > +static bool ufs_check_page(struct page *page, char *kaddr)
> > > 
> > >  {
> > >  
> > >  	struct inode *dir = page->mapping->host;
> > >  	struct super_block *sb = dir->i_sb;
> > > 
> > > -	char *kaddr = page_address(page);
> > > 
> > >  	unsigned offs, rec_len;
> > >  	unsigned limit = PAGE_SIZE;
> > >  	const unsigned chunk_mask = UFS_SB(sb)->s_uspi->s_dirblksize - 1;
> > > 
> > > @@ -185,23 +184,32 @@ static bool ufs_check_page(struct page *page)
> > > 
> > >  	return false;
> > >  
> > >  }
> > > 
> > > +/*
> > > + * Calls to ufs_get_page()/ufs_put_page() must be nested according to the
> > > + * rules documented in kmap_local_page()/kunmap_local().
> > > + *
> > > + * NOTE: ufs_find_entry() and ufs_dotdot() act as calls to ufs_get_page()
> > > + * and must be treated accordingly for nesting purposes.
> > > + */
> > > 
> > >  static void *ufs_get_page(struct inode *dir, unsigned long n, struct page
> > >  **p) {
> > > 
> > > +	char *kaddr;
> > > +
> > > 
> > >  	struct address_space *mapping = dir->i_mapping;
> > >  	struct page *page = read_mapping_page(mapping, n, NULL);
> > >  	if (!IS_ERR(page)) {
> > > 
> > > -		kmap(page);
> > > +		kaddr = kmap_local_page(page);
> > > 
> > >  		if (unlikely(!PageChecked(page))) {
> > > 
> > > -			if (!ufs_check_page(page))
> > > +			if (!ufs_check_page(page, kaddr))
> > > 
> > >  				goto fail;
> > >  		
> > >  		}
> > >  		*p = page;
> > > 
> > > -		return page_address(page);
> > > +		return kaddr;
> > > 
> > >  	}
> > >  	return ERR_CAST(page);
> > >  
> > >  fail:
> > > -	ufs_put_page(page);
> > > +	ufs_put_page(page, kaddr);
> > > 
> > >  	return ERR_PTR(-EIO);
> > >  
> > >  }
> > > 
> > > @@ -227,6 +235,13 @@ ufs_next_entry(struct super_block *sb, struct
> > > ufs_dir_entry *p)> 
> > >  					fs16_to_cpu(sb, p-
> >d_reclen));
> > >  
> > >  }
> > > 
> > > +/*
> > > + * Calls to ufs_get_page()/ufs_put_page() must be nested according to the
> > > + * rules documented in kmap_local_page()/kunmap_local().
> > > + *
> > > + * ufs_dotdot() acts as a call to ufs_get_page() and must be treated
> > > + * accordingly for nesting purposes.
> > > + */
> > > 
> > >  struct ufs_dir_entry *ufs_dotdot(struct inode *dir, struct page **p)
> > >  {
> > >  
> > >  	struct ufs_dir_entry *de = ufs_get_page(dir, 0, p);
> > > 
> > > @@ -238,12 +253,15 @@ struct ufs_dir_entry *ufs_dotdot(struct inode *dir,
> > > struct page **p)> 
> > >  }
> > >  
> > >  /*
> > > 
> > > - *	ufs_find_entry()
> > > + * Finds an entry in the specified directory with the wanted name. It
> > > returns a + * pointer to the directory's entry. The page in which the 
> entry
> > > was found is + * in the res_page out parameter. The page is returned 
> mapped
> > > and unlocked. + * The entry is guaranteed to be valid.
> > > 
> > >   *
> > > 
> > > - * finds an entry in the specified directory with the wanted name. It
> > > - * returns the page in which the entry was found, and the entry itself
> > > - * (as a parameter - res_dir). Page is returned mapped and unlocked.
> > > - * Entry is guaranteed to be valid.
> > 
> > I don't follow why this comment needed changing for this patch.  It probably
> > warrants it's own patch.
> > 
> 
> Sure, the removal of the name of function is a different logical change, so 
> I'll probably leave it as it was. 
> 
> > > + * On Success ufs_put_page() should be called on *res_page.
> > > + *
> > > + * ufs_find_entry() acts as a call to ufs_get_page() and must be treated
> > > + * accordingly for nesting purposes.
> > > 
> > >   */
> 
> But this last part should be still added. Am I wrong?

You are not wrong.  Adding this is appropriate.  Just not the rest which seemed
very minor changes anyway.

> 
> > >  struct ufs_dir_entry *ufs_find_entry(struct inode *dir, const struct qstr 
> *qstr,
> > >  				struct page **res_page)
> > > 
> > > @@ -282,7 +300,7 @@ struct ufs_dir_entry *ufs_find_entry(struct inode 
> *dir,
> > > const struct qstr *qstr,> 
> > >  					goto found;
> > >  				
> > >  				de = ufs_next_entry(sb, de);
> > >  			
> > >  			}
> > > 
> > > -			ufs_put_page(page);
> > > +			ufs_put_page(page, kaddr);
> > > 
> > >  		}
> > >  		if (++n >= npages)
> > >  		
> > >  			n = 0;
> > > 
> > > @@ -360,7 +378,7 @@ int ufs_add_link(struct dentry *dentry, struct inode
> > > *inode)> 
> > >  			de = (struct ufs_dir_entry *) ((char *) de + 
> rec_len);
> > >  		
> > >  		}
> > >  		unlock_page(page);
> > > 
> > > -		ufs_put_page(page);
> > > +		ufs_put_page(page, kaddr);
> > > 
> > >  	}
> > >  	BUG();
> > >  	return -EINVAL;
> > > 
> > > @@ -390,7 +408,7 @@ int ufs_add_link(struct dentry *dentry, struct inode
> > > *inode)> 
> > >  	mark_inode_dirty(dir);
> > >  	/* OFFSET_CACHE */
> > >  
> > >  out_put:
> > > -	ufs_put_page(page);
> > > +	ufs_put_page(page, kaddr);
> > > 
> > >  	return err;
> > >  
> > >  out_unlock:
> > >  	unlock_page(page);
> > > 
> > > @@ -468,13 +486,13 @@ ufs_readdir(struct file *file, struct dir_context
> > > *ctx)
> > > 
> > >  					       ufs_get_de_namlen(sb, 
> de),
> > >  					       fs32_to_cpu(sb, de-
> >d_ino),
> > >  					       d_type)) {
> > > 
> > > -					ufs_put_page(page);
> > > +					ufs_put_page(page, kaddr);
> > > 
> > >  					return 0;
> > >  				
> > >  				}
> > >  			
> > >  			}
> > >  			ctx->pos += fs16_to_cpu(sb, de->d_reclen);
> > >  		
> > >  		}
> > > 
> > > -		ufs_put_page(page);
> > > +		ufs_put_page(page, kaddr);
> > > 
> > >  	}
> > >  	return 0;
> > >  
> > >  }
> > > 
> > > @@ -485,10 +503,10 @@ ufs_readdir(struct file *file, struct dir_context
> > > *ctx)
> > > 
> > >   * previous entry.
> > >   */
> > >  
> > >  int ufs_delete_entry(struct inode *inode, struct ufs_dir_entry *dir,
> > > 
> > > -		     struct page * page)
> > > +		     struct page *page)
> > > 
> > >  {
> > >  
> > >  	struct super_block *sb = inode->i_sb;
> > > 
> > > -	char *kaddr = page_address(page);
> > > +	char *kaddr = (char *)((unsigned long)dir & PAGE_MASK);
> > 
> > I feel like this deserves a comment to clarify that dir points somewhere in
> > the page we need the base address of.
> 
> OK, it sounds reasonable.
> 
> > >  	unsigned int from = offset_in_page(dir) &
> > >  	~(UFS_SB(sb)->s_uspi->s_dirblksize - 1); unsigned int to =
> > >  	offset_in_page(dir) + fs16_to_cpu(sb, dir->d_reclen); loff_t pos;
> > > 
> > > @@ -527,7 +545,7 @@ int ufs_delete_entry(struct inode *inode, struct
> > > ufs_dir_entry *dir,> 
> > >  	inode->i_ctime = inode->i_mtime = current_time(inode);
> > >  	mark_inode_dirty(inode);
> > >  
> > >  out:
> > > -	ufs_put_page(page);
> > > +	ufs_put_page(page, kaddr);
> > > 
> > >  	UFSD("EXIT\n");
> > >  	return err;
> > >  
> > >  }
> > > 
> > > @@ -551,8 +569,7 @@ int ufs_make_empty(struct inode * inode, struct inode
> > > *dir)> 
> > >  		goto fail;
> > >  	
> > >  	}
> > > 
> > > -	kmap(page);
> > > -	base = (char*)page_address(page);
> > > +	base = kmap_local_page(page);
> > 
> > NIT: I'd make this conversion a separate patch.
> > 
> > Ira
> > 
> 
> We've always done multiple conversions at the same time if in the same file, 
> even if they were unrelated.
> 
> I don't understand why we want to change the usual procedure. Can you please 
> elaborate a bit more on this topic?

The difference here is we are making a lot of changes to the
ufs_{get,put}_page() calls and all their callers and this is not part of those
changes.  So reviewing those was hard enough without looking at this more
mundane change.  But like I said it is a nit so feel free to leave it as the
change looks fine.

> 
> Thanks so much for finding the missing conversions and for your other comments 
> and advice on this patch.

NP!

Ira

> 
> Fabio
> 
> > >  	memset(base, 0, PAGE_SIZE);
> > >  	
> > >  	de = (struct ufs_dir_entry *) base;
> > > 
> > > @@ -569,7 +586,7 @@ int ufs_make_empty(struct inode * inode, struct inode
> > > *dir)> 
> > >  	de->d_reclen = cpu_to_fs16(sb, chunk_size - UFS_DIR_REC_LEN(1));
> > >  	ufs_set_de_namlen(sb, de, 2);
> > >  	strcpy (de->d_name, "..");
> > > 
> > > -	kunmap(page);
> > > +	kunmap_local(base);
> > > 
> > >  	err = ufs_commit_chunk(page, 0, chunk_size);
> > >  
> > >  fail:
> > > @@ -585,9 +602,9 @@ int ufs_empty_dir(struct inode * inode)
> > > 
> > >  	struct super_block *sb = inode->i_sb;
> > >  	struct page *page = NULL;
> > >  	unsigned long i, npages = dir_pages(inode);
> > > 
> > > +	char *kaddr;
> > > 
> > >  	for (i = 0; i < npages; i++) {
> > > 
> > > -		char *kaddr;
> > > 
> > >  		struct ufs_dir_entry *de;
> > >  		
> > >  		kaddr = ufs_get_page(inode, i, &page);
> > > 
> > > @@ -620,12 +637,12 @@ int ufs_empty_dir(struct inode * inode)
> > > 
> > >  			}
> > >  			de = ufs_next_entry(sb, de);
> > >  		
> > >  		}
> > > 
> > > -		ufs_put_page(page);
> > > +		ufs_put_page(page, kaddr);
> > > 
> > >  	}
> > >  	return 1;
> > >  
> > >  not_empty:
> > > -	ufs_put_page(page);
> > > +	ufs_put_page(page, kaddr);
> > > 
> > >  	return 0;
> > >  
> > >  }
> > > 
> > > --
> > > 2.39.0
> 
> 
> 
>