[RFC PATCH] fs: Rename put_and_unmap_page() to unmap_and_put_page()

[RFC PATCH] fs: Rename put_and_unmap_page() to unmap_and_put_page()

[Fabio M. De Francesco]

With commit 849ad04cf562a ("new helper: put_and_unmap_page()"), Al Viro
introduced the put_and_unmap_page() to use in those many places where we
have a common pattern consisting of calls to kunmap_local() +
put_page().

Obviously, first we unmap and then we put pages. Instead, the original
name of this helper seems to imply that we first put and then unmap.

Therefore, rename the helper and change the only known upstreamed user
(i.e., fs/sysv) before this helper enters common use and might become
difficult to find all call sites and break the Kernel builds.

Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---

This is an RFC because I'm pretty sure that Al must have been a good
reason to use this counter intuitive name for his helper. I'm not
probably aware of some obscure helpers names convention.

Furthermore, I know that Al's VFS tree has already used this helper at
least in fs/minix and probably in other filesystems.

Therefore I didn't want to send a "real" patch.

I'm looking forward to hearing from people involved with fs and
especially from Al before sending a real patch or throwing it away.

 fs/sysv/dir.c           | 22 +++++++++++-----------
 fs/sysv/namei.c         |  8 ++++----
 include/linux/highmem.h |  2 +-
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/sysv/dir.c b/fs/sysv/dir.c
index cdb3d632c63d..d6a3bbb550c3 100644
--- a/fs/sysv/dir.c
+++ b/fs/sysv/dir.c
@@ -52,7 +52,7 @@ static int sysv_handle_dirsync(struct inode *dir)
 }
 
 /*
- * Calls to dir_get_page()/put_and_unmap_page() must be nested according to the
+ * Calls to dir_get_page()/unmap_and_put_page() must be nested according to the
  * rules documented in mm/highmem.rst.
  *
  * NOTE: sysv_find_entry() and sysv_dotdot() act as calls to dir_get_page()
@@ -103,11 +103,11 @@ static int sysv_readdir(struct file *file, struct dir_context *ctx)
 			if (!dir_emit(ctx, name, strnlen(name,SYSV_NAMELEN),
 					fs16_to_cpu(SYSV_SB(sb), de->inode),
 					DT_UNKNOWN)) {
-				put_and_unmap_page(page, kaddr);
+				unmap_and_put_page(page, kaddr);
 				return 0;
 			}
 		}
-		put_and_unmap_page(page, kaddr);
+		unmap_and_put_page(page, kaddr);
 	}
 	return 0;
 }
@@ -131,7 +131,7 @@ static inline int namecompare(int len, int maxlen,
  * itself (as a parameter - res_dir). It does NOT read the inode of the
  * entry - you'll have to do that yourself if you want to.
  *
- * On Success put_and_unmap_page() should be called on *res_page.
+ * On Success unmap_iand_put_page() should be called on *res_page.
  *
  * sysv_find_entry() acts as a call to dir_get_page() and must be treated
  * accordingly for nesting purposes.
@@ -166,7 +166,7 @@ struct sysv_dir_entry *sysv_find_entry(struct dentry *dentry, struct page **res_
 							name, de->name))
 					goto found;
 			}
-			put_and_unmap_page(page, kaddr);
+			unmap_and_put_page(page, kaddr);
 		}
 
 		if (++n >= npages)
@@ -209,7 +209,7 @@ int sysv_add_link(struct dentry *dentry, struct inode *inode)
 				goto out_page;
 			de++;
 		}
-		put_and_unmap_page(page, kaddr);
+		unmap_and_put_page(page, kaddr);
 	}
 	BUG();
 	return -EINVAL;
@@ -228,7 +228,7 @@ int sysv_add_link(struct dentry *dentry, struct inode *inode)
 	mark_inode_dirty(dir);
 	err = sysv_handle_dirsync(dir);
 out_page:
-	put_and_unmap_page(page, kaddr);
+	unmap_and_put_page(page, kaddr);
 	return err;
 out_unlock:
 	unlock_page(page);
@@ -321,12 +321,12 @@ int sysv_empty_dir(struct inode * inode)
 			if (de->name[1] != '.' || de->name[2])
 				goto not_empty;
 		}
-		put_and_unmap_page(page, kaddr);
+		unmap_and_put_page(page, kaddr);
 	}
 	return 1;
 
 not_empty:
-	put_and_unmap_page(page, kaddr);
+	unmap_iand_put_page(page, kaddr);
 	return 0;
 }
 
@@ -352,7 +352,7 @@ int sysv_set_link(struct sysv_dir_entry *de, struct page *page,
 }
 
 /*
- * Calls to dir_get_page()/put_and_unmap_page() must be nested according to the
+ * Calls to dir_get_page()/unmap_and_put_page() must be nested according to the
  * rules documented in mm/highmem.rst.
  *
  * sysv_dotdot() acts as a call to dir_get_page() and must be treated
@@ -376,7 +376,7 @@ ino_t sysv_inode_by_name(struct dentry *dentry)
 	
 	if (de) {
 		res = fs16_to_cpu(SYSV_SB(dentry->d_sb), de->inode);
-		put_and_unmap_page(page, de);
+		unmap_and_put_page(page, de);
 	}
 	return res;
 }
diff --git a/fs/sysv/namei.c b/fs/sysv/namei.c
index 2b2dba4c4f56..fcf163fea3ad 100644
--- a/fs/sysv/namei.c
+++ b/fs/sysv/namei.c
@@ -164,7 +164,7 @@ static int sysv_unlink(struct inode * dir, struct dentry * dentry)
 		inode->i_ctime = dir->i_ctime;
 		inode_dec_link_count(inode);
 	}
-	put_and_unmap_page(page, de);
+	unmap_and_put_page(page, de);
 	return err;
 }
 
@@ -227,7 +227,7 @@ static int sysv_rename(struct mnt_idmap *idmap, struct inode *old_dir,
 		if (!new_de)
 			goto out_dir;
 		err = sysv_set_link(new_de, new_page, old_inode);
-		put_and_unmap_page(new_page, new_de);
+		unmap_and_put_page(new_page, new_de);
 		if (err)
 			goto out_dir;
 		new_inode->i_ctime = current_time(new_inode);
@@ -256,9 +256,9 @@ static int sysv_rename(struct mnt_idmap *idmap, struct inode *old_dir,
 
 out_dir:
 	if (dir_de)
-		put_and_unmap_page(dir_page, dir_de);
+		unmap_and_put_page(dir_page, dir_de);
 out_old:
-	put_and_unmap_page(old_page, old_de);
+	unmap_and_put_page(old_page, old_de);
 out:
 	return err;
 }
diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index 4de1dbcd3ef6..68da30625a6c 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -507,7 +507,7 @@ static inline void folio_zero_range(struct folio *folio,
 	zero_user_segments(&folio->page, start, start + length, 0, 0);
 }
 
-static inline void put_and_unmap_page(struct page *page, void *addr)
+static inline void unmap_and_put_page(struct page *page, void *addr)
 {
 	kunmap_local(addr);
 	put_page(page);
-- 
2.40.1

Re: [RFC PATCH] fs: Rename put_and_unmap_page() to unmap_and_put_page()

[kernel test robot]

Hi Fabio,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on vfs-idmapping/for-next]
[also build test ERROR on linus/master v6.4-rc4 next-20230601]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Fabio-M-De-Francesco/fs-Rename-put_and_unmap_page-to-unmap_and_put_page/20230601-220353
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git for-next
patch link:    https://lore.kernel.org/r/20230601132317.13606-1-fmdefrancesco%40gmail.com
patch subject: [RFC PATCH] fs: Rename put_and_unmap_page() to unmap_and_put_page()
config: hexagon-randconfig-r014-20230601 (https://download.01.org/0day-ci/archive/20230602/202306021044.B12tl43B-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 4faf3aaf28226a4e950c103a14f6fc1d1fdabb1b)
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/e5e2b8aebe19674f6389f850dedf4dd6cabb434f
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Fabio-M-De-Francesco/fs-Rename-put_and_unmap_page-to-unmap_and_put_page/20230601-220353
        git checkout e5e2b8aebe19674f6389f850dedf4dd6cabb434f
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash fs/sysv/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306021044.B12tl43B-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from fs/sysv/dir.c:17:
   In file included from include/linux/pagemap.h:11:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from fs/sysv/dir.c:17:
   In file included from include/linux/pagemap.h:11:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from fs/sysv/dir.c:17:
   In file included from include/linux/pagemap.h:11:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
>> fs/sysv/dir.c:329:2: error: call to undeclared function 'unmap_iand_put_page'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
           unmap_iand_put_page(page, kaddr);
           ^
   fs/sysv/dir.c:329:2: note: did you mean 'unmap_and_put_page'?
   include/linux/highmem.h:510:20: note: 'unmap_and_put_page' declared here
   static inline void unmap_and_put_page(struct page *page, void *addr)
                      ^
   6 warnings and 1 error generated.


vim +/unmap_iand_put_page +329 fs/sysv/dir.c

   288	
   289	/*
   290	 * routine to check that the specified directory is empty (for rmdir)
   291	 */
   292	int sysv_empty_dir(struct inode * inode)
   293	{
   294		struct super_block *sb = inode->i_sb;
   295		struct page *page = NULL;
   296		unsigned long i, npages = dir_pages(inode);
   297		char *kaddr;
   298	
   299		for (i = 0; i < npages; i++) {
   300			struct sysv_dir_entry *de;
   301	
   302			kaddr = dir_get_page(inode, i, &page);
   303			if (IS_ERR(kaddr))
   304				continue;
   305	
   306			de = (struct sysv_dir_entry *)kaddr;
   307			kaddr += PAGE_SIZE-SYSV_DIRSIZE;
   308	
   309			for ( ;(char *)de <= kaddr; de++) {
   310				if (!de->inode)
   311					continue;
   312				/* check for . and .. */
   313				if (de->name[0] != '.')
   314					goto not_empty;
   315				if (!de->name[1]) {
   316					if (de->inode == cpu_to_fs16(SYSV_SB(sb),
   317								inode->i_ino))
   318						continue;
   319					goto not_empty;
   320				}
   321				if (de->name[1] != '.' || de->name[2])
   322					goto not_empty;
   323			}
   324			unmap_and_put_page(page, kaddr);
   325		}
   326		return 1;
   327	
   328	not_empty:
 > 329		unmap_iand_put_page(page, kaddr);
   330		return 0;
   331	}
   332	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC PATCH] fs: Rename put_and_unmap_page() to unmap_and_put_page()

[Fabio M. De Francesco]

On giovedì 1 giugno 2023 15:23:17 CEST Fabio M. De Francesco wrote:
> With commit 849ad04cf562a ("new helper: put_and_unmap_page()"), Al Viro
> introduced the put_and_unmap_page() to use in those many places where we
> have a common pattern consisting of calls to kunmap_local() +
> put_page().
> 
> Obviously, first we unmap and then we put pages. Instead, the original
> name of this helper seems to imply that we first put and then unmap.
> 
> Therefore, rename the helper and change the only known upstreamed user
> (i.e., fs/sysv) before this helper enters common use and might become
> difficult to find all call sites and break the Kernel builds.
> 
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
> 
> This is an RFC 

Please discard this RFC.

I just sent the real patch at
https://lore.kernel.org/linux-fsdevel/20230602103307.5637-1-fmdefrancesco@gmail.com/T/#u

and added linux-mm to the list of recipients.

Thanks,

Fabio

> because I'm pretty sure that Al must have been a good
> reason to use this counter intuitive name for his helper. I'm not
> probably aware of some obscure helpers names convention.
> 
> Furthermore, I know that Al's VFS tree has already used this helper at
> least in fs/minix and probably in other filesystems.
> 
> Therefore I didn't want to send a "real" patch.
> 
> I'm looking forward to hearing from people involved with fs and
> especially from Al before sending a real patch or throwing it away.
> 
>  fs/sysv/dir.c           | 22 +++++++++++-----------
>  fs/sysv/namei.c         |  8 ++++----
>  include/linux/highmem.h |  2 +-
>  3 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/sysv/dir.c b/fs/sysv/dir.c
> index cdb3d632c63d..d6a3bbb550c3 100644
> --- a/fs/sysv/dir.c
> +++ b/fs/sysv/dir.c
> @@ -52,7 +52,7 @@ static int sysv_handle_dirsync(struct inode *dir)
>  }
> 
>  /*
> - * Calls to dir_get_page()/put_and_unmap_page() must be nested according to
> the + * Calls to dir_get_page()/unmap_and_put_page() must be nested 
according
> to the * rules documented in mm/highmem.rst.
>   *
>   * NOTE: sysv_find_entry() and sysv_dotdot() act as calls to dir_get_page()
> @@ -103,11 +103,11 @@ static int sysv_readdir(struct file *file, struct
> dir_context *ctx) if (!dir_emit(ctx, name, strnlen(name,SYSV_NAMELEN),
>  					fs16_to_cpu(SYSV_SB(sb), de-
>inode),
>  					DT_UNKNOWN)) {
> -				put_and_unmap_page(page, kaddr);
> +				unmap_and_put_page(page, kaddr);
>  				return 0;
>  			}
>  		}
> -		put_and_unmap_page(page, kaddr);
> +		unmap_and_put_page(page, kaddr);
>  	}
>  	return 0;
>  }
> @@ -131,7 +131,7 @@ static inline int namecompare(int len, int maxlen,
>   * itself (as a parameter - res_dir). It does NOT read the inode of the
>   * entry - you'll have to do that yourself if you want to.
>   *
> - * On Success put_and_unmap_page() should be called on *res_page.
> + * On Success unmap_iand_put_page() should be called on *res_page.
>   *
>   * sysv_find_entry() acts as a call to dir_get_page() and must be treated
>   * accordingly for nesting purposes.
> @@ -166,7 +166,7 @@ struct sysv_dir_entry *sysv_find_entry(struct dentry
> *dentry, struct page **res_ name, de->name))
>  					goto found;
>  			}
> -			put_and_unmap_page(page, kaddr);
> +			unmap_and_put_page(page, kaddr);
>  		}
> 
>  		if (++n >= npages)
> @@ -209,7 +209,7 @@ int sysv_add_link(struct dentry *dentry, struct inode
> *inode) goto out_page;
>  			de++;
>  		}
> -		put_and_unmap_page(page, kaddr);
> +		unmap_and_put_page(page, kaddr);
>  	}
>  	BUG();
>  	return -EINVAL;
> @@ -228,7 +228,7 @@ int sysv_add_link(struct dentry *dentry, struct inode
> *inode) mark_inode_dirty(dir);
>  	err = sysv_handle_dirsync(dir);
>  out_page:
> -	put_and_unmap_page(page, kaddr);
> +	unmap_and_put_page(page, kaddr);
>  	return err;
>  out_unlock:
>  	unlock_page(page);
> @@ -321,12 +321,12 @@ int sysv_empty_dir(struct inode * inode)
>  			if (de->name[1] != '.' || de->name[2])
>  				goto not_empty;
>  		}
> -		put_and_unmap_page(page, kaddr);
> +		unmap_and_put_page(page, kaddr);
>  	}
>  	return 1;
> 
>  not_empty:
> -	put_and_unmap_page(page, kaddr);
> +	unmap_iand_put_page(page, kaddr);
>  	return 0;
>  }
> 
> @@ -352,7 +352,7 @@ int sysv_set_link(struct sysv_dir_entry *de, struct page
> *page, }
> 
>  /*
> - * Calls to dir_get_page()/put_and_unmap_page() must be nested according to
> the + * Calls to dir_get_page()/unmap_and_put_page() must be nested 
according
> to the * rules documented in mm/highmem.rst.
>   *
>   * sysv_dotdot() acts as a call to dir_get_page() and must be treated
> @@ -376,7 +376,7 @@ ino_t sysv_inode_by_name(struct dentry *dentry)
> 
>  	if (de) {
>  		res = fs16_to_cpu(SYSV_SB(dentry->d_sb), de->inode);
> -		put_and_unmap_page(page, de);
> +		unmap_and_put_page(page, de);
>  	}
>  	return res;
>  }
> diff --git a/fs/sysv/namei.c b/fs/sysv/namei.c
> index 2b2dba4c4f56..fcf163fea3ad 100644
> --- a/fs/sysv/namei.c
> +++ b/fs/sysv/namei.c
> @@ -164,7 +164,7 @@ static int sysv_unlink(struct inode * dir, struct dentry 
*
> dentry) inode->i_ctime = dir->i_ctime;
>  		inode_dec_link_count(inode);
>  	}
> -	put_and_unmap_page(page, de);
> +	unmap_and_put_page(page, de);
>  	return err;
>  }
> 
> @@ -227,7 +227,7 @@ static int sysv_rename(struct mnt_idmap *idmap, struct
> inode *old_dir, if (!new_de)
>  			goto out_dir;
>  		err = sysv_set_link(new_de, new_page, old_inode);
> -		put_and_unmap_page(new_page, new_de);
> +		unmap_and_put_page(new_page, new_de);
>  		if (err)
>  			goto out_dir;
>  		new_inode->i_ctime = current_time(new_inode);
> @@ -256,9 +256,9 @@ static int sysv_rename(struct mnt_idmap *idmap, struct
> inode *old_dir,
> 
>  out_dir:
>  	if (dir_de)
> -		put_and_unmap_page(dir_page, dir_de);
> +		unmap_and_put_page(dir_page, dir_de);
>  out_old:
> -	put_and_unmap_page(old_page, old_de);
> +	unmap_and_put_page(old_page, old_de);
>  out:
>  	return err;
>  }
> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index 4de1dbcd3ef6..68da30625a6c 100644
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -507,7 +507,7 @@ static inline void folio_zero_range(struct folio *folio,
>  	zero_user_segments(&folio->page, start, start + length, 0, 0);
>  }
> 
> -static inline void put_and_unmap_page(struct page *page, void *addr)
> +static inline void unmap_and_put_page(struct page *page, void *addr)
>  {
>  	kunmap_local(addr);
>  	put_page(page);
> --
> 2.40.1