breaking leases on metadata changes

breaking leases on metadata changes

[J. Bruce Fields]

So there's this longstanding problem that read leases should really be
broken on anything that changes an inode's metadata or any change to the
set of links pointing to the inode.

(Cc'ing samba-technical for confirmation of that statement from Samba's
point of view....)

So we need mutual exclusion between read leases and link, unlink,
rename, etc.

After contemplating increasingly complicated solutions for much too
long, I realized that the i_mutex is taken by all of those operations
except for rename--and it looks to me like rename could take i_mutex on
a renamed file as well.

So, here's my attempt.  It passes some simple tests.  Is it sane?

I tried updating Documentation/filesystems/directory-locking to reflect
the rename change, but without thinking it through as carefully as I'd
like to.

Also, the one thing I *know* is wrong: lockdep doesn't like the rename
change, and I haven't tried to figure out how to fix it.  (Help?)

These patches assume some previous lease fixes already committed to

	git://linux-nfs.org/~bfields/linux.git for-3.2

--b.

[PATCH 1/6] leases: split up generic_setlease into lock/unlock cases

[J. Bruce Fields]

Eventually we should probably do the same thing to the file operations
as well.

Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/locks.c |   98 ++++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 62 insertions(+), 36 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 9b8408e..b342902 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1352,18 +1352,7 @@ int fcntl_getlease(struct file *filp)
 	return type;
 }
 
-/**
- *	generic_setlease	-	sets a lease on an open file
- *	@filp: file pointer
- *	@arg: type of lease to obtain
- *	@flp: input - file_lock to use, output - file_lock inserted
- *
- *	The (input) flp->fl_lmops->lm_break function is required
- *	by break_lease().
- *
- *	Called with file_lock_lock held.
- */
-int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
+int generic_add_lease(struct file *filp, long arg, struct file_lock **flp)
 {
 	struct file_lock *fl, **before, **my_before = NULL, *lease;
 	struct dentry *dentry = filp->f_path.dentry;
@@ -1372,30 +1361,14 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 
 	lease = *flp;
 
-	error = -EACCES;
-	if ((current_fsuid() != inode->i_uid) && !capable(CAP_LEASE))
-		goto out;
-	error = -EINVAL;
-	if (!S_ISREG(inode->i_mode))
+	error = -EAGAIN;
+	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
 		goto out;
-	error = security_file_lock(filp, arg);
-	if (error)
+	if ((arg == F_WRLCK)
+	    && ((dentry->d_count > 1)
+		|| (atomic_read(&inode->i_count) > 1)))
 		goto out;
 
-	time_out_leases(inode);
-
-	BUG_ON(!(*flp)->fl_lmops->lm_break);
-
-	if (arg != F_UNLCK) {
-		error = -EAGAIN;
-		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
-			goto out;
-		if ((arg == F_WRLCK)
-		    && ((dentry->d_count > 1)
-			|| (atomic_read(&inode->i_count) > 1)))
-			goto out;
-	}
-
 	/*
 	 * At this point, we know that if there is an exclusive
 	 * lease on this file, then we hold it on this filp
@@ -1433,9 +1406,6 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 		goto out;
 	}
 
-	if (arg == F_UNLCK)
-		goto out;
-
 	error = -EINVAL;
 	if (!leases_enable)
 		goto out;
@@ -1446,6 +1416,62 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 out:
 	return error;
 }
+
+int generic_delete_lease(struct file *filp, struct file_lock **flp)
+{
+	struct file_lock *fl, **before;
+	struct dentry *dentry = filp->f_path.dentry;
+	struct inode *inode = dentry->d_inode;
+
+	for (before = &inode->i_flock;
+			((fl = *before) != NULL) && IS_LEASE(fl);
+			before = &fl->fl_next) {
+		if (fl->fl_file != filp)
+			continue;
+		return (*flp)->fl_lmops->lm_change(before, F_UNLCK);
+	}
+	return -EAGAIN;
+}
+
+/**
+ *	generic_setlease	-	sets a lease on an open file
+ *	@filp: file pointer
+ *	@arg: type of lease to obtain
+ *	@flp: input - file_lock to use, output - file_lock inserted
+ *
+ *	The (input) flp->fl_lmops->lm_break function is required
+ *	by break_lease().
+ *
+ *	Called with file_lock_lock held.
+ */
+int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
+{
+	struct dentry *dentry = filp->f_path.dentry;
+	struct inode *inode = dentry->d_inode;
+	int error;
+
+	if ((current_fsuid() != inode->i_uid) && !capable(CAP_LEASE))
+		return -EACCES;
+	if (!S_ISREG(inode->i_mode))
+		return -EINVAL;
+	error = security_file_lock(filp, arg);
+	if (error)
+		return error;
+
+	time_out_leases(inode);
+
+	BUG_ON(!(*flp)->fl_lmops->lm_break);
+
+	switch (arg) {
+	case F_UNLCK:
+		return generic_delete_lease(filp, flp);
+	case F_RDLCK:
+	case F_WRLCK:
+		return generic_add_lease(filp, arg, flp);
+	default:
+		BUG();
+	}
+}
 EXPORT_SYMBOL(generic_setlease);
 
 static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)
-- 
1.7.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/6] leases: split up generic_setlease into lock/unlock cases

[Mimi Zohar]

On Wed, 2011-09-21 at 10:58 -0400, J. Bruce Fields wrote: 
> Eventually we should probably do the same thing to the file operations
> as well.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/locks.c |   98 ++++++++++++++++++++++++++++++++++++++----------------------
>  1 files changed, 62 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 9b8408e..b342902 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1352,18 +1352,7 @@ int fcntl_getlease(struct file *filp)
>  	return type;
>  }
> 
> -/**
> - *	generic_setlease	-	sets a lease on an open file
> - *	@filp: file pointer
> - *	@arg: type of lease to obtain
> - *	@flp: input - file_lock to use, output - file_lock inserted
> - *
> - *	The (input) flp->fl_lmops->lm_break function is required
> - *	by break_lease().
> - *
> - *	Called with file_lock_lock held.
> - */
> -int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
> +int generic_add_lease(struct file *filp, long arg, struct file_lock **flp)
>  {
>  	struct file_lock *fl, **before, **my_before = NULL, *lease;
>  	struct dentry *dentry = filp->f_path.dentry;
> @@ -1372,30 +1361,14 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
> 
>  	lease = *flp;
> 
> -	error = -EACCES;
> -	if ((current_fsuid() != inode->i_uid) && !capable(CAP_LEASE))
> -		goto out;
> -	error = -EINVAL;
> -	if (!S_ISREG(inode->i_mode))
> +	error = -EAGAIN;
> +	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
>  		goto out;
> -	error = security_file_lock(filp, arg);
> -	if (error)
> +	if ((arg == F_WRLCK)
> +	    && ((dentry->d_count > 1)
> +		|| (atomic_read(&inode->i_count) > 1)))
>  		goto out;
> 
> -	time_out_leases(inode);
> -
> -	BUG_ON(!(*flp)->fl_lmops->lm_break);
> -
> -	if (arg != F_UNLCK) {
> -		error = -EAGAIN;
> -		if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
> -			goto out;
> -		if ((arg == F_WRLCK)
> -		    && ((dentry->d_count > 1)
> -			|| (atomic_read(&inode->i_count) > 1)))
> -			goto out;
> -	}
> -
>  	/*
>  	 * At this point, we know that if there is an exclusive
>  	 * lease on this file, then we hold it on this filp
> @@ -1433,9 +1406,6 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
>  		goto out;
>  	}
> 
> -	if (arg == F_UNLCK)
> -		goto out;
> -
>  	error = -EINVAL;
>  	if (!leases_enable)
>  		goto out;
> @@ -1446,6 +1416,62 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
>  out:
>  	return error;
>  }
> +
> +int generic_delete_lease(struct file *filp, struct file_lock **flp)
> +{
> +	struct file_lock *fl, **before;
> +	struct dentry *dentry = filp->f_path.dentry;
> +	struct inode *inode = dentry->d_inode;
> +
> +	for (before = &inode->i_flock;
> +			((fl = *before) != NULL) && IS_LEASE(fl);
> +			before = &fl->fl_next) {
> +		if (fl->fl_file != filp)
> +			continue;
> +		return (*flp)->fl_lmops->lm_change(before, F_UNLCK);
> +	}
> +	return -EAGAIN;
> +}
> +
> +/**
> + *	generic_setlease	-	sets a lease on an open file
> + *	@filp: file pointer
> + *	@arg: type of lease to obtain
> + *	@flp: input - file_lock to use, output - file_lock inserted
> + *
> + *	The (input) flp->fl_lmops->lm_break function is required
> + *	by break_lease().
> + *
> + *	Called with file_lock_lock held.
> + */
> +int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
> +{
> +	struct dentry *dentry = filp->f_path.dentry;
> +	struct inode *inode = dentry->d_inode;
> +	int error;
> +
> +	if ((current_fsuid() != inode->i_uid) && !capable(CAP_LEASE))
> +		return -EACCES;
> +	if (!S_ISREG(inode->i_mode))
> +		return -EINVAL;
> +	error = security_file_lock(filp, arg);
> +	if (error)
> +		return error;
> +
> +	time_out_leases(inode);
> +
> +	BUG_ON(!(*flp)->fl_lmops->lm_break);
> +
> +	switch (arg) {
> +	case F_UNLCK:
> +		return generic_delete_lease(filp, flp);
> +	case F_RDLCK:
> +	case F_WRLCK:
> +		return generic_add_lease(filp, arg, flp);
> +	default:
> +		BUG();
> +	}
> +}
>  EXPORT_SYMBOL(generic_setlease);
> 
>  static int __vfs_setlease(struct file *filp, long arg, struct file_lock **lease)

Nice, the code refactoring makes it a lot more readable.

thanks,

Mimi 

Re: [PATCH 1/6] leases: split up generic_setlease into lock/unlock cases

[J. Bruce Fields]

On Thu, Sep 22, 2011 at 01:16:14PM -0400, Mimi Zohar wrote:
> On Wed, 2011-09-21 at 10:58 -0400, J. Bruce Fields wrote: 
> > Eventually we should probably do the same thing to the file operations
> > as well.
...
> Nice, the code refactoring makes it a lot more readable.

Thanks.  I'm assuming this one is uncontroversial, so I'll go ahead and
add it to my for-3.2 tree soon.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/6] leases: break leases on any attribute modification

[J. Bruce Fields]

NFSv4 uses leases to guarantee that clients can cash metadata as well as
data.

(I suspect the same is true for Samba.)

This covers chmod, chown, etc.

Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/attr.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index 538e279..4ce31cb 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -231,6 +231,9 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
 	error = security_inode_setattr(dentry, attr);
 	if (error)
 		return error;
+	error = break_lease(inode, O_WRONLY);
+	if (error)
+		return error;
 
 	if (inode->i_op->setattr)
 		error = inode->i_op->setattr(dentry, attr);
-- 
1.7.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/6] leases: break leases on any attribute modification

[J. Bruce Fields]

On Wed, Sep 21, 2011 at 10:58:16AM -0400, J. Bruce Fields wrote:
> NFSv4 uses leases to guarantee that clients can cash metadata as well as
						  ^^^^

Um, you know what I meant.  Corrected in my local repo.--b.


> data.
> 
> (I suspect the same is true for Samba.)
> 
> This covers chmod, chown, etc.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/attr.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/attr.c b/fs/attr.c
> index 538e279..4ce31cb 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -231,6 +231,9 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
>  	error = security_inode_setattr(dentry, attr);
>  	if (error)
>  		return error;
> +	error = break_lease(inode, O_WRONLY);
> +	if (error)
> +		return error;
>  
>  	if (inode->i_op->setattr)
>  		error = inode->i_op->setattr(dentry, attr);
> -- 
> 1.7.4.1
> 

[PATCH 2/6] leases: fix write-open/read-lease race

[J. Bruce Fields]

In setlease, we use i_writecount to decide whether we can give out a
read lease.

In open, we break leases before incrementing i_writecount.

There is therefore a window between the break lease and the i_writecount
increment when setlease could add a new read lease.

This would leave us with a simultaneous write open and read lease, which
shouldn't happen.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/namei.c |    5 +----
 fs/open.c  |    4 ++++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 2826db3..6ff59e5 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2044,10 +2044,7 @@ static int may_open(struct path *path, int acc_mode, int flag)
 	if (flag & O_NOATIME && !inode_owner_or_capable(inode))
 		return -EPERM;
 
-	/*
-	 * Ensure there are no outstanding leases on the file.
-	 */
-	return break_lease(inode, flag);
+	return 0;
 }
 
 static int handle_truncate(struct file *filp)
diff --git a/fs/open.c b/fs/open.c
index f711921..22c41b5 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -685,6 +685,10 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
 	if (error)
 		goto cleanup_all;
 
+	error = break_lease(inode, f->f_flags);
+	if (error)
+		goto cleanup_all;
+
 	if (!open && f->f_op)
 		open = f->f_op->open;
 	if (open) {
-- 
1.7.4.1

Re: [PATCH 2/6] leases: fix write-open/read-lease race

[J. Bruce Fields]

On Wed, Sep 21, 2011 at 10:58:13AM -0400, J. Bruce Fields wrote:
> In setlease, we use i_writecount to decide whether we can give out a
> read lease.
> 
> In open, we break leases before incrementing i_writecount.
> 
> There is therefore a window between the break lease and the i_writecount
> increment when setlease could add a new read lease.
> 
> This would leave us with a simultaneous write open and read lease, which
> shouldn't happen.

And maybe someone that knows the open code better than me could confirm
whether it's reasonable to move the break_lease() call to __dentry_open
like this....

--b.

> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/namei.c |    5 +----
>  fs/open.c  |    4 ++++
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 2826db3..6ff59e5 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2044,10 +2044,7 @@ static int may_open(struct path *path, int acc_mode, int flag)
>  	if (flag & O_NOATIME && !inode_owner_or_capable(inode))
>  		return -EPERM;
>  
> -	/*
> -	 * Ensure there are no outstanding leases on the file.
> -	 */
> -	return break_lease(inode, flag);
> +	return 0;
>  }
>  
>  static int handle_truncate(struct file *filp)
> diff --git a/fs/open.c b/fs/open.c
> index f711921..22c41b5 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -685,6 +685,10 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
>  	if (error)
>  		goto cleanup_all;
>  
> +	error = break_lease(inode, f->f_flags);
> +	if (error)
> +		goto cleanup_all;
> +
>  	if (!open && f->f_op)
>  		open = f->f_op->open;
>  	if (open) {
> -- 
> 1.7.4.1
> 

Re: [PATCH 2/6] leases: fix write-open/read-lease race

[Mimi Zohar]

On Wed, 2011-09-21 at 11:01 -0400, J. Bruce Fields wrote: 
> On Wed, Sep 21, 2011 at 10:58:13AM -0400, J. Bruce Fields wrote:
> > In setlease, we use i_writecount to decide whether we can give out a
> > read lease.
> > 
> > In open, we break leases before incrementing i_writecount.
> > 
> > There is therefore a window between the break lease and the i_writecount
> > increment when setlease could add a new read lease.
> > 
> > This would leave us with a simultaneous write open and read lease, which
> > shouldn't happen.
> 
> And maybe someone that knows the open code better than me could confirm
> whether it's reasonable to move the break_lease() call to __dentry_open
> like this....
> 
> --b.

Moving break_lease() from may_open() to __dentry_open(), places the call
immediately after the call to __get_file_write_access(), which
increments i_writecount.

Currently, break_lease() is being called before the transition from
put_filp() to fput(). The move doesn't change this, so I would assume it
should be ok.

Mimi

> > 
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> >  fs/namei.c |    5 +----
> >  fs/open.c  |    4 ++++
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 2826db3..6ff59e5 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -2044,10 +2044,7 @@ static int may_open(struct path *path, int acc_mode, int flag)
> >  	if (flag & O_NOATIME && !inode_owner_or_capable(inode))
> >  		return -EPERM;
> >  
> > -	/*
> > -	 * Ensure there are no outstanding leases on the file.
> > -	 */
> > -	return break_lease(inode, flag);
> > +	return 0;
> >  }
> >  
> >  static int handle_truncate(struct file *filp)
> > diff --git a/fs/open.c b/fs/open.c
> > index f711921..22c41b5 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -685,6 +685,10 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
> >  	if (error)
> >  		goto cleanup_all;
> >  
> > +	error = break_lease(inode, f->f_flags);
> > +	if (error)
> > +		goto cleanup_all;
> > +
> >  	if (!open && f->f_op)
> >  		open = f->f_op->open;
> >  	if (open) {
> > -- 
> > 1.7.4.1
> > 

Re: [PATCH 2/6] leases: fix write-open/read-lease race

[J. Bruce Fields]

On Wed, Sep 21, 2011 at 10:58:13AM -0400, J. Bruce Fields wrote:
> In setlease, we use i_writecount to decide whether we can give out a
> read lease.
> 
> In open, we break leases before incrementing i_writecount.
> 
> There is therefore a window between the break lease and the i_writecount
> increment when setlease could add a new read lease.
> 
> This would leave us with a simultaneous write open and read lease, which
> shouldn't happen.

Al, could you apply this for 3.2, if you don't see any problem?

(Patch 1 of this series only touches locks.c, and I'm queueing up such
patches through the nfsd tree.  Patches 3-6 I intend to rewrite,
probably not in time for 3.2 unless I'm very lucky.)

--b.

> 
> Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/namei.c |    5 +----
>  fs/open.c  |    4 ++++
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 2826db3..6ff59e5 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2044,10 +2044,7 @@ static int may_open(struct path *path, int acc_mode, int flag)
>  	if (flag & O_NOATIME && !inode_owner_or_capable(inode))
>  		return -EPERM;
>  
> -	/*
> -	 * Ensure there are no outstanding leases on the file.
> -	 */
> -	return break_lease(inode, flag);
> +	return 0;
>  }
>  
>  static int handle_truncate(struct file *filp)
> diff --git a/fs/open.c b/fs/open.c
> index f711921..22c41b5 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -685,6 +685,10 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
>  	if (error)
>  		goto cleanup_all;
>  
> +	error = break_lease(inode, f->f_flags);
> +	if (error)
> +		goto cleanup_all;
> +
>  	if (!open && f->f_op)
>  		open = f->f_op->open;
>  	if (open) {
> -- 
> 1.7.4.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Need information about the net ads user command

[Pankaj Baranwal]

Hi,

"net ads user" command doesn't returns users starting with $.
Is there any another way to search users starting with $.

Thanks in advance for your help.

Thanks ,
Pankaj


::DISCLAIMER::
-----------------------------------------------------------------------------------------------------------------------

The contents of this e-mail and any attachment(s) are confidential and intended for the named recipient(s) only.
It shall not attach any liability on the originator or HCL or its affiliates. Any views or opinions presented in
this email are solely those of the author and may not necessarily reflect the opinions of HCL or its affiliates.
Any form of reproduction, dissemination, copying, disclosure, modification, distribution and / or publication of
this message without the prior written consent of the author of this e-mail is strictly prohibited. If you have
received this email in error please delete it and notify the sender immediately. Before opening any mail and
attachments please check them for viruses and defect.

-----------------------------------------------------------------------------------------------------------------------

Re: [PATCH 2/6] leases: fix write-open/read-lease race

[J. Bruce Fields]

On Mon, Oct 10, 2011 at 05:59:11PM -0400, bfields wrote:
> On Wed, Sep 21, 2011 at 10:58:13AM -0400, J. Bruce Fields wrote:
> > In setlease, we use i_writecount to decide whether we can give out a
> > read lease.
> > 
> > In open, we break leases before incrementing i_writecount.
> > 
> > There is therefore a window between the break lease and the i_writecount
> > increment when setlease could add a new read lease.
> > 
> > This would leave us with a simultaneous write open and read lease, which
> > shouldn't happen.
> 
> Al, could you apply this for 3.2, if you don't see any problem?

Ping?  What should I do with this patch?

--b.

> 
> (Patch 1 of this series only touches locks.c, and I'm queueing up such
> patches through the nfsd tree.  Patches 3-6 I intend to rewrite,
> probably not in time for 3.2 unless I'm very lucky.)
> 
> --b.
> 
> > 
> > Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/namei.c |    5 +----
> >  fs/open.c  |    4 ++++
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 2826db3..6ff59e5 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -2044,10 +2044,7 @@ static int may_open(struct path *path, int acc_mode, int flag)
> >  	if (flag & O_NOATIME && !inode_owner_or_capable(inode))
> >  		return -EPERM;
> >  
> > -	/*
> > -	 * Ensure there are no outstanding leases on the file.
> > -	 */
> > -	return break_lease(inode, flag);
> > +	return 0;
> >  }
> >  
> >  static int handle_truncate(struct file *filp)
> > diff --git a/fs/open.c b/fs/open.c
> > index f711921..22c41b5 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -685,6 +685,10 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
> >  	if (error)
> >  		goto cleanup_all;
> >  
> > +	error = break_lease(inode, f->f_flags);
> > +	if (error)
> > +		goto cleanup_all;
> > +
> >  	if (!open && f->f_op)
> >  		open = f->f_op->open;
> >  	if (open) {
> > -- 
> > 1.7.4.1
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] leases: break read leases on unlink

[J. Bruce Fields]

A read lease is used by NFSv4 as a guarantee that a client can perform
local read opens without informing the server.

The open operation takes the last component of the pathname as an
argument, thus is also a lookup operation, and giving the client the
above guarantee means informing the client before we allow anything that
would change the set of names pointing to the inode.

Samba, from what I can tell, has similar requirements, so this is really
a bug in the lease code, I think, not just a strange requirement from
NFS.

Therefore, we need to break leases on rename, link, and unlink.

Start with unlink.

The simplest thing to do is just to use the fact that unlink always
takes the i_mutex to prevent new leases from being acquired while the
unlink is in progress.

The lease is generally just an optimization--it's always OK not to give
one out.  So we can just do a mutex_trylock() in setlease() and fail the
setlease if we don't get the lock.

It's annoying that the lease break--which will require acknowledgement
from an nfs client or an application--happens while holding the i_mutex.
But the time for that is limited by lease_break_time, so at least
there's no (permanent) deadlock there.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/locks.c |    7 +++++--
 fs/namei.c |    3 +++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index b342902..61130c1 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1361,6 +1361,9 @@ int generic_add_lease(struct file *filp, long arg, struct file_lock **flp)
 
 	lease = *flp;
 
+	if (!mutex_trylock(&inode->i_mutex))
+		return -EAGAIN;
+
 	error = -EAGAIN;
 	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
 		goto out;
@@ -1411,9 +1414,9 @@ int generic_add_lease(struct file *filp, long arg, struct file_lock **flp)
 		goto out;
 
 	locks_insert_lock(before, lease);
-	return 0;
-
+	error = 0;
 out:
+	mutex_unlock(&inode->i_mutex);
 	return error;
 }
 
diff --git a/fs/namei.c b/fs/namei.c
index 6ff59e5..5c78f72 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2772,6 +2772,9 @@ static long do_unlinkat(int dfd, const char __user *pathname)
 		error = security_path_unlink(&nd.path, dentry);
 		if (error)
 			goto exit3;
+		error = break_lease(inode, O_WRONLY);
+		if (error)
+			goto exit3;
 		error = vfs_unlink(nd.path.dentry->d_inode, dentry);
 exit3:
 		mnt_drop_write(nd.path.mnt);
-- 
1.7.4.1

Re: [PATCH 3/6] leases: break read leases on unlink

[Christoph Hellwig]

On Wed, Sep 21, 2011 at 10:58:14AM -0400, J. Bruce Fields wrote:
> A read lease is used by NFSv4 as a guarantee that a client can perform
> local read opens without informing the server.
> 
> The open operation takes the last component of the pathname as an
> argument, thus is also a lookup operation, and giving the client the
> above guarantee means informing the client before we allow anything that
> would change the set of names pointing to the inode.

That seems like totally strange semantics.  A useful defintion of a
lase operation would mean it guarantees I/O can happen, and wouldn't
care about metadata operation.  That's the whole point in adding a
stateful open to nfs4, isn't it?

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] leases: break read leases on unlink

[J. Bruce Fields]

On Wed, Sep 21, 2011 at 11:02:41AM -0400, Christoph Hellwig wrote:
> On Wed, Sep 21, 2011 at 10:58:14AM -0400, J. Bruce Fields wrote:
> > A read lease is used by NFSv4 as a guarantee that a client can perform
> > local read opens without informing the server.
> > 
> > The open operation takes the last component of the pathname as an
> > argument, thus is also a lookup operation, and giving the client the
> > above guarantee means informing the client before we allow anything that
> > would change the set of names pointing to the inode.
> 
> That seems like totally strange semantics.  A useful defintion of a
> lase operation would mean it guarantees I/O can happen, and wouldn't
> care about metadata operation.  That's the whole point in adding a
> stateful open to nfs4, isn't it?

One of the purposes of a delegation (==lease) is to let clients perform
open() entirely locally, without waiting for any round trips to the
server.

That doesn't work if the client has to revalidate the open path on every
open().

(And as I understand it--Trond can correct me if I'm
confused--revalidating at least the last component of the path is a
requirement for the close-to-open semantics that the Linux client
guarantees to applications.)

I dunno, perhaps a more logical way to do that would be to define some
kind of lease on the parent directories.  That's not the way the
protocols work.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/6] leases: break read leases on rename

[J. Bruce Fields]

To rely on the i_mutex for exclusion between setlease and rename, we
need rename to take the i_mutex on the source as well as on any possible
target.

I suspect this is deadlock-free, but I need to think this proof through
again.  And I'm not sure what to do about lockdep.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 Documentation/filesystems/directory-locking |   11 ++++++-----
 fs/namei.c                                  |   17 +++++++++++++++--
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/Documentation/filesystems/directory-locking b/Documentation/filesystems/directory-locking
index ff7b611..c51cbed 100644
--- a/Documentation/filesystems/directory-locking
+++ b/Documentation/filesystems/directory-locking
@@ -12,8 +12,8 @@ kinds of locks - per-inode (->i_mutex) and per-filesystem
 locks victim and calls the method.
 
 4) rename() that is _not_ cross-directory.  Locking rules: caller locks
-the parent, finds source and target, if target already exists - locks it
-and then calls the method.
+the parent, finds source and target, locks source, also locks target if
+it already exists, and then calls the method.
 
 5) link creation.  Locking rules:
 	* lock parent
@@ -30,6 +30,7 @@ rules:
 		fail with -ENOTEMPTY
 	* if new parent is equal to or is a descendent of source
 		fail with -ELOOP
+	* lock source if it is not a directory.
 	* if target exists - lock it.
 	* call the method.
 
@@ -56,9 +57,9 @@ objects - A < B iff A is an ancestor of B.
     renames will be blocked on filesystem lock and we don't start changing
     the order until we had acquired all locks).
 
-(3) any operation holds at most one lock on non-directory object and
-    that lock is acquired after all other locks.  (Proof: see descriptions
-    of operations).
+(3) locks on non-directory objects are acquired only after taking locks
+    on their parents (which remain their parents by (1) and (2)).
+    (Proof: see descriptions of operations).
 
 	Now consider the minimal deadlock.  Each process is blocked on
 attempt to acquire some lock and already holds at least one lock.  Let's
diff --git a/fs/namei.c b/fs/namei.c
index 5c78f72..c0220f7 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3058,6 +3058,7 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 			    struct inode *new_dir, struct dentry *new_dentry)
 {
 	struct inode *target = new_dentry->d_inode;
+	struct inode *source = old_dentry->d_inode;
 	int error;
 
 	error = security_inode_rename(old_dir, old_dentry, new_dir, new_dentry);
@@ -3065,13 +3066,23 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 		return error;
 
 	dget(new_dentry);
-	if (target)
+	mutex_lock(&source->i_mutex);
+	error = break_lease(source, O_WRONLY);
+	if (error)
+		goto out_unlock_source;
+	if (target) {
 		mutex_lock(&target->i_mutex);
-
+		error = break_lease(target, O_WRONLY);
+		if (error)
+			goto out;
+	}
 	error = -EBUSY;
 	if (d_mountpoint(old_dentry)||d_mountpoint(new_dentry))
 		goto out;
 
+	error = break_lease(old_dentry->d_inode, O_WRONLY);
+	if (error)
+		goto out;
 	error = old_dir->i_op->rename(old_dir, old_dentry, new_dir, new_dentry);
 	if (error)
 		goto out;
@@ -3083,6 +3094,8 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 out:
 	if (target)
 		mutex_unlock(&target->i_mutex);
+out_unlock_source:
+	mutex_unlock(&source->i_mutex);
 	dput(new_dentry);
 	return error;
 }
-- 
1.7.4.1

Re: [PATCH 4/6] leases: break read leases on rename

[Mimi Zohar]

On Wed, 2011-09-21 at 10:58 -0400, J. Bruce Fields wrote: 
> To rely on the i_mutex for exclusion between setlease and rename, we
> need rename to take the i_mutex on the source as well as on any possible
> target.
> 
> I suspect this is deadlock-free, but I need to think this proof through
> again.  And I'm not sure what to do about lockdep.

Not sure that I will be of any help, but how about posting the lockdep
messages?

thanks,

Mimi

> 
> Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  Documentation/filesystems/directory-locking |   11 ++++++-----
>  fs/namei.c                                  |   17 +++++++++++++++--
>  2 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/filesystems/directory-locking b/Documentation/filesystems/directory-locking
> index ff7b611..c51cbed 100644
> --- a/Documentation/filesystems/directory-locking
> +++ b/Documentation/filesystems/directory-locking
> @@ -12,8 +12,8 @@ kinds of locks - per-inode (->i_mutex) and per-filesystem
>  locks victim and calls the method.
> 
>  4) rename() that is _not_ cross-directory.  Locking rules: caller locks
> -the parent, finds source and target, if target already exists - locks it
> -and then calls the method.
> +the parent, finds source and target, locks source, also locks target if
> +it already exists, and then calls the method.
> 
>  5) link creation.  Locking rules:
>  	* lock parent
> @@ -30,6 +30,7 @@ rules:
>  		fail with -ENOTEMPTY
>  	* if new parent is equal to or is a descendent of source
>  		fail with -ELOOP
> +	* lock source if it is not a directory.
>  	* if target exists - lock it.
>  	* call the method.
> 
> @@ -56,9 +57,9 @@ objects - A < B iff A is an ancestor of B.
>      renames will be blocked on filesystem lock and we don't start changing
>      the order until we had acquired all locks).
> 
> -(3) any operation holds at most one lock on non-directory object and
> -    that lock is acquired after all other locks.  (Proof: see descriptions
> -    of operations).
> +(3) locks on non-directory objects are acquired only after taking locks
> +    on their parents (which remain their parents by (1) and (2)).
> +    (Proof: see descriptions of operations).
> 
>  	Now consider the minimal deadlock.  Each process is blocked on
>  attempt to acquire some lock and already holds at least one lock.  Let's
> diff --git a/fs/namei.c b/fs/namei.c
> index 5c78f72..c0220f7 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3058,6 +3058,7 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
>  			    struct inode *new_dir, struct dentry *new_dentry)
>  {
>  	struct inode *target = new_dentry->d_inode;
> +	struct inode *source = old_dentry->d_inode;
>  	int error;
> 
>  	error = security_inode_rename(old_dir, old_dentry, new_dir, new_dentry);
> @@ -3065,13 +3066,23 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
>  		return error;
> 
>  	dget(new_dentry);
> -	if (target)
> +	mutex_lock(&source->i_mutex);
> +	error = break_lease(source, O_WRONLY);
> +	if (error)
> +		goto out_unlock_source;
> +	if (target) {
>  		mutex_lock(&target->i_mutex);
> -
> +		error = break_lease(target, O_WRONLY);
> +		if (error)
> +			goto out;
> +	}
>  	error = -EBUSY;
>  	if (d_mountpoint(old_dentry)||d_mountpoint(new_dentry))
>  		goto out;
> 
> +	error = break_lease(old_dentry->d_inode, O_WRONLY);
> +	if (error)
> +		goto out;
>  	error = old_dir->i_op->rename(old_dir, old_dentry, new_dir, new_dentry);
>  	if (error)
>  		goto out;
> @@ -3083,6 +3094,8 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
>  out:
>  	if (target)
>  		mutex_unlock(&target->i_mutex);
> +out_unlock_source:
> +	mutex_unlock(&source->i_mutex);
>  	dput(new_dentry);
>  	return error;
>  }

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] leases: break read leases on rename

[J. Bruce Fields]

On Thu, Sep 22, 2011 at 01:17:49PM -0400, Mimi Zohar wrote:
> On Wed, 2011-09-21 at 10:58 -0400, J. Bruce Fields wrote: 
> > To rely on the i_mutex for exclusion between setlease and rename, we
> > need rename to take the i_mutex on the source as well as on any possible
> > target.
> > 
> > I suspect this is deadlock-free, but I need to think this proof through
> > again.  And I'm not sure what to do about lockdep.
> 
> Not sure that I will be of any help, but how about posting the lockdep
> messages?

Sure, appended below, but it's not particularly surprising--we're taking
i_mutex's on four different objects (both parents, source, and target if
any) where before there were three.

I suppose the solution is another i_mutex lock class, used only on the
lock of the source inode?

--b.

=============================================
[ INFO: possible recursive locking detected ]
3.1.0-rc1-00076-g0e7e722 #599
---------------------------------------------
mount/333 is trying to acquire lock:
(&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff811126d8>] vfs_rename+0x278/0x450
Sep 20 17:05:54 pip1 kernel:
but task is already holding lock:
(&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff81112b03>] sys_renameat+0x253/0x2d0
Sep 20 17:05:54 pip1 kernel:
other info that might help us debug this:
Possible unsafe locking scenario:
Sep 20 17:05:54 pip1 kernel:
      CPU0
      ----
 lock(&sb->s_type->i_mutex_key);
 lock(&sb->s_type->i_mutex_key);
Sep 20 17:05:54 pip1 kernel:
*** DEADLOCK ***
Sep 20 17:05:54 pip1 kernel:
May be due to missing lock nesting notation
Sep 20 17:05:54 pip1 kernel:
2 locks held by mount/333:
#0: (&sb->s_type->i_mutex_key#2/1){+.+.+.}, at: [<ffffffff8110efc8>] lock_rename+0xe8/0xf0
#1:  (&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff81112b03>] sys_renameat+0x253/0x2d0
Sep 20 17:05:54 pip1 kernel:
stack backtrace:
Pid: 333, comm: mount Not tainted 3.1.0-rc1-00076-g0e7e722 #599
Call Trace:
[<ffffffff8107c9df>] __lock_acquire+0x15bf/0x1d80
[<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
[<ffffffff8107d794>] lock_acquire+0x94/0x140
[<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
[<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
[<ffffffff81979d7f>] mutex_lock_nested+0x4f/0x360
[<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
[<ffffffff8103b2b1>] ?  get_parent_ip+0x11/0x50
[<ffffffff8197ed9d>] ?  sub_preempt_count+0x9d/0xd0
[<ffffffff811126d8>] vfs_rename+0x278/0x450
[<ffffffff8103b2b1>] ?  get_parent_ip+0x11/0x50
[<ffffffff81112b5d>] sys_renameat+0x2ad/0x2d0
[<ffffffff810edab3>] ? remove_vma+0x53/0x70
[<ffffffff81079b0d>] ?  trace_hardirqs_on_caller+0xfd/0x190
[<ffffffff81079bad>] ?  trace_hardirqs_on+0xd/0x10
[<ffffffff810edab3>] ? remove_vma+0x53/0x70
[<ffffffff81982998>] ?  sysret_check+0x26/0x60
[<ffffffff81079b0d>] ?  trace_hardirqs_on_caller+0xfd/0x190
[<ffffffff81112b9b>] sys_rename+0x1b/0x20
[<ffffffff81982968>] system_call_fastpath+0x16/0x1b

Re: [PATCH 4/6] leases: break read leases on rename

[J. Bruce Fields]

On Fri, Sep 23, 2011 at 12:55:13PM -0400, J. Bruce Fields wrote:
> On Thu, Sep 22, 2011 at 01:17:49PM -0400, Mimi Zohar wrote:
> > On Wed, 2011-09-21 at 10:58 -0400, J. Bruce Fields wrote: 
> > > To rely on the i_mutex for exclusion between setlease and rename, we
> > > need rename to take the i_mutex on the source as well as on any possible
> > > target.
> > > 
> > > I suspect this is deadlock-free, but I need to think this proof through
> > > again.  And I'm not sure what to do about lockdep.
> > 
> > Not sure that I will be of any help, but how about posting the lockdep
> > messages?
> 
> Sure, appended below, but it's not particularly surprising--we're taking
> i_mutex's on four different objects (both parents, source, and target if
> any) where before there were three.
> 
> I suppose the solution is another i_mutex lock class, used only on the
> lock of the source inode?

That'd be something like this.  (Works for me, anyway.)  I'll fold it
into the previous patch.

--b.

diff --git a/fs/namei.c b/fs/namei.c
index f6de42d..06a8d95 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3069,7 +3069,7 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 		return error;
 
 	dget(new_dentry);
-	mutex_lock(&source->i_mutex);
+	mutex_lock_nested(&source->i_mutex, I_MUTEX_RENAME_SOURCE);
 	error = break_lease(source, O_WRONLY);
 	if (error)
 		goto out_unlock_source;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 76460ed..74f4979 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -839,10 +839,12 @@ static inline int inode_unhashed(struct inode *inode)
  * 0: the object of the current VFS operation
  * 1: parent
  * 2: child/target
- * 3: quota file
+ * 3: xattr
+ * 4: quota file
+ * 5: the file being renamed (used only in rename of a non-directory)
  *
  * The locking order between these classes is
- * parent -> child -> normal -> xattr -> quota
+ * parent -> child -> rename_source -> normal -> xattr -> quota
  */
 enum inode_i_mutex_lock_class
 {
@@ -850,7 +852,8 @@ enum inode_i_mutex_lock_class
 	I_MUTEX_PARENT,
 	I_MUTEX_CHILD,
 	I_MUTEX_XATTR,
-	I_MUTEX_QUOTA
+	I_MUTEX_QUOTA,
+	I_MUTEX_RENAME_SOURCE
 };
 
 /*
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] leases: break read leases on rename

[Mimi Zohar]

On Fri, 2011-09-23 at 12:55 -0400, J. Bruce Fields wrote:
> On Thu, Sep 22, 2011 at 01:17:49PM -0400, Mimi Zohar wrote:
> > On Wed, 2011-09-21 at 10:58 -0400, J. Bruce Fields wrote: 
> > > To rely on the i_mutex for exclusion between setlease and rename, we
> > > need rename to take the i_mutex on the source as well as on any possible
> > > target.
> > > 
> > > I suspect this is deadlock-free, but I need to think this proof through
> > > again.  And I'm not sure what to do about lockdep.
> > 
> > Not sure that I will be of any help, but how about posting the lockdep
> > messages?
> 
> Sure, appended below, but it's not particularly surprising--we're taking
> i_mutex's on four different objects (both parents, source, and target if
> any) where before there were three.
> 
> I suppose the solution is another i_mutex lock class, used only on the
> lock of the source inode?
> 
> --b.

I must not be missing something, but it seems taking the i_mutex here is
unnecessary.  fs/namei.c:lock_rename() already does all the locking for
you.

Mimi

> =============================================
> [ INFO: possible recursive locking detected ]
> 3.1.0-rc1-00076-g0e7e722 #599
> ---------------------------------------------
> mount/333 is trying to acquire lock:
> (&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff811126d8>] vfs_rename+0x278/0x450
> Sep 20 17:05:54 pip1 kernel:
> but task is already holding lock:
> (&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff81112b03>] sys_renameat+0x253/0x2d0
> Sep 20 17:05:54 pip1 kernel:
> other info that might help us debug this:
> Possible unsafe locking scenario:
> Sep 20 17:05:54 pip1 kernel:
>       CPU0
>       ----
>  lock(&sb->s_type->i_mutex_key);
>  lock(&sb->s_type->i_mutex_key);
> Sep 20 17:05:54 pip1 kernel:
> *** DEADLOCK ***
> Sep 20 17:05:54 pip1 kernel:
> May be due to missing lock nesting notation
> Sep 20 17:05:54 pip1 kernel:
> 2 locks held by mount/333:
> #0: (&sb->s_type->i_mutex_key#2/1){+.+.+.}, at: [<ffffffff8110efc8>] lock_rename+0xe8/0xf0
> #1:  (&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff81112b03>] sys_renameat+0x253/0x2d0
> Sep 20 17:05:54 pip1 kernel:
> stack backtrace:
> Pid: 333, comm: mount Not tainted 3.1.0-rc1-00076-g0e7e722 #599
> Call Trace:
> [<ffffffff8107c9df>] __lock_acquire+0x15bf/0x1d80
> [<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
> [<ffffffff8107d794>] lock_acquire+0x94/0x140
> [<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
> [<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
> [<ffffffff81979d7f>] mutex_lock_nested+0x4f/0x360
> [<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
> [<ffffffff8103b2b1>] ?  get_parent_ip+0x11/0x50
> [<ffffffff8197ed9d>] ?  sub_preempt_count+0x9d/0xd0
> [<ffffffff811126d8>] vfs_rename+0x278/0x450
> [<ffffffff8103b2b1>] ?  get_parent_ip+0x11/0x50
> [<ffffffff81112b5d>] sys_renameat+0x2ad/0x2d0
> [<ffffffff810edab3>] ? remove_vma+0x53/0x70
> [<ffffffff81079b0d>] ?  trace_hardirqs_on_caller+0xfd/0x190
> [<ffffffff81079bad>] ?  trace_hardirqs_on+0xd/0x10
> [<ffffffff810edab3>] ? remove_vma+0x53/0x70
> [<ffffffff81982998>] ?  sysret_check+0x26/0x60
> [<ffffffff81079b0d>] ?  trace_hardirqs_on_caller+0xfd/0x190
> [<ffffffff81112b9b>] sys_rename+0x1b/0x20
> [<ffffffff81982968>] system_call_fastpath+0x16/0x1b
> 

Re: [PATCH 4/6] leases: break read leases on rename

[J. Bruce Fields]

On Fri, Sep 23, 2011 at 03:58:25PM -0400, Mimi Zohar wrote:
> On Fri, 2011-09-23 at 12:55 -0400, J. Bruce Fields wrote:
> > On Thu, Sep 22, 2011 at 01:17:49PM -0400, Mimi Zohar wrote:
> > > On Wed, 2011-09-21 at 10:58 -0400, J. Bruce Fields wrote: 
> > > > To rely on the i_mutex for exclusion between setlease and rename, we
> > > > need rename to take the i_mutex on the source as well as on any possible
> > > > target.
> > > > 
> > > > I suspect this is deadlock-free, but I need to think this proof through
> > > > again.  And I'm not sure what to do about lockdep.
> > > 
> > > Not sure that I will be of any help, but how about posting the lockdep
> > > messages?
> > 
> > Sure, appended below, but it's not particularly surprising--we're taking
> > i_mutex's on four different objects (both parents, source, and target if
> > any) where before there were three.
> > 
> > I suppose the solution is another i_mutex lock class, used only on the
> > lock of the source inode?
> > 
> > --b.
> 
> I must not be missing something, but it seems taking the i_mutex here is
> unnecessary.  fs/namei.c:lock_rename() already does all the locking for
> you.

There are up to four locks taken (before this patch):

	- the filesystem's rename lock
	- the i_mutex on the parents of the source and target
	- the i_mutex on the target (if it exists), which will be
	  unlinked when it's renamed over.

All but the last are taken care of by lock_rename(), yes.

This adds a fifth lock, i_mutex on the source.

The reason we add the lock is for mutual exclusion between rename and
setlease--we shouldn't grant leases on a file while a rename of that
file is in progress.

--b.

> 
> Mimi
> 
> > =============================================
> > [ INFO: possible recursive locking detected ]
> > 3.1.0-rc1-00076-g0e7e722 #599
> > ---------------------------------------------
> > mount/333 is trying to acquire lock:
> > (&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff811126d8>] vfs_rename+0x278/0x450
> > Sep 20 17:05:54 pip1 kernel:
> > but task is already holding lock:
> > (&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff81112b03>] sys_renameat+0x253/0x2d0
> > Sep 20 17:05:54 pip1 kernel:
> > other info that might help us debug this:
> > Possible unsafe locking scenario:
> > Sep 20 17:05:54 pip1 kernel:
> >       CPU0
> >       ----
> >  lock(&sb->s_type->i_mutex_key);
> >  lock(&sb->s_type->i_mutex_key);
> > Sep 20 17:05:54 pip1 kernel:
> > *** DEADLOCK ***
> > Sep 20 17:05:54 pip1 kernel:
> > May be due to missing lock nesting notation
> > Sep 20 17:05:54 pip1 kernel:
> > 2 locks held by mount/333:
> > #0: (&sb->s_type->i_mutex_key#2/1){+.+.+.}, at: [<ffffffff8110efc8>] lock_rename+0xe8/0xf0
> > #1:  (&sb->s_type->i_mutex_key#2){+.+.+.}, at: [<ffffffff81112b03>] sys_renameat+0x253/0x2d0
> > Sep 20 17:05:54 pip1 kernel:
> > stack backtrace:
> > Pid: 333, comm: mount Not tainted 3.1.0-rc1-00076-g0e7e722 #599
> > Call Trace:
> > [<ffffffff8107c9df>] __lock_acquire+0x15bf/0x1d80
> > [<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
> > [<ffffffff8107d794>] lock_acquire+0x94/0x140
> > [<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
> > [<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
> > [<ffffffff81979d7f>] mutex_lock_nested+0x4f/0x360
> > [<ffffffff811126d8>] ?  vfs_rename+0x278/0x450
> > [<ffffffff8103b2b1>] ?  get_parent_ip+0x11/0x50
> > [<ffffffff8197ed9d>] ?  sub_preempt_count+0x9d/0xd0
> > [<ffffffff811126d8>] vfs_rename+0x278/0x450
> > [<ffffffff8103b2b1>] ?  get_parent_ip+0x11/0x50
> > [<ffffffff81112b5d>] sys_renameat+0x2ad/0x2d0
> > [<ffffffff810edab3>] ? remove_vma+0x53/0x70
> > [<ffffffff81079b0d>] ?  trace_hardirqs_on_caller+0xfd/0x190
> > [<ffffffff81079bad>] ?  trace_hardirqs_on+0xd/0x10
> > [<ffffffff810edab3>] ? remove_vma+0x53/0x70
> > [<ffffffff81982998>] ?  sysret_check+0x26/0x60
> > [<ffffffff81079b0d>] ?  trace_hardirqs_on_caller+0xfd/0x190
> > [<ffffffff81112b9b>] sys_rename+0x1b/0x20
> > [<ffffffff81982968>] system_call_fastpath+0x16/0x1b
> > 
> 
> 

[PATCH 6/6] leases: break read leases on link

[J. Bruce Fields]

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/namei.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index c0220f7..f6de42d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2904,8 +2904,11 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de
 	/* Make sure we don't allow creating hardlink to an unlinked file */
 	if (inode->i_nlink == 0)
 		error =  -ENOENT;
-	else
-		error = dir->i_op->link(old_dentry, dir, new_dentry);
+	else {
+		error = break_lease(inode, O_WRONLY);
+		if (!error)
+			error = dir->i_op->link(old_dentry, dir, new_dentry);
+	}
 	mutex_unlock(&inode->i_mutex);
 	if (!error)
 		fsnotify_link(dir, inode, new_dentry);
-- 
1.7.4.1

Re: breaking leases on metadata changes

[Stefan (metze) Metzmacher]

[-- Attachment #1: Type: text/plain, Size: 826 bytes --]

Hi Bruce,

> So there's this longstanding problem that read leases should really be
> broken on anything that changes an inode's metadata or any change to the
> set of links pointing to the inode.
> 
> (Cc'ing samba-technical for confirmation of that statement from Samba's
> point of view....)
> 
> So we need mutual exclusion between read leases and link, unlink,
> rename, etc.

As far as I know oplocks in SMB only care about content
and attribute only opens (without the READ/WRITE access bits) doesn't
break oplocks.

So changing the modification time of a file should not break the oplock.

I think link and rename doesn't break oplocks, while unlink would.

But Samba requirements may change for SMB 2.2 support with directory leases.
We're currently exploring what the details are.

metze



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: breaking leases on metadata changes

[J. Bruce Fields]

On Sat, Sep 24, 2011 at 08:36:04PM +0200, Stefan (metze) Metzmacher wrote:
> Hi Bruce,
> 
> > So there's this longstanding problem that read leases should really be
> > broken on anything that changes an inode's metadata or any change to the
> > set of links pointing to the inode.
> > 
> > (Cc'ing samba-technical for confirmation of that statement from Samba's
> > point of view....)
> > 
> > So we need mutual exclusion between read leases and link, unlink,
> > rename, etc.
> 
> As far as I know oplocks in SMB only care about content
> and attribute only opens (without the READ/WRITE access bits) doesn't
> break oplocks.

Thanks for looking into this!

Just wandering around msdn (but not having to background to know what
really applies)....  The discussion of "Level 2 Opportunistic Locks"
(which is what I assume you'd be using read leases to implement) at
http://msdn.microsoft.com/en-us/library/aa365713(v=VS.85).aspx says such
an oplock "allows the client to perform read operations and obtain
fileattributes using cached or read-ahead local information".  Which
suggests the oplock also covers attributes?

But I'm probably not reading the right thing.

Is it possible to get any sort of documentation or test results or
confirm the behavior here?

> So changing the modification time of a file should not break the oplock.
> 
> I think link and rename doesn't break oplocks, while unlink would.
> 
> But Samba requirements may change for SMB 2.2 support with directory leases.
> We're currently exploring what the details are.

Well, presumably you'll need to keep supporting the old semantics as
well, regardless of what the new protocol may do.

Anyway, if this is all correct then it sounds like v4 read delegations
and level 2 oplocks may be more different than I'd thought.

In which case I'm probably better off leaving leases alone for now and
defining a new lock type for v4 delegations.  Which could be exposed to
userspace later as well if it turned out to be useful to someone.

That's not to rule out fixes to lease behavior to work better for Samba
if there are any that would make sense.

--b.

Re: breaking leases on metadata changes

[J. Bruce Fields]

On Mon, Sep 26, 2011 at 10:10:27AM -0400, J. Bruce Fields wrote:
> Anyway, if this is all correct then it sounds like v4 read delegations
> and level 2 oplocks may be more different than I'd thought.

By the way, another ignorant question: poking around SMB2 documentation
I see that there are both "oplocks" and "leases".  What's the
relationship between them, and which of them should map to Linux leases?

--b.