On Wed, 3 Jul 2013 16:12:32 -0400
"J. Bruce Fields" <bfields@xxxxxxxxxx> wrote:
> From: "J. Bruce Fields" <bfields@xxxxxxxxxx>
>
> We need to break delegations on any operation that changes the set of
> links pointing to an inode. Start with unlink.
>
> Such operations also hold the i_mutex on a parent directory. Breaking a
> delegation may require waiting for a timeout (by default 90 seconds) in
> the case of a unresponsive NFS client. To avoid blocking all directory
> operations, we therefore drop locks before waiting for the delegation.
> The logic then looks like:
>
> acquire locks
> ...
> test for delegation; if found:
> take reference on inode
> release locks
> wait for delegation break
> drop reference on inode
> retry
>
> It is possible this could never terminate. (Even if we take precautions
> to prevent another delegation being acquired on the same inode, we could
> get a different inode on each retry.) But this seems very unlikely.
>
> The initial test for a delegation happens after the lock on the target
> inode is acquired, but the directory inode may have been acquired
> further up the call stack. We therefore add a "struct inode **"
> argument to any intervening functions, which we use to pass the inode
> back up to the caller in the case it needs a delegation synchronously
> broken.
>
> Cc: David Howells <dhowells@xxxxxxxxxx>
> Cc: Tyler Hicks <tyhicks@xxxxxxxxxxxxx>
> Cc: Dustin Kirkland <dustin.kirkland@xxxxxxxxxxx>
> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
> ---
> drivers/base/devtmpfs.c | 2 +-
> fs/cachefiles/namei.c | 2 +-
> fs/ecryptfs/inode.c | 2 +-
> fs/namei.c | 24 +++++++++++++++++++++---
> fs/nfsd/vfs.c | 2 +-
> include/linux/fs.h | 2 +-
> ipc/mqueue.c | 2 +-
> 7 files changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
> index 7413d06..1b8490e 100644
> --- a/drivers/base/devtmpfs.c
> +++ b/drivers/base/devtmpfs.c
> @@ -324,7 +324,7 @@ static int handle_remove(const char *nodename, struct device *dev)
> mutex_lock(&dentry->d_inode->i_mutex);
> notify_change(dentry, &newattrs);
> mutex_unlock(&dentry->d_inode->i_mutex);
> - err = vfs_unlink(parent.dentry->d_inode, dentry);
> + err = vfs_unlink(parent.dentry->d_inode, dentry, NULL);
> if (!err || err == -ENOENT)
> deleted = 1;
> }
> diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
> index 8c01c5fc..d61d884 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -294,7 +294,7 @@ static int cachefiles_bury_object(struct cachefiles_cache *cache,
> if (ret < 0) {
> cachefiles_io_error(cache, "Unlink security error");
> } else {
> - ret = vfs_unlink(dir->d_inode, rep);
> + ret = vfs_unlink(dir->d_inode, rep, NULL);
>
> if (preemptive)
> cachefiles_mark_object_buried(cache, rep);
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 5eab400..af42d88 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -153,7 +153,7 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry,
>
> dget(lower_dentry);
> lower_dir_dentry = lock_parent(lower_dentry);
> - rc = vfs_unlink(lower_dir_inode, lower_dentry);
> + rc = vfs_unlink(lower_dir_inode, lower_dentry, NULL);
> if (rc) {
> printk(KERN_ERR "Error in vfs_unlink; rc = [%d]\n", rc);
> goto out_unlock;
> diff --git a/fs/namei.c b/fs/namei.c
> index 7e76fe1..cba3db1 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3384,7 +3384,7 @@ SYSCALL_DEFINE1(rmdir, const char __user *, pathname)
> return do_rmdir(AT_FDCWD, pathname);
> }
>
> -int vfs_unlink(struct inode *dir, struct dentry *dentry)
> +int vfs_unlink(struct inode *dir, struct dentry *dentry, struct inode **delegated_inode)
nit: this might be a good time to add a kerneldoc header on this
function. The delegated_inode thing might not be clear to the
uninitiated.
> {
> struct inode *target = dentry->d_inode;
> int error = may_delete(dir, dentry, 0);
> @@ -3401,11 +3401,20 @@ int vfs_unlink(struct inode *dir, struct dentry *dentry)
> else {
> error = security_inode_unlink(dir, dentry);
> if (!error) {
> + error = break_deleg(target, O_WRONLY|O_NONBLOCK);
> + if (error) {
> + if (error == -EWOULDBLOCK && delegated_inode) {
> + *delegated_inode = target;
> + ihold(target);
> + }
> + goto out;
> + }
> error = dir->i_op->unlink(dir, dentry);
> if (!error)
> dont_mount(dentry);
> }
> }
> +out:
> mutex_unlock(&target->i_mutex);
>
> /* We don't d_delete() NFS sillyrenamed files--they still exist. */
> @@ -3430,6 +3439,7 @@ static long do_unlinkat(int dfd, const char __user *pathname)
> struct dentry *dentry;
> struct nameidata nd;
> struct inode *inode = NULL;
> + struct inode *delegated_inode = NULL;
> unsigned int lookup_flags = 0;
> retry:
> name = user_path_parent(dfd, pathname, &nd, lookup_flags);
> @@ -3444,7 +3454,7 @@ retry:
> error = mnt_want_write(nd.path.mnt);
> if (error)
> goto exit1;
> -
> +retry_deleg:
> mutex_lock_nested(&nd.path.dentry->d_inode->i_mutex, I_MUTEX_PARENT);
> dentry = lookup_hash(&nd);
> error = PTR_ERR(dentry);
> @@ -3459,13 +3469,21 @@ retry:
> error = security_path_unlink(&nd.path, dentry);
> if (error)
> goto exit2;
> - error = vfs_unlink(nd.path.dentry->d_inode, dentry);
> + error = vfs_unlink(nd.path.dentry->d_inode, dentry, &delegated_inode);
> exit2:
> dput(dentry);
> }
> mutex_unlock(&nd.path.dentry->d_inode->i_mutex);
> if (inode)
> iput(inode); /* truncate the inode here */
> + inode = NULL;
> + if (delegated_inode) {
> + error = break_deleg(delegated_inode, O_WRONLY);
> + iput(delegated_inode);
> + delegated_inode = NULL;
> + if (!error)
> + goto retry_deleg;
> + }
> mnt_drop_write(nd.path.mnt);
> exit1:
> path_put(&nd.path);
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 84ce601..6ccaca2 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1882,7 +1882,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
> if (host_err)
> goto out_put;
> if (type != S_IFDIR)
> - host_err = vfs_unlink(dirp, rdentry);
> + host_err = vfs_unlink(dirp, rdentry, NULL);
> else
> host_err = vfs_rmdir(dirp, rdentry);
> if (!host_err)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c6cc686..f951588 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1463,7 +1463,7 @@ extern int vfs_mknod(struct inode *, struct dentry *, umode_t, dev_t);
> extern int vfs_symlink(struct inode *, struct dentry *, const char *);
> extern int vfs_link(struct dentry *, struct inode *, struct dentry *);
> extern int vfs_rmdir(struct inode *, struct dentry *);
> -extern int vfs_unlink(struct inode *, struct dentry *);
> +extern int vfs_unlink(struct inode *, struct dentry *, struct inode **);
> extern int vfs_rename(struct inode *, struct dentry *, struct inode *, struct dentry *);
>
> /*
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index e4e47f6..384eb35 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -884,7 +884,7 @@ SYSCALL_DEFINE1(mq_unlink, const char __user *, u_name)
> err = -ENOENT;
> } else {
> ihold(inode);
> - err = vfs_unlink(dentry->d_parent->d_inode, dentry);
> + err = vfs_unlink(dentry->d_parent->d_inode, dentry, NULL);
> }
> dput(dentry);
>
We probably also ought to eyeball some of these other cases where you
passing in NULL as the deleg_inode too. It's probably reasonable in
most cases -- exporting a filesystem that you also mount using ecryptfs
seems silly, but you never know...
Looks reasonable otherwise, if a little convoluted.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
