From: "J. Bruce Fields" <bfields@fieldses.org>
To: Jeff Layton <jlayton@primarydata.com>
Cc: linux-nfs@vger.kernel.org, hch@infradead.org,
Trond Myklebust <trond.myklebust@primarydata.com>
Subject: Re: [PATCH v3 30/38] nfsd: Protect adding/removing lock owners using client_lock
Date: Fri, 1 Aug 2014 15:44:14 -0400 [thread overview]
Message-ID: <20140801194413.GD24461@fieldses.org> (raw)
In-Reply-To: <1406684083-19736-31-git-send-email-jlayton@primarydata.com>
On Tue, Jul 29, 2014 at 09:34:35PM -0400, Jeff Layton wrote:
> From: Trond Myklebust <trond.myklebust@primarydata.com>
>
> Once we remove client mutex protection, we'll need to ensure that
> stateowner lookup and creation are atomic between concurrent compounds.
> Ensure that alloc_init_lock_stateowner checks the hashtable under the
> client_lock before adding a new element.
Not worth respinning any patches for this, but "alloc_init_..." was
never my favorite naming scheme, and isn't so accurate at this point.
"find_or_create_..." is a little cumbersome too, but maybe it'd do.
--b.
>
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
> fs/nfsd/nfs4state.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 61 insertions(+), 8 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index c4bb7f2b29d9..7c15918d20f0 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1000,26 +1000,42 @@ static void release_lock_stateid(struct nfs4_ol_stateid *stp)
> nfs4_put_stid(&stp->st_stid);
> }
>
> -static void unhash_lockowner(struct nfs4_lockowner *lo)
> +static void unhash_lockowner_locked(struct nfs4_lockowner *lo)
> {
> + struct nfsd_net *nn = net_generic(lo->lo_owner.so_client->net,
> + nfsd_net_id);
> +
> + lockdep_assert_held(&nn->client_lock);
> +
> list_del_init(&lo->lo_owner.so_strhash);
> }
>
> static void release_lockowner_stateids(struct nfs4_lockowner *lo)
> {
> + struct nfsd_net *nn = net_generic(lo->lo_owner.so_client->net,
> + nfsd_net_id);
> struct nfs4_ol_stateid *stp;
>
> + lockdep_assert_held(&nn->client_lock);
> +
> while (!list_empty(&lo->lo_owner.so_stateids)) {
> stp = list_first_entry(&lo->lo_owner.so_stateids,
> struct nfs4_ol_stateid, st_perstateowner);
> + spin_unlock(&nn->client_lock);
> release_lock_stateid(stp);
> + spin_lock(&nn->client_lock);
> }
> }
>
> static void release_lockowner(struct nfs4_lockowner *lo)
> {
> - unhash_lockowner(lo);
> + struct nfsd_net *nn = net_generic(lo->lo_owner.so_client->net,
> + nfsd_net_id);
> +
> + spin_lock(&nn->client_lock);
> + unhash_lockowner_locked(lo);
> release_lockowner_stateids(lo);
> + spin_unlock(&nn->client_lock);
> nfs4_put_stateowner(&lo->lo_owner);
> }
>
> @@ -4801,7 +4817,7 @@ nevermind:
> }
>
> static struct nfs4_lockowner *
> -find_lockowner_str(clientid_t *clid, struct xdr_netobj *owner,
> +find_lockowner_str_locked(clientid_t *clid, struct xdr_netobj *owner,
> struct nfsd_net *nn)
> {
> unsigned int strhashval = ownerstr_hashval(clid->cl_id, owner);
> @@ -4818,9 +4834,25 @@ find_lockowner_str(clientid_t *clid, struct xdr_netobj *owner,
> return NULL;
> }
>
> +static struct nfs4_lockowner *
> +find_lockowner_str(clientid_t *clid, struct xdr_netobj *owner,
> + struct nfsd_net *nn)
> +{
> + struct nfs4_lockowner *lo;
> +
> + spin_lock(&nn->client_lock);
> + lo = find_lockowner_str_locked(clid, owner, nn);
> + spin_unlock(&nn->client_lock);
> + return lo;
> +}
> +
> static void nfs4_unhash_lockowner(struct nfs4_stateowner *sop)
> {
> - unhash_lockowner(lockowner(sop));
> + struct nfsd_net *nn = net_generic(sop->so_client->net, nfsd_net_id);
> +
> + spin_lock(&nn->client_lock);
> + unhash_lockowner_locked(lockowner(sop));
> + spin_unlock(&nn->client_lock);
> }
>
> static void nfs4_free_lockowner(struct nfs4_stateowner *sop)
> @@ -4843,9 +4875,12 @@ static const struct nfs4_stateowner_operations lockowner_ops = {
> * strhashval = ownerstr_hashval
> */
> static struct nfs4_lockowner *
> -alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp, struct nfs4_ol_stateid *open_stp, struct nfsd4_lock *lock) {
> - struct nfs4_lockowner *lo;
> +alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
> + struct nfs4_ol_stateid *open_stp,
> + struct nfsd4_lock *lock)
> +{
> struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> + struct nfs4_lockowner *lo, *ret;
>
> lo = alloc_stateowner(lockowner_slab, &lock->lk_new_owner, clp);
> if (!lo)
> @@ -4854,7 +4889,16 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp, str
> lo->lo_owner.so_is_open_owner = 0;
> lo->lo_owner.so_seqid = lock->lk_new_lock_seqid;
> lo->lo_owner.so_ops = &lockowner_ops;
> - list_add(&lo->lo_owner.so_strhash, &nn->ownerstr_hashtbl[strhashval]);
> + spin_lock(&nn->client_lock);
> + ret = find_lockowner_str_locked(&clp->cl_clientid,
> + &lock->lk_new_owner, nn);
> + if (ret == NULL) {
> + list_add(&lo->lo_owner.so_strhash,
> + &nn->ownerstr_hashtbl[strhashval]);
> + ret = lo;
> + } else
> + nfs4_free_lockowner(&lo->lo_owner);
> + spin_unlock(&nn->client_lock);
> return lo;
> }
>
> @@ -5395,6 +5439,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
> unsigned int hashval = ownerstr_hashval(clid->cl_id, owner);
> __be32 status;
> struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> + struct nfs4_client *clp;
>
> dprintk("nfsd4_release_lockowner clientid: (%08x/%08x):\n",
> clid->cl_boot, clid->cl_id);
> @@ -5408,6 +5453,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
> status = nfserr_locks_held;
>
> /* Find the matching lock stateowner */
> + spin_lock(&nn->client_lock);
> list_for_each_entry(tmp, &nn->ownerstr_hashtbl[hashval], so_strhash) {
> if (tmp->so_is_open_owner)
> continue;
> @@ -5417,6 +5463,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
> break;
> }
> }
> + spin_unlock(&nn->client_lock);
>
> /* No matching owner found, maybe a replay? Just declare victory... */
> if (!sop) {
> @@ -5426,16 +5473,22 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
>
> lo = lockowner(sop);
> /* see if there are still any locks associated with it */
> + clp = cstate->clp;
> + spin_lock(&clp->cl_lock);
> list_for_each_entry(stp, &sop->so_stateids, st_perstateowner) {
> if (check_for_locks(stp->st_stid.sc_file, lo)) {
> - nfs4_put_stateowner(sop);
> + spin_unlock(&clp->cl_lock);
> goto out;
> }
> }
> + spin_unlock(&clp->cl_lock);
>
> status = nfs_ok;
> + sop = NULL;
> release_lockowner(lo);
> out:
> + if (sop)
> + nfs4_put_stateowner(sop);
> nfs4_unlock_state();
> return status;
> }
> --
> 1.9.3
>
next prev parent reply other threads:[~2014-08-01 19:44 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-30 1:34 [PATCH v3 00/38] nfsd: stateid and stateowner refcounting overhaul Jeff Layton
2014-07-30 1:34 ` [PATCH v3 01/38] nfsd: Add reference counting to the lock and open stateids Jeff Layton
2014-07-30 1:34 ` [PATCH v3 02/38] nfsd: Cleanup the freeing of stateids Jeff Layton
2014-07-30 20:57 ` Christoph Hellwig
2014-07-31 16:50 ` J. Bruce Fields
2014-07-30 1:34 ` [PATCH v3 03/38] nfsd: Add a struct nfs4_file field to struct nfs4_stid Jeff Layton
2014-07-30 1:34 ` [PATCH v3 04/38] nfsd: Replace nfs4_ol_stateid->st_file with the st_stid.sc_file Jeff Layton
2014-07-30 20:59 ` Christoph Hellwig
2014-07-31 16:52 ` J. Bruce Fields
2014-07-30 1:34 ` [PATCH v3 05/38] nfsd4: use cl_lock to synchronize all stateid idr calls Jeff Layton
2014-07-30 20:59 ` Christoph Hellwig
2014-07-30 1:34 ` [PATCH v3 06/38] nfsd: do filp_close in sc_free callback for lock stateids Jeff Layton
2014-07-30 21:08 ` Christoph Hellwig
2014-07-30 1:34 ` [PATCH v3 07/38] nfsd: Add locking to protect the state owner lists Jeff Layton
2014-07-30 1:34 ` [PATCH v3 08/38] nfsd: clean up races in lock stateid searching and creation Jeff Layton
2014-07-30 1:34 ` [PATCH v3 09/38] nfsd: ensure atomicity in nfsd4_free_stateid and nfsd4_validate_stateid Jeff Layton
2014-07-30 1:34 ` [PATCH v3 10/38] nfsd: Add reference counting to lock stateids Jeff Layton
2014-07-30 1:34 ` [PATCH v3 11/38] nfsd: nfsd4_locku() must reference the lock stateid Jeff Layton
2014-07-30 1:34 ` [PATCH v3 12/38] nfsd: Ensure that nfs4_open_delegation() references the delegation stateid Jeff Layton
2014-07-30 1:34 ` [PATCH v3 13/38] nfsd: nfsd4_process_open2() must reference " Jeff Layton
2014-07-30 1:34 ` [PATCH v3 14/38] nfsd: nfsd4_process_open2() must reference the open stateid Jeff Layton
2014-07-30 1:34 ` [PATCH v3 15/38] nfsd: Prepare nfsd4_close() for open stateid referencing Jeff Layton
2014-07-30 1:34 ` [PATCH v3 16/38] nfsd: nfsd4_open_confirm() must reference the open stateid Jeff Layton
2014-07-30 1:34 ` [PATCH v3 17/38] nfsd: Add reference counting to nfs4_preprocess_confirmed_seqid_op Jeff Layton
2014-07-30 1:34 ` [PATCH v3 18/38] nfsd: Migrate the stateid reference into nfs4_preprocess_seqid_op Jeff Layton
2014-07-30 1:34 ` [PATCH v3 19/38] nfsd: Migrate the stateid reference into nfs4_lookup_stateid() Jeff Layton
2014-07-30 1:34 ` [PATCH v3 20/38] nfsd: Migrate the stateid reference into nfs4_find_stateid_by_type() Jeff Layton
2014-07-31 20:04 ` J. Bruce Fields
2014-07-30 1:34 ` [PATCH v3 21/38] nfsd: Add reference counting to state owners Jeff Layton
2014-08-01 16:48 ` J. Bruce Fields
2014-08-01 16:52 ` Jeff Layton
2014-07-30 1:34 ` [PATCH v3 22/38] nfsd: Add a mutex to protect the NFSv4.0 open owner replay cache Jeff Layton
2014-07-30 1:34 ` [PATCH v3 23/38] nfsd: clean up lockowner refcounting when finding them Jeff Layton
2014-07-30 1:34 ` [PATCH v3 24/38] nfsd: add an operation for unhashing a stateowner Jeff Layton
2014-07-30 1:34 ` [PATCH v3 25/38] nfsd: Make lock stateid take a reference to the lockowner Jeff Layton
2014-07-30 1:34 ` [PATCH v3 26/38] nfsd: clean up refcounting for lockowners Jeff Layton
2014-07-30 1:34 ` [PATCH v3 27/38] nfsd: make openstateids hold references to their openowners Jeff Layton
2014-08-01 19:29 ` J. Bruce Fields
2014-07-30 1:34 ` [PATCH v3 28/38] nfsd: don't allow CLOSE to proceed until refcount on stateid drops Jeff Layton
2014-07-30 1:34 ` [PATCH v3 29/38] nfsd: Protect adding/removing open state owners using client_lock Jeff Layton
2014-07-30 1:34 ` [PATCH v3 30/38] nfsd: Protect adding/removing lock " Jeff Layton
2014-08-01 19:44 ` J. Bruce Fields [this message]
2014-07-30 1:34 ` [PATCH v3 31/38] nfsd: Move the open owner hash table into struct nfs4_client Jeff Layton
2014-08-02 10:39 ` Kinglong Mee
2014-08-02 13:11 ` Trond Myklebust
2014-08-02 13:23 ` Jeff Layton
2014-08-02 13:51 ` Kinglong Mee
2014-08-02 13:43 ` Kinglong Mee
2014-08-02 14:05 ` Trond Myklebust
2014-08-02 14:20 ` Kinglong Mee
2014-08-02 14:47 ` Trond Myklebust
2014-08-02 22:59 ` Jeff Layton
2014-08-03 1:59 ` Trond Myklebust
[not found] ` <CAPakX04ctGujqpJ48WqT1-r5=q0T8D1Ji=q1P5UTuwdw_hTsGg@mail.gmail.com>
2014-08-03 14:15 ` Kinglong Mee
2014-08-03 14:49 ` Jeff Layton
2014-08-05 18:46 ` Bruce Fields
2014-07-30 1:34 ` [PATCH v3 32/38] nfsd: clean up and reorganize release_lockowner Jeff Layton
2014-07-30 1:34 ` [PATCH v3 33/38] nfsd: add locking to stateowner release Jeff Layton
2014-07-30 1:34 ` [PATCH v3 34/38] nfsd: optimize destroy_lockowner cl_lock thrashing Jeff Layton
2014-07-30 1:34 ` [PATCH v3 35/38] nfsd: close potential race in nfsd4_free_stateid Jeff Layton
2014-07-30 1:34 ` [PATCH v3 36/38] nfsd: reduce cl_lock thrashing in release_openowner Jeff Layton
2014-07-30 1:34 ` [PATCH v3 37/38] nfsd: don't thrash the cl_lock while freeing an open stateid Jeff Layton
2014-07-30 1:34 ` [PATCH v3 38/38] nfsd: rename unhash_generic_stateid to unhash_ol_stateid Jeff Layton
2014-08-01 20:25 ` [PATCH v3 00/38] nfsd: stateid and stateowner refcounting overhaul J. Bruce Fields
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140801194413.GD24461@fieldses.org \
--to=bfields@fieldses.org \
--cc=hch@infradead.org \
--cc=jlayton@primarydata.com \
--cc=linux-nfs@vger.kernel.org \
--cc=trond.myklebust@primarydata.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).