break leases on nfsd rename, setattr, etc.

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

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/vfs.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

fs/nfsd/vfs.c +0 -0

index 839ed88..f31321a 100644

J. Bruce Fields:

Quoted 13 lines

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

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/vfs.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

fs/nfsd/vfs.c +0 -0

index 839ed88..f31321a 100644

fs/nfsd/vfs.c out: · line 279

Quoted 8 lines

fs/nfsd/vfs.c +5 -0

@@ -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:

(quoted diff) +2 -1

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

Signature · 10 lines

-- 
----
thanks
Mi Jinlong


--
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

Quoted 44 lines

J. Bruce Fields:

Quoted 13 lines

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

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/vfs.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

fs/nfsd/vfs.c +0 -0

index 839ed88..f31321a 100644

fs/nfsd/vfs.c out: · line 279

Quoted 8 lines

fs/nfsd/vfs.c +5 -0

@@ -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 }

Quoted 3 lines

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?

J. Bruce Fields :

Quoted 49 lines · Mi Jinlong

Quoted 44 lines

J. Bruce Fields:

Quoted 13 lines

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

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/vfs.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

fs/nfsd/vfs.c +0 -0

index 839ed88..f31321a 100644

fs/nfsd/vfs.c out: · line 279

Quoted 8 lines

fs/nfsd/vfs.c +5 -0

@@ -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 }

Quoted 3 lines

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().

Quoted 6 lines

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.