From 98e5a91a6136af01198cfe9a3499f090718fd6ff Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Wed, 19 Jul 2017 11:10:19 -0500 Subject: gfs2: Fixup to "Get rid of flush_delayed_work in gfs2_evict_inode" When commit 4fd1a57952 moved the call to flush_delayed_work from gfs2_evict_inode to gfs2_inode_lookup to avoid calling into DLM during evict, a similar call should have been added to gfs2_create_inode: that's another code path in which glocks of previous inodes may be reused. The flush of the iopen glock work queue added by 4fd1a57952, on the other hand, is unnecessary and can be removed. Signed-off-by: Andreas Gruenbacher Signed-off-by: Bob Peterson --- fs/gfs2/inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'fs/gfs2/inode.c') diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index acca501f8110..f9302f16a28e 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -174,7 +174,6 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type, error = gfs2_glock_nq_init(io_gl, LM_ST_SHARED, GL_EXACT, &ip->i_iopen_gh); if (unlikely(error)) goto fail_put; - flush_delayed_work(&ip->i_iopen_gh.gh_gl->gl_work); glock_set_object(ip->i_iopen_gh.gh_gl, ip); gfs2_glock_put(io_gl); io_gl = NULL; @@ -706,8 +705,9 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, error = gfs2_glock_get(sdp, ip->i_no_addr, &gfs2_inode_glops, CREATE, &ip->i_gl); if (error) goto fail_free_inode; - + flush_delayed_work(&ip->i_gl->gl_work); glock_set_object(ip->i_gl, ip); + error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, GL_SKIP, ghs + 1); if (error) goto fail_free_inode; -- cgit v1.2.3 From df3d87bde121213560fde0edb71bc46f0f75692c Mon Sep 17 00:00:00 2001 From: Bob Peterson Date: Tue, 18 Jul 2017 11:35:04 -0500 Subject: GFS2: Introduce helper for clearing gl_object This patch introduces a new helper function in glock.h that clears gl_object, with an added integrity check. An additional integrity check has been added to glock_set_object, plus comments. This is step 1 in a series to ensure gl_object integrity. Signed-off-by: Bob Peterson Reviewed-by: Andreas Gruenbacher --- fs/gfs2/glock.h | 34 ++++++++++++++++++++++++++++++++++ fs/gfs2/inode.c | 4 ++-- fs/gfs2/super.c | 4 ++-- 3 files changed, 38 insertions(+), 4 deletions(-) (limited to 'fs/gfs2/inode.c') diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h index 9ad4a6ac6c84..526d2123f758 100644 --- a/fs/gfs2/glock.h +++ b/fs/gfs2/glock.h @@ -13,6 +13,7 @@ #include #include #include "incore.h" +#include "util.h" /* Options for hostdata parser */ @@ -257,11 +258,44 @@ static inline bool gfs2_holder_initialized(struct gfs2_holder *gh) return gh->gh_gl; } +/** + * glock_set_object - set the gl_object field of a glock + * @gl: the glock + * @object: the object + */ static inline void glock_set_object(struct gfs2_glock *gl, void *object) { spin_lock(&gl->gl_lockref.lock); + if (gfs2_assert_warn(gl->gl_name.ln_sbd, gl->gl_object == NULL)) + gfs2_dump_glock(NULL, gl); gl->gl_object = object; spin_unlock(&gl->gl_lockref.lock); } +/** + * glock_clear_object - clear the gl_object field of a glock + * @gl: the glock + * @object: the object + * + * I'd love to similarly add this: + * else if (gfs2_assert_warn(gl->gl_sbd, gl->gl_object == object)) + * gfs2_dump_glock(NULL, gl); + * Unfortunately, that's not possible because as soon as gfs2_delete_inode + * frees the block in the rgrp, another process can reassign it for an I_NEW + * inode in gfs2_create_inode because that calls new_inode, not gfs2_iget. + * That means gfs2_delete_inode may subsequently try to call this function + * for a glock that's already pointing to a brand new inode. If we clear the + * new inode's gl_object, we'll introduce metadata corruption. Function + * gfs2_delete_inode calls clear_inode which calls gfs2_clear_inode which also + * tries to clear gl_object, so it's more than just gfs2_delete_inode. + * + */ +static inline void glock_clear_object(struct gfs2_glock *gl, void *object) +{ + spin_lock(&gl->gl_lockref.lock); + if (gl->gl_object == object) + gl->gl_object = NULL; + spin_unlock(&gl->gl_lockref.lock); +} + #endif /* __GLOCK_DOT_H__ */ diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index f9302f16a28e..2578bd824e34 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -201,14 +201,14 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type, fail_refresh: ip->i_iopen_gh.gh_flags |= GL_NOCACHE; - glock_set_object(ip->i_iopen_gh.gh_gl, NULL); + glock_clear_object(ip->i_iopen_gh.gh_gl, ip); gfs2_glock_dq_uninit(&ip->i_iopen_gh); fail_put: if (io_gl) gfs2_glock_put(io_gl); if (gfs2_holder_initialized(&i_gh)) gfs2_glock_dq_uninit(&i_gh); - glock_set_object(ip->i_gl, NULL); + glock_clear_object(ip->i_gl, ip); fail: iget_failed(inode); return ERR_PTR(error); diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c index fdedec379b78..5fdc54158ff6 100644 --- a/fs/gfs2/super.c +++ b/fs/gfs2/super.c @@ -1640,13 +1640,13 @@ out: gfs2_ordered_del_inode(ip); clear_inode(inode); gfs2_dir_hash_inval(ip); - glock_set_object(ip->i_gl, NULL); + glock_clear_object(ip->i_gl, ip); wait_on_bit_io(&ip->i_flags, GIF_GLOP_PENDING, TASK_UNINTERRUPTIBLE); gfs2_glock_add_to_lru(ip->i_gl); gfs2_glock_put(ip->i_gl); ip->i_gl = NULL; if (gfs2_holder_initialized(&ip->i_iopen_gh)) { - glock_set_object(ip->i_iopen_gh.gh_gl, NULL); + glock_clear_object(ip->i_iopen_gh.gh_gl, ip); ip->i_iopen_gh.gh_flags |= GL_NOCACHE; gfs2_glock_dq_uninit(&ip->i_iopen_gh); } -- cgit v1.2.3 From 4d7c18c7df89ef549f2de79b0faf873b49dea57a Mon Sep 17 00:00:00 2001 From: Bob Peterson Date: Tue, 18 Jul 2017 12:15:01 -0500 Subject: GFS2: Set gl_object in inode lookup only after block type check Before this patch, the inode glock's gl_object was set after a reference was acquired, but before the block type was verified. In cases where the block was unlinked, then freed and reused on another node, a residule delete callback (delete_work) would try to look up the inode, eventually failing the block check, but only after it overwrites gl_object with a pointer to the wrong inode. This patch moves the assignment of gl_object after the block check so it won't be improperly overwritten. Likewise, at the end of the function, gfs2_inode_lookup was clearing gl_object after it unlocked the glock, which meant another process might free the glock in the meantime. This patch guards against that case. Signed-off-by: Bob Peterson Reviewed-by: Andreas Gruenbacher --- fs/gfs2/inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'fs/gfs2/inode.c') diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index 2578bd824e34..fd6e1da3c5ab 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -145,7 +145,6 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type, if (unlikely(error)) goto fail; flush_delayed_work(&ip->i_gl->gl_work); - glock_set_object(ip->i_gl, ip); error = gfs2_glock_get(sdp, no_addr, &gfs2_iopen_glops, CREATE, &io_gl); if (unlikely(error)) @@ -170,6 +169,7 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type, } } + glock_set_object(ip->i_gl, ip); set_bit(GIF_INVALID, &ip->i_flags); error = gfs2_glock_nq_init(io_gl, LM_ST_SHARED, GL_EXACT, &ip->i_iopen_gh); if (unlikely(error)) @@ -206,9 +206,9 @@ fail_refresh: fail_put: if (io_gl) gfs2_glock_put(io_gl); + glock_clear_object(ip->i_gl, ip); if (gfs2_holder_initialized(&i_gh)) gfs2_glock_dq_uninit(&i_gh); - glock_clear_object(ip->i_gl, ip); fail: iget_failed(inode); return ERR_PTR(error); -- cgit v1.2.3 From 9c1b28081f43c0f14ccbcad02a6e0f227c072da2 Mon Sep 17 00:00:00 2001 From: Bob Peterson Date: Tue, 18 Jul 2017 12:26:07 -0500 Subject: GFS2: Clear gl_object if gfs2_create_inode fails If function gfs2_create_inode fails after the inode has been created (for example, if the inode_refresh fails for some reason) the function was setting gl_object but never clearing it again. The glocks are left pointing to a freed inode. This patch adds the calls to clear gl_object in the appropriate error paths. Signed-off-by: Bob Peterson Reviewed-by: Andreas Gruenbacher --- fs/gfs2/inode.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'fs/gfs2/inode.c') diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index fd6e1da3c5ab..1427328c6c86 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -775,14 +775,17 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, return error; fail_gunlock3: + glock_clear_object(io_gl, ip); gfs2_glock_dq_uninit(&ip->i_iopen_gh); gfs2_glock_put(io_gl); fail_gunlock2: if (io_gl) clear_bit(GLF_INODE_CREATING, &io_gl->gl_flags); fail_free_inode: - if (ip->i_gl) + if (ip->i_gl) { + glock_clear_object(ip->i_gl, ip); gfs2_glock_put(ip->i_gl); + } gfs2_rsqa_delete(ip, NULL); fail_free_acls: if (default_acl) -- cgit v1.2.3 From 61b91cfdc6c0c49a8cc8258cbee846551029d694 Mon Sep 17 00:00:00 2001 From: Andreas Gruenbacher Date: Tue, 1 Aug 2017 09:54:33 -0500 Subject: gfs2: Fix trivial typos Signed-off-by: Andreas Gruenbacher Signed-off-by: Bob Peterson --- fs/gfs2/inode.c | 2 +- fs/gfs2/super.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'fs/gfs2/inode.c') diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index 1427328c6c86..863749e29bf9 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -109,7 +109,7 @@ static void gfs2_set_iop(struct inode *inode) * @no_addr: The inode number * @no_formal_ino: The inode generation number * @blktype: Requested block type (GFS2_BLKST_DINODE or GFS2_BLKST_UNLINKED; - * GFS2_BLKST_FREE do indicate not to verify) + * GFS2_BLKST_FREE to indicate not to verify) * * If @type is DT_UNKNOWN, the inode type is fetched from disk. * diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c index 1918bb5fc943..6c39bb1ec100 100644 --- a/fs/gfs2/super.c +++ b/fs/gfs2/super.c @@ -1296,7 +1296,7 @@ static int gfs2_remount_fs(struct super_block *sb, int *flags, char *data) * gfs2_drop_inode - Drop an inode (test for remote unlink) * @inode: The inode to drop * - * If we've received a callback on an iopen lock then its because a + * If we've received a callback on an iopen lock then it's because a * remote node tried to deallocate the inode but failed due to this node * still having the inode open. Here we mark the link count zero * since we know that it must have reached zero if the GLF_DEMOTE flag -- cgit v1.2.3