Re: [V9fs-developer] [RFC PATCH] First RFC version of a new cache=mmap model.

From: Dominique Martinet <dominique.martinet@cea.fr>
To: Rob Landley <rob@landley.net>
Cc: ericvh@gmail.com, v9fs-developer@lists.sourceforge.net,
	linux-fsdevel@vger.kernel.org
Subject: Re: [V9fs-developer] [RFC PATCH] First RFC version of a new cache=mmap model.
Date: Sat, 7 Sep 2013 16:46:30 +0200	[thread overview]
Message-ID: <20130907144630.GA2909@nautica> (raw)
In-Reply-To: <1378547842.8385.2@driftwood>

Hi,

Rob Landley wrote on Sat, Sep 07, 2013 :
> On 09/03/2013 09:37:58 AM, Dominique Martinet wrote:
> >  - Add cache=mmap option
> >  - Try to keep most operations synchronous
> > 
> > Signed-off-by: Dominique Martinet <dominique.martinet@cea.fr>
> > ---
> > 
> > Hi,
> > 
> > I brought up the issue of mmap being read-only with cache=none a bit
> > ago, and worked on it since.
> > 
> > Here's what this patch does:
> >  - Add an option cache=mmap (or just mmap, like other caches), nothing
> > changes for default or other caches.
> 
> Interesting.
> 
> Should this become the default behavior instead of "none" for  
> non-readonly mounts? (What's the point of the "none" mode now that we  
> have writeable mmap implemented, is there any advantage?)

Well, if there is no issue with the principle of flushing the map on
close, it might eventually become more widely used and maybe the
default.

I don't think there should be any inconsistency with what the
documentation says mmap should do, but this doesn't mean everything will
work as expected and it's not because this passes all the tests I tried
that it won't break anything (and in a very subtle way that would
silently corrupt data, so definitely not something we want)

This definitely needs some consideration/more reviews and, in my
opinion, much more testing before pretending to get there.

By the way, two more questions from my end, if anyone can help:
 - the mkwrite function calls "v9fs_fscache_wait_on_page_write(inode,
page);". I'm afraid I don't understand why we need to wait that
everything has been flushed before we make the page writable, what does
it do? Check the the page doesn't need to be invalidated/re-read before
the actual edits, so as not to cause corruptions around the edited
parts? Does mkwrite require/imply exclusive access to the modified
pages?

 - given that we have the vm_area_struct we are closing, for the flush,
would we have any better call than "write_inode_now" on the whole inode
since we ought to know which pages are concerned by the vma? (think
about a file with many mmaps on different parts of the file, we don't
want to flush it all everytime)

Would also appreciate comments from maintainers/anyone before I repost a
clean patch to push through, might as well fix things as early as
possible :)

Thanks,
-- 
Dominique