REVIEWER'S NOTES
================

I am basing this on the mm-new branch in Andrew's tree, so let me know if I
should rebase anything here. Given the mm bits touched I did think perhaps
we should take it through the mm tree, however it may be more sensible to
take it through an fs tree - let me know!

Apologies for the noise/churn, but there are some prerequisite steps here
that inform an ordering - "fs: consistently use file_has_valid_mmap_hooks()
helper" being especially critical, and so I put the bulk of the work in the
same series.

Let me know if there's anything I can do to make life easier here.

Thanks!

===============

In commit c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file
callback"), a new hook for mmap was introduced - f_op->mmap_prepare().

This is preferred to the existing f_op->mmap() hook as it does require a
VMA to be established yet, thus allowing the mmap logic to invoke this hook
far, far earlier, prior to inserting a VMA into the virtual address space,
or performing any other heavy handed operations.

This allows for much simpler unwinding on error, and for there to be a
single attempt at merging a VMA rather than having to possibly reattempt a
merge based on potentially altered VMA state.

Far more importantly, it prevents inappropriate manipulation of
incompletely initialised VMA state, which is something that has been the
cause of bugs and complexity in the past.

The intent is to gradually deprecate f_op->mmap, and in that vein this
series coverts the majority of file systems to using f_op->mmap_prepare.

Prerequisite steps are taken - firstly ensuring all checks for mmap
capabilities use the file_has_valid_mmap_hooks() helper rather than
directly checking for f_op->mmap (which is now not a valid check) and
secondly updating daxdev_mapping_supported() to not require a VMA parameter
to allow ext4 and xfs to be converted.

Commit bb666b7c2707 ("mm: add mmap_prepare() compatibility layer for nested
file systems") handles the nasty edge-case of nested file systems like
overlayfs, which introduces a compatibility shim to allow
f_op->mmap_prepare() to be invoked from an f_op->mmap() callback.

This allows for nested filesystems to continue to function correctly with
all file systems regardless of which callback is used. Once we finally
convert all file systems, this shim can be removed.

As a result, ecryptfs, fuse, and overlayfs remain unaltered so they can
nest all other file systems.

We additionally do not update resctl - as this requires an update to
remap_pfn_range() (or an alternative to it) which we defer to a later
series, equally we do not update cramfs which needs a mixed mapping
insertion with the same issue, nor do we update procfs, hugetlbfs, syfs or
kernfs all of which require VMAs for internal state and hooks. We shall
return to all of these later.

Lorenzo Stoakes (10):
  mm: rename call_mmap/mmap_prepare to vfs_mmap/mmap_prepare
  mm/nommu: use file_has_valid_mmap_hooks() helper
  fs: consistently use file_has_valid_mmap_hooks() helper
  fs/dax: make it possible to check dev dax support without a VMA
  fs/ext4: transition from deprecated .mmap hook to .mmap_prepare
  fs/xfs: transition from deprecated .mmap hook to .mmap_prepare
  mm/filemap: introduce generic_file_*_mmap_prepare() helpers
  fs: convert simple use of generic_file_*_mmap() to .mmap_prepare()
  fs: convert most other generic_file_*mmap() users to .mmap_prepare()
  fs: replace mmap hook with .mmap_prepare for simple mappings

 block/fops.c                               |  9 +++---
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  2 +-
 fs/9p/vfs_file.c                           | 13 +++++----
 fs/adfs/file.c                             |  2 +-
 fs/affs/file.c                             |  2 +-
 fs/afs/file.c                              | 11 ++++----
 fs/aio.c                                   |  8 +++---
 fs/backing-file.c                          |  4 +--
 fs/bcachefs/fs.c                           |  8 +++---
 fs/bfs/file.c                              |  2 +-
 fs/binfmt_elf.c                            |  4 +--
 fs/binfmt_elf_fdpic.c                      |  2 +-
 fs/btrfs/file.c                            |  7 +++--
 fs/ceph/addr.c                             |  5 ++--
 fs/ceph/file.c                             |  2 +-
 fs/ceph/super.h                            |  2 +-
 fs/coda/file.c                             |  6 ++--
 fs/ecryptfs/file.c                         |  2 +-
 fs/erofs/data.c                            | 16 ++++++-----
 fs/exfat/file.c                            |  7 +++--
 fs/ext2/file.c                             | 12 ++++----
 fs/ext4/file.c                             | 13 +++++----
 fs/f2fs/file.c                             |  7 +++--
 fs/fat/file.c                              |  2 +-
 fs/hfs/inode.c                             |  2 +-
 fs/hfsplus/inode.c                         |  2 +-
 fs/hostfs/hostfs_kern.c                    |  2 +-
 fs/hpfs/file.c                             |  2 +-
 fs/jffs2/file.c                            |  2 +-
 fs/jfs/file.c                              |  2 +-
 fs/minix/file.c                            |  2 +-
 fs/nfs/file.c                              | 13 +++++----
 fs/nfs/internal.h                          |  2 +-
 fs/nfs/nfs4file.c                          |  2 +-
 fs/nilfs2/file.c                           |  8 +++---
 fs/ntfs3/file.c                            | 15 +++++-----
 fs/ocfs2/file.c                            |  4 +--
 fs/ocfs2/mmap.c                            |  5 ++--
 fs/ocfs2/mmap.h                            |  2 +-
 fs/omfs/file.c                             |  2 +-
 fs/orangefs/file.c                         | 10 ++++---
 fs/ramfs/file-mmu.c                        |  2 +-
 fs/ramfs/file-nommu.c                      | 12 ++++----
 fs/read_write.c                            |  2 +-
 fs/romfs/mmap-nommu.c                      |  6 ++--
 fs/smb/client/cifsfs.c                     | 12 ++++----
 fs/smb/client/cifsfs.h                     |  4 +--
 fs/smb/client/file.c                       | 14 ++++++----
 fs/ubifs/file.c                            |  8 +++---
 fs/ufs/file.c                              |  2 +-
 fs/vboxsf/file.c                           |  8 +++---
 fs/xfs/xfs_file.c                          | 15 +++++-----
 fs/zonefs/file.c                           | 10 ++++---
 include/linux/dax.h                        | 16 ++++++-----
 include/linux/fs.h                         | 11 ++++----
 ipc/shm.c                                  |  2 +-
 mm/filemap.c                               | 29 ++++++++++++++++++++
 mm/internal.h                              |  2 +-
 mm/nommu.c                                 |  2 +-
 mm/vma.c                                   |  2 +-
 tools/testing/vma/vma_internal.h           | 32 ++++++++++++++++++----
 61 files changed, 245 insertions(+), 171 deletions(-)

--
2.49.0

On Mon, Jun 16, 2025 at 08:33:19PM +0100, Lorenzo Stoakes wrote:
> REVIEWER'S NOTES
> ================
> 
> I am basing this on the mm-new branch in Andrew's tree, so let me know if I
> should rebase anything here. Given the mm bits touched I did think perhaps
> we should take it through the mm tree, however it may be more sensible to
> take it through an fs tree - let me know!
> 
> Apologies for the noise/churn, but there are some prerequisite steps here
> that inform an ordering - "fs: consistently use file_has_valid_mmap_hooks()
> helper" being especially critical, and so I put the bulk of the work in the
> same series.
> 
> Let me know if there's anything I can do to make life easier here.

Documentation/filesystems/porting.rst?

On Mon, 2025-06-16 at 21:41 +0100, Al Viro wrote:
> On Mon, Jun 16, 2025 at 08:33:19PM +0100, Lorenzo Stoakes wrote:
> > REVIEWER'S NOTES
> > ================
> > 
> > I am basing this on the mm-new branch in Andrew's tree, so let me know if I
> > should rebase anything here. Given the mm bits touched I did think perhaps
> > we should take it through the mm tree, however it may be more sensible to
> > take it through an fs tree - let me know!
> > 
> > Apologies for the noise/churn, but there are some prerequisite steps here
> > that inform an ordering - "fs: consistently use file_has_valid_mmap_hooks()
> > helper" being especially critical, and so I put the bulk of the work in the
> > same series.
> > 
> > Let me know if there's anything I can do to make life easier here.
> 
> Documentation/filesystems/porting.rst?

Also, an entry for ->mmap_prepare in Documentation/filesystems/vfs.rst
would be good.

I went there first to understand what the requirements of mmap_prepare
are, but there is nothing.
-- 
Jeff Layton <jlayton@kernel.org>

On Tue, Jun 17, 2025 at 09:45:32AM -0400, Jeff Layton wrote:
> On Mon, 2025-06-16 at 21:41 +0100, Al Viro wrote:
> > On Mon, Jun 16, 2025 at 08:33:19PM +0100, Lorenzo Stoakes wrote:
> > > REVIEWER'S NOTES
> > > ================
> > >
> > > I am basing this on the mm-new branch in Andrew's tree, so let me know if I
> > > should rebase anything here. Given the mm bits touched I did think perhaps
> > > we should take it through the mm tree, however it may be more sensible to
> > > take it through an fs tree - let me know!
> > >
> > > Apologies for the noise/churn, but there are some prerequisite steps here
> > > that inform an ordering - "fs: consistently use file_has_valid_mmap_hooks()
> > > helper" being especially critical, and so I put the bulk of the work in the
> > > same series.
> > >
> > > Let me know if there's anything I can do to make life easier here.
> >
> > Documentation/filesystems/porting.rst?
>
> Also, an entry for ->mmap_prepare in Documentation/filesystems/vfs.rst
> would be good.
>
> I went there first to understand what the requirements of mmap_prepare
> are, but there is nothing.

Ack, on it.

> --
> Jeff Layton <jlayton@kernel.org>
>

Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:

> This is preferred to the existing f_op->mmap() hook as it does require a
> VMA to be established yet,

Did you mean ".. doesn't require a VMA to be established yet, ..."

David

On Tue, Jun 17, 2025 at 03:05:59PM +0100, David Howells wrote:
> Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
>
> > This is preferred to the existing f_op->mmap() hook as it does require a
> > VMA to be established yet,
>
> Did you mean ".. doesn't require a VMA to be established yet, ..."
>
> David
>

Yeah apologies, indeed I did :)

On Mon, 16 Jun 2025 20:33:19 +0100, Lorenzo Stoakes wrote:
> REVIEWER'S NOTES
> ================
> 
> I am basing this on the mm-new branch in Andrew's tree, so let me know if I
> should rebase anything here. Given the mm bits touched I did think perhaps
> we should take it through the mm tree, however it may be more sensible to
> take it through an fs tree - let me know!
> 
> [...]

This looks good. I fixed up the minor review comments.
Looking forward to further cleanups in this area.

---

Applied to the vfs-6.17.mmap_prepare branch of the vfs/vfs.git tree.
Patches in the vfs-6.17.mmap_prepare branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.17.mmap_prepare

[01/10] mm: rename call_mmap/mmap_prepare to vfs_mmap/mmap_prepare
        https://git.kernel.org/vfs/vfs/c/20ca475d9860
[02/10] mm/nommu: use file_has_valid_mmap_hooks() helper
        https://git.kernel.org/vfs/vfs/c/c6900f227f89
[03/10] fs: consistently use file_has_valid_mmap_hooks() helper
        https://git.kernel.org/vfs/vfs/c/b013ed403197
[04/10] fs/dax: make it possible to check dev dax support without a VMA
        https://git.kernel.org/vfs/vfs/c/0335f6afd348
[05/10] fs/ext4: transition from deprecated .mmap hook to .mmap_prepare
        https://git.kernel.org/vfs/vfs/c/8c90ae8fe5e3
[06/10] fs/xfs: transition from deprecated .mmap hook to .mmap_prepare
        https://git.kernel.org/vfs/vfs/c/6528d29b46d8
[07/10] mm/filemap: introduce generic_file_*_mmap_prepare() helpers
        https://git.kernel.org/vfs/vfs/c/5b44297bcfa4
[08/10] fs: convert simple use of generic_file_*_mmap() to .mmap_prepare()
        https://git.kernel.org/vfs/vfs/c/951ea2f4844c
[09/10] fs: convert most other generic_file_*mmap() users to .mmap_prepare()
        https://git.kernel.org/vfs/vfs/c/a5ee9a82981d
[10/10] fs: replace mmap hook with .mmap_prepare for simple mappings
        https://git.kernel.org/vfs/vfs/c/a1e5b36c4034

On Mon, Jun 16, 2025 at 08:33:19PM +0100, Lorenzo Stoakes wrote:

> The intent is to gradually deprecate f_op->mmap, and in that vein this
> series coverts the majority of file systems to using f_op->mmap_prepare.

I saw this on lwn and just wanted to give a little bit of thought on
this topic..

It looks to me like we need some more infrastructure to convert
anything that uses remap_pfn/etc in the mmap() callback

I would like to suggest we add a vma->prepopulate() callback which is
where the remap_pfn should go. Once the VMA is finalized and fully
operational the vma_ops have the opportunity to prepopulate any PTEs.

This could then actually be locked properly so it is safe with
concurrent unmap_mapping_range() (current mmap callback is not safe)

Jason

[culling cc list down because for it was insane, +David]

(apologies if I cut anybody who wants to be involved in discussion, just
being practical here).

On Fri, Aug 01, 2025 at 11:00:57AM -0300, Jason Gunthorpe wrote:
> On Mon, Jun 16, 2025 at 08:33:19PM +0100, Lorenzo Stoakes wrote:
>
> > The intent is to gradually deprecate f_op->mmap, and in that vein this
> > series coverts the majority of file systems to using f_op->mmap_prepare.
>
> I saw this on lwn and just wanted to give a little bit of thought on
> this topic..

Thanks, sure.

>
> It looks to me like we need some more infrastructure to convert
> anything that uses remap_pfn/etc in the mmap() callback

Yes absolutely :) I realised we needed extra infra, and later noticed (and
in discussion with others) that remap in particular would be a pain.

I wanted to lay the groundwork first, as we could safely do this, convert
nearly all file systems to .mmap_prepare(), and get a big win on that, then
take it further in the next cycle, which I was planning to do.

>
> I would like to suggest we add a vma->prepopulate() callback which is
> where the remap_pfn should go. Once the VMA is finalized and fully
> operational the vma_ops have the opportunity to prepopulate any PTEs.

I assume you mean vma->vm_ops->prepopulate ?

We also have to think about other places where we prepopulate also, for
instance the perf mmap call now prepopulates (ahem that was me).

So I do like this as a generalisation.

>
> This could then actually be locked properly so it is safe with
> concurrent unmap_mapping_range() (current mmap callback is not safe)

Which lock in particular is problematic? You'd want to hold an rmap write
lock to avoid racing zap?

>
> Jason

BTW I'll loop you in on discussion/series here. I think what you outline is
likely how this will work.

Cheers, Lorenzo

>>
>> I would like to suggest we add a vma->prepopulate() callback which is
>> where the remap_pfn should go. Once the VMA is finalized and fully
>> operational the vma_ops have the opportunity to prepopulate any PTEs.
> 
> I assume you mean vma->vm_ops->prepopulate ?
> 
> We also have to think about other places where we prepopulate also, for
> instance the perf mmap call now prepopulates (ahem that was me).
> 
> So I do like this as a generalisation.

Sounds interesting to me.

-- 
Cheers,

David / dhildenb

On Fri, Aug 01, 2025 at 03:12:48PM +0100, Lorenzo Stoakes wrote:
> > I would like to suggest we add a vma->prepopulate() callback which is
> > where the remap_pfn should go. Once the VMA is finalized and fully
> > operational the vma_ops have the opportunity to prepopulate any PTEs.
> 
> I assume you mean vma->vm_ops->prepopulate ?

Yes

> We also have to think about other places where we prepopulate also, for
> instance the perf mmap call now prepopulates (ahem that was me).

Yes, vfio would also like to do this but can't due to the below issue.

> > This could then actually be locked properly so it is safe with
> > concurrent unmap_mapping_range() (current mmap callback is not safe)
> 
> Which lock in particular is problematic? You'd want to hold an rmap write
> lock to avoid racing zap?

I have forgotten, but there was a race with how the current mmap op
was called relative to when the VMA was tracked.

ie we should be able to do 

     CPU0                              CPU1
vm_ops_prepopulate()
   mutex_lock()
   if (!is_mapping_valid)
      return -EINVAL;                                       
   <fill ptes>
   mutex_unlock()
                                        mutex_lock()
					is_mapping_valid = false
					unmap_mapping_range()	  
                                        mutex_unlock()

And be sure there are no races. Use the lock of your choice for the
mutex.

The above is not true today under mmap, IIRC the VMA is not added to
the lists that unmap_mapping_range walks until after mmap() returns.

Jason

On Mon, 16 Jun 2025 20:33:19 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:

> I am basing this on the mm-new branch in Andrew's tree, so let me know if I
> should rebase anything here. Given the mm bits touched I did think perhaps
> we should take it through the mm tree, however it may be more sensible to
> take it through an fs tree - let me know!

It's more fs/ than mm/ purely from a footprint point of view.  But is
there any expectation that there will be additional patches which build
on this?

I'll scoop it into mm-new for now, see what happens.

Minus all the cc's.  Sorry ;)

On Mon, Jun 16, 2025 at 04:11:11PM -0700, Andrew Morton wrote:
> On Mon, 16 Jun 2025 20:33:19 +0100 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> 
> > I am basing this on the mm-new branch in Andrew's tree, so let me know if I
> > should rebase anything here. Given the mm bits touched I did think perhaps
> > we should take it through the mm tree, however it may be more sensible to
> > take it through an fs tree - let me know!
> 
> It's more fs/ than mm/ purely from a footprint point of view.  But
> there any expectation that there will be additional patches which build
> on this?
> 
> I'll scoop it into mm-new for now, see what happens.

I'm going to carry this in the vfs-6.17.mmap_prepare branch after fixing
up the various minor issues spotted in the series.

Mailserver is rejecting with "too many recipients" - aieeee

On Mon, Jun 16, 2025 at 08:33:19PM +0100, Lorenzo Stoakes wrote:
> REVIEWER'S NOTES
> ================
> 
> I am basing this on the mm-new branch in Andrew's tree, so let me know if I
> should rebase anything here. Given the mm bits touched I did think perhaps
> we should take it through the mm tree, however it may be more sensible to
> take it through an fs tree - let me know!
> 
> Apologies for the noise/churn, but there are some prerequisite steps here
> that inform an ordering - "fs: consistently use file_has_valid_mmap_hooks()
> helper" being especially critical, and so I put the bulk of the work in the
> same series.
> 
> Let me know if there's anything I can do to make life easier here.

This seems to be more of an mm thing than a filesystem thing? I don't
see any code changes on the filesystem side from a quick scan, just
renaming?

Are there any behavioural changes for the filesystem to be aware of?

How's it been tested, any considerations there?

If this is as simple as it looks, ack from me (and if it is that simply,
why so many recipients?).

If there are _any_ behavioural changes on the mm side that might
interact with the filesystem in funny ways, please make sure the whole
fstests suite has been run.