When gfs2_lookup_by_inum() calls gfs2_inode_lookup() for an uncached inode, gfs2_inode_lookup() will place a new tentative inode into the inode cache before verifying that there is a valid inode at the given address. This can race with gfs2_create_inode() which doesn't check for duplicates inodes. gfs2_create_inode() will try to assign the new inode to the corresponding inode glock, and glock_set_object() will complain that the glock is still in use by gfs2_inode_lookup's tentative inode. We noticed this bug after adding commit 486408d690e1 ("gfs2: Cancel remote delete work asynchronously") which allowed delete_work_func() to race with gfs2_create_inode(), but the same race exists for open-by-handle. Fix that by switching from insert_inode_hash() to insert_inode_locked4(), which does check for duplicate inodes. We know we've just managed to to allocate the new inode, so an inode tentatively created by gfs2_inode_lookup() will eventually go away and insert_inode_locked4() will always succeed. In addition, don't flush the inode glock work anymore (this can now only make things worse) and clean up glock_{set,clear}_object for the inode glock somewhat.
Unified · Split
| @@ -707,18 +707,19 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, · 706 unmodified lines | |||
| 707 | error = gfs2_glock_get(sdp, ip->i_no_addr, &gfs2_inode_glops, CREATE, &ip->i_gl); | 707 | error = gfs2_glock_get(sdp, ip->i_no_addr, &gfs2_inode_glops, CREATE, &ip->i_gl); |
| 708 | if (error) | 708 | if (error) |
| 709 | goto fail_free_inode; | 709 | goto fail_free_inode; |
| 710 | flush_delayed_work(&ip->i_gl->gl_work); | ||
| 711 | 710 | ||
| 712 | error = gfs2_glock_get(sdp, ip->i_no_addr, &gfs2_iopen_glops, CREATE, &io_gl); | 711 | error = gfs2_glock_get(sdp, ip->i_no_addr, &gfs2_iopen_glops, CREATE, &io_gl); |
| 713 | if (error) | 712 | if (error) |
| 714 | goto fail_free_inode; | 713 | goto fail_free_inode; |
| 715 | gfs2_cancel_delete_work(io_gl); | 714 | gfs2_cancel_delete_work(io_gl); |
| 716 | 715 | ||
| 716 | error = insert_inode_locked4(inode, ip->i_no_addr, iget_test, &ip->i_no_addr); | ||
| 717 | BUG_ON(error); | ||
| 718 | |||
| 717 | error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, GL_SKIP, ghs + 1); | 719 | error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, GL_SKIP, ghs + 1); |
| 718 | if (error) | 720 | if (error) |
| 719 | goto fail_gunlock2; | 721 | goto fail_gunlock2; |
| 720 | 722 | ||
| 721 | glock_set_object(ip->i_gl, ip); | ||
| 722 | error = gfs2_trans_begin(sdp, blocks, 0); | 723 | error = gfs2_trans_begin(sdp, blocks, 0); |
| 723 | if (error) | 724 | if (error) |
| 724 | goto fail_gunlock2; | 725 | goto fail_gunlock2; |
| @@ -734,9 +735,9 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, · 9 unmodified lines | |||
| 734 | if (error) | 735 | if (error) |
| 735 | goto fail_gunlock2; | 736 | goto fail_gunlock2; |
| 736 | 737 | ||
| 738 | glock_set_object(ip->i_gl, ip); | ||
| 737 | glock_set_object(io_gl, ip); | 739 | glock_set_object(io_gl, ip); |
| 738 | gfs2_set_iop(inode); | 740 | gfs2_set_iop(inode); |
| 739 | insert_inode_hash(inode); | ||
| 740 | 741 | ||
| 741 | free_vfs_inode = 0; /* After this point, the inode is no longer | 742 | free_vfs_inode = 0; /* After this point, the inode is no longer |
| 742 | considered free. Any failures need to undo | 743 | considered free. Any failures need to undo |
| @@ -778,17 +779,17 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, · 35 unmodified lines | |||
| 778 | gfs2_glock_dq_uninit(ghs + 1); | 779 | gfs2_glock_dq_uninit(ghs + 1); |
| 779 | gfs2_glock_put(io_gl); | 780 | gfs2_glock_put(io_gl); |
| 780 | gfs2_qa_put(dip); | 781 | gfs2_qa_put(dip); |
| 782 | unlock_new_inode(inode); | ||
| 781 | return error; | 783 | return error; |
| 782 | 784 | ||
| 783 | fail_gunlock3: | 785 | fail_gunlock3: |
| 786 | glock_clear_object(ip->i_gl, ip); | ||
| 784 | glock_clear_object(io_gl, ip); | 787 | glock_clear_object(io_gl, ip); |
| 785 | gfs2_glock_dq_uninit(&ip->i_iopen_gh); | 788 | gfs2_glock_dq_uninit(&ip->i_iopen_gh); |
| 786 | fail_gunlock2: | 789 | fail_gunlock2: |
| 787 | glock_clear_object(io_gl, ip); | ||
| 788 | gfs2_glock_put(io_gl); | 790 | gfs2_glock_put(io_gl); |
| 789 | fail_free_inode: | 791 | fail_free_inode: |
| 790 | if (ip->i_gl) { | 792 | if (ip->i_gl) { |
| 791 | glock_clear_object(ip->i_gl, ip); | ||
| 792 | if (free_vfs_inode) /* else evict will do the put for us */ | 793 | if (free_vfs_inode) /* else evict will do the put for us */ |
| 793 | gfs2_glock_put(ip->i_gl); | 794 | gfs2_glock_put(ip->i_gl); |
| 794 | } | 795 | } |
| @@ -806,7 +807,10 @@ fail_gunlock: · 11 unmodified lines | |||
| 806 | mark_inode_dirty(inode); | 807 | mark_inode_dirty(inode); |
| 807 | set_bit(free_vfs_inode ? GIF_FREE_VFS_INODE : GIF_ALLOC_FAILED, | 808 | set_bit(free_vfs_inode ? GIF_FREE_VFS_INODE : GIF_ALLOC_FAILED, |
| 808 | &GFS2_I(inode)->i_flags); | 809 | &GFS2_I(inode)->i_flags); |
| 809 | iput(inode); | 810 | if (inode->i_state & I_NEW) |
| 811 | iget_failed(inode); | ||
| 812 | else | ||
| 813 | iput(inode); | ||
| 810 | } | 814 | } |
| 811 | if (gfs2_holder_initialized(ghs + 1)) | 815 | if (gfs2_holder_initialized(ghs + 1)) |
| 812 | gfs2_glock_dq_uninit(ghs + 1); | 816 | gfs2_glock_dq_uninit(ghs + 1); |