gfs2: Fix and clean up create / evict interaction

When gfs2_create_inode() fails after creating a new inode, it uses the
GIF_FREE_VFS_INODE and GIF_ALLOC_FAILED inode flags to communicate to
gfs2_evict_inode() which parts of the inode need to be deallocated and
destroyed.  In some error cases, the inode ends up being allocated on
disk and then accidentally left behind.  In others, the inode is
partially constructed and then not properly destroyed.  Clean this up by
completely handling the inode deallocation and destruction in
gfs2_evict_inode().

This means that gfs2_evict_inode() may now be faced with partially
constructed inodes, so add the necessary checks to cope with that.  In
particular, make sure that for incompletely constructed inodes, we're
not accessing the buffers backing the on-disk blocks; the contents may
be undefined.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index b91f15a..c057f3b 100644 (file)
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -409,6 +409,8 @@ static int alloc_dinode(struct gfs2_inode *ip, u32 flags, unsigned *dblocks)
        ip->i_no_formal_ino = ip->i_generation;
        ip->i_inode.i_ino = ip->i_no_addr;
        ip->i_goal = ip->i_no_addr;
+       if (*dblocks > 1)
+               ip->i_eattr = ip->i_no_addr + 1;
 
 out_trans_end:
        gfs2_trans_end(sdp);
@@ -589,6 +591,12 @@ static int gfs2_initxattrs(struct inode *inode, const struct xattr *xattr_array,
  * @size: The initial size of the inode (ignored for directories)
  * @excl: Force fail if inode exists
  *
+ * FIXME: Change to allocate the disk blocks and write them out in the same
+ * transaction.  That way, we can no longer end up in a situation in which an
+ * inode is allocated, the node crashes, and the block looks like a valid
+ * inode.  (With atomic creates in place, we will also no longer need to zero
+ * the link count and dirty the inode here on failure.)
+ *
  * Returns: 0 on success, or error code
  */
 
@@ -604,7 +612,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
        struct gfs2_inode *dip = GFS2_I(dir), *ip;
        struct gfs2_sbd *sdp = GFS2_SB(&dip->i_inode);
        struct gfs2_glock *io_gl;
-       int error, free_vfs_inode = 1;
+       int error;
        u32 aflags = 0;
        unsigned blocks = 1;
        struct gfs2_diradd da = { .bh = NULL, .save_loc = 1, };
@@ -742,10 +750,8 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
        if (error)
                goto fail_gunlock3;
 
-       if (blocks > 1) {
-               ip->i_eattr = ip->i_no_addr + 1;
+       if (blocks > 1)
                gfs2_init_xattr(ip);
-       }
        init_dinode(dip, ip, symname);
        gfs2_trans_end(sdp);
 
@@ -753,9 +759,6 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
        glock_set_object(io_gl, ip);
        gfs2_set_iop(inode);
 
-       free_vfs_inode = 0; /* After this point, the inode is no longer
-                              considered free. Any failures need to undo
-                              the gfs2 structures. */
        if (default_acl) {
                error = __gfs2_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
                if (error)
@@ -804,10 +807,6 @@ fail_gunlock3:
 fail_gunlock2:
        gfs2_glock_put(io_gl);
 fail_free_inode:
-       if (ip->i_gl) {
-               if (free_vfs_inode) /* else evict will do the put for us */
-                       gfs2_glock_put(ip->i_gl);
-       }
        gfs2_rs_deltree(&ip->i_res);
        gfs2_qa_put(ip);
 fail_free_acls:
@@ -817,11 +816,10 @@ fail_gunlock:
        gfs2_dir_no_add(&da);
        gfs2_glock_dq_uninit(&d_gh);
        if (!IS_ERR_OR_NULL(inode)) {
+               set_bit(GIF_ALLOC_FAILED, &ip->i_flags);
                clear_nlink(inode);
-               if (!free_vfs_inode)
+               if (ip->i_no_addr)
                        mark_inode_dirty(inode);
-               set_bit(free_vfs_inode ? GIF_FREE_VFS_INODE : GIF_ALLOC_FAILED,
-                       &ip->i_flags);
                if (inode->i_state & I_NEW)
                        iget_failed(inode);
                else

diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c
index 6ed728a..3c41b86 100644 (file)
--- a/fs/gfs2/meta_io.c
+++ b/fs/gfs2/meta_io.c
@@ -442,6 +442,12 @@ void gfs2_journal_wipe(struct gfs2_inode *ip, u64 bstart, u32 blen)
        struct buffer_head *bh;
        int ty;
 
+       if (!ip->i_gl) {
+               /* This can only happen during incomplete inode creation. */
+               BUG_ON(!test_bit(GIF_ALLOC_FAILED, &ip->i_flags));
+               return;
+       }
+
        gfs2_ail1_wipe(sdp, bstart, blen);
        while (blen) {
                ty = REMOVE_META;

diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index b018957..eac9b0c 100644 (file)
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -475,6 +475,12 @@ static void gfs2_dirty_inode(struct inode *inode, int flags)
        int need_endtrans = 0;
        int ret;
 
+       if (unlikely(!ip->i_gl)) {
+               /* This can only happen during incomplete inode creation. */
+               BUG_ON(!test_bit(GIF_ALLOC_FAILED, &ip->i_flags));
+               return;
+       }
+
        if (unlikely(gfs2_withdrawn(sdp)))
                return;
        if (!gfs2_glock_is_locked_by_me(ip->i_gl)) {
@@ -927,8 +933,7 @@ static int gfs2_drop_inode(struct inode *inode)
 {
        struct gfs2_inode *ip = GFS2_I(inode);
 
-       if (!test_bit(GIF_FREE_VFS_INODE, &ip->i_flags) &&
-           inode->i_nlink &&
+       if (inode->i_nlink &&
            gfs2_holder_initialized(&ip->i_iopen_gh)) {
                struct gfs2_glock *gl = ip->i_iopen_gh.gh_gl;
                if (test_bit(GLF_DEMOTE, &gl->gl_flags))
@@ -1076,7 +1081,13 @@ static void gfs2_final_release_pages(struct gfs2_inode *ip)
        struct inode *inode = &ip->i_inode;
        struct gfs2_glock *gl = ip->i_gl;
 
-       truncate_inode_pages(gfs2_glock2aspace(ip->i_gl), 0);
+       if (unlikely(!gl)) {
+               /* This can only happen during incomplete inode creation. */
+               BUG_ON(!test_bit(GIF_ALLOC_FAILED, &ip->i_flags));
+               return;
+       }
+
+       truncate_inode_pages(gfs2_glock2aspace(gl), 0);
        truncate_inode_pages(&inode->i_data, 0);
 
        if (atomic_read(&gl->gl_revokes) == 0) {
@@ -1218,10 +1229,8 @@ static enum dinode_demise evict_should_delete(struct inode *inode,
        struct gfs2_sbd *sdp = sb->s_fs_info;
        int ret;
 
-       if (test_bit(GIF_ALLOC_FAILED, &ip->i_flags)) {
-               BUG_ON(!gfs2_glock_is_locked_by_me(ip->i_gl));
+       if (unlikely(test_bit(GIF_ALLOC_FAILED, &ip->i_flags)))
                goto should_delete;
-       }
 
        if (test_bit(GIF_DEFERRED_DELETE, &ip->i_flags))
                return SHOULD_DEFER_EVICTION;
@@ -1298,9 +1307,11 @@ static int evict_unlinked_inode(struct inode *inode)
           do, gfs2_create_inode can create another inode at the same block
           location and try to set gl_object again. We clear gl_object here so
           that subsequent inode creates don't see an old gl_object. */
-       glock_clear_object(ip->i_gl, ip);
+       if (ip->i_gl) {
+               glock_clear_object(ip->i_gl, ip);
+               gfs2_inode_remember_delete(ip->i_gl, ip->i_no_formal_ino);
+       }
        ret = gfs2_dinode_dealloc(ip);
-       gfs2_inode_remember_delete(ip->i_gl, ip->i_no_formal_ino);
 out:
        return ret;
 }
@@ -1367,12 +1378,7 @@ static void gfs2_evict_inode(struct inode *inode)
        struct gfs2_holder gh;
        int ret;
 
-       if (test_bit(GIF_FREE_VFS_INODE, &ip->i_flags)) {
-               clear_inode(inode);
-               return;
-       }
-
-       if (inode->i_nlink || sb_rdonly(sb))
+       if (inode->i_nlink || sb_rdonly(sb) || !ip->i_no_addr)
                goto out;
 
        gfs2_holder_mark_uninitialized(&gh);
@@ -1429,6 +1435,7 @@ static struct inode *gfs2_alloc_inode(struct super_block *sb)
        ip = alloc_inode_sb(sb, gfs2_inode_cachep, GFP_KERNEL);
        if (!ip)
                return NULL;
+       ip->i_no_addr = 0;
        ip->i_flags = 0;
        ip->i_gl = NULL;
        gfs2_holder_mark_uninitialized(&ip->i_iopen_gh);

diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
index f6a6605..518c067 100644 (file)
--- a/fs/gfs2/xattr.c
+++ b/fs/gfs2/xattr.c
@@ -1412,11 +1412,13 @@ static int ea_dealloc_block(struct gfs2_inode *ip)
        ip->i_eattr = 0;
        gfs2_add_inode_blocks(&ip->i_inode, -1);
 
-       error = gfs2_meta_inode_buffer(ip, &dibh);
-       if (!error) {
-               gfs2_trans_add_meta(ip->i_gl, dibh);
-               gfs2_dinode_out(ip, dibh->b_data);
-               brelse(dibh);
+       if (likely(!test_bit(GIF_ALLOC_FAILED, &ip->i_flags))) {
+               error = gfs2_meta_inode_buffer(ip, &dibh);
+               if (!error) {
+                       gfs2_trans_add_meta(ip->i_gl, dibh);
+                       gfs2_dinode_out(ip, dibh->b_data);
+                       brelse(dibh);
+               }
        }
 
        gfs2_trans_end(sdp);
@@ -1445,14 +1447,16 @@ int gfs2_ea_dealloc(struct gfs2_inode *ip)
        if (error)
                return error;
 
-       error = ea_foreach(ip, ea_dealloc_unstuffed, NULL);
-       if (error)
-               goto out_quota;
-
-       if (ip->i_diskflags & GFS2_DIF_EA_INDIRECT) {
-               error = ea_dealloc_indirect(ip);
+       if (likely(!test_bit(GIF_ALLOC_FAILED, &ip->i_flags))) {
+               error = ea_foreach(ip, ea_dealloc_unstuffed, NULL);
                if (error)
                        goto out_quota;
+
+               if (ip->i_diskflags & GFS2_DIF_EA_INDIRECT) {
+                       error = ea_dealloc_indirect(ip);
+                       if (error)
+                               goto out_quota;
+               }
        }
 
        error = ea_dealloc_block(ip);