* [PATCH 1/8] nfsd4: centralize renew_client() calls
2011-10-17 21:55 fix for open leaks, for 3.2 J. Bruce Fields
@ 2011-10-17 21:55 ` J. Bruce Fields
2011-10-17 21:55 ` [PATCH 2/8] nfsd4: make is_open_owner boolean J. Bruce Fields
` (7 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: J. Bruce Fields @ 2011-10-17 21:55 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
There doesn't seem to be any harm to renewing the client a bit earlier,
when it is looked up. That saves us from having to sprinkle
renew_client calls over quite so many places.
Also remove a redundant comment and do a little cleanup.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 23 ++++++++++-------------
1 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2042805..d90461e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -931,9 +931,6 @@ renew_client_locked(struct nfs4_client *clp)
return;
}
- /*
- * Move client to the end to the LRU list.
- */
dprintk("renewing client (clientid %08x/%08x)\n",
clp->cl_clientid.cl_boot,
clp->cl_clientid.cl_id);
@@ -1220,8 +1217,10 @@ find_confirmed_client(clientid_t *clid)
unsigned int idhashval = clientid_hashval(clid->cl_id);
list_for_each_entry(clp, &conf_id_hashtbl[idhashval], cl_idhash) {
- if (same_clid(&clp->cl_clientid, clid))
+ if (same_clid(&clp->cl_clientid, clid)) {
+ renew_client(clp);
return clp;
+ }
}
return NULL;
}
@@ -2372,11 +2371,15 @@ same_owner_str(struct nfs4_stateowner *sop, struct xdr_netobj *owner,
static struct nfs4_openowner *
find_openstateowner_str(unsigned int hashval, struct nfsd4_open *open)
{
- struct nfs4_stateowner *so = NULL;
+ struct nfs4_stateowner *so;
+ struct nfs4_openowner *oo;
list_for_each_entry(so, &open_ownerstr_hashtbl[hashval], so_strhash) {
- if (same_owner_str(so, &open->op_owner, &open->op_clientid))
- return container_of(so, struct nfs4_openowner, oo_owner);
+ if (same_owner_str(so, &open->op_owner, &open->op_clientid)) {
+ oo = openowner(so);
+ renew_client(oo->oo_owner.so_client);
+ return oo;
+ }
}
return NULL;
}
@@ -2536,7 +2539,6 @@ renew:
open->op_openowner = oo;
}
list_del_init(&oo->oo_close_lru);
- renew_client(oo->oo_owner.so_client);
return nfs_ok;
}
@@ -2970,7 +2972,6 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
dprintk("nfsd4_renew: clientid not found!\n");
goto out;
}
- renew_client(clp);
status = nfserr_cb_path_down;
if (!list_empty(&clp->cl_delegations)
&& clp->cl_cb_state != NFSD4_CB_UP)
@@ -3275,7 +3276,6 @@ nfs4_preprocess_stateid_op(struct nfsd4_compound_state *cstate,
status = nfs4_check_delegmode(dp, flags);
if (status)
goto out;
- renew_client(dp->dl_stid.sc_client);
if (filpp) {
*filpp = dp->dl_file->fi_deleg_file;
BUG_ON(!*filpp);
@@ -3293,7 +3293,6 @@ nfs4_preprocess_stateid_op(struct nfsd4_compound_state *cstate,
status = nfs4_check_openmode(stp, flags);
if (status)
goto out;
- renew_client(stp->st_stateowner->so_client);
if (filpp) {
if (flags & RD_STATE)
*filpp = find_readable_file(stp->st_file);
@@ -3412,7 +3411,6 @@ nfs4_preprocess_seqid_op(struct nfsd4_compound_state *cstate, u32 seqid,
return status;
*stpp = openlockstateid(s);
cstate->replay_owner = (*stpp)->st_stateowner;
- renew_client((*stpp)->st_stateowner->so_client);
return nfs4_seqid_op_checks(cstate, stateid, seqid, *stpp);
}
@@ -3643,7 +3641,6 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
status = check_stateid_generation(stateid, &dp->dl_stid.sc_stateid, nfsd4_has_session(cstate));
if (status)
goto out;
- renew_client(dp->dl_stid.sc_client);
unhash_delegation(dp);
out:
--
1.7.5.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 2/8] nfsd4: make is_open_owner boolean
2011-10-17 21:55 fix for open leaks, for 3.2 J. Bruce Fields
2011-10-17 21:55 ` [PATCH 1/8] nfsd4: centralize renew_client() calls J. Bruce Fields
@ 2011-10-17 21:55 ` J. Bruce Fields
2011-10-17 21:55 ` [PATCH 3/8] nfsd4: simplify process_open1 logic J. Bruce Fields
` (6 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: J. Bruce Fields @ 2011-10-17 21:55 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/state.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index aa14f06..87eecfd 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -335,13 +335,13 @@ struct nfs4_replay {
struct nfs4_stateowner {
struct list_head so_strhash; /* hash by op_name */
struct list_head so_stateids;
- int so_is_open_owner; /* 1=openowner,0=lockowner */
struct nfs4_client * so_client;
/* after increment in ENCODE_SEQID_OP_TAIL, represents the next
* sequence id expected from the client: */
u32 so_seqid;
struct xdr_netobj so_owner; /* open owner name */
struct nfs4_replay so_replay;
+ bool so_is_open_owner;
};
struct nfs4_openowner {
--
1.7.5.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 3/8] nfsd4: simplify process_open1 logic
2011-10-17 21:55 fix for open leaks, for 3.2 J. Bruce Fields
2011-10-17 21:55 ` [PATCH 1/8] nfsd4: centralize renew_client() calls J. Bruce Fields
2011-10-17 21:55 ` [PATCH 2/8] nfsd4: make is_open_owner boolean J. Bruce Fields
@ 2011-10-17 21:55 ` J. Bruce Fields
2011-10-17 21:55 ` [PATCH 4/8] nfsd4: clean up open owners on OPEN failure J. Bruce Fields
` (5 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: J. Bruce Fields @ 2011-10-17 21:55 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
No change in behavior.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 22 ++++++++--------------
1 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d90461e..62aa91a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2506,7 +2506,6 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
struct nfs4_client *clp = NULL;
unsigned int strhashval;
struct nfs4_openowner *oo = NULL;
- __be32 status;
if (STALE_CLIENTID(&open->op_clientid))
return nfserr_stale_clientid;
@@ -2515,30 +2514,25 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
oo = find_openstateowner_str(strhashval, open);
open->op_openowner = oo;
if (!oo) {
- /* Make sure the client's lease hasn't expired. */
clp = find_confirmed_client(clientid);
if (clp == NULL)
return nfserr_expired;
- goto renew;
+ goto new_owner;
}
if (!(oo->oo_flags & NFS4_OO_CONFIRMED)) {
/* Replace unconfirmed owners without checking for replay. */
clp = oo->oo_owner.so_client;
release_openowner(oo);
open->op_openowner = NULL;
- goto renew;
- }
- status = nfsd4_check_seqid(cstate, &oo->oo_owner, open->op_seqid);
- if (status)
- return status;
-renew:
- if (open->op_openowner == NULL) {
- oo = alloc_init_open_stateowner(strhashval, clp, open);
- if (oo == NULL)
- return nfserr_jukebox;
- open->op_openowner = oo;
+ goto new_owner;
}
list_del_init(&oo->oo_close_lru);
+ return nfsd4_check_seqid(cstate, &oo->oo_owner, open->op_seqid);
+new_owner:
+ oo = alloc_init_open_stateowner(strhashval, clp, open);
+ if (oo == NULL)
+ return nfserr_jukebox;
+ open->op_openowner = oo;
return nfs_ok;
}
--
1.7.5.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 4/8] nfsd4: clean up open owners on OPEN failure
2011-10-17 21:55 fix for open leaks, for 3.2 J. Bruce Fields
` (2 preceding siblings ...)
2011-10-17 21:55 ` [PATCH 3/8] nfsd4: simplify process_open1 logic J. Bruce Fields
@ 2011-10-17 21:55 ` J. Bruce Fields
2011-10-17 21:55 ` [PATCH 5/8] nfsd4: preallocate nfs4_file in process_open1() J. Bruce Fields
` (4 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: J. Bruce Fields @ 2011-10-17 21:55 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
If process_open1() creates a new open owner, but the open later fails,
the current code will leave the open owner around. It won't be on the
close_lru list, and the client isn't expected to send a CLOSE, so it
will hang around as long as the client does.
Similarly, if process_open1() removes an existing open owner from the
close lru, anticipating that an open owner that previously had no
associated stateid's now will, but the open subsequently fails, then
we'll again be left with the same leak.
Fix both problems.
Reported-by: Bryan Schumaker <bjschuma@netapp.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4proc.c | 1 +
fs/nfsd/nfs4state.c | 20 ++++++++++++++++++--
fs/nfsd/state.h | 1 +
fs/nfsd/xdr4.h | 1 +
4 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 5b192a2..10b50d7 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -409,6 +409,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
*/
status = nfsd4_process_open2(rqstp, &cstate->current_fh, open);
out:
+ nfsd4_cleanup_open_state(open, status);
if (open->op_openowner)
cstate->replay_owner = &open->op_openowner->oo_owner;
else
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 62aa91a..2c9a1a2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2320,7 +2320,7 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfs4_client *clp, str
return NULL;
oo->oo_owner.so_is_open_owner = 1;
oo->oo_owner.so_seqid = open->op_seqid;
- oo->oo_flags = 0;
+ oo->oo_flags = NFS4_OO_NEW;
oo->oo_time = 0;
oo->oo_last_closed_stid = NULL;
INIT_LIST_HEAD(&oo->oo_close_lru);
@@ -2526,7 +2526,6 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
open->op_openowner = NULL;
goto new_owner;
}
- list_del_init(&oo->oo_close_lru);
return nfsd4_check_seqid(cstate, &oo->oo_owner, open->op_seqid);
new_owner:
oo = alloc_init_open_stateowner(strhashval, clp, open);
@@ -2946,6 +2945,23 @@ out:
return status;
}
+void nfsd4_cleanup_open_state(struct nfsd4_open *open, __be32 status)
+{
+ if (open->op_openowner) {
+ struct nfs4_openowner *oo = open->op_openowner;
+
+ if (!list_empty(&oo->oo_owner.so_stateids))
+ list_del_init(&oo->oo_close_lru);
+ if (oo->oo_flags & NFS4_OO_NEW) {
+ if (status) {
+ release_openowner(oo);
+ open->op_openowner = NULL;
+ } else
+ oo->oo_flags &= ~NFS4_OO_NEW;
+ }
+ }
+}
+
__be32
nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
clientid_t *clid)
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 87eecfd..eab9dae 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -359,6 +359,7 @@ struct nfs4_openowner {
time_t oo_time; /* time of placement on so_close_lru */
#define NFS4_OO_CONFIRMED 1
#define NFS4_OO_PURGE_CLOSE 2
+#define NFS4_OO_NEW 4
unsigned char oo_flags;
};
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 4c8a7ec..32e6fd8 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -554,6 +554,7 @@ extern __be32 nfsd4_process_open1(struct nfsd4_compound_state *,
struct nfsd4_open *open);
extern __be32 nfsd4_process_open2(struct svc_rqst *rqstp,
struct svc_fh *current_fh, struct nfsd4_open *open);
+extern void nfsd4_cleanup_open_state(struct nfsd4_open *open, __be32 status);
extern __be32 nfsd4_open_confirm(struct svc_rqst *rqstp,
struct nfsd4_compound_state *, struct nfsd4_open_confirm *oc);
extern __be32 nfsd4_close(struct svc_rqst *rqstp,
--
1.7.5.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 5/8] nfsd4: preallocate nfs4_file in process_open1()
2011-10-17 21:55 fix for open leaks, for 3.2 J. Bruce Fields
` (3 preceding siblings ...)
2011-10-17 21:55 ` [PATCH 4/8] nfsd4: clean up open owners on OPEN failure J. Bruce Fields
@ 2011-10-17 21:55 ` J. Bruce Fields
2011-10-17 21:55 ` [PATCH 6/8] nfsd4: do idr preallocation with stateid allocation J. Bruce Fields
` (3 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: J. Bruce Fields @ 2011-10-17 21:55 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
Creating a new file is an irrevocable step--once it's visible in the
filesystem, other processes may have seen it and done something with it,
and unlinking it wouldn't simply undo the effects of the create.
Therefore, in the case where OPEN creates a new file, we shouldn't do
the create until we know that the rest of the OPEN processing will
succeed.
For example, we should preallocate a struct file in case we need it
until waiting to allocate it till process_open2(), which is already too
late.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 60 ++++++++++++++++++++++++++++++--------------------
fs/nfsd/xdr4.h | 1 +
2 files changed, 37 insertions(+), 24 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 2c9a1a2..ae5d250 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -104,6 +104,11 @@ opaque_hashval(const void *ptr, int nbytes)
static struct list_head del_recall_lru;
+static void nfsd4_free_file(struct nfs4_file *f)
+{
+ kmem_cache_free(file_slab, f);
+}
+
static inline void
put_nfs4_file(struct nfs4_file *fi)
{
@@ -111,7 +116,7 @@ put_nfs4_file(struct nfs4_file *fi)
list_del(&fi->fi_hash);
spin_unlock(&recall_lock);
iput(fi->fi_inode);
- kmem_cache_free(file_slab, fi);
+ nfsd4_free_file(fi);
}
}
@@ -2190,30 +2195,28 @@ out:
return status;
}
+static struct nfs4_file *nfsd4_alloc_file(void)
+{
+ return kmem_cache_alloc(file_slab, GFP_KERNEL);
+}
+
/* OPEN Share state helper functions */
-static inline struct nfs4_file *
-alloc_init_file(struct inode *ino)
+static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino)
{
- struct nfs4_file *fp;
unsigned int hashval = file_hashval(ino);
- fp = kmem_cache_alloc(file_slab, GFP_KERNEL);
- if (fp) {
- atomic_set(&fp->fi_ref, 1);
- INIT_LIST_HEAD(&fp->fi_hash);
- INIT_LIST_HEAD(&fp->fi_stateids);
- INIT_LIST_HEAD(&fp->fi_delegations);
- fp->fi_inode = igrab(ino);
- fp->fi_had_conflict = false;
- fp->fi_lease = NULL;
- memset(fp->fi_fds, 0, sizeof(fp->fi_fds));
- memset(fp->fi_access, 0, sizeof(fp->fi_access));
- spin_lock(&recall_lock);
- list_add(&fp->fi_hash, &file_hashtbl[hashval]);
- spin_unlock(&recall_lock);
- return fp;
- }
- return NULL;
+ atomic_set(&fp->fi_ref, 1);
+ INIT_LIST_HEAD(&fp->fi_hash);
+ INIT_LIST_HEAD(&fp->fi_stateids);
+ INIT_LIST_HEAD(&fp->fi_delegations);
+ fp->fi_inode = igrab(ino);
+ fp->fi_had_conflict = false;
+ fp->fi_lease = NULL;
+ memset(fp->fi_fds, 0, sizeof(fp->fi_fds));
+ memset(fp->fi_access, 0, sizeof(fp->fi_access));
+ spin_lock(&recall_lock);
+ list_add(&fp->fi_hash, &file_hashtbl[hashval]);
+ spin_unlock(&recall_lock);
}
static void
@@ -2509,6 +2512,13 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
if (STALE_CLIENTID(&open->op_clientid))
return nfserr_stale_clientid;
+ /*
+ * In case we need it later, after we've already created the
+ * file and don't want to risk a further failure:
+ */
+ open->op_file = nfsd4_alloc_file();
+ if (open->op_file == NULL)
+ return nfserr_jukebox;
strhashval = open_ownerstr_hashval(clientid->cl_id, &open->op_owner);
oo = find_openstateowner_str(strhashval, open);
@@ -2884,9 +2894,9 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
if (open->op_claim_type == NFS4_OPEN_CLAIM_DELEGATE_CUR)
goto out;
status = nfserr_jukebox;
- fp = alloc_init_file(ino);
- if (fp == NULL)
- goto out;
+ fp = open->op_file;
+ open->op_file = NULL;
+ nfsd4_init_file(fp, ino);
}
/*
@@ -2960,6 +2970,8 @@ void nfsd4_cleanup_open_state(struct nfsd4_open *open, __be32 status)
oo->oo_flags &= ~NFS4_OO_NEW;
}
}
+ if (open->op_file)
+ nfsd4_free_file(open->op_file);
}
__be32
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 32e6fd8..502dd43 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -228,6 +228,7 @@ struct nfsd4_open {
u32 op_rflags; /* response */
int op_truncate; /* used during processing */
struct nfs4_openowner *op_openowner; /* used during processing */
+ struct nfs4_file *op_file; /* used during processing */
struct nfs4_acl *op_acl;
};
#define op_iattr iattr
--
1.7.5.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 6/8] nfsd4: do idr preallocation with stateid allocation
2011-10-17 21:55 fix for open leaks, for 3.2 J. Bruce Fields
` (4 preceding siblings ...)
2011-10-17 21:55 ` [PATCH 5/8] nfsd4: preallocate nfs4_file in process_open1() J. Bruce Fields
@ 2011-10-17 21:55 ` J. Bruce Fields
2011-10-17 21:55 ` [PATCH 7/8] nfsd4: preallocate open stateid in process_open1() J. Bruce Fields
` (2 subsequent siblings)
8 siblings, 0 replies; 12+ messages in thread
From: J. Bruce Fields @ 2011-10-17 21:55 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
Move idr preallocation out of stateid initialization, into stateid
allocation, so that we no longer have to handle any errors from the
former.
This is a little subtle due to the way the idr code manages these
preallocated items--document that in comments.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 76 +++++++++++++++++++++++++--------------------------
fs/nfsd/state.h | 4 +-
2 files changed, 39 insertions(+), 41 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ae5d250..1f8c781 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -215,13 +215,12 @@ static inline int get_new_stid(struct nfs4_stid *stid)
int new_stid;
int error;
- if (!idr_pre_get(stateids, GFP_KERNEL))
- return -ENOMEM;
-
error = idr_get_new_above(stateids, stid, min_stateid, &new_stid);
/*
- * All this code is currently serialized; the preallocation
- * above should still be ours:
+ * Note: the necessary preallocation was done in
+ * nfs4_alloc_stateid(). The idr code caps the number of
+ * preallocations that can exist at a time, but the state lock
+ * prevents anyone from using ours before we get here:
*/
BUG_ON(error);
/*
@@ -240,7 +239,7 @@ static inline int get_new_stid(struct nfs4_stid *stid)
return new_stid;
}
-static inline __be32 init_stid(struct nfs4_stid *stid, struct nfs4_client *cl, unsigned char type)
+static void init_stid(struct nfs4_stid *stid, struct nfs4_client *cl, unsigned char type)
{
stateid_t *s = &stid->sc_stateid;
int new_id;
@@ -249,12 +248,24 @@ static inline __be32 init_stid(struct nfs4_stid *stid, struct nfs4_client *cl, u
stid->sc_client = cl;
s->si_opaque.so_clid = cl->cl_clientid;
new_id = get_new_stid(stid);
- if (new_id < 0)
- return nfserr_jukebox;
s->si_opaque.so_id = (u32)new_id;
/* Will be incremented before return to client: */
s->si_generation = 0;
- return 0;
+}
+
+static struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *slab)
+{
+ struct idr *stateids = &cl->cl_stateids;
+
+ if (!idr_pre_get(stateids, GFP_KERNEL))
+ return NULL;
+ /*
+ * Note: if we fail here (or any time between now and the time
+ * we actually get the new idr), we won't need to undo the idr
+ * preallocation, since the idr code caps the number of
+ * preallocated entries.
+ */
+ return kmem_cache_alloc(slab, GFP_KERNEL);
}
static struct nfs4_delegation *
@@ -262,7 +273,6 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
{
struct nfs4_delegation *dp;
struct nfs4_file *fp = stp->st_file;
- __be32 status;
dprintk("NFSD alloc_init_deleg\n");
/*
@@ -276,14 +286,10 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
return NULL;
if (num_delegations > max_delegations)
return NULL;
- dp = kmem_cache_alloc(deleg_slab, GFP_KERNEL);
+ dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab));
if (dp == NULL)
return dp;
- status = init_stid(&dp->dl_stid, clp, NFS4_DELEG_STID);
- if (status) {
- kmem_cache_free(deleg_slab, dp);
- return NULL;
- }
+ init_stid(&dp->dl_stid, clp, NFS4_DELEG_STID);
/*
* delegation seqid's are never incremented. The 4.1 special
* meaning of seqid 0 isn't meaningful, really, but let's avoid
@@ -2331,14 +2337,11 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfs4_client *clp, str
return oo;
}
-static inline __be32 init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp, struct nfsd4_open *open) {
+static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp, struct nfsd4_open *open) {
struct nfs4_openowner *oo = open->op_openowner;
struct nfs4_client *clp = oo->oo_owner.so_client;
- __be32 status;
- status = init_stid(&stp->st_stid, clp, NFS4_OPEN_STID);
- if (status)
- return status;
+ init_stid(&stp->st_stid, clp, NFS4_OPEN_STID);
INIT_LIST_HEAD(&stp->st_lockowners);
list_add(&stp->st_perstateowner, &oo->oo_owner.so_stateids);
list_add(&stp->st_perfile, &fp->fi_stateids);
@@ -2350,7 +2353,6 @@ static inline __be32 init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_
__set_bit(open->op_share_access, &stp->st_access_bmap);
__set_bit(open->op_share_deny, &stp->st_deny_bmap);
stp->st_openstp = NULL;
- return nfs_ok;
}
static void
@@ -2614,10 +2616,14 @@ nfs4_check_open(struct nfs4_file *fp, struct nfsd4_open *open, struct nfs4_ol_st
return nfs_ok;
}
-static inline struct nfs4_ol_stateid *
-nfs4_alloc_stateid(void)
+static struct nfs4_ol_stateid * nfs4_alloc_stateid(struct nfs4_client *clp)
{
- return kmem_cache_alloc(stateid_slab, GFP_KERNEL);
+ return openlockstateid(nfs4_alloc_stid(clp, stateid_slab));
+}
+
+static void nfs4_free_stateid(struct nfs4_ol_stateid *s)
+{
+ kmem_cache_free(stateid_slab, s);
}
static inline int nfs4_access_to_access(u32 nfs4_access)
@@ -2661,15 +2667,16 @@ nfs4_new_open(struct svc_rqst *rqstp, struct nfs4_ol_stateid **stpp,
struct nfsd4_open *open)
{
struct nfs4_ol_stateid *stp;
+ struct nfs4_client *cl = open->op_openowner->oo_owner.so_client;
__be32 status;
- stp = nfs4_alloc_stateid();
+ stp = nfs4_alloc_stateid(cl);
if (stp == NULL)
return nfserr_jukebox;
status = nfs4_get_vfs_file(rqstp, fp, cur_fh, open);
if (status) {
- kmem_cache_free(stateid_slab, stp);
+ nfs4_free_stateid(stp);
return status;
}
*stpp = stp;
@@ -2912,11 +2919,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
status = nfs4_new_open(rqstp, &stp, fp, current_fh, open);
if (status)
goto out;
- status = init_open_stateid(stp, fp, open);
- if (status) {
- release_open_stateid(stp);
- goto out;
- }
+ init_open_stateid(stp, fp, open);
status = nfsd4_truncate(rqstp, current_fh, open);
if (status) {
release_open_stateid(stp);
@@ -3812,16 +3815,11 @@ alloc_init_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp, struct
{
struct nfs4_ol_stateid *stp;
struct nfs4_client *clp = lo->lo_owner.so_client;
- __be32 status;
- stp = nfs4_alloc_stateid();
+ stp = nfs4_alloc_stateid(clp);
if (stp == NULL)
return NULL;
- status = init_stid(&stp->st_stid, clp, NFS4_LOCK_STID);
- if (status) {
- free_generic_stateid(stp);
- return NULL;
- }
+ init_stid(&stp->st_stid, clp, NFS4_LOCK_STID);
list_add(&stp->st_perfile, &fp->fi_stateids);
list_add(&stp->st_perstateowner, &lo->lo_owner.so_stateids);
stp->st_stateowner = &lo->lo_owner;
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index eab9dae..1a58200 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -85,6 +85,7 @@ struct nfs4_stid {
};
struct nfs4_delegation {
+ struct nfs4_stid dl_stid; /* must be first field */
struct list_head dl_perfile;
struct list_head dl_perclnt;
struct list_head dl_recall_lru; /* delegation recalled */
@@ -93,7 +94,6 @@ struct nfs4_delegation {
u32 dl_type;
time_t dl_time;
/* For recall: */
- struct nfs4_stid dl_stid;
struct knfsd_fh dl_fh;
int dl_retries;
struct nfsd4_callback dl_recall;
@@ -434,7 +434,7 @@ static inline struct file *find_any_file(struct nfs4_file *f)
/* "ol" stands for "Open or Lock". Better suggestions welcome. */
struct nfs4_ol_stateid {
- struct nfs4_stid st_stid;
+ struct nfs4_stid st_stid; /* must be first field */
struct list_head st_perfile;
struct list_head st_perstateowner;
struct list_head st_lockowners;
--
1.7.5.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 7/8] nfsd4: preallocate open stateid in process_open1()
2011-10-17 21:55 fix for open leaks, for 3.2 J. Bruce Fields
` (5 preceding siblings ...)
2011-10-17 21:55 ` [PATCH 6/8] nfsd4: do idr preallocation with stateid allocation J. Bruce Fields
@ 2011-10-17 21:55 ` J. Bruce Fields
2011-10-17 21:55 ` [PATCH 8/8] nfsd4: warn on open failure after create J. Bruce Fields
2011-10-17 21:57 ` fix for open leaks, for 3.2 J. Bruce Fields
8 siblings, 0 replies; 12+ messages in thread
From: J. Bruce Fields @ 2011-10-17 21:55 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
As with the nfs4_file, we'd prefer to find out about any failure before
creating a new file rather than after.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4state.c | 49 ++++++++++++++++++++-----------------------------
fs/nfsd/xdr4.h | 1 +
2 files changed, 21 insertions(+), 29 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1f8c781..3e1d4e0 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -268,6 +268,11 @@ static struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cac
return kmem_cache_alloc(slab, GFP_KERNEL);
}
+static struct nfs4_ol_stateid * nfs4_alloc_stateid(struct nfs4_client *clp)
+{
+ return openlockstateid(nfs4_alloc_stid(clp, stateid_slab));
+}
+
static struct nfs4_delegation *
alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct svc_fh *current_fh, u32 type)
{
@@ -2511,6 +2516,7 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
struct nfs4_client *clp = NULL;
unsigned int strhashval;
struct nfs4_openowner *oo = NULL;
+ __be32 status;
if (STALE_CLIENTID(&open->op_clientid))
return nfserr_stale_clientid;
@@ -2538,12 +2544,20 @@ nfsd4_process_open1(struct nfsd4_compound_state *cstate,
open->op_openowner = NULL;
goto new_owner;
}
- return nfsd4_check_seqid(cstate, &oo->oo_owner, open->op_seqid);
+ status = nfsd4_check_seqid(cstate, &oo->oo_owner, open->op_seqid);
+ if (status)
+ return status;
+ clp = oo->oo_owner.so_client;
+ goto alloc_stateid;
new_owner:
oo = alloc_init_open_stateowner(strhashval, clp, open);
if (oo == NULL)
return nfserr_jukebox;
open->op_openowner = oo;
+alloc_stateid:
+ open->op_stp = nfs4_alloc_stateid(clp);
+ if (!open->op_stp)
+ return nfserr_jukebox;
return nfs_ok;
}
@@ -2616,11 +2630,6 @@ nfs4_check_open(struct nfs4_file *fp, struct nfsd4_open *open, struct nfs4_ol_st
return nfs_ok;
}
-static struct nfs4_ol_stateid * nfs4_alloc_stateid(struct nfs4_client *clp)
-{
- return openlockstateid(nfs4_alloc_stid(clp, stateid_slab));
-}
-
static void nfs4_free_stateid(struct nfs4_ol_stateid *s)
{
kmem_cache_free(stateid_slab, s);
@@ -2661,28 +2670,6 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
return nfs_ok;
}
-static __be32
-nfs4_new_open(struct svc_rqst *rqstp, struct nfs4_ol_stateid **stpp,
- struct nfs4_file *fp, struct svc_fh *cur_fh,
- struct nfsd4_open *open)
-{
- struct nfs4_ol_stateid *stp;
- struct nfs4_client *cl = open->op_openowner->oo_owner.so_client;
- __be32 status;
-
- stp = nfs4_alloc_stateid(cl);
- if (stp == NULL)
- return nfserr_jukebox;
-
- status = nfs4_get_vfs_file(rqstp, fp, cur_fh, open);
- if (status) {
- nfs4_free_stateid(stp);
- return status;
- }
- *stpp = stp;
- return 0;
-}
-
static inline __be32
nfsd4_truncate(struct svc_rqst *rqstp, struct svc_fh *fh,
struct nfsd4_open *open)
@@ -2916,9 +2903,11 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
if (status)
goto out;
} else {
- status = nfs4_new_open(rqstp, &stp, fp, current_fh, open);
+ status = nfs4_get_vfs_file(rqstp, fp, current_fh, open);
if (status)
goto out;
+ stp = open->op_stp;
+ open->op_stp = NULL;
init_open_stateid(stp, fp, open);
status = nfsd4_truncate(rqstp, current_fh, open);
if (status) {
@@ -2975,6 +2964,8 @@ void nfsd4_cleanup_open_state(struct nfsd4_open *open, __be32 status)
}
if (open->op_file)
nfsd4_free_file(open->op_file);
+ if (open->op_stp)
+ nfs4_free_stateid(open->op_stp);
}
__be32
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 502dd43..ce8c591 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -229,6 +229,7 @@ struct nfsd4_open {
int op_truncate; /* used during processing */
struct nfs4_openowner *op_openowner; /* used during processing */
struct nfs4_file *op_file; /* used during processing */
+ struct nfs4_ol_stateid *op_stp; /* used during processing */
struct nfs4_acl *op_acl;
};
#define op_iattr iattr
--
1.7.5.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 8/8] nfsd4: warn on open failure after create
2011-10-17 21:55 fix for open leaks, for 3.2 J. Bruce Fields
` (6 preceding siblings ...)
2011-10-17 21:55 ` [PATCH 7/8] nfsd4: preallocate open stateid in process_open1() J. Bruce Fields
@ 2011-10-17 21:55 ` J. Bruce Fields
2011-10-17 21:57 ` fix for open leaks, for 3.2 J. Bruce Fields
8 siblings, 0 replies; 12+ messages in thread
From: J. Bruce Fields @ 2011-10-17 21:55 UTC (permalink / raw)
To: linux-nfs; +Cc: J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
If we create the object and then return failure to the client, we're
left with an unexpected file in the filesystem.
I'm trying to eliminate such cases but not 100% sure I have so an
assertion might be helpful for now.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
fs/nfsd/nfs4proc.c | 7 ++++---
fs/nfsd/vfs.c | 2 +-
fs/nfsd/vfs.h | 2 +-
fs/nfsd/xdr4.h | 3 ++-
4 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 10b50d7..710b97b 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -194,7 +194,6 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
{
struct svc_fh resfh;
__be32 status;
- int created = 0;
fh_init(&resfh, NFS4_FHSIZE);
open->op_truncate = 0;
@@ -223,7 +222,7 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
open->op_fname.len, &open->op_iattr,
&resfh, open->op_createmode,
(u32 *)open->op_verf.data,
- &open->op_truncate, &created);
+ &open->op_truncate, &open->op_created);
/*
* Following rfc 3530 14.2.16, use the returned bitmask
@@ -253,7 +252,7 @@ do_open_lookup(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfsd4_o
/* set reply cache */
fh_copy_shallow(&open->op_openowner->oo_owner.so_replay.rp_openfh,
&resfh.fh_handle);
- if (!created)
+ if (!open->op_created)
status = do_open_permission(rqstp, current_fh, open,
NFSD_MAY_NOP);
@@ -318,6 +317,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
/* We don't yet support WANT bits: */
open->op_share_access &= NFS4_SHARE_ACCESS_MASK;
+ open->op_created = 0;
/*
* RFC5661 18.51.3
* Before RECLAIM_COMPLETE done, server should deny new lock
@@ -408,6 +408,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
* set, (2) sets open->op_stateid, (3) sets open->op_delegation.
*/
status = nfsd4_process_open2(rqstp, &cstate->current_fh, open);
+ WARN_ON(status && open->op_created);
out:
nfsd4_cleanup_open_state(open, status);
if (open->op_openowner)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 4dd9128..7a2e442 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1370,7 +1370,7 @@ __be32
do_nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
char *fname, int flen, struct iattr *iap,
struct svc_fh *resfhp, int createmode, u32 *verifier,
- int *truncp, int *created)
+ bool *truncp, bool *created)
{
struct dentry *dentry, *dchild = NULL;
struct inode *dirp;
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 503f3bf..3f54ad0 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -62,7 +62,7 @@ __be32 nfsd_access(struct svc_rqst *, struct svc_fh *, u32 *, u32 *);
__be32 do_nfsd_create(struct svc_rqst *, struct svc_fh *,
char *name, int len, struct iattr *attrs,
struct svc_fh *res, int createmode,
- u32 *verifier, int *truncp, int *created);
+ u32 *verifier, bool *truncp, bool *created);
__be32 nfsd_commit(struct svc_rqst *, struct svc_fh *,
loff_t, unsigned long);
#endif /* CONFIG_NFSD_V3 */
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index ce8c591..e305735 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -226,7 +226,8 @@ struct nfsd4_open {
u32 op_recall; /* recall */
struct nfsd4_change_info op_cinfo; /* response */
u32 op_rflags; /* response */
- int op_truncate; /* used during processing */
+ bool op_truncate; /* used during processing */
+ bool op_created; /* used during processing */
struct nfs4_openowner *op_openowner; /* used during processing */
struct nfs4_file *op_file; /* used during processing */
struct nfs4_ol_stateid *op_stp; /* used during processing */
--
1.7.5.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: fix for open leaks, for 3.2
2011-10-17 21:55 fix for open leaks, for 3.2 J. Bruce Fields
` (7 preceding siblings ...)
2011-10-17 21:55 ` [PATCH 8/8] nfsd4: warn on open failure after create J. Bruce Fields
@ 2011-10-17 21:57 ` J. Bruce Fields
2011-10-17 22:00 ` Bryan Schumaker
8 siblings, 1 reply; 12+ messages in thread
From: J. Bruce Fields @ 2011-10-17 21:57 UTC (permalink / raw)
To: bjschuma; +Cc: linux-nfs
On Mon, Oct 17, 2011 at 05:55:49PM -0400, J. Bruce Fields wrote:
> Bryan Schumaker noticed that we have a longstanding leak of an open
> owner each time the first open with a new open owner fails.
>
> The following patches fix that, along with a few other less likely leaks
> that occur if a memory allocation fails after a file is created.
By the way, Bryan, I didn't try to write a real test case for that
leak--would it be easy for you to rerun the test you were running before
and verify whether it's gone?
--b.
>
> I intend to commit these to for-3.2. Any review welcomed.
>
> --b.
> --
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: fix for open leaks, for 3.2
2011-10-17 21:57 ` fix for open leaks, for 3.2 J. Bruce Fields
@ 2011-10-17 22:00 ` Bryan Schumaker
2011-10-21 18:25 ` Bryan Schumaker
0 siblings, 1 reply; 12+ messages in thread
From: Bryan Schumaker @ 2011-10-17 22:00 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs
On 10/17/2011 05:57 PM, J. Bruce Fields wrote:
> On Mon, Oct 17, 2011 at 05:55:49PM -0400, J. Bruce Fields wrote:
>> Bryan Schumaker noticed that we have a longstanding leak of an open
>> owner each time the first open with a new open owner fails.
>>
>> The following patches fix that, along with a few other less likely leaks
>> that occur if a memory allocation fails after a file is created.
>
> By the way, Bryan, I didn't try to write a real test case for that
> leak--would it be easy for you to rerun the test you were running before
> and verify whether it's gone?
Sure, no problem. I'll let you know what I find.
- Bryan
>
> --b.
>
>>
>> I intend to commit these to for-3.2. Any review welcomed.
>>
>> --b.
>> --
>> 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
> --
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: fix for open leaks, for 3.2
2011-10-17 22:00 ` Bryan Schumaker
@ 2011-10-21 18:25 ` Bryan Schumaker
0 siblings, 0 replies; 12+ messages in thread
From: Bryan Schumaker @ 2011-10-21 18:25 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: linux-nfs
On 10/17/2011 06:00 PM, Bryan Schumaker wrote:
> On 10/17/2011 05:57 PM, J. Bruce Fields wrote:
>> On Mon, Oct 17, 2011 at 05:55:49PM -0400, J. Bruce Fields wrote:
>>> Bryan Schumaker noticed that we have a longstanding leak of an open
>>> owner each time the first open with a new open owner fails.
>>>
>>> The following patches fix that, along with a few other less likely leaks
>>> that occur if a memory allocation fails after a file is created.
>>
>> By the way, Bryan, I didn't try to write a real test case for that
>> leak--would it be easy for you to rerun the test you were running before
>> and verify whether it's gone?
>
> Sure, no problem. I'll let you know what I find.
Looks like these patches worked. I never deleted more than one client structure during my test.
- Bryan
>
> - Bryan
>>
>> --b.
>>
>>>
>>> I intend to commit these to for-3.2. Any review welcomed.
>>>
>>> --b.
>>> --
>>> 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
>> --
>> 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
>
> --
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread