Re: [PATCH v4 4/5] cramfs: add mmap support

[Nicolas Pitre]

On Tue, 3 Oct 2017, Christoph Hellwig wrote:

> On Mon, Oct 02, 2017 at 07:33:29PM -0400, Nicolas Pitre wrote:
> > On Tue, 3 Oct 2017, Richard Weinberger wrote:
> > 
> > > On Mon, Oct 2, 2017 at 12:29 AM, Nicolas Pitre <nicolas.pitre@xxxxxxxxxx> wrote:
> > > > On Sun, 1 Oct 2017, Christoph Hellwig wrote:
> > > >
> > > >> up_read(&mm->mmap_sem) in the fault path is a still a complete
> > > >> no-go,
> > > >>
> > > >> NAK
> > > >
> > > > Care to elaborate?
> > > >
> > > > What about mm/filemap.c:__lock_page_or_retry() then?
> > > 
> > > As soon you up_read() in the page fault path other tasks will race
> > > with you before
> > > you're able to grab the write lock.
> > 
> > But I _know_ that.
> > 
> > Could you highlight an area in my code where this is not accounted for?
> 
> Existing users of lock_page_or_retry return VM_FAULT_RETRY right after
> up()ing mmap_sem, and they must already have a reference to the page
> which is the only thing touched until then.
> 
> Your patch instead goes for an exclusive mmap_sem if it can, and
> even if there is nothing that breaks with that scheme right now
> there s nothing documenting that this actually safe, and we are
> way down in the complex page fault path.

It is pretty obvious looking at the existing code that if you want to 
safely manipulate a vma you need the write lock. There are many things 
in the kernel tree that are not explicitly documented. Did that stop 
people from adding new code?

I agree that the fault path is quite complex. I've studied it carefully 
before coming up with this scheme. This is not something that came about 
just because the sunshine felt good when I woke up one day.

So if you agree that I've done a reasonable job creating a scheme that 
currently doesn't break then IMHO this should be good enough, 
*especially* for such an isolated and specialized use case with zero 
impact on anyone else. And if things break in the future than I will be 
the one working out the pieces not you, and _that_ can be written down 
somewhere if necessary so nobody has an obligation to bend backward for 
not breaking it.

Unless you have a better scheme altogether  to suggest of course, given 
the existing constraints.


Nicolas