lkml.org 
[lkml]   [2015]   [Sep]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: LTP regressions due to 6dc296e7df4c ("mm: make sure all file VMAs have ->vm_ops set")
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


\
 
 \ /
  Last update: 2015-09-15 16:01    [W:0.402 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site