GFS2: kernel changes to support new gfs2_grow command (Try 3)
PATCH 1/1 GFS2: kernel changes to support new gfs2_grow command (Try 3)
Quoted 23 lines · Robert Peterson
Quoted 5 lines · David Teigland
Quoted 4 lines · Robert Peterson
(quoted diff) +0
-0@@ -447,7 +479,12 @@ static int gfs2_ri_update(struct gfs2_inode *ip) u64 junk = ip->i_di.di_size; int error;Quoted 16 lines
Review static int gfs2_ri_update(struct gfs2_inode *ip) · line 490
Quoted 10 lines
(quoted diff) +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_holdtakes 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.
Quoted 6 lines
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_updatewhile there are still partial rindex entries. This can happen when we need to allocate a new page to the rindex, which callsgfs2_inplace_reserve_i, which eventually gets togfs2_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