Messages in this thread | | | Date | Fri, 17 Jul 2015 19:27:26 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH] mm-move-mremap-from-file_operations-to-vm_operations_struct-fix |
| |
Benjamin,
it seems that we do not understand each other,
On 07/17, Benjamin LaHaise wrote: > > On Fri, Jul 17, 2015 at 01:52:28AM +0200, Oleg Nesterov wrote: > > > > > > > > +int filemap_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf) > > > > +{ > > > > + BUG(); > > > > + return 0; > > > > +} > > > > + > > > > static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm, > > > > unsigned long addr, void *buf, int len, int write) > > > > { > > > > > > So if anyone starts testing aio on NOMMU, this patch will make the > > > whole thing immediately go BUG. This isn't helpful :( > > > > Well, I'm afraid I could miss something, but _afaics_ this can not > > happen. filemap_page_mkwrite() can't be called if NOMMU. > > > > In particular, simply because sys_io_setup() is the only user (if > > NOMMU) and it can't succeed. But even if I missed something and it > > can succeed, ->page_mkwrite() must not be called anyway. But this, > > again, unless I missed something ;) > > > > > Yes, making AIO depend on MMU sounds better. > > > > Perhaps Benjamin can change his mind or correct me. > > Either try to fix it correctly,
And I think this fix is correct. In a sense that we only add filemap_page_mkwrite() to make the linker happy, it can never be called and thus we can never hit this BUG().
Please look at filemap_fault() in nommu.c,
int filemap_fault(struct vm_area_struct *vma, struct vm_fault *vmf) { BUG(); return 0; }
this is the same thing. If nothing else, mm/memory.c is not even compiled if NOMMU.
> or disable the config.
Yes, I think this makes more sense. but see below...
> Making it just > compile but be knowingly broken is worse than either of those 2 options.
Why? See above. I think this change makes no difference except it fixes the build.
Again, of course I could miss something. Could you explain your point?
> My point was that it is valid for someone to want to use the functionality > on a nommu system, and given that it should have worked before the page > migration code was added, It Would Be Nice(tm) to return it to that state.
Perhaps it worked on NOMMU before, I have no idea. But currently, afaics, it can not. Even sys_io_setup() can't suceed. So I do not understand why do we allow NOMMU && CONFIG_AIO.
But this is another issue. Of course I won't insist, please forget.
> Adding a BUG() like that to the code is just plain broken.
Why? Could you explain what I have missed?
Oleg.
| |