Re: [PATCH 09/10] fs: convert most other generic_file_*mmap() users to .mmap_prepare()

[Jan Kara]

On Mon 16-06-25 20:33:28, Lorenzo Stoakes wrote:
> Update nearly all generic_file_mmap() and generic_file_readonly_mmap()
> callers to use generic_file_mmap_prepare() and
> generic_file_readonly_mmap_prepare() respectively.
> 
> We update blkdev, 9p, afs, erofs, ext2, nfs, ntfs3, smb, ubifs and vboxsf
> file systems this way.
> 
> Remaining users we cannot yet update are ecryptfs, fuse and cramfs. The
> former two are nested file systems that must support any underlying file
> ssytem, and cramfs inserts a mixed mapping which currently requires a VMA.
> 
> Once all file systems have been converted to mmap_prepare(), we can then
> update nested file systems.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>

Overall the patch looks good. Just a couple of notes regarding pointless
local variable being created...

> ---
>  block/fops.c           |  9 +++++----
>  fs/9p/vfs_file.c       | 11 ++++++-----
>  fs/afs/file.c          | 11 ++++++-----
>  fs/erofs/data.c        | 16 +++++++++-------
>  fs/ext2/file.c         | 12 +++++++-----
>  fs/nfs/file.c          | 13 +++++++------
>  fs/nfs/internal.h      |  2 +-
>  fs/nfs/nfs4file.c      |  2 +-
>  fs/ntfs3/file.c        | 15 ++++++++-------
>  fs/smb/client/cifsfs.c | 12 ++++++------
>  fs/smb/client/cifsfs.h |  4 ++--
>  fs/smb/client/file.c   | 14 ++++++++------
>  fs/ubifs/file.c        |  8 ++++----
>  fs/vboxsf/file.c       |  8 ++++----
>  14 files changed, 74 insertions(+), 63 deletions(-)
> 
> diff --git a/block/fops.c b/block/fops.c
> index 1309861d4c2c..5a0ebc81e489 100644
> --- a/block/fops.c
> +++ b/block/fops.c
> @@ -911,14 +911,15 @@ static long blkdev_fallocate(struct file *file, int mode, loff_t start,
>  	return error;
>  }
>  
> -static int blkdev_mmap(struct file *file, struct vm_area_struct *vma)
> +static int blkdev_mmap_prepare(struct vm_area_desc *desc)
>  {
> +	struct file *file = desc->file;
>  	struct inode *bd_inode = bdev_file_inode(file);

I guess no need to create 'file' variable here since it has only one use in
the line above...

>  
>  	if (bdev_read_only(I_BDEV(bd_inode)))
> -		return generic_file_readonly_mmap(file, vma);
> +		return generic_file_readonly_mmap_prepare(desc);
>  
> -	return generic_file_mmap(file, vma);
> +	return generic_file_mmap_prepare(desc);
>  }
>  
>  const struct file_operations def_blk_fops = {

...

> @@ -492,16 +492,17 @@ static void afs_drop_open_mmap(struct afs_vnode *vnode)
>  /*
>   * Handle setting up a memory mapping on an AFS file.
>   */
> -static int afs_file_mmap(struct file *file, struct vm_area_struct *vma)
> +static int afs_file_mmap_prepare(struct vm_area_desc *desc)
>  {
> +	struct file *file = desc->file;
>  	struct afs_vnode *vnode = AFS_FS_I(file_inode(file));

Same comment about pointless local variable here as well.

>  	int ret;
>  
>  	afs_add_open_mmap(vnode);
>  
> -	ret = generic_file_mmap(file, vma);
> +	ret = generic_file_mmap_prepare(desc);
>  	if (ret == 0)
> -		vma->vm_ops = &afs_vm_ops;
> +		desc->vm_ops = &afs_vm_ops;
>  	else
>  		afs_drop_open_mmap(vnode);
>  	return ret;
> diff --git a/fs/erofs/data.c b/fs/erofs/data.c
> index 6a329c329f43..52dfd1a44c43 100644
> --- a/fs/erofs/data.c
> +++ b/fs/erofs/data.c
> @@ -409,20 +409,22 @@ static const struct vm_operations_struct erofs_dax_vm_ops = {
>  	.huge_fault	= erofs_dax_huge_fault,
>  };
>  
> -static int erofs_file_mmap(struct file *file, struct vm_area_struct *vma)
> +static int erofs_file_mmap_prepare(struct vm_area_desc *desc)
>  {
> +	struct file *file = desc->file;
> +
>  	if (!IS_DAX(file_inode(file)))

And here...

> -		return generic_file_readonly_mmap(file, vma);
> +		return generic_file_readonly_mmap_prepare(desc);
>  
> -	if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & VM_MAYWRITE))
> +	if ((desc->vm_flags & VM_SHARED) && (desc->vm_flags & VM_MAYWRITE))
>  		return -EINVAL;
>  
> -	vma->vm_ops = &erofs_dax_vm_ops;
> -	vm_flags_set(vma, VM_HUGEPAGE);
> +	desc->vm_ops = &erofs_dax_vm_ops;
> +	desc->vm_flags |= VM_HUGEPAGE;
>  	return 0;
>  }
...
> diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c
> index 9835672267d2..2ed5173cfa73 100644
> --- a/fs/smb/client/file.c
> +++ b/fs/smb/client/file.c
> @@ -2995,8 +2995,9 @@ static const struct vm_operations_struct cifs_file_vm_ops = {
>  	.page_mkwrite = cifs_page_mkwrite,
>  };
>  
> -int cifs_file_strict_mmap(struct file *file, struct vm_area_struct *vma)
> +int cifs_file_strict_mmap_prepare(struct vm_area_desc *desc)
>  {
> +	struct file *file = desc->file;
>  	int xid, rc = 0;
>  	struct inode *inode = file_inode(file);

Again pointless local variable 'file' here.

>  
> @@ -3005,16 +3006,17 @@ int cifs_file_strict_mmap(struct file *file, struct vm_area_struct *vma)
>  	if (!CIFS_CACHE_READ(CIFS_I(inode)))
>  		rc = cifs_zap_mapping(inode);
>  	if (!rc)
> -		rc = generic_file_mmap(file, vma);
> +		rc = generic_file_mmap_prepare(desc);
>  	if (!rc)
> -		vma->vm_ops = &cifs_file_vm_ops;
> +		desc->vm_ops = &cifs_file_vm_ops;
>  
>  	free_xid(xid);
>  	return rc;
>  }
>  
> -int cifs_file_mmap(struct file *file, struct vm_area_struct *vma)
> +int cifs_file_mmap_prepare(struct vm_area_desc *desc)
>  {
> +	struct file *file = desc->file;
>  	int rc, xid;

And here (the only use is in cifs_revalidate_file(file)).

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR