Skip to contentSkip to conversation

remove lease callbacks

PATCH New J. Bruce Fields 6 participants 10 messages Jan 4, 2011 – Jan 9, 2011 3 messages not in the archive

Subsystems (5)
Documentation VFS NFS client NFS server +1 more subsystems
lore
Share & tools
lore

J. Bruce Fields

author
#0 lore raw

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.

A message that is not in the archive (sent off-list or not yet received): 1294110419-3105-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

#2 lore raw

PATCH 1/4 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 <[email protected]>
---
 fs/nfsd/nfs4state.c |   18 ------------------
 1 files changed, 0 insertions(+), 18 deletions(-)
fs/nfsd/nfs4state.c +0 -18
index b82e368..2e44ad2 100644
@@ -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,
 };
Signature · 6 lines
1.7.1

--
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
author
#3 lore raw

PATCH 2/4 nfsd4: use a single struct file for delegations

In reply to 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 <[email protected]>
---
 fs/nfsd/nfs4state.c |   10 +++++-----
 fs/nfsd/state.h     |    1 +
 2 files changed, 6 insertions(+), 5 deletions(-)
fs/nfsd/nfs4state.c +5 -5
index 2e44ad2..cbe1b81 100644
@@ -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. */
fs/nfsd/state.h +1 -0
index 39adc27..84b2302 100644
@@ -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;
Signature · 1 line
1.7.1
author
#4 lore raw

PATCH 3/4 locks: eliminate fl_mylease callback

In reply to 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 <[email protected]>
---
 fs/locks.c          |    8 +-------
 fs/nfsd/nfs4state.c |   21 +--------------------
 include/linux/fs.h  |    1 -
 3 files changed, 2 insertions(+), 28 deletions(-)
fs/locks.c +1 -7
index 8729347..5cb6506 100644
@@ -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))
 			/*
fs/nfsd/nfs4state.c +1 -20
index cbe1b81..87d4c48 100644
@@ -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);
include/linux/fs.h +0 -1
index 090f0ea..73ce69f 100644
@@ -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);
 };
Signature · 1 line
1.7.1
#5 lore raw

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

Quoted 5 lines · 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.

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

#6 lore raw

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

Quoted 9 lines · Christoph Hellwig
Quoted 5 lines · 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.

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 <[email protected]>
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 <[email protected]>
Documentation/filesystems/Locking +0 -2
index b6426f1..075be12 100644
@@ -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
fs/nfsd/nfs4state.c +0 -18
index b82e368..2e44ad2 100644
@@ -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,
 };
#7 lore raw

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

Quoted 3 lines · J. Bruce Fields

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.

Quoted 1 line

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

#9 lore raw

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

Quoted 7 lines · Christoph Hellwig
Quoted 3 lines · J. Bruce Fields

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

Quoted 4 lines
Quoted 1 line

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

#11 lore raw

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

Quoted 6 lines · J. Bruce Fields
Quoted 2 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#12 lore raw

PATCH 4/4 locks: minor setlease cleanup

In reply to J. Bruce Fields
Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/locks.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/locks.c +1 -1
index 5cb6506..feaac63 100644
@@ -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))
 			/*
Signature · 1 line
1.7.1