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

Jan Kara
2025-06-17T10:11:06Z

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 <lorenzo.stoakes@oracle.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

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 <jack@suse.com>
SUSE Labs, CR