break leases on nfsd rename, setattr, etc.

break leases on nfsd rename, setattr, etc.

[J. Bruce Fields]

NFSv4 read delegations should be broken on anything that modifies file
metadata, or modifies (by rename, link, or unlink) any of the links that
point to the file.

It's going to take a while longer to finish vfs patches that solve the
problem completely; for now I wonder if an (incomplete) nfsd-only fix
would be worthwhile?

--b.

[PATCH 1/2] nfsd4: break lease on nfsd setattr

[J. Bruce Fields]

Leases (delegations) should really be broken on any metadata change, not
just on size change.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/vfs.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index f8919f5..839ed88 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -374,14 +374,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 				goto out;
 		}
 
-		/*
-		 * If we are changing the size of the file, then
-		 * we need to break all leases.
-		 */
-		host_err = break_lease(inode, O_WRONLY | O_NONBLOCK);
-		if (host_err) /* ENOMEM or EWOULDBLOCK */
-			goto out_nfserr;
-
 		host_err = get_write_access(inode);
 		if (host_err)
 			goto out_nfserr;
@@ -422,7 +414,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 
 	err = nfserr_notsync;
 	if (!check_guard || guardtime == inode->i_ctime.tv_sec) {
+		host_err = break_lease(inode, O_WRONLY | O_NONBLOCK);
+		if (host_err)
+			goto out_nfserr;
 		fh_lock(fhp);
+
 		host_err = notify_change(dentry, iap);
 		err = nfserrno(host_err);
 		fh_unlock(fhp);
-- 
1.7.1

[PATCH 2/2] nfsd: break lease on unlink, link, and rename

[J. Bruce Fields]

Any change to any of the links pointing to an entry should also break
delegations.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/vfs.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 839ed88..f31321a 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -272,6 +272,13 @@ out:
 	return err;
 }
 
+static int nfsd_break_lease(struct inode *inode)
+{
+	if (!S_ISREG(inode->i_mode))
+		return 0;
+	return break_lease(inode, O_WRONLY | O_NONBLOCK);
+}
+
 /*
  * Commit metadata changes to stable storage.
  */
@@ -414,7 +421,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
 
 	err = nfserr_notsync;
 	if (!check_guard || guardtime == inode->i_ctime.tv_sec) {
-		host_err = break_lease(inode, O_WRONLY | O_NONBLOCK);
+		host_err = nfsd_break_lease(inode);
 		if (host_err)
 			goto out_nfserr;
 		fh_lock(fhp);
@@ -1641,6 +1648,12 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
 		err = nfserrno(host_err);
 		goto out_dput;
 	}
+	err = nfserr_noent;
+	if (!dold->d_inode)
+		goto out_drop_write;
+	host_err = nfsd_break_lease(dold->d_inode);
+	if (host_err)
+		goto out_drop_write;
 	host_err = vfs_link(dold, dirp, dnew);
 	if (!host_err) {
 		err = nfserrno(commit_metadata(ffhp));
@@ -1652,6 +1665,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
 		else
 			err = nfserrno(host_err);
 	}
+out_drop_write:
 	mnt_drop_write(tfhp->fh_export->ex_path.mnt);
 out_dput:
 	dput(dnew);
@@ -1733,15 +1747,17 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
 	if (host_err)
 		goto out_dput_new;
 
+	host_err = nfsd_break_lease(odentry->d_inode);
+	if (host_err)
+		goto out_drop_write;
 	host_err = vfs_rename(fdir, odentry, tdir, ndentry);
 	if (!host_err) {
 		host_err = commit_metadata(tfhp);
 		if (!host_err)
 			host_err = commit_metadata(ffhp);
 	}
-
+out_drop_write:
 	mnt_drop_write(ffhp->fh_export->ex_path.mnt);
-
  out_dput_new:
 	dput(ndentry);
  out_dput_old:
@@ -1804,11 +1820,14 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
 	if (host_err)
 		goto out_nfserr;
 
+	host_err = nfsd_break_lease(rdentry->d_inode);
+	if (host_err)
+		goto out_put;
 	if (type != S_IFDIR)
 		host_err = vfs_unlink(dirp, rdentry);
 	else
 		host_err = vfs_rmdir(dirp, rdentry);
-
+out_put:
 	dput(rdentry);
 
 	if (!host_err)
-- 
1.7.1

Re: [PATCH 2/2] nfsd: break lease on unlink, link, and rename

[Mi Jinlong]

J. Bruce Fields:
> Any change to any of the links pointing to an entry should also break
> delegations.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/nfsd/vfs.c |   27 +++++++++++++++++++++++----
>  1 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 839ed88..f31321a 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -272,6 +272,13 @@ out:
>  	return err;
>  }
>  
> +static int nfsd_break_lease(struct inode *inode)
> +{
> +	if (!S_ISREG(inode->i_mode))
> +		return 0;
> +	return break_lease(inode, O_WRONLY | O_NONBLOCK);

Hi Bruce,

break_lease will return -EWOULDBLOCK here if break success, not 0.
  at __break_lease:
   
    1193  */
    1194 int __break_lease(struct inode *inode, unsigned int mode)
    1195 {
         ... ...  
    1253         if (i_have_this_lease || (mode & O_NONBLOCK)) {
    1254                 error = -EWOULDBLOCK;
    1255                 goto out;
    1256         }
          ... ... 
    1283 out:
    1284         unlock_flocks();
    1285         if (!IS_ERR(new_fl))
    1286                 locks_free_lock(new_fl);
    1287         return error;
    1288 }

If we just return the error -EWOULDBLOCK (which mapped to -EAGAIN) at
nfsd_break_lease(), the NFS request will be drop for -EAGAIN is mapped
to nfserr_dropit at nfserrno(). As that, client will hang up about 60s
until a retry success.

Maybe we should have a check here as:

-       return break_lease(inode, O_WRONLY | O_NONBLOCK);
+       int status = break_lease(inode, O_WRONLY | O_NONBLOCK);
+       return -EWOULDBLOCK == status ? 0 : status;

-- 
----
thanks
Mi Jinlong

Re: [PATCH 2/2] nfsd: break lease on unlink, link, and rename

[J. Bruce Fields]

On Fri, Jan 14, 2011 at 04:21:53PM +0800, Mi Jinlong wrote:
> 
> 
> J. Bruce Fields:
> > Any change to any of the links pointing to an entry should also break
> > delegations.
> > 
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> >  fs/nfsd/vfs.c |   27 +++++++++++++++++++++++----
> >  1 files changed, 23 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 839ed88..f31321a 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -272,6 +272,13 @@ out:
> >  	return err;
> >  }
> >  
> > +static int nfsd_break_lease(struct inode *inode)
> > +{
> > +	if (!S_ISREG(inode->i_mode))
> > +		return 0;
> > +	return break_lease(inode, O_WRONLY | O_NONBLOCK);
> 
> Hi Bruce,
> 
> break_lease will return -EWOULDBLOCK here if break success, not 0.
>   at __break_lease:
>    
>     1193  */
>     1194 int __break_lease(struct inode *inode, unsigned int mode)
>     1195 {
>          ... ...  
>     1253         if (i_have_this_lease || (mode & O_NONBLOCK)) {
>     1254                 error = -EWOULDBLOCK;
>     1255                 goto out;
>     1256         }
>           ... ... 
>     1283 out:
>     1284         unlock_flocks();
>     1285         if (!IS_ERR(new_fl))
>     1286                 locks_free_lock(new_fl);
>     1287         return error;
>     1288 }
> 
> If we just return the error -EWOULDBLOCK (which mapped to -EAGAIN) at
> nfsd_break_lease(), the NFS request will be drop for -EAGAIN is mapped
> to nfserr_dropit at nfserrno().

See my for-2.6.38-incoming branch (especially 062304a815fe and
surrounding).  I was getting tired of needing special handling for
EAGAIN and EWOULDBLOCK, so I removed that mapping and am setting a flag
on the rqstp instead to indicate the need to drop a request.

Does that look reasonable to you?

--b.

> As that, client will hang up about 60s
> until a retry success.
> 
> Maybe we should have a check here as:
> 
> -       return break_lease(inode, O_WRONLY | O_NONBLOCK);
> +       int status = break_lease(inode, O_WRONLY | O_NONBLOCK);
> +       return -EWOULDBLOCK == status ? 0 : status;
> 
> -- 
> ----
> thanks
> Mi Jinlong
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] nfsd: break lease on unlink, link, and rename

[Mi Jinlong]

J. Bruce Fields :
> On Fri, Jan 14, 2011 at 04:21:53PM +0800, Mi Jinlong wrote:
>>
>> J. Bruce Fields:
>>> Any change to any of the links pointing to an entry should also break
>>> delegations.
>>>
>>> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>>> ---
>>>  fs/nfsd/vfs.c |   27 +++++++++++++++++++++++----
>>>  1 files changed, 23 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>> index 839ed88..f31321a 100644
>>> --- a/fs/nfsd/vfs.c
>>> +++ b/fs/nfsd/vfs.c
>>> @@ -272,6 +272,13 @@ out:
>>>  	return err;
>>>  }
>>>  
>>> +static int nfsd_break_lease(struct inode *inode)
>>> +{
>>> +	if (!S_ISREG(inode->i_mode))
>>> +		return 0;
>>> +	return break_lease(inode, O_WRONLY | O_NONBLOCK);
>> Hi Bruce,
>>
>> break_lease will return -EWOULDBLOCK here if break success, not 0.
>>   at __break_lease:
>>    
>>     1193  */
>>     1194 int __break_lease(struct inode *inode, unsigned int mode)
>>     1195 {
>>          ... ...  
>>     1253         if (i_have_this_lease || (mode & O_NONBLOCK)) {
>>     1254                 error = -EWOULDBLOCK;
>>     1255                 goto out;
>>     1256         }
>>           ... ... 
>>     1283 out:
>>     1284         unlock_flocks();
>>     1285         if (!IS_ERR(new_fl))
>>     1286                 locks_free_lock(new_fl);
>>     1287         return error;
>>     1288 }
>>
>> If we just return the error -EWOULDBLOCK (which mapped to -EAGAIN) at
>> nfsd_break_lease(), the NFS request will be drop for -EAGAIN is mapped
>> to nfserr_dropit at nfserrno().
> 
> See my for-2.6.38-incoming branch (especially 062304a815fe and
> surrounding).  I was getting tired of needing special handling for
> EAGAIN and EWOULDBLOCK, so I removed that mapping and am setting a flag
> on the rqstp instead to indicate the need to drop a request.
> 
> Does that look reasonable to you?

  I'm sorry for missing those patch you have post before,
  I just test the two patch at v2.6.37. ^_^

  Those patch is great.

  According to the discussing before, after the two patch for breaking delegation,
  a race as you said "
    One problem is that there's a race: nothing I can see stops anyone from
    getting another lease after may_delete() but before the delete happens."
  also exist here. We can't make sure that a new delegation don't be granted
  between nfsd_break_lease() and unlink file complete or rename complete. 

---
thanks
Mi Jinlong

Re: [PATCH 2/2] nfsd: break lease on unlink, link, and rename

[J. Bruce Fields]

On Mon, Jan 17, 2011 at 10:09:00AM +0800, Mi Jinlong wrote:
> 
> 
> J. Bruce Fields :
> > On Fri, Jan 14, 2011 at 04:21:53PM +0800, Mi Jinlong wrote:
> >>
> >> J. Bruce Fields:
> >>> Any change to any of the links pointing to an entry should also break
> >>> delegations.
> >>>
> >>> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> >>> ---
> >>>  fs/nfsd/vfs.c |   27 +++++++++++++++++++++++----
> >>>  1 files changed, 23 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> >>> index 839ed88..f31321a 100644
> >>> --- a/fs/nfsd/vfs.c
> >>> +++ b/fs/nfsd/vfs.c
> >>> @@ -272,6 +272,13 @@ out:
> >>>  	return err;
> >>>  }
> >>>  
> >>> +static int nfsd_break_lease(struct inode *inode)
> >>> +{
> >>> +	if (!S_ISREG(inode->i_mode))
> >>> +		return 0;
> >>> +	return break_lease(inode, O_WRONLY | O_NONBLOCK);
> >> Hi Bruce,
> >>
> >> break_lease will return -EWOULDBLOCK here if break success, not 0.
> >>   at __break_lease:
> >>    
> >>     1193  */
> >>     1194 int __break_lease(struct inode *inode, unsigned int mode)
> >>     1195 {
> >>          ... ...  
> >>     1253         if (i_have_this_lease || (mode & O_NONBLOCK)) {
> >>     1254                 error = -EWOULDBLOCK;
> >>     1255                 goto out;
> >>     1256         }
> >>           ... ... 
> >>     1283 out:
> >>     1284         unlock_flocks();
> >>     1285         if (!IS_ERR(new_fl))
> >>     1286                 locks_free_lock(new_fl);
> >>     1287         return error;
> >>     1288 }
> >>
> >> If we just return the error -EWOULDBLOCK (which mapped to -EAGAIN) at
> >> nfsd_break_lease(), the NFS request will be drop for -EAGAIN is mapped
> >> to nfserr_dropit at nfserrno().
> > 
> > See my for-2.6.38-incoming branch (especially 062304a815fe and
> > surrounding).  I was getting tired of needing special handling for
> > EAGAIN and EWOULDBLOCK, so I removed that mapping and am setting a flag
> > on the rqstp instead to indicate the need to drop a request.
> > 
> > Does that look reasonable to you?
> 
>   I'm sorry for missing those patch you have post before,
>   I just test the two patch at v2.6.37. ^_^
> 
>   Those patch is great.
> 
>   According to the discussing before, after the two patch for breaking delegation,
>   a race as you said "
>     One problem is that there's a race: nothing I can see stops anyone from
>     getting another lease after may_delete() but before the delete happens."
>   also exist here. We can't make sure that a new delegation don't be granted
>   between nfsd_break_lease() and unlink file complete or rename complete. 

That's correct.  There are also at least a couple more preexisting races
in the lease code that can allow conflicting leases and opens.  Also,
this patch doesn't address the problem of unlinks and renames not done
by nfs clients.

--b.