Re: [PATCH 05/10] fs/ext4: transition from deprecated .mmap hook to .mmap_prepare

On Mon 16-06-25 20:33:24, Lorenzo Stoakes wrote:
> Since commit c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file
> callback"), the f_op->mmap() hook has been deprecated in favour of
> f_op->mmap_prepare().
> 
> This callback is invoked in the mmap() logic far earlier, so error handling
> can be performed more safely without complicated and bug-prone state
> unwinding required should an error arise.
> 
> This hook also avoids passing a pointer to a not-yet-correctly-established
> VMA avoiding any issues with referencing this data structure.
> 
> It rather provides a pointer to the new struct vm_area_desc descriptor type
> which contains all required state and allows easy setting of required
> parameters without any consideration needing to be paid to locking or
> reference counts.
> 
> Note that nested filesystems like overlayfs are compatible with an
> .mmap_prepare() callback since commit bb666b7c2707 ("mm: add mmap_prepare()
> compatibility layer for nested file systems").
> 
> Signed-off-by: Lorenzo Stoakes <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

								Honza

> ---
>  fs/ext4/file.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 08a814fcd956..38180e527dbe 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -804,9 +804,10 @@ static const struct vm_operations_struct ext4_file_vm_ops = {
>  	.page_mkwrite   = ext4_page_mkwrite,
>  };
>  
> -static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
> +static int ext4_file_mmap_prepare(struct vm_area_desc *desc)
>  {
>  	int ret;
> +	struct file *file = desc->file;
>  	struct inode *inode = file->f_mapping->host;
>  	struct dax_device *dax_dev = EXT4_SB(inode->i_sb)->s_daxdev;
>  
> @@ -821,15 +822,15 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma)
>  	 * We don't support synchronous mappings for non-DAX files and
>  	 * for DAX files if underneath dax_device is not synchronous.
>  	 */
> -	if (!daxdev_mapping_supported(vma->vm_flags, vma->vm_file, dax_dev))
> +	if (!daxdev_mapping_supported(desc->vm_flags, file, dax_dev))
>  		return -EOPNOTSUPP;
>  
>  	file_accessed(file);
>  	if (IS_DAX(file_inode(file))) {
> -		vma->vm_ops = &ext4_dax_vm_ops;
> -		vm_flags_set(vma, VM_HUGEPAGE);
> +		desc->vm_ops = &ext4_dax_vm_ops;
> +		desc->vm_flags |= VM_HUGEPAGE;
>  	} else {
> -		vma->vm_ops = &ext4_file_vm_ops;
> +		desc->vm_ops = &ext4_file_vm_ops;
>  	}
>  	return 0;
>  }
> @@ -968,7 +969,7 @@ const struct file_operations ext4_file_operations = {
>  #ifdef CONFIG_COMPAT
>  	.compat_ioctl	= ext4_compat_ioctl,
>  #endif
> -	.mmap		= ext4_file_mmap,
> +	.mmap_prepare	= ext4_file_mmap_prepare,
>  	.open		= ext4_file_open,
>  	.release	= ext4_release_file,
>  	.fsync		= ext4_sync_file,
> -- 
> 2.49.0
> 
-- 
Jan Kara <[email protected]>
SUSE Labs, CR