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. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
