On Fri, 4 Jan 2008, Matthew Wilcox wrote: > > Hi Bruce, > > The current implementation of vfs_setlease/generic_setlease/etc is a > bit quirky. I've been thinking it over for the past couple of days, > and I think we need to refactor it to work sensibly. > > As you may have noticed, I've been mulling over getting rid of the > BKL in fs/locks.c and the lease interface is particularly problematic. > Here's one reason why: > > fcntl_setlease > lock_kernel > vfs_setlease > lock_kernel > generic_setlease > unlock_kernel > fasync_helper > unlock_kernel > > This is perfectly legitimate for the BKL of course, but other spinlocks > can't be acquired recursively in this way. At first I thought I'd just > push the call to generic_setlease into __vfs_setlease and have two ways > of calling it: > > fcntl_setlease > lock_kernel > __vfs_setlease > fasync_helper > unlock_kernel > > vfs_setlease > lock_kernel > __vfs_setlease > unlock_kernel > > But generic_setlease allocates memory (GFP_KERNEL) under the lock, so > that's bad for converting to spnlocks later. Then I thought I'd move > the spinlock acquisition into generic_setlease(). But I can't do that > either as fcntl_setlease() needs to hold the spinlock around the call > to generic_setlease() and fasync_helper(). > > Then I started to wonder about the current split of functionality between > fcntl_setlease, vfs_setlease and generic_setlease. The check for no > other process having this file open should be common to all filesystems. > And it should be honoured by NFS (it shouldn't hand out a delegation if > a local user has the file open), so it needs to be in vfs_setlease(). > > Now I'm worrying about the mechanics of calling out to a filesystem to > perform a lock. Obviously, a network filesystem is going to have to > sleep waiting for a response from the fileserver, so that precludes the > use of a spinlock held over the call to f_op->setlease. I'm not keen on > the idea of EXPORT_SYMBOL_GPL() a spinlock -- verifying locking is quite > hard enough without worrying what a filesystem might be doing with it. > > So I think we need to refactor the interface, and I'd like to hear your > thoughts on my ideas of how to handle this. > > First, have clients of vfs_setlease call lease_alloc() and pass it in, > rather than allocate it on the stack and have this horrible interface > where we may pass back an allocated lock. This allows us to not allocate > memory (hence sleep) during generic_setlease(). > > Second, move some of the functionality of generic_setlease() to > vfs_setlease(), as mentioned above. > > Third, change the purpose of f_op->setlease. We can rename it if you > like to catch any out of tree users. I'd propose using it like this: fwiw, i've done some work on extending the lease subsystem to help support the full range of requirements for NFSv4 file and directory delegations (e.g., breaking a lease when unlinking a file) and we ended up actually doing most of what you've just suggested here, which i take to be a good sign. most of my refactoring came out of trying to simplify locking and avoid holding locks too long (rather than specifically focusing on cluster-oriented stuff, but the goals dovetail) and your work on getting the BKL out of locks.c entirely is something i really like and look forward to. thanks, d . > vfs_setlease() > if (f_op->setlease()) > res = f_op->setlease() > if (res) > return res; > lock_kernel() > generic_setlease() > unlock_kernel() > > fcntl_setlease > if (f_op->setlease()) > res = f_op->setlease() > if (res) > return res; > lock_kernel > generic_setlease() > ... fasync ... > unlock_kernel > > So 'f_op->setlease' now means "Try to get a lease from the fileserver". > We can optimise this a bit to not even call setlease if, say, we already > have a read lease and we're trying to get a second read lease. But we > have to record our local leases (that way they'll show up in /proc/locks). > > I think the combination of these three ideas gives us a sane interface > to the various setlease functions, and let us convert from lock_kernel() > to spin_lock() later. Have I missed something? I don't often think > about the needs of cluster filesystems, so I may misunderstand how they > need this operation to work. > > At some point, we need to revisit the logic for 'is this file open > by another process' -- it's clearly buggy since it doesn't hold the > inode_lock while checking i_count, so it could race against an __iget(). > > -- > 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." > - > 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 > - 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
