Subject: Re: Interlock issues with VOP_PUTPAGES, was Re: CVS commit: src/sys/nfs
To: Bill Studenmund <wrstuden@netbsd.org>
From: Chuck Silvers <chuq@chuq.com>
List: tech-kern
Date: 10/04/2003 15:50:14
hi,

VOP_PUTPAGES() is used for two somewhat different purposes:

 (1) to flush and/or invalidate the pages for a given logical range of
     a vnode.  the higher level operations that use this are fsync(),
     msync(), truncate(), write() (to flush pages out behind sequential
     write()s), etc.

 (2) to flush and invalidate a given physical page, used by the pagedaemon.
     this is called via the vnode pager's pgo_put method.

the latter usage is the one which wants the uvm_object lock (aka. the
vnode "interlock" for vnode objects) to be held.  the former usages
don't really need the lock to be held for themselves, but since the
interface is used for the latter purpose as well, they take the lock
just to satisfy the interface constraints.  it would be possible to
change the pagedaemon's usage so as to not need the lock held either,
but that's how it was before UBC and I didn't change it when I added
VOP_{GET,PUT}PAGES as the implementation of pgo_{get,put} for the vnode
pager.

for all the existing layered file systems, data is cached only in the
underlying vnode, so we're never in case (2) for layerfs vnodes.
that's why the lock juggling in layer_putpages() is safe.
if someone were to write a layered fs that tried to cache data in
layerfs vnodes, they would have to deal with this (as well as a host
of other cache-coherency issues).

-Chuck


On Wed, Oct 01, 2003 at 12:26:58PM -0700, Bill Studenmund wrote:
> On Tue, Aug 26, 2003 at 04:40:10PM +0000, Paul Kranenburg wrote:
> > 
> > Module Name:	src
> > Committed By:	pk
> > Date:		Tue Aug 26 16:40:10 UTC 2003
> > 
> > Modified Files:
> > 	src/sys/nfs: nfs_bio.c
> > 
> > Log Message:
> > VOP_PUTPAGES() must be called with the vnode's interlock held.
> 
> I've been thinking about this, and I think it's really not right.
> Not that pk changed nfs, but that VOP_PUTPAGES() needs this interlock
> held.
> 
> I've got some concerns with this:
> 
> 1) It's not documented. The only locking mentioned for VOP_PUTPAGES()
> is that the vnode lock doesn't change state. The requirement for grabbing
> the interlock isn't mentioned.
> 
> It doesn't mesh well with the rest of the vnode system design.
> 
> 2) AFAIK it is the only place where we have to hold an interlock
> before calling a VOP call (getpages may also count, I don't know).
> 
> 3) It doesn't work at all with our layered file systems. Yes, Chuck
> added code in layer_putpages() to shuffle around the interlocks,
> but it's still not right. The interlock that the upper code grabbed
> isn't the one the putpages code deals with. Which makes me kinda
> wonder what's going on. :-)
> 
> A lot of places I've seen just grab the interlock just before the call.
> 
> So what is up with this interlock? Is there some other way we can
> address this problem?
> 
> Take care,
> 
> Bill