remove lease callbacks

remove lease callbacks

[J. Bruce Fields]

A couple lock manager calls used by the lease code look unnecessary to
me; the following patches remove them.  I'm planning on submitting them
with the nfsd patches for 2.6.38, barring any objections.

--b.

[PATCH 1/4] nfsd4: eliminate lease delete callback

[J. Bruce Fields]

nfsd controls the lifetime of the lease, not the lock code, so there's
no need for this callback on lease destruction.

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

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b82e368..2e44ad2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2296,23 +2296,6 @@ void nfsd_break_deleg_cb(struct file_lock *fl)
 }
 
 /*
- * The file_lock is being reapd.
- *
- * Called by locks_free_lock() with lock_flocks() held.
- */
-static
-void nfsd_release_deleg_cb(struct file_lock *fl)
-{
-	struct nfs4_delegation *dp = (struct nfs4_delegation *)fl->fl_owner;
-
-	dprintk("NFSD nfsd_release_deleg_cb: fl %p dp %p dl_count %d\n", fl,dp, atomic_read(&dp->dl_count));
-
-	if (!(fl->fl_flags & FL_LEASE) || !dp)
-		return;
-	dp->dl_flock = NULL;
-}
-
-/*
  * Called from setlease() with lock_flocks() held
  */
 static
@@ -2341,7 +2324,6 @@ int nfsd_change_deleg_cb(struct file_lock **onlist, int arg)
 
 static const struct lock_manager_operations nfsd_lease_mng_ops = {
 	.fl_break = nfsd_break_deleg_cb,
-	.fl_release_private = nfsd_release_deleg_cb,
 	.fl_mylease = nfsd_same_client_deleg_cb,
 	.fl_change = nfsd_change_deleg_cb,
 };
-- 
1.7.1

[PATCH 1/4] nfsd4: eliminate lease delete callback

[J. Bruce Fields]

nfsd controls the lifetime of the lease, not the lock code, so there's
no need for this callback on lease destruction.

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

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b82e368..2e44ad2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2296,23 +2296,6 @@ void nfsd_break_deleg_cb(struct file_lock *fl)
 }
 
 /*
- * The file_lock is being reapd.
- *
- * Called by locks_free_lock() with lock_flocks() held.
- */
-static
-void nfsd_release_deleg_cb(struct file_lock *fl)
-{
-	struct nfs4_delegation *dp = (struct nfs4_delegation *)fl->fl_owner;
-
-	dprintk("NFSD nfsd_release_deleg_cb: fl %p dp %p dl_count %d\n", fl,dp, atomic_read(&dp->dl_count));
-
-	if (!(fl->fl_flags & FL_LEASE) || !dp)
-		return;
-	dp->dl_flock = NULL;
-}
-
-/*
  * Called from setlease() with lock_flocks() held
  */
 static
@@ -2341,7 +2324,6 @@ int nfsd_change_deleg_cb(struct file_lock **onlist, int arg)
 
 static const struct lock_manager_operations nfsd_lease_mng_ops = {
 	.fl_break = nfsd_break_deleg_cb,
-	.fl_release_private = nfsd_release_deleg_cb,
 	.fl_mylease = nfsd_same_client_deleg_cb,
 	.fl_change = nfsd_change_deleg_cb,
 };
-- 
1.7.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 2/4] nfsd4: use a single struct file for delegations

[J. Bruce Fields]

When we converted to sharing struct filess between nfs4 opens I went too
far and also used the same mechanism for delegations.  But keeping
a reference to the struct file ensures it will outlast the lease, and
allows us to remove the lease with the same file as we added it.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4state.c |   10 +++++-----
 fs/nfsd/state.h     |    1 +
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2e44ad2..cbe1b81 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -230,7 +230,8 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_stateid *stp, struct svc_f
 	dp->dl_client = clp;
 	get_nfs4_file(fp);
 	dp->dl_file = fp;
-	nfs4_file_get_access(fp, O_RDONLY);
+	dp->dl_vfs_file = find_readable_file(fp);
+	get_file(dp->dl_vfs_file);
 	dp->dl_flock = NULL;
 	dp->dl_type = type;
 	dp->dl_stateid.si_boot = boot_time;
@@ -252,6 +253,7 @@ nfs4_put_delegation(struct nfs4_delegation *dp)
 	if (atomic_dec_and_test(&dp->dl_count)) {
 		dprintk("NFSD: freeing dp %p\n",dp);
 		put_nfs4_file(dp->dl_file);
+		fput(dp->dl_vfs_file);
 		kmem_cache_free(deleg_slab, dp);
 		num_delegations--;
 	}
@@ -265,12 +267,10 @@ nfs4_put_delegation(struct nfs4_delegation *dp)
 static void
 nfs4_close_delegation(struct nfs4_delegation *dp)
 {
-	struct file *filp = find_readable_file(dp->dl_file);
-
 	dprintk("NFSD: close_delegation dp %p\n",dp);
+	/* XXX: do we even need this check?: */
 	if (dp->dl_flock)
-		vfs_setlease(filp, F_UNLCK, &dp->dl_flock);
-	nfs4_file_put_access(dp->dl_file, O_RDONLY);
+		vfs_setlease(dp->dl_vfs_file, F_UNLCK, &dp->dl_flock);
 }
 
 /* Called under the state lock. */
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 39adc27..84b2302 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -81,6 +81,7 @@ struct nfs4_delegation {
 	atomic_t		dl_count;       /* ref count */
 	struct nfs4_client	*dl_client;
 	struct nfs4_file	*dl_file;
+	struct file		*dl_vfs_file;
 	struct file_lock	*dl_flock;
 	u32			dl_type;
 	time_t			dl_time;
-- 
1.7.1

[PATCH 3/4] locks: eliminate fl_mylease callback

[J. Bruce Fields]

The nfs server only supports read delegations for now, so we don't care
how conflicts are determined.  All we care is that unlocks are
recognized as matching the leases they are meant to remove.  After the
last patch, a comparison of struct files will work for that purpose.  So
we no longer need this callback.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/locks.c          |    8 +-------
 fs/nfsd/nfs4state.c |   21 +--------------------
 include/linux/fs.h  |    1 -
 3 files changed, 2 insertions(+), 28 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 8729347..5cb6506 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -444,15 +444,9 @@ static void lease_release_private_callback(struct file_lock *fl)
 	fl->fl_file->f_owner.signum = 0;
 }
 
-static int lease_mylease_callback(struct file_lock *fl, struct file_lock *try)
-{
-	return fl->fl_file == try->fl_file;
-}
-
 static const struct lock_manager_operations lease_manager_ops = {
 	.fl_break = lease_break_callback,
 	.fl_release_private = lease_release_private_callback,
-	.fl_mylease = lease_mylease_callback,
 	.fl_change = lease_modify,
 };
 
@@ -1405,7 +1399,7 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 	for (before = &inode->i_flock;
 			((fl = *before) != NULL) && IS_LEASE(fl);
 			before = &fl->fl_next) {
-		if (lease->fl_lmops->fl_mylease(fl, lease))
+		if (fl->fl_file == lease->fl_file)
 			my_before = before;
 		else if (fl->fl_type == (F_INPROGRESS | F_UNLCK))
 			/*
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index cbe1b81..87d4c48 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2295,24 +2295,6 @@ void nfsd_break_deleg_cb(struct file_lock *fl)
 	nfsd4_cb_recall(dp);
 }
 
-/*
- * Called from setlease() with lock_flocks() held
- */
-static
-int nfsd_same_client_deleg_cb(struct file_lock *onlist, struct file_lock *try)
-{
-	struct nfs4_delegation *onlistd =
-		(struct nfs4_delegation *)onlist->fl_owner;
-	struct nfs4_delegation *tryd =
-		(struct nfs4_delegation *)try->fl_owner;
-
-	if (onlist->fl_lmops != try->fl_lmops)
-		return 0;
-
-	return onlistd->dl_client == tryd->dl_client;
-}
-
-
 static
 int nfsd_change_deleg_cb(struct file_lock **onlist, int arg)
 {
@@ -2324,7 +2306,6 @@ int nfsd_change_deleg_cb(struct file_lock **onlist, int arg)
 
 static const struct lock_manager_operations nfsd_lease_mng_ops = {
 	.fl_break = nfsd_break_deleg_cb,
-	.fl_mylease = nfsd_same_client_deleg_cb,
 	.fl_change = nfsd_change_deleg_cb,
 };
 
@@ -2630,7 +2611,7 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open, struct nfs4_sta
 	dp->dl_flock = fl;
 
 	/* vfs_setlease checks to see if delegation should be handed out.
-	 * the lock_manager callbacks fl_mylease and fl_change are used
+	 * the lock_manager callback fl_change is used
 	 */
 	if ((status = vfs_setlease(fl->fl_file, fl->fl_type, &fl))) {
 		dprintk("NFSD: setlease failed [%d], no delegation\n", status);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 090f0ea..73ce69f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1059,7 +1059,6 @@ struct lock_manager_operations {
 	int (*fl_grant)(struct file_lock *, struct file_lock *, int);
 	void (*fl_release_private)(struct file_lock *);
 	void (*fl_break)(struct file_lock *);
-	int (*fl_mylease)(struct file_lock *, struct file_lock *);
 	int (*fl_change)(struct file_lock **, int);
 };
 
-- 
1.7.1

Re: [PATCH 3/4] locks: eliminate fl_mylease callback

[Christoph Hellwig]

On Mon, Jan 03, 2011 at 10:06:58PM -0500, J. Bruce Fields wrote:
> The nfs server only supports read delegations for now, so we don't care
> how conflicts are determined.  All we care is that unlocks are
> recognized as matching the leases they are meant to remove.  After the
> last patch, a comparison of struct files will work for that purpose.  So
> we no longer need this callback.

Please also update Documentation/filesystems/Locking for method
removals.

Re: [PATCH 3/4] locks: eliminate fl_mylease callback

[J. Bruce Fields]

On Tue, Jan 04, 2011 at 01:07:23AM -0500, Christoph Hellwig wrote:
> On Mon, Jan 03, 2011 at 10:06:58PM -0500, J. Bruce Fields wrote:
> > The nfs server only supports read delegations for now, so we don't care
> > how conflicts are determined.  All we care is that unlocks are
> > recognized as matching the leases they are meant to remove.  After the
> > last patch, a comparison of struct files will work for that purpose.  So
> > we no longer need this callback.
> 
> Please also update Documentation/filesystems/Locking for method
> removals.

Whoops, thanks for the reminder.

Looks like we never added fl_mylease?

That leaves the fl_release_private patch, updated as follows.

--b.

commit 3d801116bb23a1f446627ce1976950c7a126541e
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Sat Oct 30 17:41:26 2010 -0400

    nfsd4: eliminate lease delete callback
    
    nfsd controls the lifetime of the lease, not the lock code, so there's
    no need for this callback on lease destruction.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index b6426f1..075be12 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -327,14 +327,12 @@ fl_release_private:	yes	yes
 prototypes:
 	int (*fl_compare_owner)(struct file_lock *, struct file_lock *);
 	void (*fl_notify)(struct file_lock *);  /* unblock callback */
-	void (*fl_release_private)(struct file_lock *);
 	void (*fl_break)(struct file_lock *); /* break_lease callback */
 
 locking rules:
 			BKL	may block
 fl_compare_owner:	yes	no
 fl_notify:		yes	no
-fl_release_private:	yes	yes
 fl_break:		yes	no
 
 	Currently only NFSD and NLM provide instances of this class. None of the
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b82e368..2e44ad2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2296,23 +2296,6 @@ void nfsd_break_deleg_cb(struct file_lock *fl)
 }
 
 /*
- * The file_lock is being reapd.
- *
- * Called by locks_free_lock() with lock_flocks() held.
- */
-static
-void nfsd_release_deleg_cb(struct file_lock *fl)
-{
-	struct nfs4_delegation *dp = (struct nfs4_delegation *)fl->fl_owner;
-
-	dprintk("NFSD nfsd_release_deleg_cb: fl %p dp %p dl_count %d\n", fl,dp, atomic_read(&dp->dl_count));
-
-	if (!(fl->fl_flags & FL_LEASE) || !dp)
-		return;
-	dp->dl_flock = NULL;
-}
-
-/*
  * Called from setlease() with lock_flocks() held
  */
 static
@@ -2341,7 +2324,6 @@ int nfsd_change_deleg_cb(struct file_lock **onlist, int arg)
 
 static const struct lock_manager_operations nfsd_lease_mng_ops = {
 	.fl_break = nfsd_break_deleg_cb,
-	.fl_release_private = nfsd_release_deleg_cb,
 	.fl_mylease = nfsd_same_client_deleg_cb,
 	.fl_change = nfsd_change_deleg_cb,
 };

Re: [PATCH 3/4] locks: eliminate fl_mylease callback

[Christoph Hellwig]

On Tue, Jan 04, 2011 at 01:18:16PM -0500, J. Bruce Fields wrote:
> Whoops, thanks for the reminder.
> 
> Looks like we never added fl_mylease?

I actually addded it recently, but it seems like you're based on an
older tree.

> That leaves the fl_release_private patch, updated as follows.

I don't think you removed all instances of it yet.  It is still
implemented by lease_manager_ops in fs/locks.c

Re: [PATCH 3/4] locks: eliminate fl_mylease callback

[J. Bruce Fields]

On Fri, Jan 07, 2011 at 08:40:15AM -0500, Christoph Hellwig wrote:
> On Tue, Jan 04, 2011 at 01:18:16PM -0500, J. Bruce Fields wrote:
> > Whoops, thanks for the reminder.
> > 
> > Looks like we never added fl_mylease?
> 
> I actually addded it recently, but it seems like you're based on an
> older tree.

Whoops, I'll probably resolve that before submitting a pull request....

> > That leaves the fl_release_private patch, updated as follows.
> 
> I don't think you removed all instances of it yet.  It is still
> implemented by lease_manager_ops in fs/locks.c

Argh, yes, I'd forgotten about that.  The original patch is still right,
so we could just back out the documentation patch, or maybe there's a
better alternative for the remaining user.

--b.

Re: [PATCH 3/4] locks: eliminate fl_mylease callback

[J. Bruce Fields]

On Fri, Jan 07, 2011 at 08:40:15AM -0500, Christoph Hellwig wrote:
> On Tue, Jan 04, 2011 at 01:18:16PM -0500, J. Bruce Fields wrote:
> > Whoops, thanks for the reminder.
> > 
> > Looks like we never added fl_mylease?
> 
> I actually addded it recently, but it seems like you're based on an
> older tree.

Whoops, I'll probably resolve that before submitting a pull request....

> > That leaves the fl_release_private patch, updated as follows.
> 
> I don't think you removed all instances of it yet.  It is still
> implemented by lease_manager_ops in fs/locks.c

Argh, yes, I'd forgotten about that.  The original patch is still right,
so we could just back out the documentation patch, or maybe there's a
better alternative for the remaining user.

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

Re: [PATCH 3/4] locks: eliminate fl_mylease callback

[Christoph Hellwig]

On Sun, Jan 09, 2011 at 02:55:09PM -0500, J. Bruce Fields wrote:
> > I don't think you removed all instances of it yet.  It is still
> > implemented by lease_manager_ops in fs/locks.c
> 
> Argh, yes, I'd forgotten about that.  The original patch is still right,
> so we could just back out the documentation patch, or maybe there's a
> better alternative for the remaining user.

Given that leases are only releases in a few controlled places the
code could probably be moved into those.  But it's probably to risky to
try that so late in the merge window.

Re: [PATCH 3/4] locks: eliminate fl_mylease callback

[Christoph Hellwig]

On Sun, Jan 09, 2011 at 02:55:09PM -0500, J. Bruce Fields wrote:
> > I don't think you removed all instances of it yet.  It is still
> > implemented by lease_manager_ops in fs/locks.c
> 
> Argh, yes, I'd forgotten about that.  The original patch is still right,
> so we could just back out the documentation patch, or maybe there's a
> better alternative for the remaining user.

Given that leases are only releases in a few controlled places the
code could probably be moved into those.  But it's probably to risky to
try that so late in the merge window.

--
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/4] locks: minor setlease cleanup

[J. Bruce Fields]

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

diff --git a/fs/locks.c b/fs/locks.c
index 5cb6506..feaac63 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1399,7 +1399,7 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp)
 	for (before = &inode->i_flock;
 			((fl = *before) != NULL) && IS_LEASE(fl);
 			before = &fl->fl_next) {
-		if (fl->fl_file == lease->fl_file)
+		if (fl->fl_file == filp)
 			my_before = before;
 		else if (fl->fl_type == (F_INPROGRESS | F_UNLCK))
 			/*
-- 
1.7.1