fixing leases/delegations
Subsystems (4)
NFSv4 (along with, from what I'm told, Samba) has a problem with read leases, which is that they are currently only broken on write opens, whereas they should really be broken on any operation that changes an inode's metadata or any of the links pointing to it.
Also, to break leases in a non-racy way it's not sufficient to have a single break_lease() call from the vfs code; we also need to prevent anyone from acquiring a new lease while the operation is still in progress.
So here's one attempt to deal with those problems, by adding to the inode a counter which is incremented whenever we start such an operation and decremented when we finish it.
I only handle unlink; we'd also need to do link, rename, chown, and chmod, at least.
Comments? Better ideas?
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1305746388-18241-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
RFC PATCH 1/2 locks: introduce i_blockleases to close lease races
Since break_lease is called before i_writecount is incremented, there's a window between the two where a setlease call would have no way to know that an open is about to happen.
We fix this by adding a new inode field, i_blockleases, that is incremented while a lease-breaking operation is in progress.
We will later reuse i_blockleases to enforce lease-breaking for rename, unlink, etc.
--- fs/inode.c | 1 + fs/locks.c | 28 ++++++++++++++++++++++++++++ fs/namei.c | 6 +++++- include/linux/fs.h | 3 +++ 4 files changed, 37 insertions(+), 1 deletions(-)
fs/inode.c +1 -0
@@ -198,6 +198,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode) inode->i_uid =0 ; inode->i_gid =0 ;atomic_set (&inode->i_writecount,0 ); +atomic_set (&inode->i_blockleases,0 ); inode->i_size =0 ; inode->i_blocks =0 ; inode->i_bytes =0 ;
fs/locks.c +28 -0
@@ -1164,6 +1164,32 @@ static void time_out_leases(struct inode *inode) } } +/* Disallow all leases (read or write): */ +void disallow_leases (struct inode *inode,int flags) +{ +if (!inode) +return ; +if (!S_ISREG (inode->i_mode)) +return ; +if ((flags &O_ACCMODE ) ==O_RDONLY ) +return ; +atomic_inc (&inode->i_blockleases); +} +EXPORT_SYMBOL_GPL (disallow_leases); + +void reallow_leases (struct inode *inode,int flags) +{ +if (!inode) +return ; +if (!S_ISREG (inode->i_mode)) +return ; +if ((flags &O_ACCMODE ) ==O_RDONLY ) +return ; +BUG_ON (atomic_read (&inode->i_blockleases) <=0 ); +atomic_dec (&inode->i_blockleases); +} +EXPORT_SYMBOL_GPL (reallow_leases); +/** * __break_lease - revoke all outstanding leases on file * @inode: the inode of the file to return
@@ -1369,6 +1395,8 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)if (arg !=F_UNLCK ) { error = -EAGAIN ; +if (atomic_read (&inode->i_blockleases)) + goto out;if ((arg ==F_RDLCK ) && (atomic_read (&inode->i_writecount) >0 )) goto out; if ((arg == F_WRLCK)
fs/namei.c +5 -1
@@ -2277,10 +2277,14 @@ ok: want_write =1 ; } common: +disallow_leases (nd->path.dentry->d_inode, open_flag); error =may_open (&nd->path, acc_mode, open_flag); -if (error) +if (error) { +reallow_leases (nd->path.dentry->d_inode, open_flag); goto exit; + } filp =nameidata_to_filp (nd); +reallow_leases (nd->path.dentry->d_inode, open_flag);if (!IS_ERR (filp)) { error =ima_file_check (filp, op->acc_mode);if (error) {
include/linux/fs.h +3 -0
@@ -796,6 +796,7 @@ struct inode {#ifdef CONFIG_IMA atomic_t i_readcount;/* struct files open RO */ #endif +atomic_t i_blockleases;/* setlease fails when >0 */ atomic_t i_writecount;#ifdef CONFIG_SECURITY void *i_security;
@@ -1161,6 +1162,8 @@ extern void lease_get_mtime(struct inode *, struct timespec *time);extern int generic_setlease (struct file *,long ,struct file_lock **);extern int vfs_setlease (struct file *,long ,struct file_lock **);extern int lease_modify (struct file_lock **,int ); +extern void disallow_leases (struct inode *,int flags); +extern void reallow_leases (struct inode *,int flags);extern int lock_may_read (struct inode *,loff_t start,unsigned long count);extern int lock_may_write (struct inode *,loff_t start,unsigned long count);extern void lock_flocks (void );
Signature · 6 lines
1.7.4.1 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 2/2 locks: break lease on unlink
In reply to a message not in the archiveNFSv4 uses leases (delegations) to allow clients to do opens locally. An open takes a component name, so to do a local open a client needs to know that at least that last component name points at the same time as long as they hold the delegation. For that reason, the NFSv4 spec requires that delegations be broken on unlink.
Waiting for a lease to be broken may mean waiting for an nfs client or a user process to give it up, so doing that while holding the i_mutex would risk deadlocks. So instead we do a non-blocking lease break, then drop the i_mutex and do a blocking lease break if necessary.
--- fs/namei.c | 32 ++++++++++++++++++++++++++++---- fs/nfsd/vfs.c | 6 +++++- 2 files changed, 33 insertions(+), 5 deletions(-)
fs/namei.c +28 -4
@@ -2799,6 +2799,7 @@ static long do_unlinkat(int dfd, const char __user *pathname) nd.flags &= ~LOOKUP_PARENT ; +retry:mutex_lock_nested (&nd.path.dentry->d_inode->i_mutex,I_MUTEX_PARENT ); dentry =lookup_hash (&nd); error =PTR_ERR (dentry);
@@ -2806,15 +2807,27 @@ static long do_unlinkat(int dfd, const char __user *pathname)/* Why not before? Because we want correct error value */ if (nd.last.name[nd.last.len]) goto slashes; - inode = dentry->d_inode; -if (inode) -ihold (inode); +if (inode && inode != dentry->d_inode) { +reallow_leases (inode,O_WRONLY ); +iput (inode); + inode =NULL ; + } +if (!inode) { + inode = dentry->d_inode; +if (inode) +ihold (inode); +disallow_leases (inode,O_WRONLY ); + } error =mnt_want_write (nd.path.mnt);if (error) goto exit2; error =security_path_unlink (&nd.path, dentry);if (error) goto exit3; +if (inode) + error =break_lease (inode,O_WRONLY |O_NONBLOCK ); +if (error) + goto exit3; error =vfs_unlink (nd.path.dentry->d_inode, dentry); exit3:mnt_drop_write (nd.path.mnt);
@@ -2822,8 +2835,19 @@ exit3:dput (dentry); }mutex_unlock (&nd.path.dentry->d_inode->i_mutex); -if (inode) +if (inode) { +/* XXX: safe to use EWOULDBLOCK==EAGAIN to do this?: */ +if (error == -EWOULDBLOCK ) { +/* +* Wait this time, then retry with leases left +* disabled: +*/ +break_lease (inode,O_WRONLY ); + goto retry; + } +reallow_leases (inode,O_WRONLY );iput (inode);/* truncate the inode here */ + } exit1:path_put (&nd.path);putname (name);
fs/nfsd/vfs.c +5 -1
@@ -1826,13 +1826,17 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,if (host_err) goto out_put; +disallow_leases (rdentry->d_inode,O_WRONLY ); host_err =nfsd_break_lease (rdentry->d_inode); -if (host_err) +if (host_err) { +reallow_leases (rdentry->d_inode,O_WRONLY ); goto out_drop_write; + }if (type !=S_IFDIR ) host_err =vfs_unlink (dirp, rdentry);else host_err =vfs_rmdir (dirp, rdentry); +reallow_leases (rdentry->d_inode,O_WRONLY );if (!host_err) host_err =commit_metadata (fhp); out_drop_write:
Signature · 6 lines
1.7.4.1 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html