Fix nlm state refcount going up from zero

Once a refcount goes to zero, we should not be using it any more
and let the thread that made it zero free up the entry. If zero
refcount entry is found in the hash table, don't use it, remove
it from the hash table and insert a new nlm state.

Also, under some cases, we do delete non zero refcnt nlm state objects
from the hash table as well. This change should avoid logging spurious
messages from dec_nlm_state_ref() for not finding the nlm state in the
hash table and accidentally deleting someone else's nlm state.

Change-Id: I3917327f2fdfe8df0e5d9d2a64786ba4c1eb4cd4
Signed-off-by: Malahal Naineni <malahal@us.ibm.com>

src/SAL/nlm_state.c

@@ -264,7 +264,6 @@ void dec_nlm_state_ref(state_t *state)
 	hash_error_t rc;
 	struct gsh_buffdesc buffkey;
 	struct gsh_buffdesc old_value;
-	struct gsh_buffdesc old_key;
 	int32_t refcount;
 	struct fsal_obj_handle *obj;
 
@@ -294,10 +293,22 @@ void dec_nlm_state_ref(state_t *state)
 	rc = hashtable_getlatch(ht_nlm_states, &buffkey, &old_value, true,
 				&latch);
 
-	if (rc != HASHTABLE_SUCCESS) {
-		if (rc == HASHTABLE_ERROR_NO_SUCH_KEY)
-			hashtable_releaselatched(ht_nlm_states, &latch);
+	/* Another thread that needs this entry might have deleted this
+	 * nlm state to insert its own nlm state. So expect not to find
+	 * this nlm state or find someone else's nlm state!
+	 */
+	switch (rc) {
+	case HASHTABLE_SUCCESS:
+		if (old_value.addr == state) { /* our own state */
+			hashtable_deletelatched(ht_nlm_states, &buffkey,
+						&latch, NULL, NULL);
+		}
+		break;
 
+	case HASHTABLE_ERROR_NO_SUCH_KEY:
+		break;
+
+	default:
 		if (!str_valid)
 			display_nlm_state(&dspbuf, state);
 
@@ -307,23 +318,6 @@ void dec_nlm_state_ref(state_t *state)
 		return;
 	}
 
-	refcount = atomic_fetch_int32_t(&state->state_refcount);
-
-	if (refcount > 0) {
-		if (str_valid)
-			LogDebug(COMPONENT_STATE,
-				 "Did not release refcount now=%"PRId32" {%s}",
-				 refcount, str);
-
-		hashtable_releaselatched(ht_nlm_states, &latch);
-
-		return;
-	}
-
-	/* use the key to delete the entry */
-	hashtable_deletelatched(ht_nlm_states, &buffkey, &latch, &old_key,
-				&old_value);
-
 	/* Release the latch */
 	hashtable_releaselatched(ht_nlm_states, &latch);
 
@@ -379,8 +373,8 @@ int get_nlm_state(enum state_type state_type,
 	struct display_buffer dspbuf = {sizeof(str), str, str};
 	struct hash_latch latch;
 	hash_error_t rc;
-	struct gsh_buffdesc buffkey, old_key;
-	struct gsh_buffdesc buffval, old_value;
+	struct gsh_buffdesc buffkey;
+	struct gsh_buffdesc buffval;
 
 	*pstate = NULL;
 	memset(&key, 0, sizeof(key));
@@ -402,26 +396,26 @@ int get_nlm_state(enum state_type state_type,
 	rc = hashtable_getlatch(ht_nlm_states, &buffkey, &buffval, true,
 				&latch);
 
-	/* If we found it, return it */
-	if (rc == HASHTABLE_SUCCESS) {
+	switch (rc) {
+	case HASHTABLE_SUCCESS:
 		state = buffval.addr;
 
-		if (nsm_state_applies && state->state_seqid != nsm_state) {
-			/* We are getting new locks before the old ones are
-			 * gone. We need to unhash this state_t and create a
-			 * new one.
+		if (nsm_state_applies &&
+		    (state->state_seqid != nsm_state ||
+		     atomic_fetch_int32_t(&state->state_refcount) == 0)) {
+			/* We are getting new locks before the old ones
+			 * are gone or the state is in the process of
+			 * getting deleted. We need to unhash this
+			 * state_t and create a new one.
 			 *
 			 * Keep the latch after the delete to proceed with
 			 * the new insert.
 			 */
 
 			/* use the key to delete the entry */
-			hashtable_deletelatched(ht_nlm_states,
-						&buffkey,
-						&latch,
-						&old_key,
-						&old_value);
-			goto new_state;
+			hashtable_deletelatched(ht_nlm_states, &buffkey,
+						&latch, NULL, NULL);
+			break;
 		}
 
 		/* Return the found NLM State */
@@ -440,10 +434,11 @@ int get_nlm_state(enum state_type state_type,
 
 		*pstate = state;
 		return 0;
-	}
 
-	/* An error occurred, return NULL */
-	if (rc != HASHTABLE_ERROR_NO_SUCH_KEY) {
+	case HASHTABLE_ERROR_NO_SUCH_KEY:
+		break;
+
+	default: /* An error occurred, return NULL */
 		display_nlm_state(&dspbuf, &key);
 
 		LogCrit(COMPONENT_STATE, "Error %s, could not find {%s}",
@@ -454,8 +449,6 @@ int get_nlm_state(enum state_type state_type,
 		return NLM4_DENIED_NOLOCKS;
 	}
 
- new_state:
-
 	/* If the nsm state doesn't apply, we don't want to create a new
 	 * state_t if one didn't exist already.
 	 */