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@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
> ---
>
> 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@xxxxxxxxx/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
