[PATCH 4/6] afs: let vnode->update_cnt protected by atomic lock
Yuanhan Liu
yliu.null at gmail.com
Fri Jun 22 14:53:45 EDT 2012
On Sat, Jun 23, 2012 at 2:26 AM, David Howells <dhowells at redhat.com> wrote:
> Yuanhan Liu <yliu.null at gmail.com> wrote:
>
>> Well, It simplified the lock usage and reduce a bit lock race; it also
>> reduce some codes ;)
>
> Sadly, simplification isn't an automatic guarantor of correctness.
Yes, I can feel that.
>
>> Yes, you are right. I will combine that. Is that applicable to you? If
>> so, I will make a patch tomorrow.
>
> I'm certainly willing to look at such a patch. I don't think things like
> atomic_dec_return() existed when I originally wrote the code.
>
> However, you do need to beware of interdependent variables within the same
> lock section. AFS_VNODE_CB_BROKEN and AFS_VNODE_DELETED vs update_cnt for
> example.
Yes, I do see few such cases like afs_vnode_finalise_status_update().
But I guess it's safe just like old code does. Except there are
something else we should be aware of above the old code?
Anyway, I cooked that patch just now. Comments?
--
>From 79ff80a132602977136292b80c43dcdd9eae8e09 Mon Sep 17 00:00:00 2001
From: Yuanhan Liu <yliu.null at gmail.com>
Date: Fri, 22 Jun 2012 19:16:04 +0800
Subject: [PATCH] afs: let vnode->update_cnt protected by atomic lock
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.
v2: comments from David, commbine atomic_dec() and
ASSERTCMP(atomic_read(), .., ..) into one single atomic
operation: ASSERTCMP(atomic_dec_return(), .., ..);
Signed-off-by: Yuanhan Liu <yliu.null at gmail.com>
---
fs/afs/dir.c | 12 +---
fs/afs/internal.h | 2 +-
fs/afs/super.c | 2 +-
fs/afs/vnode.c | 150 ++++++++++++++++++-----------------------------------
4 files changed, 55 insertions(+), 111 deletions(-)
diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index e22dc4b..98e3688 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -806,9 +806,7 @@ static int afs_mkdir(struct inode *dir, struct
dentry *dentry, umode_t mode)
/* apply the status report we've got for the new vnode */
vnode = AFS_FS_I(inode);
- spin_lock(&vnode->lock);
- vnode->update_cnt++;
- spin_unlock(&vnode->lock);
+ atomic_inc(&vnode->update_cnt);
afs_vnode_finalise_status_update(vnode, server);
afs_put_server(server);
@@ -991,9 +989,7 @@ static int afs_create(struct inode *dir, struct
dentry *dentry, umode_t mode,
/* apply the status report we've got for the new vnode */
vnode = AFS_FS_I(inode);
- spin_lock(&vnode->lock);
- vnode->update_cnt++;
- spin_unlock(&vnode->lock);
+ atomic_inc(&vnode->update_cnt);
afs_vnode_finalise_status_update(vnode, server);
afs_put_server(server);
@@ -1111,9 +1107,7 @@ static int afs_symlink(struct inode *dir, struct
dentry *dentry,
/* apply the status report we've got for the new vnode */
vnode = AFS_FS_I(inode);
- spin_lock(&vnode->lock);
- vnode->update_cnt++;
- spin_unlock(&vnode->lock);
+ atomic_inc(&vnode->update_cnt);
afs_vnode_finalise_status_update(vnode, server);
afs_put_server(server);
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index a306bb6..053fdeb 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -344,7 +344,7 @@ struct afs_vnode {
struct mutex permits_lock; /* lock for altering permits list */
struct mutex validate_lock; /* lock for validating this vnode */
wait_queue_head_t update_waitq; /* status fetch waitqueue */
- int update_cnt; /* number of outstanding ops that will update the
+ atomic_t update_cnt; /* number of outstanding ops that will update the
* status */
spinlock_t writeback_lock; /* lock for writebacks */
spinlock_t lock; /* waitqueue/flags lock */
diff --git a/fs/afs/super.c b/fs/afs/super.c
index 459f1c5..b8df82c 100644
--- a/fs/afs/super.c
+++ b/fs/afs/super.c
@@ -479,9 +479,9 @@ static struct inode *afs_alloc_inode(struct super_block *sb)
memset(&vnode->status, 0, sizeof(vnode->status));
vnode->volume = NULL;
- vnode->update_cnt = 0;
vnode->flags = 1 << AFS_VNODE_UNSET;
vnode->cb_promised = false;
+ atomic_set(&vnode->update_cnt, 0);
_leave(" = %p", &vnode->vfs_inode);
return &vnode->vfs_inode;
diff --git a/fs/afs/vnode.c b/fs/afs/vnode.c
index 25cf4c3..f776ea8 100644
--- a/fs/afs/vnode.c
+++ b/fs/afs/vnode.c
@@ -220,8 +220,7 @@ void afs_vnode_finalise_status_update(struct
afs_vnode *vnode,
spin_lock(&vnode->lock);
clear_bit(AFS_VNODE_CB_BROKEN, &vnode->flags);
afs_vnode_note_promise(vnode, server);
- vnode->update_cnt--;
- ASSERTCMP(vnode->update_cnt, >=, 0);
+ ASSERTCMP(atomic_dec_return(&vnode->update_cnt), >=, 0);
spin_unlock(&vnode->lock);
wake_up_all(&vnode->update_waitq);
@@ -246,8 +245,7 @@ static void afs_vnode_status_update_failed(struct
afs_vnode *vnode, int ret)
afs_vnode_deleted_remotely(vnode);
}
- vnode->update_cnt--;
- ASSERTCMP(vnode->update_cnt, >=, 0);
+ ASSERTCMP(atomic_dec_return(&vnode->update_cnt), >=, 0);
spin_unlock(&vnode->lock);
wake_up_all(&vnode->update_waitq);
@@ -298,11 +296,11 @@ int afs_vnode_fetch_status(struct afs_vnode *vnode,
return 0;
}
- ASSERTCMP(vnode->update_cnt, >=, 0);
+ ASSERTCMP(atomic_read(&vnode->update_cnt), >=, 0);
- if (vnode->update_cnt > 0) {
+ if (atomic_read(&vnode->update_cnt) > 0) {
/* someone else started a fetch */
- _debug("wait on fetch %d", vnode->update_cnt);
+ _debug("wait on fetch %d", atomic_read(&vnode->update_cnt));
set_current_state(TASK_UNINTERRUPTIBLE);
ASSERT(myself.func != NULL);
@@ -317,7 +315,7 @@ int afs_vnode_fetch_status(struct afs_vnode *vnode,
/* check to see if it got updated and invalidated all
* before we saw it */
- if (vnode->update_cnt == 0) {
+ if (atomic_read(&vnode->update_cnt) == 0) {
remove_wait_queue(&vnode->update_waitq,
&myself);
set_current_state(TASK_RUNNING);
@@ -342,7 +340,7 @@ int afs_vnode_fetch_status(struct afs_vnode *vnode,
get_anyway:
/* okay... we're going to have to initiate the op */
- vnode->update_cnt++;
+ atomic_inc(&vnode->update_cnt);
spin_unlock(&vnode->lock);
@@ -374,17 +372,15 @@ get_anyway:
afs_vnode_status_update_failed(vnode, ret);
}
- ASSERTCMP(vnode->update_cnt, >=, 0);
+ ASSERTCMP(atomic_read(&vnode->update_cnt), >=, 0);
- _leave(" = %d [cnt %d]", ret, vnode->update_cnt);
+ _leave(" = %d [cnt %d]", ret, atomic_read(&vnode->update_cnt));
return ret;
no_server:
- spin_lock(&vnode->lock);
- vnode->update_cnt--;
- ASSERTCMP(vnode->update_cnt, >=, 0);
- spin_unlock(&vnode->lock);
- _leave(" = %ld [cnt %d]", PTR_ERR(server), vnode->update_cnt);
+ ASSERTCMP(atomic_dec_return(&vnode->update_cnt), >=, 0);
+ _leave(" = %ld [cnt %d]", PTR_ERR(server),
+ atomic_read(&vnode->update_cnt));
return PTR_ERR(server);
}
@@ -406,9 +402,7 @@ int afs_vnode_fetch_data(struct afs_vnode *vnode,
struct key *key,
key_serial(key));
/* this op will fetch the status */
- spin_lock(&vnode->lock);
- vnode->update_cnt++;
- spin_unlock(&vnode->lock);
+ atomic_inc(&vnode->update_cnt);
/* merge in AFS status fetches and clear outstanding callback on this
* vnode */
@@ -437,10 +431,7 @@ int afs_vnode_fetch_data(struct afs_vnode *vnode,
struct key *key,
return ret;
no_server:
- spin_lock(&vnode->lock);
- vnode->update_cnt--;
- ASSERTCMP(vnode->update_cnt, >=, 0);
- spin_unlock(&vnode->lock);
+ ASSERTCMP(atomic_dec_return(&vnode->update_cnt), >=, 0);
return PTR_ERR(server);
}
@@ -464,9 +455,7 @@ int afs_vnode_create(struct afs_vnode *vnode,
struct key *key,
name);
/* this op will fetch the status on the directory we're creating in */
- spin_lock(&vnode->lock);
- vnode->update_cnt++;
- spin_unlock(&vnode->lock);
+ atomic_inc(&vnode->update_cnt);
do {
/* pick a server to query */
@@ -490,15 +479,13 @@ int afs_vnode_create(struct afs_vnode *vnode,
struct key *key,
*_server = NULL;
}
- _leave(" = %d [cnt %d]", ret, vnode->update_cnt);
+ _leave(" = %d [cnt %d]", ret, atomic_read(&vnode->update_cnt));
return ret;
no_server:
- spin_lock(&vnode->lock);
- vnode->update_cnt--;
- ASSERTCMP(vnode->update_cnt, >=, 0);
- spin_unlock(&vnode->lock);
- _leave(" = %ld [cnt %d]", PTR_ERR(server), vnode->update_cnt);
+ ASSERTCMP(atomic_dec_return(&vnode->update_cnt), >=, 0);
+ _leave(" = %ld [cnt %d]", PTR_ERR(server),
+ atomic_read(&vnode->update_cnt));
return PTR_ERR(server);
}
@@ -520,9 +507,7 @@ int afs_vnode_remove(struct afs_vnode *vnode,
struct key *key, const char *name,
name);
/* this op will fetch the status on the directory we're removing from */
- spin_lock(&vnode->lock);
- vnode->update_cnt++;
- spin_unlock(&vnode->lock);
+ atomic_inc(&vnode->update_cnt);
do {
/* pick a server to query */
@@ -545,15 +530,13 @@ int afs_vnode_remove(struct afs_vnode *vnode,
struct key *key, const char *name,
afs_vnode_status_update_failed(vnode, ret);
}
- _leave(" = %d [cnt %d]", ret, vnode->update_cnt);
+ _leave(" = %d [cnt %d]", ret, atomic_read(&vnode->update_cnt));
return ret;
no_server:
- spin_lock(&vnode->lock);
- vnode->update_cnt--;
- ASSERTCMP(vnode->update_cnt, >=, 0);
- spin_unlock(&vnode->lock);
- _leave(" = %ld [cnt %d]", PTR_ERR(server), vnode->update_cnt);
+ ASSERTCMP(atomic_dec_return(&vnode->update_cnt), >=, 0);
+ _leave(" = %ld [cnt %d]", PTR_ERR(server),
+ atomic_read(&vnode->update_cnt));
return PTR_ERR(server);
}
@@ -579,12 +562,8 @@ int afs_vnode_link(struct afs_vnode *dvnode,
struct afs_vnode *vnode,
name);
/* this op will fetch the status on the directory we're removing from */
- spin_lock(&vnode->lock);
- vnode->update_cnt++;
- spin_unlock(&vnode->lock);
- spin_lock(&dvnode->lock);
- dvnode->update_cnt++;
- spin_unlock(&dvnode->lock);
+ atomic_inc(&vnode->update_cnt);
+ atomic_inc(&dvnode->update_cnt);
do {
/* pick a server to query */
@@ -609,19 +588,14 @@ int afs_vnode_link(struct afs_vnode *dvnode,
struct afs_vnode *vnode,
afs_vnode_status_update_failed(dvnode, ret);
}
- _leave(" = %d [cnt %d]", ret, vnode->update_cnt);
+ _leave(" = %d [cnt %d]", ret, atomic_read(&vnode->update_cnt));
return ret;
no_server:
- spin_lock(&vnode->lock);
- vnode->update_cnt--;
- ASSERTCMP(vnode->update_cnt, >=, 0);
- spin_unlock(&vnode->lock);
- spin_lock(&dvnode->lock);
- dvnode->update_cnt--;
- ASSERTCMP(dvnode->update_cnt, >=, 0);
- spin_unlock(&dvnode->lock);
- _leave(" = %ld [cnt %d]", PTR_ERR(server), vnode->update_cnt);
+ ASSERTCMP(atomic_dec_return(&vnode->update_cnt), >=, 0);
+ ASSERTCMP(atomic_dec_return(&dvnode->update_cnt), >=, 0);
+ _leave(" = %ld [cnt %d]", PTR_ERR(server),
+ atomic_read(&vnode->update_cnt));
return PTR_ERR(server);
}
@@ -646,9 +620,7 @@ int afs_vnode_symlink(struct afs_vnode *vnode,
struct key *key,
name, content);
/* this op will fetch the status on the directory we're creating in */
- spin_lock(&vnode->lock);
- vnode->update_cnt++;
- spin_unlock(&vnode->lock);
+ atomic_inc(&vnode->update_cnt);
do {
/* pick a server to query */
@@ -672,15 +644,13 @@ int afs_vnode_symlink(struct afs_vnode *vnode,
struct key *key,
*_server = NULL;
}
- _leave(" = %d [cnt %d]", ret, vnode->update_cnt);
+ _leave(" = %d [cnt %d]", ret, atomic_read(&vnode->update_cnt));
return ret;
no_server:
- spin_lock(&vnode->lock);
- vnode->update_cnt--;
- ASSERTCMP(vnode->update_cnt, >=, 0);
- spin_unlock(&vnode->lock);
- _leave(" = %ld [cnt %d]", PTR_ERR(server), vnode->update_cnt);
+ ASSERTCMP(atomic_dec_return(&vnode->update_cnt), >=, 0);
+ _leave(" = %ld [cnt %d]", PTR_ERR(server),
+ atomic_read(&vnode->update_cnt));
return PTR_ERR(server);
}
@@ -711,14 +681,9 @@ int afs_vnode_rename(struct afs_vnode *orig_dvnode,
/* this op will fetch the status on both the directories we're dealing
* with */
- spin_lock(&orig_dvnode->lock);
- orig_dvnode->update_cnt++;
- spin_unlock(&orig_dvnode->lock);
- if (new_dvnode != orig_dvnode) {
- spin_lock(&new_dvnode->lock);
- new_dvnode->update_cnt++;
- spin_unlock(&new_dvnode->lock);
- }
+ atomic_inc(&orig_dvnode->update_cnt);
+ if (new_dvnode != orig_dvnode)
+ atomic_inc(&new_dvnode->update_cnt);
do {
/* pick a server to query */
@@ -745,21 +710,16 @@ int afs_vnode_rename(struct afs_vnode *orig_dvnode,
afs_vnode_status_update_failed(new_dvnode, ret);
}
- _leave(" = %d [cnt %d]", ret, orig_dvnode->update_cnt);
+ _leave(" = %d [cnt %d]", ret, atomic_read(&orig_dvnode->update_cnt));
return ret;
no_server:
- spin_lock(&orig_dvnode->lock);
- orig_dvnode->update_cnt--;
- ASSERTCMP(orig_dvnode->update_cnt, >=, 0);
- spin_unlock(&orig_dvnode->lock);
- if (new_dvnode != orig_dvnode) {
- spin_lock(&new_dvnode->lock);
- new_dvnode->update_cnt--;
- ASSERTCMP(new_dvnode->update_cnt, >=, 0);
- spin_unlock(&new_dvnode->lock);
- }
- _leave(" = %ld [cnt %d]", PTR_ERR(server), orig_dvnode->update_cnt);
+ ASSERTCMP(atomic_dec_return(&orig_dvnode->update_cnt), >=, 0);
+ if (new_dvnode != orig_dvnode)
+ ASSERTCMP(atomic_dec_return(&new_dvnode->update_cnt), >=, 0);
+
+ _leave(" = %ld [cnt %d]", PTR_ERR(server),
+ atomic_read(&orig_dvnode->update_cnt));
return PTR_ERR(server);
}
@@ -782,9 +742,7 @@ int afs_vnode_store_data(struct afs_writeback *wb,
pgoff_t first, pgoff_t last,
first, last, offset, to);
/* this op will fetch the status */
- spin_lock(&vnode->lock);
- vnode->update_cnt++;
- spin_unlock(&vnode->lock);
+ atomic_inc(&vnode->update_cnt);
do {
/* pick a server to query */
@@ -811,10 +769,7 @@ int afs_vnode_store_data(struct afs_writeback
*wb, pgoff_t first, pgoff_t last,
return ret;
no_server:
- spin_lock(&vnode->lock);
- vnode->update_cnt--;
- ASSERTCMP(vnode->update_cnt, >=, 0);
- spin_unlock(&vnode->lock);
+ ASSERTCMP(atomic_dec_return(&vnode->update_cnt), >=, 0);
return PTR_ERR(server);
}
@@ -835,9 +790,7 @@ int afs_vnode_setattr(struct afs_vnode *vnode,
struct key *key,
key_serial(key));
/* this op will fetch the status */
- spin_lock(&vnode->lock);
- vnode->update_cnt++;
- spin_unlock(&vnode->lock);
+ atomic_inc(&vnode->update_cnt);
do {
/* pick a server to query */
@@ -863,10 +816,7 @@ int afs_vnode_setattr(struct afs_vnode *vnode,
struct key *key,
return ret;
no_server:
- spin_lock(&vnode->lock);
- vnode->update_cnt--;
- ASSERTCMP(vnode->update_cnt, >=, 0);
- spin_unlock(&vnode->lock);
+ ASSERTCMP(atomic_dec_return(&vnode->update_cnt), >=, 0);
return PTR_ERR(server);
}
--
1.7.4.4
--
yliu
More information about the linux-afs
mailing list