On Fri, Mar 13, 2026 at 5:00 AM Lorenzo Stoakes (Oracle) <[email protected]> wrote: > > On Fri, Mar 13, 2026 at 04:07:43AM -0700, Usama Arif wrote: > > On Thu, 12 Mar 2026 20:27:20 +0000 "Lorenzo Stoakes (Oracle)" > > <[email protected]> wrote: > > > > > Commit 9d5403b1036c ("fs: convert most other generic_file_*mmap() users to > > > .mmap_prepare()") updated AFS to use the mmap_prepare callback in favour > > > of > > > the deprecated mmap callback. > > > > > > However, it did not account for the fact that mmap_prepare can fail to map > > > due to an out of memory error, and thus should not be incrementing a > > > reference count on mmap_prepare.
This is a bit confusing. I see the current implementation does afs_add_open_mmap() and then if generic_file_mmap_prepare() fails it does afs_drop_open_mmap(), therefore refcounting seems to be balanced. Is there really a problem? > > > > > > With the newly added vm_ops->mapped callback available, we can simply > > > defer > > > this operation to that callback which is only invoked once the mapping is > > > successfully in place (but not yet visible to userspace as the mmap and > > > VMA > > > write locks are held). > > > > > > Therefore add afs_mapped() to implement this callback for AFS. > > > > > > In practice the mapping allocations are 'too small to fail' so this is > > > something that realistically should never happen in practice (or would do > > > so in a case where the process is about to die anyway), but we should > > > still > > > handle this. nit: I would drop the above paragraph. If it's impossible why are you handling it? If it's unlikely, then handling it is even more important. > > > > > > Signed-off-by: Lorenzo Stoakes (Oracle) <[email protected]> > > > --- > > > fs/afs/file.c | 20 ++++++++++++++++---- > > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > > > diff --git a/fs/afs/file.c b/fs/afs/file.c > > > index f609366fd2ac..69ef86f5e274 100644 > > > --- a/fs/afs/file.c > > > +++ b/fs/afs/file.c > > > @@ -28,6 +28,8 @@ static ssize_t afs_file_splice_read(struct file *in, > > > loff_t *ppos, > > > static void afs_vm_open(struct vm_area_struct *area); > > > static void afs_vm_close(struct vm_area_struct *area); > > > static vm_fault_t afs_vm_map_pages(struct vm_fault *vmf, pgoff_t > > > start_pgoff, pgoff_t end_pgoff); > > > +static int afs_mapped(unsigned long start, unsigned long end, pgoff_t > > > pgoff, > > > + const struct file *file, void **vm_private_data); > > > > > > const struct file_operations afs_file_operations = { > > > .open = afs_open, > > > @@ -61,6 +63,7 @@ const struct address_space_operations afs_file_aops = { > > > }; > > > > > > static const struct vm_operations_struct afs_vm_ops = { > > > + .mapped = afs_mapped, > > > .open = afs_vm_open, > > > .close = afs_vm_close, > > > .fault = filemap_fault, > > > @@ -500,13 +503,22 @@ static int afs_file_mmap_prepare(struct > > > vm_area_desc *desc) > > > afs_add_open_mmap(vnode); > > > > Is the above afs_add_open_mmap an additional one, which could cause a > > reference > > leak? Does the above one need to be removed and only the one in afs_mapped() > > needs to be kept? > > Ah yeah good spot, will fix thanks! > > > > > > > > > ret = generic_file_mmap_prepare(desc); > > > - if (ret == 0) > > > - desc->vm_ops = &afs_vm_ops; > > > - else > > > - afs_drop_open_mmap(vnode); > > > + if (ret) > > > + return ret; > > > + > > > + desc->vm_ops = &afs_vm_ops; > > > return ret; > > > } > > > > > > +static int afs_mapped(unsigned long start, unsigned long end, pgoff_t > > > pgoff, > > > + const struct file *file, void **vm_private_data) > > > +{ > > > + struct afs_vnode *vnode = AFS_FS_I(file_inode(file)); > > > + > > > + afs_add_open_mmap(vnode); > > > + return 0; > > > +} > > > + > > > static void afs_vm_open(struct vm_area_struct *vma) > > > { > > > afs_add_open_mmap(AFS_FS_I(file_inode(vma->vm_file))); > > > -- > > > 2.53.0 > > > > > > > > Cheers, Lorenzo
