Re: On setting a lease across a cluster

[Matthew Wilcox <matthew@wil.cx>]

On Fri, Jan 04, 2008 at 03:53:04PM -0500, J. Bruce Fields wrote:
> So, the problem is that fcntl_setlease() does
> 
> 	vfs_setlease()
> 	fasync_helper()
> 
> which the bkl held over both, and you want to preserve that?
> 
> But what that BKL is doing is a mystery to me--the very first thing that
> fasync_helper() does is kmem_cache_alloc(., GFP_KERNEL).  So you won't
> be introducing any new problem if you lock those two operations
> separately.  Unless I'm totally missing something.

A very good point.

So yet another race caused by using the BKL rather than thinking ... but
maybe it's an inconsequential race.  The consequences are that (if the
kmalloc in fasync_helper sleeps) a lease appears that isn't fully set-up
yet (and may be removed if the kmalloc fails).  Actually, it seems bad
if the kmalloc eventually succeeds -- there's a window while kmalloc is
sleeping where another process could open the file, break the lease,
fl_fasync will be NULL, so no signal is sent.  Then 30 seconds later the 
lease is removed without the leaseholder being sent a signal.  Bad.

How can we fix this situation?  I think we need a better interface than
fasync_helper() -- fasync_alloc() and fasync_setup() would seem to do
the trick.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."