* [Cluster-devel] [GFS2 PATCH v2] GFS2: Use local iopen glock holder in gfs2_evict_inode
[not found] <801923599.20160945.1466089868074.JavaMail.zimbra@redhat.com>
@ 2016-06-16 15:11 ` Bob Peterson
2016-06-16 15:51 ` Bob Peterson
0 siblings, 1 reply; 2+ messages in thread
From: Bob Peterson @ 2016-06-16 15:11 UTC (permalink / raw)
To: cluster-devel.redhat.com
Hi,
I found a couple problems with the previous version of this patch
during testing. Here is my replacement, version 2.
Patch description:
Before this patch, function gfs2_evict_inode unlocked the iopen
glock (from SH), waited for completion, then locked it again in
EXclusive mode. That's all well and good except that other processes
(not in gfs2_evict_inode) can try to do lookups, and function
gfs2_inode_lookup tries to lock the iopen glock in SH again. This
second lookup can and does wipe out the holder's pid with getpid().
The first putpid (from glock_holder_uninit) will be successful, but
the second one will crash the kernel with:
BUG: unable to handle kernel paging request
This patch introduces a holder variable, io_gh, local to function
gfs2_evict_inode, which will keep its own getpid() and subsequent
putpid() from interfering with one another. So simultaneous inode
lookups won't change the value out from under gfs2_evict_inode.
Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
fs/gfs2/super.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 9b2ff353..21a8ba8 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -1518,7 +1518,7 @@ static void gfs2_evict_inode(struct inode *inode)
struct super_block *sb = inode->i_sb;
struct gfs2_sbd *sdp = sb->s_fs_info;
struct gfs2_inode *ip = GFS2_I(inode);
- struct gfs2_holder gh;
+ struct gfs2_holder gh, iopen_gh;
struct address_space *metamapping;
int error;
@@ -1527,6 +1527,7 @@ static void gfs2_evict_inode(struct inode *inode)
return;
}
+ memset(&iopen_gh, 0, sizeof(iopen_gh));
if (inode->i_nlink || (sb->s_flags & MS_RDONLY))
goto out;
@@ -1555,9 +1556,15 @@ static void gfs2_evict_inode(struct inode *inode)
test_bit(HIF_HOLDER, &ip->i_iopen_gh.gh_iflags)) {
ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
gfs2_glock_dq_wait(&ip->i_iopen_gh);
- gfs2_holder_reinit(LM_ST_EXCLUSIVE, LM_FLAG_TRY_1CB | GL_NOCACHE,
- &ip->i_iopen_gh);
- error = gfs2_glock_nq(&ip->i_iopen_gh);
+ /* This is subtle: Now we need to uninit the i_iopen_holder,
+ but if we do that before we obtain the new reference with
+ the local holder, the uninit's glock_put will free the
+ glock, which causes the new ref. to crash. So we need to do
+ this in a very specific order. Can't use glock_nq_init. */
+ gfs2_holder_init(ip->i_iopen_gh.gh_gl, LM_ST_EXCLUSIVE,
+ LM_FLAG_TRY_1CB | GL_NOCACHE, &iopen_gh);
+ gfs2_holder_uninit(&ip->i_iopen_gh);
+ error = gfs2_glock_nq(&iopen_gh);
if (error)
goto out_truncate;
}
@@ -1610,12 +1617,12 @@ out_unlock:
if (gfs2_rs_active(&ip->i_res))
gfs2_rs_deltree(&ip->i_res);
- if (ip->i_iopen_gh.gh_gl) {
- if (test_bit(HIF_HOLDER, &ip->i_iopen_gh.gh_iflags)) {
- ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
- gfs2_glock_dq_wait(&ip->i_iopen_gh);
+ if (iopen_gh.gh_gl) {
+ if (test_bit(HIF_HOLDER, &iopen_gh.gh_iflags)) {
+ iopen_gh.gh_flags |= GL_NOCACHE;
+ gfs2_glock_dq_wait(&iopen_gh);
}
- gfs2_holder_uninit(&ip->i_iopen_gh);
+ gfs2_holder_uninit(&iopen_gh);
}
gfs2_glock_dq_uninit(&gh);
if (error && error != GLR_TRYFAILED && error != -EROFS)
^ permalink raw reply related [flat|nested] 2+ messages in thread
* [Cluster-devel] [GFS2 PATCH v2] GFS2: Use local iopen glock holder in gfs2_evict_inode
2016-06-16 15:11 ` [Cluster-devel] [GFS2 PATCH v2] GFS2: Use local iopen glock holder in gfs2_evict_inode Bob Peterson
@ 2016-06-16 15:51 ` Bob Peterson
0 siblings, 0 replies; 2+ messages in thread
From: Bob Peterson @ 2016-06-16 15:51 UTC (permalink / raw)
To: cluster-devel.redhat.com
Self-NACK. While this patch seems to fix the problem, in theory
it should not be necessary because the i_iopen_gh that may be used
by other processes should be unique to I_NEW inode. So I still have
some investigating to do here. Sorry for the noise.
Bob Peterson
Red Hat File Systems
----- Original Message -----
| Hi,
|
| I found a couple problems with the previous version of this patch
| during testing. Here is my replacement, version 2.
|
| Patch description:
|
| Before this patch, function gfs2_evict_inode unlocked the iopen
| glock (from SH), waited for completion, then locked it again in
| EXclusive mode. That's all well and good except that other processes
| (not in gfs2_evict_inode) can try to do lookups, and function
| gfs2_inode_lookup tries to lock the iopen glock in SH again. This
| second lookup can and does wipe out the holder's pid with getpid().
| The first putpid (from glock_holder_uninit) will be successful, but
| the second one will crash the kernel with:
| BUG: unable to handle kernel paging request
| This patch introduces a holder variable, io_gh, local to function
| gfs2_evict_inode, which will keep its own getpid() and subsequent
| putpid() from interfering with one another. So simultaneous inode
| lookups won't change the value out from under gfs2_evict_inode.
|
| Signed-off-by: Bob Peterson <rpeterso@redhat.com>
| ---
| fs/gfs2/super.c | 25 ++++++++++++++++---------
| 1 file changed, 16 insertions(+), 9 deletions(-)
|
| diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
| index 9b2ff353..21a8ba8 100644
| --- a/fs/gfs2/super.c
| +++ b/fs/gfs2/super.c
| @@ -1518,7 +1518,7 @@ static void gfs2_evict_inode(struct inode *inode)
| struct super_block *sb = inode->i_sb;
| struct gfs2_sbd *sdp = sb->s_fs_info;
| struct gfs2_inode *ip = GFS2_I(inode);
| - struct gfs2_holder gh;
| + struct gfs2_holder gh, iopen_gh;
| struct address_space *metamapping;
| int error;
|
| @@ -1527,6 +1527,7 @@ static void gfs2_evict_inode(struct inode *inode)
| return;
| }
|
| + memset(&iopen_gh, 0, sizeof(iopen_gh));
| if (inode->i_nlink || (sb->s_flags & MS_RDONLY))
| goto out;
|
| @@ -1555,9 +1556,15 @@ static void gfs2_evict_inode(struct inode *inode)
| test_bit(HIF_HOLDER, &ip->i_iopen_gh.gh_iflags)) {
| ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
| gfs2_glock_dq_wait(&ip->i_iopen_gh);
| - gfs2_holder_reinit(LM_ST_EXCLUSIVE, LM_FLAG_TRY_1CB | GL_NOCACHE,
| - &ip->i_iopen_gh);
| - error = gfs2_glock_nq(&ip->i_iopen_gh);
| + /* This is subtle: Now we need to uninit the i_iopen_holder,
| + but if we do that before we obtain the new reference with
| + the local holder, the uninit's glock_put will free the
| + glock, which causes the new ref. to crash. So we need to do
| + this in a very specific order. Can't use glock_nq_init. */
| + gfs2_holder_init(ip->i_iopen_gh.gh_gl, LM_ST_EXCLUSIVE,
| + LM_FLAG_TRY_1CB | GL_NOCACHE, &iopen_gh);
| + gfs2_holder_uninit(&ip->i_iopen_gh);
| + error = gfs2_glock_nq(&iopen_gh);
| if (error)
| goto out_truncate;
| }
| @@ -1610,12 +1617,12 @@ out_unlock:
| if (gfs2_rs_active(&ip->i_res))
| gfs2_rs_deltree(&ip->i_res);
|
| - if (ip->i_iopen_gh.gh_gl) {
| - if (test_bit(HIF_HOLDER, &ip->i_iopen_gh.gh_iflags)) {
| - ip->i_iopen_gh.gh_flags |= GL_NOCACHE;
| - gfs2_glock_dq_wait(&ip->i_iopen_gh);
| + if (iopen_gh.gh_gl) {
| + if (test_bit(HIF_HOLDER, &iopen_gh.gh_iflags)) {
| + iopen_gh.gh_flags |= GL_NOCACHE;
| + gfs2_glock_dq_wait(&iopen_gh);
| }
| - gfs2_holder_uninit(&ip->i_iopen_gh);
| + gfs2_holder_uninit(&iopen_gh);
| }
| gfs2_glock_dq_uninit(&gh);
| if (error && error != GLR_TRYFAILED && error != -EROFS)
|
|
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-06-16 15:51 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <801923599.20160945.1466089868074.JavaMail.zimbra@redhat.com>
2016-06-16 15:11 ` [Cluster-devel] [GFS2 PATCH v2] GFS2: Use local iopen glock holder in gfs2_evict_inode Bob Peterson
2016-06-16 15:51 ` Bob Peterson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.