I know that the Christoph mentioned pushing out the open and lock
stateid changes next, but we need to finish up the delegation changes in
order to prepare for those. Most of these have been posted before, and
they should be merged on top of the two lm_break callback patches that I
posted yesterday.
Jeff Layton (4):
nfsd: nfs4_alloc_init_lease should take a nfs4_file arg
locks: add file_has_lease to prevent delegation break races
nfsd: Protect the nfs4_file delegation fields using the fi_lock
nfsd: Fix delegation revocation
Trond Myklebust (3):
nfsd: Ensure stateids remain unique until they are freed
nfsd: Move the delegation reference counter into the struct nfs4_stid
nfsd: Simplify stateid management
fs/locks.c | 26 ++++++++++
fs/nfsd/nfs4state.c | 134 +++++++++++++++++++++++++++++++---------------------
fs/nfsd/state.h | 3 +-
include/linux/fs.h | 6 +++
4 files changed, 115 insertions(+), 54 deletions(-)
--
1.9.3
On Wed, 16 Jul 2014 07:29:28 -0700
Christoph Hellwig <[email protected]> wrote:
> On Wed, Jul 16, 2014 at 10:28:24AM -0400, Jeff Layton wrote:
> > Unfortunately, that creates a pile of merge conflicts in the later
> > patches. I'll just go ahead and change that and resend all of the
> > delegation patches that Bruce hasn't merged yet. Stay tuned...
>
> Just keep it as is if it's too painful. This isn't really something
> that matters too much in the end.
>
Too late, I went ahead and just did it...
--
Jeff Layton <[email protected]>
On Wed, Jul 16, 2014 at 10:28:24AM -0400, Jeff Layton wrote:
> Unfortunately, that creates a pile of merge conflicts in the later
> patches. I'll just go ahead and change that and resend all of the
> delegation patches that Bruce hasn't merged yet. Stay tuned...
Just keep it as is if it's too painful. This isn't really something
that matters too much in the end.
Move more of the delegation fields to be protected by the fi_lock. It's
more granular than the state_lock and in later patches we'll want to
be able to rely on it in addition to the state_lock.
Also, the current code in nfs4_setlease calls vfs_setlease and uses the
client_mutex to ensure that it doesn't disappear before we can hash the
delegation. With the client_mutex gone, we'll have a potential race
condition.
It's possible that the delegation could be recalled after we acquire the
lease but before we ever get around to hashing it. If that happens, then
we'd have a nfs4_file that *thinks* it has a delegation, when it
actually has none.
Attempt to acquire a delegation. If that succeeds, take the state_lock
and recheck to make sure the lease is still there. If it is, then take
the fi_lock and set up the rest of the delegation fields. This prevents
the race because the delegation break workqueue job must also take the
state_lock.
Signed-off-by: Trond Myklebust <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfs4state.c | 51 ++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 38 insertions(+), 13 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index df3e9ef1fb38..b7b4cc8a30fb 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -624,6 +624,8 @@ nfs4_put_delegation(struct nfs4_delegation *dp)
static void nfs4_put_deleg_lease(struct nfs4_file *fp)
{
+ lockdep_assert_held(&state_lock);
+
if (!fp->fi_lease)
return;
if (atomic_dec_and_test(&fp->fi_delegees)) {
@@ -643,11 +645,10 @@ static void
hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp)
{
lockdep_assert_held(&state_lock);
+ lockdep_assert_held(&fp->fi_lock);
dp->dl_stid.sc_type = NFS4_DELEG_STID;
- spin_lock(&fp->fi_lock);
list_add(&dp->dl_perfile, &fp->fi_delegations);
- spin_unlock(&fp->fi_lock);
list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
}
@@ -659,17 +660,17 @@ unhash_delegation(struct nfs4_delegation *dp)
spin_lock(&state_lock);
dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
+ spin_lock(&fp->fi_lock);
list_del_init(&dp->dl_perclnt);
list_del_init(&dp->dl_recall_lru);
- spin_lock(&fp->fi_lock);
list_del_init(&dp->dl_perfile);
spin_unlock(&fp->fi_lock);
- spin_unlock(&state_lock);
if (fp) {
nfs4_put_deleg_lease(fp);
- put_nfs4_file(fp);
dp->dl_file = NULL;
}
+ spin_unlock(&state_lock);
+ put_nfs4_file(fp);
}
static void destroy_revoked_delegation(struct nfs4_delegation *dp)
@@ -3492,7 +3493,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
{
struct nfs4_file *fp = dp->dl_file;
struct file_lock *fl;
- int status;
+ int status = 0;
fl = nfs4_alloc_init_lease(fp, NFS4_OPEN_DELEGATE_READ);
if (!fl)
@@ -3500,15 +3501,31 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
fl->fl_file = find_readable_file(fp);
status = vfs_setlease(fl->fl_file, fl->fl_type, &fl);
if (status)
- goto out_free;
+ goto out_fput;
+ spin_lock(&state_lock);
+ /* Did the lease get broken before we took the lock? */
+ status = -EAGAIN;
+ if (!file_has_lease(fl->fl_file))
+ goto out_unlock;
+ spin_lock(&fp->fi_lock);
+ /* Race breaker */
+ if (fp->fi_lease) {
+ status = 0;
+ atomic_inc(&fp->fi_delegees);
+ hash_delegation_locked(dp, fp);
+ spin_unlock(&fp->fi_lock);
+ goto out_unlock;
+ }
fp->fi_lease = fl;
fp->fi_deleg_file = fl->fl_file;
atomic_set(&fp->fi_delegees, 1);
- spin_lock(&state_lock);
hash_delegation_locked(dp, fp);
+ spin_unlock(&fp->fi_lock);
spin_unlock(&state_lock);
return 0;
-out_free:
+out_unlock:
+ spin_unlock(&state_lock);
+out_fput:
if (fl->fl_file)
fput(fl->fl_file);
locks_free_lock(fl);
@@ -3517,19 +3534,27 @@ out_free:
static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
{
+ int status = 0;
+
if (fp->fi_had_conflict)
return -EAGAIN;
get_nfs4_file(fp);
+ spin_lock(&state_lock);
+ spin_lock(&fp->fi_lock);
dp->dl_file = fp;
- if (!fp->fi_lease)
+ if (!fp->fi_lease) {
+ spin_unlock(&fp->fi_lock);
+ spin_unlock(&state_lock);
return nfs4_setlease(dp);
- spin_lock(&state_lock);
+ }
atomic_inc(&fp->fi_delegees);
if (fp->fi_had_conflict) {
- spin_unlock(&state_lock);
- return -EAGAIN;
+ status = -EAGAIN;
+ goto out_unlock;
}
hash_delegation_locked(dp, fp);
+out_unlock:
+ spin_unlock(&fp->fi_lock);
spin_unlock(&state_lock);
return 0;
}
--
1.9.3
On Wed, 16 Jul 2014 06:49:45 -0700
Christoph Hellwig <[email protected]> wrote:
> On Wed, Jul 16, 2014 at 08:17:38AM -0400, Jeff Layton wrote:
> > The problem with keeping them in the caller is that it defeats the main
> > purpose of the patch. The idea here is to make sure that we keep the
> > stateids in the IDR tree for as long as possible to help ensure
> > uniqueness. If we keep it in the caller, we're still removing the stateids
> > from the IDR hash earlier than we should, and that's a larger potential
> > for collisions.
>
> I don't understand how that has anything to do with doing the call
> in either nfs4_free_stid or it's two caller. For the open and lock
> stateids nothing changes in either case, and for delegations we
> move from destroy_(revoked_)delegation to the final put in
> nfs4_put_delegation for both variants. While I'd normally prefer
> what you do in the patch it just seems like churn with the further
> changes.
>
Ahh ok. I misunderstood what you were suggesting. Moving it into the
actual callers of nfs4_free_stid is fine.
Unfortunately, that creates a pile of merge conflicts in the later
patches. I'll just go ahead and change that and resend all of the
delegation patches that Bruce hasn't merged yet. Stay tuned...
--
Jeff Layton <[email protected]>
On Tue, Jul 15, 2014 at 06:37:44AM -0400, Jeff Layton wrote:
> From: Trond Myklebust <[email protected]>
>
> Add an extra delegation state to allow the stateid to remain in the idr
> tree until the last reference has been released. This will be necessary
> to ensure uniqueness once the client_mutex is removed.
>
> [jlayton: reset the sc_type under the state_lock in unhash_delegation]
>
> Signed-off-by: Trond Myklebust <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 9 ++++-----
> fs/nfsd/state.h | 1 +
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 1b01a20827ab..df3e9ef1fb38 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -609,6 +609,7 @@ static void remove_stid(struct nfs4_stid *s)
>
> static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
> {
> + remove_stid(s);
> kmem_cache_free(slab, s);
> }
Might make sense to just keep it in the caller. In your big series
remove_stid_locked ends up there again anyway, and it avoids some churn
in this series that made me investiage how you keep open and lock
stateids hashed longer in this patch (you don't of course..).
Otherwise looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
From: Trond Myklebust <[email protected]>
We will want to add reference counting to the lock stateid and open
stateids too in later patches.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfsd/nfs4state.c | 6 +++---
fs/nfsd/state.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b7b4cc8a30fb..87674a25a15b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -472,6 +472,7 @@ kmem_cache *slab)
stid->sc_stateid.si_opaque.so_clid = cl->cl_clientid;
/* Will be incremented before return to client: */
stid->sc_stateid.si_generation = 0;
+ atomic_set(&stid->sc_count, 1);
/*
* It shouldn't be a problem to reuse an opaque stateid value.
@@ -595,7 +596,6 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
dp->dl_type = NFS4_OPEN_DELEGATE_READ;
fh_copy_shallow(&dp->dl_fh, ¤t_fh->fh_handle);
dp->dl_time = 0;
- atomic_set(&dp->dl_count, 1);
INIT_WORK(&dp->dl_recall.cb_work, nfsd4_run_cb_recall);
return dp;
}
@@ -616,7 +616,7 @@ static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
void
nfs4_put_delegation(struct nfs4_delegation *dp)
{
- if (atomic_dec_and_test(&dp->dl_count)) {
+ if (atomic_dec_and_test(&dp->dl_stid.sc_count)) {
nfs4_free_stid(deleg_slab, &dp->dl_stid);
num_delegations--;
}
@@ -3118,7 +3118,7 @@ static void nfsd_break_one_deleg(struct nfs4_delegation *dp)
* lock) we know the server hasn't removed the lease yet, we know
* it's safe to take a reference.
*/
- atomic_inc(&dp->dl_count);
+ atomic_inc(&dp->dl_stid.sc_count);
nfsd4_cb_recall(dp);
}
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 996d61eeb357..e68a9ae30fd7 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -73,6 +73,7 @@ struct nfsd4_callback {
};
struct nfs4_stid {
+ atomic_t sc_count;
#define NFS4_OPEN_STID 1
#define NFS4_LOCK_STID 2
#define NFS4_DELEG_STID 4
@@ -91,7 +92,6 @@ struct nfs4_delegation {
struct list_head dl_perfile;
struct list_head dl_perclnt;
struct list_head dl_recall_lru; /* delegation recalled */
- atomic_t dl_count; /* ref count */
struct nfs4_file *dl_file;
u32 dl_type;
time_t dl_time;
--
1.9.3
On Tue, Jul 15, 2014 at 06:37:42AM -0400, Jeff Layton wrote:
> I know that the Christoph mentioned pushing out the open and lock
> stateid changes next, but we need to finish up the delegation changes in
> order to prepare for those. Most of these have been posted before, and
> they should be merged on top of the two lm_break callback patches that I
> posted yesterday.
Fixing delegations up first sounds perfectly fine to.
Ensure that the delegations cannot be found by the laundromat etc once
we add them to the various 'revoke' lists.
Signed-off-by: Trond Myklebust <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfs4state.c | 45 +++++++++++++++++++++++++--------------------
1 file changed, 25 insertions(+), 20 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1cfc39b505cc..34235475e8d6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -650,13 +650,13 @@ hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp)
list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
}
-/* Called under the state lock. */
static void
-unhash_delegation(struct nfs4_delegation *dp)
+unhash_delegation_locked(struct nfs4_delegation *dp)
{
struct nfs4_file *fp = dp->dl_file;
- spin_lock(&state_lock);
+ lockdep_assert_held(&state_lock);
+
dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
spin_lock(&fp->fi_lock);
list_del_init(&dp->dl_perclnt);
@@ -665,18 +665,19 @@ unhash_delegation(struct nfs4_delegation *dp)
spin_unlock(&fp->fi_lock);
if (fp)
nfs4_put_deleg_lease(fp);
- spin_unlock(&state_lock);
}
-static void destroy_revoked_delegation(struct nfs4_delegation *dp)
+static void unhash_and_destroy_delegation(struct nfs4_delegation *dp)
{
- list_del_init(&dp->dl_recall_lru);
+ spin_lock(&state_lock);
+ unhash_delegation_locked(dp);
+ spin_unlock(&state_lock);
nfs4_put_delegation(dp);
}
-static void destroy_delegation(struct nfs4_delegation *dp)
+static void destroy_revoked_delegation(struct nfs4_delegation *dp)
{
- unhash_delegation(dp);
+ list_del_init(&dp->dl_recall_lru);
nfs4_put_delegation(dp);
}
@@ -685,11 +686,10 @@ static void revoke_delegation(struct nfs4_delegation *dp)
struct nfs4_client *clp = dp->dl_stid.sc_client;
if (clp->cl_minorversion == 0)
- destroy_delegation(dp);
+ destroy_revoked_delegation(dp);
else {
- unhash_delegation(dp);
dp->dl_stid.sc_type = NFS4_REVOKED_DELEG_STID;
- list_add(&dp->dl_recall_lru, &clp->cl_revoked);
+ list_move(&dp->dl_recall_lru, &clp->cl_revoked);
}
}
@@ -1446,15 +1446,16 @@ destroy_client(struct nfs4_client *clp)
spin_lock(&state_lock);
while (!list_empty(&clp->cl_delegations)) {
dp = list_entry(clp->cl_delegations.next, struct nfs4_delegation, dl_perclnt);
- list_del_init(&dp->dl_perclnt);
+ unhash_delegation_locked(dp);
/* Ensure that deleg break won't try to requeue it */
++dp->dl_time;
- list_move(&dp->dl_recall_lru, &reaplist);
+ list_add(&dp->dl_recall_lru, &reaplist);
}
spin_unlock(&state_lock);
while (!list_empty(&reaplist)) {
dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);
- destroy_delegation(dp);
+ list_del_init(&dp->dl_recall_lru);
+ nfs4_put_delegation(dp);
}
list_splice_init(&clp->cl_revoked, &reaplist);
while (!list_empty(&reaplist)) {
@@ -3638,7 +3639,7 @@ nfs4_open_delegation(struct net *net, struct svc_fh *fh,
open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
return;
out_free:
- destroy_delegation(dp);
+ nfs4_put_delegation(dp);
out_no_deleg:
open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE;
if (open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS &&
@@ -3877,7 +3878,8 @@ nfs4_laundromat(struct nfsd_net *nn)
new_timeo = min(new_timeo, t);
break;
}
- list_move(&dp->dl_recall_lru, &reaplist);
+ unhash_delegation_locked(dp);
+ list_add(&dp->dl_recall_lru, &reaplist);
}
spin_unlock(&state_lock);
list_for_each_safe(pos, next, &reaplist) {
@@ -4509,7 +4511,7 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
if (status)
goto out;
- destroy_delegation(dp);
+ unhash_and_destroy_delegation(dp);
out:
nfs4_unlock_state();
@@ -5352,7 +5354,8 @@ static u64 nfsd_find_all_delegations(struct nfs4_client *clp, u64 max,
* don't monkey with it now that we are.
*/
++dp->dl_time;
- list_move(&dp->dl_recall_lru, victims);
+ unhash_delegation_locked(dp);
+ list_add(&dp->dl_recall_lru, victims);
}
if (++count == max)
break;
@@ -5607,12 +5610,14 @@ nfs4_state_shutdown_net(struct net *net)
spin_lock(&state_lock);
list_for_each_safe(pos, next, &nn->del_recall_lru) {
dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
- list_move(&dp->dl_recall_lru, &reaplist);
+ unhash_delegation_locked(dp);
+ list_add(&dp->dl_recall_lru, &reaplist);
}
spin_unlock(&state_lock);
list_for_each_safe(pos, next, &reaplist) {
dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
- destroy_delegation(dp);
+ list_del_init(&dp->dl_recall_lru);
+ nfs4_put_delegation(dp);
}
nfsd4_client_tracking_exit(net);
--
1.9.3
On Tue, Jul 15, 2014 at 06:37:47AM -0400, Jeff Layton wrote:
> From: Trond Myklebust <[email protected]>
>
> We will want to add reference counting to the lock stateid and open
> stateids too in later patches.
>
> Signed-off-by: Trond Myklebust <[email protected]>
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
On Tue, Jul 15, 2014 at 06:37:43AM -0400, Jeff Layton wrote:
> No need to pass the delegation pointer in here as it's only used to get
> the nfs4_file pointer.
>
> Signed-off-by: Jeff Layton <[email protected]>
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
No need to pass the delegation pointer in here as it's only used to get
the nfs4_file pointer.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfs4state.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index bdf8ac3393bd..1b01a20827ab 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3474,7 +3474,7 @@ static bool nfsd4_cb_channel_good(struct nfs4_client *clp)
return clp->cl_minorversion && clp->cl_cb_state == NFSD4_CB_UNKNOWN;
}
-static struct file_lock *nfs4_alloc_init_lease(struct nfs4_delegation *dp, int flag)
+static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag)
{
struct file_lock *fl;
@@ -3486,7 +3486,7 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_delegation *dp, int f
fl->fl_flags = FL_DELEG;
fl->fl_type = flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK;
fl->fl_end = OFFSET_MAX;
- fl->fl_owner = (fl_owner_t)(dp->dl_file);
+ fl->fl_owner = (fl_owner_t)fp;
fl->fl_pid = current->tgid;
return fl;
}
@@ -3497,7 +3497,7 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
struct file_lock *fl;
int status;
- fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ);
+ fl = nfs4_alloc_init_lease(fp, NFS4_OPEN_DELEGATE_READ);
if (!fl)
return -ENOMEM;
fl->fl_file = find_readable_file(fp);
--
1.9.3
From: Trond Myklebust <[email protected]>
We want to convert to an atomic type so that we don't need to lock
across the call to alloc_init_deleg(). Then convert to a long type so
that we match the size of 'max_delegations'.
None of this is a problem today, but it will be once we remove
client_mutex protection.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfsd/nfs4state.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 34235475e8d6..243e091fda6c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -343,7 +343,7 @@ find_any_file(struct nfs4_file *f)
return ret;
}
-static int num_delegations;
+static atomic_long_t num_delegations;
unsigned long max_delegations;
/*
@@ -571,22 +571,23 @@ static struct nfs4_delegation *
alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct svc_fh *current_fh)
{
struct nfs4_delegation *dp;
+ long n;
dprintk("NFSD alloc_init_deleg\n");
- if (num_delegations > max_delegations)
- return NULL;
+ n = atomic_long_inc_return(&num_delegations);
+ if (n < 0 || n > max_delegations)
+ goto out_dec;
if (delegation_blocked(¤t_fh->fh_handle))
- return NULL;
+ goto out_dec;
dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab));
if (dp == NULL)
- return dp;
+ goto out_dec;
/*
* delegation seqid's are never incremented. The 4.1 special
* meaning of seqid 0 isn't meaningful, really, but let's avoid
* 0 anyway just for consistency and use 1:
*/
dp->dl_stid.sc_stateid.si_generation = 1;
- num_delegations++;
INIT_LIST_HEAD(&dp->dl_perfile);
INIT_LIST_HEAD(&dp->dl_perclnt);
INIT_LIST_HEAD(&dp->dl_recall_lru);
@@ -594,6 +595,9 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
fh_copy_shallow(&dp->dl_fh, ¤t_fh->fh_handle);
INIT_WORK(&dp->dl_recall.cb_work, nfsd4_run_cb_recall);
return dp;
+out_dec:
+ atomic_long_dec(&num_delegations);
+ return NULL;
}
static void remove_stid(struct nfs4_stid *s)
@@ -616,7 +620,7 @@ nfs4_put_delegation(struct nfs4_delegation *dp)
if (dp->dl_file)
put_nfs4_file(dp->dl_file);
nfs4_free_stid(deleg_slab, &dp->dl_stid);
- num_delegations--;
+ atomic_long_dec(&num_delegations);
}
}
--
1.9.3
On Tue, 15 Jul 2014 10:17:52 -0700
Christoph Hellwig <[email protected]> wrote:
> On Tue, Jul 15, 2014 at 06:37:45AM -0400, Jeff Layton wrote:
> > Once we remove the client_mutex, we'll have a potential race between
> > setting a lease on a file and breaking the delegation. We may set the
> > lease, but by the time we go to hash it, it may have already been
> > broken. Currently, we know that this won't happen as the nfs4_laundromat
> > takes the client_mutex, but we want to remove that.
> >
> > As part of that fix, add a function that can tell us whether a
> > particular file has a lease set on it. In a later nfsd patch, we'll use
> > that to close the potential race window.
> >
> > Signed-off-by: Jeff Layton <[email protected]>
>
> Looks reasonable, I don't really understand why we can drop out of the
> loop when finding the first non-lease, but other code in locks.c also
> does this, so I assume it is intentional.
>
It is intentional. The i_flock list is ordered. See the comments over
the struct file_lock definition. Took me a while to understand that too
(which is why I added the comment a few months ago).
> The only real question I have is why this is in locks.c, while
> check_for_locks that does the equivalent for real file locks is
> implemented in the nfsd code. But I think it's better to have these
> sorts of interfaces in locks.c and check_for_locks should move there
> eventually.
>
Agreed, though I'll defer that until after this series is merged.
> Reviewed-by: Christoph Hellwig <[email protected]>
Thanks!
--
Jeff Layton <[email protected]>
From: Trond Myklebust <[email protected]>
Don't allow stateids to clear the open file pointer until they are
being destroyed. Also, move to kzalloc and get rid of explicit
zeroing of fields.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfsd/nfs4state.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 87674a25a15b..1cfc39b505cc 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -459,7 +459,7 @@ kmem_cache *slab)
struct nfs4_stid *stid;
int new_id;
- stid = kmem_cache_alloc(slab, GFP_KERNEL);
+ stid = kmem_cache_zalloc(slab, GFP_KERNEL);
if (!stid)
return NULL;
@@ -467,11 +467,9 @@ kmem_cache *slab)
if (new_id < 0)
goto out_free;
stid->sc_client = cl;
- stid->sc_type = 0;
stid->sc_stateid.si_opaque.so_id = new_id;
stid->sc_stateid.si_opaque.so_clid = cl->cl_clientid;
/* Will be incremented before return to client: */
- stid->sc_stateid.si_generation = 0;
atomic_set(&stid->sc_count, 1);
/*
@@ -592,10 +590,8 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
INIT_LIST_HEAD(&dp->dl_perfile);
INIT_LIST_HEAD(&dp->dl_perclnt);
INIT_LIST_HEAD(&dp->dl_recall_lru);
- dp->dl_file = NULL;
dp->dl_type = NFS4_OPEN_DELEGATE_READ;
fh_copy_shallow(&dp->dl_fh, ¤t_fh->fh_handle);
- dp->dl_time = 0;
INIT_WORK(&dp->dl_recall.cb_work, nfsd4_run_cb_recall);
return dp;
}
@@ -617,6 +613,8 @@ void
nfs4_put_delegation(struct nfs4_delegation *dp)
{
if (atomic_dec_and_test(&dp->dl_stid.sc_count)) {
+ if (dp->dl_file)
+ put_nfs4_file(dp->dl_file);
nfs4_free_stid(deleg_slab, &dp->dl_stid);
num_delegations--;
}
@@ -665,12 +663,9 @@ unhash_delegation(struct nfs4_delegation *dp)
list_del_init(&dp->dl_recall_lru);
list_del_init(&dp->dl_perfile);
spin_unlock(&fp->fi_lock);
- if (fp) {
+ if (fp)
nfs4_put_deleg_lease(fp);
- dp->dl_file = NULL;
- }
spin_unlock(&state_lock);
- put_nfs4_file(fp);
}
static void destroy_revoked_delegation(struct nfs4_delegation *dp)
@@ -878,12 +873,12 @@ static void unhash_generic_stateid(struct nfs4_ol_stateid *stp)
static void close_generic_stateid(struct nfs4_ol_stateid *stp)
{
release_all_access(stp);
- put_nfs4_file(stp->st_file);
- stp->st_file = NULL;
}
static void free_generic_stateid(struct nfs4_ol_stateid *stp)
{
+ if (stp->st_file)
+ put_nfs4_file(stp->st_file);
nfs4_free_stid(stateid_slab, &stp->st_stid);
}
@@ -4445,6 +4440,10 @@ static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
if (list_empty(&oo->oo_owner.so_stateids))
release_openowner(oo);
} else {
+ if (s->st_file) {
+ put_nfs4_file(s->st_file);
+ s->st_file = NULL;
+ }
oo->oo_last_closed_stid = s;
/*
* In the 4.0 case we need to keep the owners around a
--
1.9.3
Once we remove the client_mutex, we'll have a potential race between
setting a lease on a file and breaking the delegation. We may set the
lease, but by the time we go to hash it, it may have already been
broken. Currently, we know that this won't happen as the nfs4_laundromat
takes the client_mutex, but we want to remove that.
As part of that fix, add a function that can tell us whether a
particular file has a lease set on it. In a later nfsd patch, we'll use
that to close the potential race window.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/locks.c | 26 ++++++++++++++++++++++++++
include/linux/fs.h | 6 ++++++
2 files changed, 32 insertions(+)
diff --git a/fs/locks.c b/fs/locks.c
index 717fbc404e6b..005cc86927e3 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1308,6 +1308,32 @@ static bool leases_conflict(struct file_lock *lease, struct file_lock *breaker)
}
/**
+ * file_has_lease - does the given file have a lease set on it?
+ * @file: struct file on which we want to check the lease
+ *
+ * Returns true if a lease was is set on the given file description,
+ * false otherwise.
+ */
+bool
+file_has_lease(struct file *file)
+{
+ bool ret = false;
+ struct inode *inode = file_inode(file);
+ struct file_lock *fl;
+
+ spin_lock(&inode->i_lock);
+ for (fl = inode->i_flock; fl && IS_LEASE(fl); fl = fl->fl_next) {
+ if (fl->fl_file == file) {
+ ret = true;
+ break;
+ }
+ }
+ spin_unlock(&inode->i_lock);
+ return ret;
+}
+EXPORT_SYMBOL(file_has_lease);
+
+/**
* __break_lease - revoke all outstanding leases on file
* @inode: the inode of the file to return
* @mode: O_RDONLY: break only write leases; O_WRONLY or O_RDWR:
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e11d60cc867b..7ae6f4869669 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -953,6 +953,7 @@ extern int vfs_test_lock(struct file *, struct file_lock *);
extern int vfs_lock_file(struct file *, unsigned int, struct file_lock *, struct file_lock *);
extern int vfs_cancel_lock(struct file *filp, struct file_lock *fl);
extern int flock_lock_file_wait(struct file *filp, struct file_lock *fl);
+extern bool file_has_lease(struct file *file);
extern int __break_lease(struct inode *inode, unsigned int flags, unsigned int type);
extern void lease_get_mtime(struct inode *, struct timespec *time);
extern int generic_setlease(struct file *, long, struct file_lock **);
@@ -1064,6 +1065,11 @@ static inline int flock_lock_file_wait(struct file *filp,
return -ENOLCK;
}
+static inline bool file_has_lease(struct file *file)
+{
+ return false;
+}
+
static inline int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
{
return 0;
--
1.9.3
From: Trond Myklebust <[email protected]>
Add an extra delegation state to allow the stateid to remain in the idr
tree until the last reference has been released. This will be necessary
to ensure uniqueness once the client_mutex is removed.
[jlayton: reset the sc_type under the state_lock in unhash_delegation]
Signed-off-by: Trond Myklebust <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfs4state.c | 9 ++++-----
fs/nfsd/state.h | 1 +
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1b01a20827ab..df3e9ef1fb38 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -609,6 +609,7 @@ static void remove_stid(struct nfs4_stid *s)
static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
{
+ remove_stid(s);
kmem_cache_free(slab, s);
}
@@ -657,6 +658,7 @@ unhash_delegation(struct nfs4_delegation *dp)
struct nfs4_file *fp = dp->dl_file;
spin_lock(&state_lock);
+ dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
list_del_init(&dp->dl_perclnt);
list_del_init(&dp->dl_recall_lru);
spin_lock(&fp->fi_lock);
@@ -670,19 +672,15 @@ unhash_delegation(struct nfs4_delegation *dp)
}
}
-
-
static void destroy_revoked_delegation(struct nfs4_delegation *dp)
{
list_del_init(&dp->dl_recall_lru);
- remove_stid(&dp->dl_stid);
nfs4_put_delegation(dp);
}
static void destroy_delegation(struct nfs4_delegation *dp)
{
unhash_delegation(dp);
- remove_stid(&dp->dl_stid);
nfs4_put_delegation(dp);
}
@@ -885,7 +883,6 @@ static void close_generic_stateid(struct nfs4_ol_stateid *stp)
static void free_generic_stateid(struct nfs4_ol_stateid *stp)
{
- remove_stid(&stp->st_stid);
nfs4_free_stid(stateid_slab, &stp->st_stid);
}
@@ -4036,7 +4033,9 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
return nfs_ok;
default:
printk("unknown stateid type %x\n", s->sc_type);
+ /* Fallthrough */
case NFS4_CLOSED_STID:
+ case NFS4_CLOSED_DELEG_STID:
return nfserr_bad_stateid;
}
}
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 81b7522e3f67..996d61eeb357 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -80,6 +80,7 @@ struct nfs4_stid {
#define NFS4_CLOSED_STID 8
/* For a deleg stateid kept around only to process free_stateid's: */
#define NFS4_REVOKED_DELEG_STID 16
+#define NFS4_CLOSED_DELEG_STID 32
unsigned char sc_type;
stateid_t sc_stateid;
struct nfs4_client *sc_client;
--
1.9.3
On Wed, Jul 16, 2014 at 08:17:38AM -0400, Jeff Layton wrote:
> The problem with keeping them in the caller is that it defeats the main
> purpose of the patch. The idea here is to make sure that we keep the
> stateids in the IDR tree for as long as possible to help ensure
> uniqueness. If we keep it in the caller, we're still removing the stateids
> from the IDR hash earlier than we should, and that's a larger potential
> for collisions.
I don't understand how that has anything to do with doing the call
in either nfs4_free_stid or it's two caller. For the open and lock
stateids nothing changes in either case, and for delegations we
move from destroy_(revoked_)delegation to the final put in
nfs4_put_delegation for both variants. While I'd normally prefer
what you do in the patch it just seems like churn with the further
changes.
On Tue, 15 Jul 2014 10:15:47 -0700
Christoph Hellwig <[email protected]> wrote:
> On Tue, Jul 15, 2014 at 06:37:44AM -0400, Jeff Layton wrote:
> > From: Trond Myklebust <[email protected]>
> >
> > Add an extra delegation state to allow the stateid to remain in the idr
> > tree until the last reference has been released. This will be necessary
> > to ensure uniqueness once the client_mutex is removed.
> >
> > [jlayton: reset the sc_type under the state_lock in unhash_delegation]
> >
> > Signed-off-by: Trond Myklebust <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/nfsd/nfs4state.c | 9 ++++-----
> > fs/nfsd/state.h | 1 +
> > 2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 1b01a20827ab..df3e9ef1fb38 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -609,6 +609,7 @@ static void remove_stid(struct nfs4_stid *s)
> >
> > static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
> > {
> > + remove_stid(s);
> > kmem_cache_free(slab, s);
> > }
>
> Might make sense to just keep it in the caller. In your big series
> remove_stid_locked ends up there again anyway, and it avoids some churn
> in this series that made me investiage how you keep open and lock
> stateids hashed longer in this patch (you don't of course..).
>
The problem with keeping them in the caller is that it defeats the main
purpose of the patch. The idea here is to make sure that we keep the
stateids in the IDR tree for as long as possible to help ensure
uniqueness. If we keep it in the caller, we're still removing the stateids
from the IDR hash earlier than we should, and that's a larger potential
for collisions.
> Otherwise looks good,
>
> Reviewed-by: Christoph Hellwig <[email protected]>
Thanks!
> --
> 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
--
Jeff Layton <[email protected]>
On Tue, Jul 15, 2014 at 06:37:45AM -0400, Jeff Layton wrote:
> Once we remove the client_mutex, we'll have a potential race between
> setting a lease on a file and breaking the delegation. We may set the
> lease, but by the time we go to hash it, it may have already been
> broken. Currently, we know that this won't happen as the nfs4_laundromat
> takes the client_mutex, but we want to remove that.
>
> As part of that fix, add a function that can tell us whether a
> particular file has a lease set on it. In a later nfsd patch, we'll use
> that to close the potential race window.
>
> Signed-off-by: Jeff Layton <[email protected]>
Looks reasonable, I don't really understand why we can drop out of the
loop when finding the first non-lease, but other code in locks.c also
does this, so I assume it is intentional.
The only real question I have is why this is in locks.c, while
check_for_locks that does the equivalent for real file locks is
implemented in the nfsd code. But I think it's better to have these
sorts of interfaces in locks.c and check_for_locks should move there
eventually.
Reviewed-by: Christoph Hellwig <[email protected]>