mm: move ->mremap() from file_operations to vm_operations_struct

Quoted 5 lines · Oleg Nesterov

I never understood why ->mremap() lives in file_operations, not in vm_operations_struct. To me vma->vm_file->f_op in move_vma() just looks strange, vma->vm_ops->mremap(new_vma) looks "obviously better".

Quoted 5 lines

And afaics more useful. CRIU remaps vdso, but this does not update mm->context.vdso. OK, probably this does not matter currently, CRIU can't c/r the compat tasks, and 64-bit apps do not use context.vdso. Afaics. Still, I think we might want to have special_mapping_remap() and we can't do this because ->vm_file == NULL.

I would like this. Then I could clean up and resubmit my patch to keep context.vdso up to date.

Oleg, can you let me know what patch, if any, I should be reviewing?

vma->vm_ops->mremap() looks more natural and clean in move_vma(), and this way ->mremap() can have more users. Say, vdso.

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/aio.c           |   25 ++++++++++++++++---------
 include/linux/fs.h |    1 -
 include/linux/mm.h |    1 +
 mm/mremap.c        |    4 ++--

 4 files changed, 19 insertions(+), 12 deletions(-)

fs/aio.c +0 -0

index 9bc1335..6fe662a 100644
@@ -308,15 +308,9 @@ static void aio_free_ring(struct kioctx *ctx)
 	}
 }

fs/aio.c +1 -0

 {
+	struct file *file = vma->vm_file;
 	struct mm_struct *mm = vma->vm_mm;
 	struct kioctx_table *table;
 	int i, res = -EINVAL;

Quoted 3 lines · Oleg Nesterov

vma->vm_ops->mremap() looks more natural and clean in move_vma(), and this way ->mremap() can have more users. Say, vdso.

Quoted 6 lines

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/aio.c           |   25 ++++++++++++++++---------
 include/linux/fs.h |    1 -
 include/linux/mm.h |    1 +
 mm/mremap.c        |    4 ++--

Please, update Documentation/filesystems/Locking.

fs/aio.c static void aio_free_ring(struct kioctx *ctx) · line 311

Quoted 2 lines

fs/aio.c +1 -1

-static int aio_ring_remap(struct file *file, struct vm_area_struct *vma)
+static int aio_ring_remap(struct vm_area_struct *vma)

I guess aio_ring_mremap() would be a better name.

Quoted 13 lines · Andy Lutomirski

Quoted 10 lines · Oleg Nesterov

I never understood why ->mremap() lives in file_operations, not in vm_operations_struct. To me vma->vm_file->f_op in move_vma() just looks strange, vma->vm_ops->mremap(new_vma) looks "obviously better".

And afaics more useful. CRIU remaps vdso, but this does not update mm->context.vdso. OK, probably this does not matter currently, CRIU can't c/r the compat tasks, and 64-bit apps do not use context.vdso. Afaics. Still, I think we might want to have special_mapping_remap() and we can't do this because ->vm_file == NULL.

Quoted 2 lines

I would like this. Then I could clean up and resubmit my patch to keep context.vdso up to date.

Cough... where can I find this patch ? ;)

vma->vm_ops->mremap() looks more natural and clean in move_vma(), and this way ->mremap() can have more users. Say, vdso.

vma->vm_ops->mremap() looks more natural and clean in move_vma(), and this way ->mremap() can have more users. Say, vdso.

Signed-off-by Oleg Nesterov <[email protected]>

Quoted 14 lines · Oleg Nesterov

Quoted 13 lines · Andy Lutomirski

Quoted 10 lines · Oleg Nesterov

I never understood why ->mremap() lives in file_operations, not in vm_operations_struct. To me vma->vm_file->f_op in move_vma() just looks strange, vma->vm_ops->mremap(new_vma) looks "obviously better".

And afaics more useful. CRIU remaps vdso, but this does not update mm->context.vdso. OK, probably this does not matter currently, CRIU can't c/r the compat tasks, and 64-bit apps do not use context.vdso. Afaics. Still, I think we might want to have special_mapping_remap() and we can't do this because ->vm_file == NULL.

Quoted 5 lines

Quoted 2 lines

I would like this. Then I could clean up and resubmit my patch to keep context.vdso up to date.

Cough... where can I find this patch ? ;)

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/log/?h=vdso/vma_tracking

Quoted 12 lines · Kirill A. Shutemov

Quoted 9 lines · Oleg Nesterov

vma->vm_ops->mremap() looks more natural and clean in move_vma(), and this way ->mremap() can have more users. Say, vdso.

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/aio.c           |   25 ++++++++++++++++---------
 include/linux/fs.h |    1 -
 include/linux/mm.h |    1 +
 mm/mremap.c        |    4 ++--

Quoted 1 line

Please, update Documentation/filesystems/Locking.

OK, thanks, will do.

Quoted 14 lines · Oleg Nesterov

Quoted 12 lines · Kirill A. Shutemov

Quoted 9 lines · Oleg Nesterov

vma->vm_ops->mremap() looks more natural and clean in move_vma(), and this way ->mremap() can have more users. Say, vdso.

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/aio.c           |   25 ++++++++++++++++---------
 include/linux/fs.h |    1 -
 include/linux/mm.h |    1 +
 mm/mremap.c        |    4 ++--

Quoted 3 lines

Quoted 1 line

Please, update Documentation/filesystems/Locking.

OK, thanks, will do.

Wait... Documentation/filesystems/Locking doesn't mention ->mremap() at all.

So you actually ask me to add the new documentation? ;)

Oh well... OK, I'll try if you think this is necessary.

I tried to make the minimal change before ->mremap() finds another user in file_operations. I thinks it needs more arguments, at least new_addr and new_len, otherwise it is not easy to document it. The same for f_op->mremap() of course.

Currently this does not matter, the only user is aio.c and VM_DONTEXPAND means that it is not mergeable, so mremap() always creates the new vma.

Hmm. Can't we do this change and add the documentation later?

Quoted 5 lines · Oleg Nesterov

Wait... Documentation/filesystems/Locking doesn't mention ->mremap() at all.

So you actually ask me to add the new documentation? ;)

I tried ;)

Quoted 5 lines

I tried to make the minimal change before ->mremap() finds another user in file_operations. I thinks it needs more arguments, at least new_addr and new_len, otherwise it is not easy to document it. The same for f_op->mremap() of course.

Quoted 5 lines

Currently this does not matter, the only user is aio.c and VM_DONTEXPAND means that it is not mergeable, so mremap() always creates the new vma.

Hmm. Can't we do this change and add the documentation later?

I'm fine with that.

vma->vm_ops->mremap() looks more natural and clean in move_vma(), and this way ->mremap() can have more users. Say, vdso.

While at it, s/aio_ring_remap/aio_ring_mremap/.

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/aio.c           |   25 ++++++++++++++++---------
 include/linux/fs.h |    1 -
 include/linux/mm.h |    1 +
 mm/mremap.c        |    4 ++--
 4 files changed, 19 insertions(+), 12 deletions(-)

fs/aio.c +16 -9

index 9bc1335..a632f14 100644
@@ -308,15 +308,9 @@ static void aio_free_ring(struct kioctx *ctx)
 	}
 }
 
-static int aio_ring_mmap(struct file *file, struct vm_area_struct *vma)
-{
-	vma->vm_flags |= VM_DONTEXPAND;
-	vma->vm_ops = &generic_file_vm_ops;
-	return 0;
-}
-
-static int aio_ring_remap(struct file *file, struct vm_area_struct *vma)
+static int aio_ring_mremap(struct vm_area_struct *vma)
 {
+	struct file *file = vma->vm_file;
 	struct mm_struct *mm = vma->vm_mm;
 	struct kioctx_table *table;
 	int i, res = -EINVAL;

@@ -339,9 +333,22 @@ static int aio_ring_remap(struct file *file, struct vm_area_struct *vma)
 	return res;
 }
 
+static const struct vm_operations_struct aio_ring_vm_ops = {
+	.mremap		= aio_ring_mremap,
+	.fault		= filemap_fault,
+	.map_pages	= filemap_map_pages,
+	.page_mkwrite	= filemap_page_mkwrite,
+};
+
+static int aio_ring_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	vma->vm_flags |= VM_DONTEXPAND;
+	vma->vm_ops = &aio_ring_vm_ops;
+	return 0;
+}
+
 static const struct file_operations aio_ring_fops = {
 	.mmap = aio_ring_mmap,
-	.mremap = aio_ring_remap,
 };
 
 #if IS_ENABLED(CONFIG_MIGRATION)

include/linux/fs.h +0 -1

index 35ec87e..42aac09 100644
@@ -1582,7 +1582,6 @@ struct file_operations {
 	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
 	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
 	int (*mmap) (struct file *, struct vm_area_struct *);
-	int (*mremap)(struct file *, struct vm_area_struct *);
 	int (*open) (struct inode *, struct file *);
 	int (*flush) (struct file *, fl_owner_t id);
 	int (*release) (struct inode *, struct file *);

include/linux/mm.h +1 -0

index a0fe3d3..0295b4a 100644
@@ -244,6 +244,7 @@ struct vm_fault {
 struct vm_operations_struct {
 	void (*open)(struct vm_area_struct * area);
 	void (*close)(struct vm_area_struct * area);
+	int (*mremap)(struct vm_area_struct * area);
 	int (*fault)(struct vm_area_struct *vma, struct vm_fault *vmf);
 	void (*map_pages)(struct vm_area_struct *vma, struct vm_fault *vmf);
 

mm/mremap.c +0 -0

index ed1b13a..aeba807 100644

Quoted 13 lines

mm/mremap.c +2 -2

@@ -276,8 +276,8 @@ static unsigned long move_vma(struct vm_area_struct *vma,
 				     need_rmap_locks);
 	if (moved_len < old_len) {
 		err = -ENOMEM;
-	} else if (vma->vm_file && vma->vm_file->f_op->mremap) {
-		err = vma->vm_file->f_op->mremap(vma->vm_file, new_vma);
+	} else if (vma->vm_ops && vma->vm_ops->mremap) {
+		err = vma->vm_ops->mremap(new_vma);
 	}
 

mm/mremap.c static unsigned long move_vma(struct vm_area_struct *vma, · line 283

Quoted 1 line

mm/mremap.c +0 -0

 	if (unlikely(err)) {

I'm not sure what is target tree for the patch.

Last hunk is not going to
apply on Linus' tree or -next. Hm?

Otherwise, looks good.

Acked-by Kirill A. Shutemov <[email protected]>

vma->vm_ops->mremap() looks more natural and clean in move_vma(), and this way ->mremap() can have more users. Say, vdso.

While at it, s/aio_ring_remap/aio_ring_mremap/.

Signed-off-by Oleg Nesterov <[email protected]>