* [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.