GFS2: kernel changes to support new gfs2_grow command (Try 3)

fs/gfs2/ops_address.c out_uninit: · line 461

Quoted 1 line · Robert Peterson

fs/gfs2/ops_address.c +1 -0

+	__u64 fs_total, new_free, new_free_readable;

use u64, __u64 is for structs shared with user space

fs/gfs2/ops_address.c out_uninit: · line 482

Quoted 3 lines

fs/gfs2/ops_address.c +3 -0

+	printk(KERN_WARNING "GFS2: File system extended by %llu "
+	       "blocks (%llu%cB)\n", new_free, new_free_readable,
+	       stg_abbrev[factor]);

use fs_warn(), and unless there's some precedent elsewhere I don't think the human-readable translation is appropriate in the kernel.

fs/gfs2/rgrp.c static int gfs2_ri_update(struct gfs2_inode *ip) · line 486

Quoted 2 lines

fs/gfs2/rgrp.c +2 -0

+	if (!gfs2_glock_is_held_excl(ip->i_gl) &&
+	    !gfs2_glock_is_held_shrd(ip->i_gl) &&

Be extremely wary of using these "is_held" functions algorithmically; needing to use them is usually an indication that you're doing something wrong (I'll have to study this case a bit more to say anything helpful).

It's instructional to note that the glock_is_held functions are _never_ used outside of assertions in all of gfs1 and gfs2! There's an exception to every rule, but...

Dave

Quoted 6 lines

fs/gfs2/rgrp.c static int gfs2_ri_update(struct gfs2_inode *ip) · line 486

Quoted 2 lines

fs/gfs2/rgrp.c +2 -0

+	if (!gfs2_glock_is_held_excl(ip->i_gl) &&
+	    !gfs2_glock_is_held_shrd(ip->i_gl) &&

Be extremely wary of using these "is_held" functions algorithmically; needing to use them is usually an indication that you're doing something wrong (I'll have to study this case a bit more to say anything helpful).

I was using these functions to determine if someone was doing writes to the rindex file through the meta_fs, in which case I don't want to exit with a consistency error. If the rindex is being written to, there may be partial pieces of rindex entries left on a page (i.e. between two gfs2_write_commits) before the writing is over. In these cases, it's normal and we should ignore the partial entries (and re-read them when the writing is complete and the version number has changed).

My concern here is if a different node decides to do an ri_update while a gfs2_grow is happening. For example, if someone does a mount of the file system on a different node, the rindex may briefly be in an inconsistent state.

* gfs2_ri_update - Pull in a new resource index from the disk
* @gl: The glock covering the rindex inode
*

fs/gfs2/rgrp.c +0 -0

@@ -447,7 +479,12 @@ static int gfs2_ri_update(struct gfs2_inode *ip)
 	u64 junk = ip->i_di.di_size;
 	int error;
 

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.

Why is this needed now if it wasn't before?

Quoted 4 lines · Robert Peterson

fs/gfs2/rgrp.c +0 -0

@@ -447,7 +479,12 @@ static int gfs2_ri_update(struct gfs2_inode *ip)
 	u64 junk = ip->i_di.di_size;
 	int error;
 

fs/gfs2/rgrp.c static int gfs2_ri_update(struct gfs2_inode *ip) · line 490

Quoted 10 lines

fs/gfs2/rgrp.c +6 -1

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

fs/gfs2/rgrp.c static int gfs2_ri_update(struct gfs2_inode *ip) · line 499

Quoted 7 lines

fs/gfs2/rgrp.c +3 -0

@@ -457,6 +494,9 @@ static int gfs2_ri_update(struct gfs2_inode *ip)
 	file_ra_state_init(&ra_state, inode->i_mapping);
 	for (sdp->sd_rgrps = 0;; sdp->sd_rgrps++) {
 		loff_t pos = sdp->sd_rgrps * sizeof(struct gfs2_rindex);
+
+		if (pos + sizeof(struct gfs2_rindex) >= ip->i_di.di_size)
+			break;

Why is this needed now if it wasn't before?

fs/gfs2/rgrp.c int gfs2_inplace_reserve_i(struct gfs2_inode *ip, · line 1045

Quoted 10 lines · Robert Peterson

fs/gfs2/rgrp.c +7 -1

@@ -978,18 +1038,25 @@ int gfs2_inplace_reserve_i(struct gfs2_inode *ip, 
 
-	error = gfs2_rindex_hold(sdp, &al->al_ri_gh);
+	/* We need to hold the rindex unless the inode we're using is
+	   the rindex itself, in which case it's already held. */
+	if (ip != GFS2_I(sdp->sd_rindex))
+		error = gfs2_rindex_hold(sdp, &al->al_ri_gh);
+	else if (!sdp->sd_rgrps) /* We may not have the rindex read in, so: */
+		error = gfs2_check_rindex_version(sdp);
+

I don't see why we need this change, the original looks correct.

So there are the two distinct parts to the fs-grow procedure. The first is writing to the rindex and statfs files -- this is what was missing in gfs2, and I'm ignoring that part for now.

The second part is gfs detecting that a grow took place and updating its list of rg's. I don't think this part needs any changing at all, it should work the same way it always has -- this is what I'm focussing on.

Quoted 14 lines · David Teigland

fs/gfs2/rgrp.c int gfs2_inplace_reserve_i(struct gfs2_inode *ip, · line 1046

Quoted 11 lines · Robert Peterson

fs/gfs2/rgrp.c +7 -1

@@ -978,18 +1038,25 @@ int gfs2_inplace_reserve_i(struct gfs2_inode *ip, 
 
-	error = gfs2_rindex_hold(sdp, &al->al_ri_gh);
+	/* We need to hold the rindex unless the inode we're using is
+	   the rindex itself, in which case it's already held. */
+	if (ip != GFS2_I(sdp->sd_rindex))
+		error = gfs2_rindex_hold(sdp, &al->al_ri_gh);
+	else if (!sdp->sd_rgrps) /* We may not have the rindex read in, so: 
 */
+		error = gfs2_check_rindex_version(sdp);
+

I don't see why we need this change, the original looks correct.

If we're updating through the meta_fs, the rindex file will already be glocked and held through its inode.

Quoted 13 lines · David Teigland

fs/gfs2/rgrp.c int gfs2_inplace_reserve_i(struct gfs2_inode *ip, char *file, unsigned int line) · line 1052

Quoted 11 lines · Robert Peterson

fs/gfs2/rgrp.c +5 -1

@@ -978,18 +1038,25 @@ int gfs2_inplace_reserve_i(struct gfs2_inode *ip, 
 
-    error = gfs2_rindex_hold(sdp, &al->al_ri_gh);
+    /* We need to hold the rindex unless the inode we're using is
+       the rindex itself, in which case it's already held. */
+    if (ip != GFS2_I(sdp->sd_rindex))
+        error = gfs2_rindex_hold(sdp, &al->al_ri_gh);
+    else if (!sdp->sd_rgrps) /* We may not have the rindex read in, 

so: */

fs/gfs2/rgrp.c +2 -0

+        error = gfs2_check_rindex_version(sdp);
+

Quoted 1 line

I don't see why we need this change, the original looks correct.

If we're updating through the meta_fs, the rindex file will already be glocked and held through its inode.

Quoted 7 lines

So there are the two distinct parts to the fs-grow procedure. The first is writing to the rindex and statfs files -- this is what was missing in gfs2, and I'm ignoring that part for now.

The second part is gfs detecting that a grow took place and updating its list of rg's. I don't think this part needs any changing at all, it should work the same way it always has -- this is what I'm focussing on.

I'll re-test test the original code path to make sure that this part of the fix is still really necessary.

Regards,

Bob Peterson
Red Hat Cluster Suite

Quoted 27 lines · Robert Peterson

Quoted 13 lines · David Teigland

fs/gfs2/rgrp.c int gfs2_inplace_reserve_i(struct gfs2_inode *ip, char *file, unsigned int line) · line 1052

Quoted 11 lines · Robert Peterson

fs/gfs2/rgrp.c +5 -1

@@ -978,18 +1038,25 @@ int gfs2_inplace_reserve_i(struct gfs2_inode *ip, 
 
-    error = gfs2_rindex_hold(sdp, &al->al_ri_gh);
+    /* We need to hold the rindex unless the inode we're using is
+       the rindex itself, in which case it's already held. */
+    if (ip != GFS2_I(sdp->sd_rindex))
+        error = gfs2_rindex_hold(sdp, &al->al_ri_gh);
+    else if (!sdp->sd_rgrps) /* We may not have the rindex read in, 

so: */

fs/gfs2/rgrp.c +2 -0

+        error = gfs2_check_rindex_version(sdp);
+

Quoted 1 line

I don't see why we need this change, the original looks correct.

If we're updating through the meta_fs, the rindex file will already be glocked and held through its inode.

Quoted 7 lines

So there are the two distinct parts to the fs-grow procedure. The first is writing to the rindex and statfs files -- this is what was missing in gfs2, and I'm ignoring that part for now.

The second part is gfs detecting that a grow took place and updating its list of rg's. I don't think this part needs any changing at all, it should work the same way it always has -- this is what I'm focussing on.

Quoted 8 lines

I'll re-test test the original code path to make sure that this part of the fix is still really necessary.

Regards,

Bob Peterson
Red Hat Cluster Suite

Hi Dave,

Well, I retested this code path and found that this code is still necessary (unless I should somehow be doing this a different way). Without the code, the gfs2_rindex_hold tries to add a holder, but there's already a holder due to the meta_fs. Under the right conditions, you get:

original: gfs2_prepare_write+0x49/0x237 [gfs2]
new: gfs2_rindex_hold+0x2b/0x52a [gfs2]