On Sun, Mar 15, 2026 at 07:32:54PM -0700, Suren Baghdasaryan wrote: > 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?
Firstly, mmap_prepare is invoked before we try to merge, so the VMA could in theory get merged and then the refcounting will be wrong. Secondly, mmap_prepare occurs at such at time where it is _possible_ that allocation failures as described below could happen. I'll update the commit message to reflect the merge aspect actually. > > > > > > > > > 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. Sure I can drop it, but it's an ongoing thing with these small allocations. I wish we could just move to a scenario where we can simpy assume allocations will always succeed :) Vlasta - thoughts? Cheers, Lorenzo > > > > > > > > > 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
