Messages in this thread | | | Date | Fri, 1 Jul 2022 09:58:13 -0600 | Subject | Re: [PATCH v2 7/9] mm/mshare: Add unlink and munmap support | From | Khalid Aziz <> |
| |
On 6/30/22 15:50, Darrick J. Wong wrote: > On Wed, Jun 29, 2022 at 04:53:58PM -0600, Khalid Aziz wrote: >> Number of mappings of an mshare region should be tracked so it can >> be removed when there are no more references to it and associated >> file has been deleted. This add code to support the unlink operation >> for associated file, remove the mshare region on file deletion if >> refcount goes to zero, add munmap operation to maintain refcount >> to mshare region and remove it on last munmap if file has been >> deleted. >> >> Signed-off-by: Khalid Aziz <khalid.aziz@oracle.com> >> --- >> mm/mshare.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 63 insertions(+), 1 deletion(-) >> >> diff --git a/mm/mshare.c b/mm/mshare.c >> index 088a6cab1e93..90ce0564a138 100644 >> --- a/mm/mshare.c >> +++ b/mm/mshare.c >> @@ -29,6 +29,7 @@ static struct super_block *msharefs_sb; >> struct mshare_data { >> struct mm_struct *mm; >> refcount_t refcnt; >> + int deleted; >> struct mshare_info *minfo; >> }; >> >> @@ -48,6 +49,7 @@ msharefs_read(struct kiocb *iocb, struct iov_iter *iov) >> size_t ret; >> struct mshare_info m_info; >> >> + mmap_read_lock(info->mm); >> if (info->minfo != NULL) { >> m_info.start = info->minfo->start; >> m_info.size = info->minfo->size; >> @@ -55,18 +57,42 @@ msharefs_read(struct kiocb *iocb, struct iov_iter *iov) >> m_info.start = 0; >> m_info.size = 0; >> } >> + mmap_read_unlock(info->mm); >> ret = copy_to_iter(&m_info, sizeof(m_info), iov); >> if (!ret) >> return -EFAULT; >> return ret; >> } >> >> +static void >> +msharefs_close(struct vm_area_struct *vma) >> +{ >> + struct mshare_data *info = vma->vm_private_data; >> + >> + if (refcount_dec_and_test(&info->refcnt)) { >> + mmap_read_lock(info->mm); >> + if (info->deleted) { >> + mmap_read_unlock(info->mm); >> + mmput(info->mm); >> + kfree(info->minfo); >> + kfree(info); > > Aren't filesystems supposed to take care of disposing of the file data > in destroy_inode? IIRC struct inode doesn't go away until all fds are > closed, mappings are torn down, and there are no more references from > dentries. I could be misremembering since it's been a few months since > I went looking at the (VFS) inode lifecycle.
Documentation (vfs.rst) says - "this method is called by destroy_inode() to release resources allocated for struct inode. It is only required if ->alloc_inode was defined and simply undoes anything done by ->alloc_inode.". I am not defining alloc_inode, so I assumed I do not need to define destroy_inode and the standard destroy_inode will do the right thing since standard alloc_inode is being used.
Are you suggesting per-region mshare_data should be freed in destroy_inode instead of in close?
> >> + } else { >> + mmap_read_unlock(info->mm); >> + } >> + } >> +} >> + >> +static const struct vm_operations_struct msharefs_vm_ops = { >> + .close = msharefs_close, >> +}; >> + >> static int >> msharefs_mmap(struct file *file, struct vm_area_struct *vma) >> { >> struct mshare_data *info = file->private_data; >> struct mm_struct *mm = info->mm; >> >> + mmap_write_lock(mm); >> /* >> * If this mshare region has been set up once already, bail out >> */ >> @@ -80,10 +106,14 @@ msharefs_mmap(struct file *file, struct vm_area_struct *vma) >> mm->task_size = vma->vm_end - vma->vm_start; >> if (!mm->task_size) >> mm->task_size--; >> + mmap_write_unlock(mm); >> info->minfo->start = mm->mmap_base; >> info->minfo->size = mm->task_size; >> + info->deleted = 0; >> + refcount_inc(&info->refcnt); >> vma->vm_flags |= VM_SHARED_PT; >> vma->vm_private_data = info; >> + vma->vm_ops = &msharefs_vm_ops; >> return 0; >> } >> >> @@ -240,6 +270,38 @@ msharefs_mkdir(struct user_namespace *mnt_userns, struct inode *dir, >> return ret; >> } >> >> +static int >> +msharefs_unlink(struct inode *dir, struct dentry *dentry) >> +{ >> + struct inode *inode = d_inode(dentry); >> + struct mshare_data *info = inode->i_private; >> + >> + /* >> + * Unmap the mshare region if it is still mapped in >> + */ >> + vm_munmap(info->minfo->start, info->minfo->size); >> + >> + /* >> + * Mark msharefs file for deletion so it can not be opened >> + * and used for mshare mappings any more >> + */ >> + simple_unlink(dir, dentry); >> + mmap_write_lock(info->mm); >> + info->deleted = 1; >> + mmap_write_unlock(info->mm); > > What if the file is hardlinked?
It looks like that is a bug currently. I need to account for that. Thanks!
-- Khalid
| |