From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Teigland <teigland@redhat.com>
Date: Tue, 8 May 2007 15:43:05 -0500
Subject: [Cluster-devel] [PATCH] GFS2: kernel changes to support new
	gfs2_grow command (Try 3)
Message-ID: <20070508204305.GC4030@redhat.com>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

On Tue, May 08, 2007 at 09:56:05AM -0500, Robert Peterson wrote:
> David Teigland wrote:
> >On Wed, May 02, 2007 at 08:57:08PM -0500, Robert Peterson wrote:
> >>@@ -447,7 +479,12 @@ static int gfs2_ri_update(struct gfs2_inode *ip)
> >>	u64 junk = ip->i_di.di_size;
> >>	int error;
> >>
> >>-	if (do_div(junk, sizeof(struct gfs2_rindex))) {
> >>+	/* If someone is holding the rindex file with a glock, they must
> >>+	   be updating it, in which case we may have partial entries.
> >>+	   In this case, we ignore the partials. */
> >>+	if (!gfs2_glock_is_held_excl(ip->i_gl) &&
> >>+	    !gfs2_glock_is_held_shrd(ip->i_gl) &&
> >>+	    do_div(junk, sizeof(struct gfs2_rindex))) {
> >>		gfs2_consist_inode(ip);
> >>		return -EIO;
> >>	}
> >
> >So the use of glock_is_held _is_ part of an assertion, not part of an
> >algorithm which I was worried about before.  We should only ever get to
> >this spot with a shared glock, right? (rindex_hold takes it).  So a plain
> >old assertion that the glock is shared at the beginning would be ok, but
> >this particular check doesn't make sense to me.
> 
> For the sake of completeness, I retested without this change to make
> sure it was also still necessary.  It was.  The problem is that
> we can now call gfs2_ri_update while there are still partial rindex 
> entries.  This can happen when we need to allocate a new page to the
> rindex, which calls gfs2_inplace_reserve_i, which eventually gets to 
> gfs2_check_rindex_version.  Without the change, you get:

+       else if (!sdp->sd_rgrps) /* We may not have the rindex read in, so: */
+               error = gfs2_check_rindex_version(sdp);

This is a very special exception; instead of using this more general
purpose function (gfs2_check_rindex_version) for the purpose of reading in
the rgrps, and then being required to add the further special case checks
up above -- I suggest using a new function here that just reads in the
rgrps without the checks above that you've had to modify.

The path I see us going down here is adding a special-case-exception in
one spot, then finding that it's not quite right, so adding another
special-case-exception on top of it ... and so on.  It becomes a house of
cards very quickly that way.

Dave