[Cluster-devel] [GFS2] Don't use journal lock type

[Cluster-devel] [GFS2] Don't use journal lock type

[Steven Whitehouse]

Hi,

In GFS2 journals are just normal inodes, so we should use the normal
inode lock type rather than the journal lock type,

Steve.

-------------------------------------------------------------------------
diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index 777ca46..976a395 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -488,7 +488,3 @@ const struct gfs2_glock_operations gfs2_quota_glops = {
 	.go_type = LM_TYPE_QUOTA,
 };
 
-const struct gfs2_glock_operations gfs2_journal_glops = {
-	.go_type = LM_TYPE_JOURNAL,
-};
-
diff --git a/fs/gfs2/glops.h b/fs/gfs2/glops.h
index a1d9b5b..cddac16 100644
--- a/fs/gfs2/glops.h
+++ b/fs/gfs2/glops.h
@@ -20,6 +20,5 @@ extern const struct gfs2_glock_operations gfs2_iopen_glops;
 extern const struct gfs2_glock_operations gfs2_flock_glops;
 extern const struct gfs2_glock_operations gfs2_nondisk_glops;
 extern const struct gfs2_glock_operations gfs2_quota_glops;
-extern const struct gfs2_glock_operations gfs2_journal_glops;
 
 #endif /* __GLOPS_DOT_H__ */
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 58c730b..ba1bb05 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -348,7 +348,7 @@ static int init_journal(struct gfs2_sbd *sdp, int undo)
 		sdp->sd_jdesc = gfs2_jdesc_find(sdp, sdp->sd_lockstruct.ls_jid);
 
 		error = gfs2_glock_nq_num(sdp, sdp->sd_lockstruct.ls_jid,
-					  &gfs2_journal_glops,
+					  &gfs2_inode_glops,
 					  LM_ST_EXCLUSIVE, LM_FLAG_NOEXP,
 					  &sdp->sd_journal_gh);
 		if (error) {
diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
index 5ada38c..aaac6a1 100644
--- a/fs/gfs2/recovery.c
+++ b/fs/gfs2/recovery.c
@@ -452,7 +452,7 @@ int gfs2_recover_journal(struct gfs2_jdesc *jd)
 
 		/* Aquire the journal lock so we can do recovery */
 
-		error = gfs2_glock_nq_num(sdp, jd->jd_jid, &gfs2_journal_glops,
+		error = gfs2_glock_nq_num(sdp, jd->jd_jid, &gfs2_inode_glops,
 					  LM_ST_EXCLUSIVE,
 					  LM_FLAG_NOEXP | LM_FLAG_TRY | GL_NOCACHE,
 					  &j_gh);

[Cluster-devel] [GFS2] Don't use journal lock type

[David Teigland]

On Tue, Aug 07, 2007 at 10:26:06AM +0100, Steven Whitehouse wrote:
>  		error = gfs2_glock_nq_num(sdp, sdp->sd_lockstruct.ls_jid,
> -					  &gfs2_journal_glops,
> +					  &gfs2_inode_glops,
>  					  LM_ST_EXCLUSIVE, LM_FLAG_NOEXP,
>  					  &sdp->sd_journal_gh);

The lock for journal N is now the same as the lock for the inode at
block N -- that doesn't work.

[Cluster-devel] [GFS2] Don't use journal lock type

[Steven Whitehouse]

Hi,

On Tue, 2007-08-07 at 08:45 -0500, David Teigland wrote:
> On Tue, Aug 07, 2007 at 10:26:06AM +0100, Steven Whitehouse wrote:
> >  		error = gfs2_glock_nq_num(sdp, sdp->sd_lockstruct.ls_jid,
> > -					  &gfs2_journal_glops,
> > +					  &gfs2_inode_glops,
> >  					  LM_ST_EXCLUSIVE, LM_FLAG_NOEXP,
> >  					  &sdp->sd_journal_gh);
> 
> The lock for journal N is now the same as the lock for the inode at
> block N -- that doesn't work.
> 

Yes, thats the point of the patch, after all journals are now inodes. If
we don't do that then two things happen:

 - Accesses of the journal inodes via the meta fs will not be "in sync"
with what the kernel sees
 - More importantly, and the reason why this patch was created is that
upon recovery of a journal, the cache wasn't being flushed resulting in
the blocks being sometimes "seen" again the next time a node recovers
the same remote journal.

Steve.

[Cluster-devel] [GFS2] Don't use journal lock type

[David Teigland]

On Tue, Aug 07, 2007 at 02:52:31PM +0100, Steven Whitehouse wrote:
> Hi,
> 
> On Tue, 2007-08-07 at 08:45 -0500, David Teigland wrote:
> > On Tue, Aug 07, 2007 at 10:26:06AM +0100, Steven Whitehouse wrote:
> > >  		error = gfs2_glock_nq_num(sdp, sdp->sd_lockstruct.ls_jid,
> > > -					  &gfs2_journal_glops,
> > > +					  &gfs2_inode_glops,
> > >  					  LM_ST_EXCLUSIVE, LM_FLAG_NOEXP,
> > >  					  &sdp->sd_journal_gh);
> > 
> > The lock for journal N is now the same as the lock for the inode at
> > block N -- that doesn't work.
> > 
> 
> Yes, thats the point of the patch, after all journals are now inodes. If
> we don't do that then two things happen:
> 
>  - Accesses of the journal inodes via the meta fs will not be "in sync"
> with what the kernel sees
>  - More importantly, and the reason why this patch was created is that
> upon recovery of a journal, the cache wasn't being flushed resulting in
> the blocks being sometimes "seen" again the next time a node recovers
> the same remote journal.

The journal with id N does not live in the inode at fs block N.  So,
you're still using two different locks for the journal, and one of them
now collides with something else.

[Cluster-devel] [GFS2] Don't use journal lock type

[Steven Whitehouse]

Hi,

On Tue, 2007-08-07 at 09:02 -0500, David Teigland wrote:
> On Tue, Aug 07, 2007 at 02:52:31PM +0100, Steven Whitehouse wrote:
> > Hi,
> > 
> > On Tue, 2007-08-07 at 08:45 -0500, David Teigland wrote:
> > > On Tue, Aug 07, 2007 at 10:26:06AM +0100, Steven Whitehouse wrote:
> > > >  		error = gfs2_glock_nq_num(sdp, sdp->sd_lockstruct.ls_jid,
> > > > -					  &gfs2_journal_glops,
> > > > +					  &gfs2_inode_glops,
> > > >  					  LM_ST_EXCLUSIVE, LM_FLAG_NOEXP,
> > > >  					  &sdp->sd_journal_gh);
> > > 
> > > The lock for journal N is now the same as the lock for the inode at
> > > block N -- that doesn't work.
> > > 
> > 
> > Yes, thats the point of the patch, after all journals are now inodes. If
> > we don't do that then two things happen:
> > 
> >  - Accesses of the journal inodes via the meta fs will not be "in sync"
> > with what the kernel sees
> >  - More importantly, and the reason why this patch was created is that
> > upon recovery of a journal, the cache wasn't being flushed resulting in
> > the blocks being sometimes "seen" again the next time a node recovers
> > the same remote journal.
> 
> The journal with id N does not live in the inode at fs block N.  So,
> you're still using two different locks for the journal, and one of them
> now collides with something else.
> 
I see now! Well spotted, and I'll send a new patch shortly to use the
inode's own lock (which is available in both places anyway),

Steve.