Re: [PATCH 10/10] fs: replace mmap hook with .mmap_prepare for simple mappings

Jan Kara
2025-06-17T10:36:47Z

On Mon 16-06-25 20:33:29, 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").
>
> In this patch we apply this change to file systems with relatively simple
> mmap() hook logic - exfat, ceph, f2fs, bcachefs, zonefs, btrfs, ocfs2,
> orangefs, nilfs2, romfs, ramfs and aio.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Two small nits below. Otherwise feel free to add:

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

> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 60a621b00c65..37522137c380 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -2330,13 +2330,14 @@ static const struct vm_operations_struct ceph_vmops = {
> .page_mkwrite = ceph_page_mkwrite,
> };
>
> -int ceph_mmap(struct file *file, struct vm_area_struct *vma)
> +int ceph_mmap_prepare(struct vm_area_desc *desc)
> {
> + struct file *file = desc->file;
> struct address_space *mapping = file->f_mapping;

Pointless local variable here...

>
> if (!mapping->a_ops->read_folio)
> return -ENOEXEC;
> - vma->vm_ops = &ceph_vmops;
> + desc->vm_ops = &ceph_vmops;
> return 0;
> }
>
...
> diff --git a/fs/exfat/file.c b/fs/exfat/file.c
> index 841a5b18e3df..d63213c8a823 100644
> --- a/fs/exfat/file.c
> +++ b/fs/exfat/file.c
> @@ -683,13 +683,14 @@ static const struct vm_operations_struct exfat_file_vm_ops = {
> .page_mkwrite = exfat_page_mkwrite,
> };
>
> -static int exfat_file_mmap(struct file *file, struct vm_area_struct *vma)
> +static int exfat_file_mmap_prepare(struct vm_area_desc *desc)
> {
> + struct file *file = desc->file;

Missing empty line here.

> if (unlikely(exfat_forced_shutdown(file_inode(file)->i_sb)))
> return -EIO;
>
> file_accessed(file);
> - vma->vm_ops = &exfat_file_vm_ops;
> + desc->vm_ops = &exfat_file_vm_ops;
> return 0;
> }
>

Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR