Re: On setting a lease across a cluster

["J. Bruce Fields" <bfields@fieldses.org>]

On Fri, Jan 04, 2008 at 02:08:18PM -0700, Matthew Wilcox wrote:
> 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.

Or re-check the lease after doing the fasync_helper() setup and remove
it if it's been broken in the interim?

(Not that fasync_helper() couldn't independently use a little love:

- The documentation:

	/*
	 * fasync_helper() is used by some character device drivers
	 * (mainly mice) to set up the fasync queue. It returns negative
	 * on error, 0 if it did no changes and positive if it
	 * added/deleted the entry.
	 */

  could be more helpful.
- I find the "on" parameter a little confusing.  (Shouldn't we just have
  two separate functions for those two cases?)
- It should return ERR_PTR(-ERRNO) or the fasync_struct rather than
  using an fasync_struct ** to return the result.
- And what's up with FASYNC_MAGIC?  I thought the consensus was not to
  do that kind of thing in the kernel.

)

--b.