Re: LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")

[Kirill A. Shutemov]

On Tue, Sep 15, 2015 at 02:12:01PM +0200, Oleg Nesterov wrote:
> On 09/14, Kirill A. Shutemov wrote:
> >
> > On Mon, Sep 14, 2015 at 07:05:47PM +0200, Oleg Nesterov wrote:
> > > On 09/14, Kirill A. Shutemov wrote:
> > > >
> > > > Fix is below. I don't really like it, but I cannot find any better
> > > > solution.
> > >
> > > Me too...
> > >
> > > But this change "documents" the nasty special "vm_file && !vm_ops" case, and
> > > I am not sure how we can remove it later...
> > >
> > > So perhaps we should change vma_is_anonymous() back to check ->fault too,
> > >
> > > 	 static inline bool vma_is_anonymous(struct vm_area_struct *vma)
> > > 	 {
> > > 	-	return !vma->vm_ops;
> > > 	+	return !vma->vm_ops || !vma->vm_ops->fault;
> >
> > No. This would give a lot false positives from drives which setup page
> > tables upfront and don't use ->fault at all.
> 
> And? I mean, I am not sure I understand what exactly do you dislike.
> 
> Firstly, I still think that (in the long term) we should change them
> to use .faul = no_fault() which just returns VM_FAULT_SIGBUS.

I would rather like to see consolidated fault path between file and anon
with ->vm_ops set for both. So vma_is_anonymous() will be trivial
vma->vm_ops == anon_vm_ops.

> Until then I do not see why the change above can be really bad. The
> VM_SHARED case is fine, do_anonymous_page() will return VM_FAULT_SIGBUS.
> 
> So afaics the only problem is that after the change above the private
> mapping can silently get an anonymous page after (say) MADV_DONTNEED
> instead of the nice SIGBUS from do_fault(). I agree, this is not good,
> but see above.

So, what the point to introduce vma_is_anonymous() if it often produces
false result? vma_is_anonymous_or_maybe_not()?

> Or I missed something else?
> 
> Let me repeat, I am not going to really argue, you understand this all
> much better than me. But imho we should try to avoid the special case
> added by your change as much as possible, in this sense the change above
> looks "obviously better" at least as a short-term fix.
> 
> 
> Whether we need to keep the vm_ops/fault check in __vma_link_rb() and
> mmap_region() is another issue. But if we keep them, then I think we
> should at least turn the !vma->vm_ops check in mmap_region into
> WARN_ON() as well.

It would require first fix all known cases where ->f_op->mmap() returns
vma->vm_ops == NULL. Not subject for 4.3, I think.

-- 
 Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/