Re: On setting a lease across a cluster

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux