[PATCH 4/6] afs: let vnode->update_cnt protected by atomic lock

[David Howells]

Yuanhan Liu <yliu.null at gmail.com> wrote:

> Instead of
>   spin_lock(&vnode->lock)
>   vnode->update_cnt++;
>   spin_unlock(&vnode->lock),
> 
> A simply atomic_inc(&vnode->update_cnt) would do that.
> 
> So, here define update_cnt as atomic_t.

Your assertion is flawed.

This is only true if update_cnt is not dependent on anything else within the
locked section - but this is not necessarily the case.

Further, you cannot simply replace two non-atomic ops on the same variable
within a single locked section with a pair of atomic ops and expect it still
to work if you move some other modification op out of the locked section.  You
*have* to combine the ops into a single atomic op.

This patch will break afs_vnode_fetch_status().  Note there the tests and the
increment within the same locked section.  You have made the test and the
increment no longer atomic.

Also this:

	-	spin_lock(&vnode->lock);
	-	vnode->update_cnt--;
	-	ASSERTCMP(vnode->update_cnt, >=, 0);
	-	spin_unlock(&vnode->lock);
	+	atomic_dec(&vnode->update_cnt);
	+	ASSERTCMP(atomic_read(&vnode->update_cnt), >=, 0);

is not a valid change.  What guarantee do you have that update_cnt won't
decremented further between the two lines?  You've taken two ops that were
effectively atomic with respect to each other and made them non-atomic.  If
you're going to combine, you have to use something like atomic_sub_return().

David