[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Cluster-devel] [PATCH] GFS2: kernel changes to support new gfs2_grow command (Try 3)



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:

GFS2: fsid=bob_cluster2:test_gfs.0: fatal: filesystem consistency error
GFS2: fsid=bob_cluster2:test_gfs.0:   inode = 19 25439
GFS2: fsid=bob_cluster2:test_gfs.0:   function = gfs2_ri_update, file = fs/gfs2/rgrp.c, line = 486
GFS2: fsid=bob_cluster2:test_gfs.0: about to withdraw this file system
GFS2: fsid=bob_cluster2:test_gfs.0: telling LM to withdraw
GFS2: fsid=bob_cluster2:test_gfs.0: withdrawn
[<e02a5c2f>] gfs2_lm_withdraw+0x82/0x8d [gfs2]
[<e02b72d0>] gfs2_consist_inode_i+0x6f/0x75 [gfs2]
[<e02b34da>] gfs2_check_rindex_version+0x11e/0x468 [gfs2]
[<c04078a5>] __sched_text_start+0x715/0x7c4
[<e029a335>] gfs2_extent_map+0x68/0x9c [gfs2]
[<e02b3dbb>] gfs2_inplace_reserve_i+0x9a/0x443 [gfs2]
[<e029a4fb>] gfs2_write_alloc_required+0x192/0x1ce [gfs2]
[<e02aad58>] gfs2_prepare_write+0x12d/0x237 [gfs2]
[<e02aac2b>] gfs2_prepare_write+0x0/0x237 [gfs2]
[<c0138422>] generic_file_buffered_write+0x25b/0x60f
[<c03b6594>] tcp_v4_do_rcv+0x28/0x307
[<c016857e>] __mark_inode_dirty+0xdd/0x15c
[<c011dbb6>] current_fs_time+0x41/0x46
[<c0138cbd>] __generic_file_aio_write_nolock+0x4e7/0x560
[<c013afef>] get_page_from_freelist+0x24f/0x2cf
[<c0138d8b>] generic_file_aio_write+0x55/0xb3
[<c01514cd>] do_sync_write+0xc7/0x10a
[<e02a3293>] gfs2_holder_uninit+0xb/0x1b [gfs2]
[<c01296cd>] autoremove_wake_function+0x0/0x35
[<e02a3293>] gfs2_holder_uninit+0xb/0x1b [gfs2]
[<e02ac3d8>] gfs2_llseek+0x76/0x9a [gfs2]
[<c0151406>] do_sync_write+0x0/0x10a
[<c0151c30>] vfs_write+0x8a/0x10c
[<c015219f>] sys_write+0x41/0x67
[<c01030d8>] sysenter_past_esp+0x5d/0x81
[<c0400000>] svc_defer+0x6b/0x126
=======================

@@ -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?

For the same reason as the above check is necessary.

Regards,

Bob Peterson
Red Hat Cluster Suite


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]