From 5509826f1e548d14bb888c1cb6e3bbf23f855770 Mon Sep 17 00:00:00 2001 From: "S. Wendy Cheng" Date: Thu, 18 Jan 2007 15:56:34 -0500 Subject: [GFS2] Fix change nlink deadlock Bugzilla 215088 Fix deadlock in gfs2_change_nlink() while installing RHEL5 into GFS2 partition. The gfs2_rename() apparently needs block allocation for the new name (into the directory) where it requires rg locks. At the same time, while updating the nlink count for the replaced file, gfs2_change_nlink() tries to return the inode meta-data back to resource group where it needs rg locks too. Our logic doesn't allow process to acquire these locks recursively by the same process (RHEL installer) that results a BUG call. This only happens within rename code path and only if the destination file exists before the rename operation. Signed-off-by: S. Wendy Cheng Signed-off-by: Steven Whitehouse --- fs/gfs2/inode.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) (limited to 'fs/gfs2/inode.c') diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index d122074c45e1..6bc443644c3c 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -281,16 +281,14 @@ out: } /** - * gfs2_change_nlink - Change nlink count on inode + * gfs2_change_nlink_i - Change nlink count on inode * @ip: The GFS2 inode * @diff: The change in the nlink count required * * Returns: errno */ - -int gfs2_change_nlink(struct gfs2_inode *ip, int diff) +int gfs2_change_nlink_i(struct gfs2_inode *ip, int diff) { - struct gfs2_sbd *sdp = ip->i_inode.i_sb->s_fs_info; struct buffer_head *dibh; u32 nlink; int error; @@ -322,6 +320,20 @@ int gfs2_change_nlink(struct gfs2_inode *ip, int diff) brelse(dibh); mark_inode_dirty(&ip->i_inode); + return error; +} + +int gfs2_change_nlink(struct gfs2_inode *ip, int diff) +{ + struct gfs2_sbd *sdp = ip->i_inode.i_sb->s_fs_info; + int error; + + /* update the nlink */ + error = gfs2_change_nlink_i(ip, diff); + if (error) + return error; + + /* return meta data block back to rg */ if (ip->i_inode.i_nlink == 0) { struct gfs2_rgrpd *rgd; struct gfs2_holder ri_gh, rg_gh; -- cgit v1.2.2 From 6c93fd1e578669364e026a0d44c669b871e2a8c4 Mon Sep 17 00:00:00 2001 From: Russell Cattelan Date: Mon, 8 Jan 2007 17:47:51 -0600 Subject: [GFS2] BZ 217008 fsfuzzer fix. Update the quilt header comments to match the code changes. Change gfs2_lookup_simple to return an error in the case of a NULL inode. The callers of gfs2_lookup_simple do not check for NULL in the no entry case and such would end up dereferencing a NULL ptr. This fixes: http://projects.info-pull.com/mokb/MOKB-15-11-2006.html Signed-off-by: Russell Cattelan Signed-off-by: Steven Whitehouse --- fs/gfs2/inode.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) (limited to 'fs/gfs2/inode.c') diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index 6bc443644c3c..bab338f6b610 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -361,8 +361,18 @@ out: struct inode *gfs2_lookup_simple(struct inode *dip, const char *name) { struct qstr qstr; + struct inode *inode; gfs2_str2qstr(&qstr, name); - return gfs2_lookupi(dip, &qstr, 1, NULL); + inode = gfs2_lookupi(dip, &qstr, 1, NULL); + /* gfs2_lookupi has inconsistent callers: vfs + * related routines expect NULL for no entry found, + * gfs2_lookup_simple callers expect ENOENT + * and do not check for NULL. + */ + if (inode == NULL) + return ERR_PTR(-ENOENT); + else + return inode; } -- cgit v1.2.2 From 87d21e07f3880b8d489f0b4a639deb1362101838 Mon Sep 17 00:00:00 2001 From: "S. Wendy Cheng" Date: Thu, 18 Jan 2007 16:07:03 -0500 Subject: [GFS2] Fix gfs2_rename deadlock Second round of gfs2_rename lock re-ordering to allow Anaconda adding root partition on top of gfs2. Previous to this patch the recursive lock detector in glock.c can be triggered due to attempting to lock the rgrp twice. This fixes it by checking to see whether the rgrp is already locked. This fixes Red Hat bugzilla #221237 Signed-off-by: S. Wendy Cheng Signed-off-by: Steven Whitehouse --- fs/gfs2/inode.c | 50 +++++++++++++++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 19 deletions(-) (limited to 'fs/gfs2/inode.c') diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index bab338f6b610..58c2ce785fed 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -281,13 +281,13 @@ out: } /** - * gfs2_change_nlink_i - Change nlink count on inode + * gfs2_change_nlink - Change nlink count on inode * @ip: The GFS2 inode * @diff: The change in the nlink count required * * Returns: errno */ -int gfs2_change_nlink_i(struct gfs2_inode *ip, int diff) +int gfs2_change_nlink(struct gfs2_inode *ip, int diff) { struct buffer_head *dibh; u32 nlink; @@ -320,40 +320,52 @@ int gfs2_change_nlink_i(struct gfs2_inode *ip, int diff) brelse(dibh); mark_inode_dirty(&ip->i_inode); + if (ip->i_inode.i_nlink == 0) + error = gfs2_change_nlink_i(ip); + return error; } -int gfs2_change_nlink(struct gfs2_inode *ip, int diff) +int gfs2_change_nlink_i(struct gfs2_inode *ip) { struct gfs2_sbd *sdp = ip->i_inode.i_sb->s_fs_info; - int error; - - /* update the nlink */ - error = gfs2_change_nlink_i(ip, diff); - if (error) - return error; - - /* return meta data block back to rg */ - if (ip->i_inode.i_nlink == 0) { - struct gfs2_rgrpd *rgd; - struct gfs2_holder ri_gh, rg_gh; + struct gfs2_inode *rindex = GFS2_I(sdp->sd_rindex); + struct gfs2_glock *ri_gl = rindex->i_gl; + struct gfs2_rgrpd *rgd; + struct gfs2_holder ri_gh, rg_gh; + int existing, error; + /* if we come from rename path, we could have the lock already */ + existing = gfs2_glock_is_locked_by_me(ri_gl); + if (!existing) { error = gfs2_rindex_hold(sdp, &ri_gh); if (error) goto out; - error = -EIO; - rgd = gfs2_blk2rgrpd(sdp, ip->i_num.no_addr); - if (!rgd) - goto out_norgrp; + } + + /* find the matching rgd */ + error = -EIO; + rgd = gfs2_blk2rgrpd(sdp, ip->i_num.no_addr); + if (!rgd) + goto out_norgrp; + + /* + * Eventually we may want to move rgd(s) to a linked list + * and piggyback the free logic into one of gfs2 daemons + * to gain some performance. + */ + if (!rgd->rd_gl || !gfs2_glock_is_locked_by_me(rgd->rd_gl)) { error = gfs2_glock_nq_init(rgd->rd_gl, LM_ST_EXCLUSIVE, 0, &rg_gh); if (error) goto out_norgrp; gfs2_unlink_di(&ip->i_inode); /* mark inode unlinked */ gfs2_glock_dq_uninit(&rg_gh); + } + out_norgrp: + if (!existing) gfs2_glock_dq_uninit(&ri_gh); - } out: return error; } -- cgit v1.2.2 From 03dc6a538e42bcc8d5dfabcee208b639db85a80c Mon Sep 17 00:00:00 2001 From: Adrian Bunk Date: Sat, 13 Jan 2007 10:56:41 +0100 Subject: [GFS2] make gfs2_change_nlink_i() static On Thu, Jan 11, 2007 at 10:26:27PM -0800, Andrew Morton wrote: >... > Changes since 2.6.20-rc3-mm1: >... > git-gfs2-nmw.patch >... > git trees >... This patch makes the needlessly globlal gfs2_change_nlink_i() static. Signed-off-by: Adrian Bunk Signed-off-by: Steven Whitehouse --- fs/gfs2/inode.c | 88 ++++++++++++++++++++++++++++----------------------------- 1 file changed, 44 insertions(+), 44 deletions(-) (limited to 'fs/gfs2/inode.c') diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index 58c2ce785fed..260316954ad7 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -280,6 +280,50 @@ out: return error; } +static int gfs2_change_nlink_i(struct gfs2_inode *ip) +{ + struct gfs2_sbd *sdp = ip->i_inode.i_sb->s_fs_info; + struct gfs2_inode *rindex = GFS2_I(sdp->sd_rindex); + struct gfs2_glock *ri_gl = rindex->i_gl; + struct gfs2_rgrpd *rgd; + struct gfs2_holder ri_gh, rg_gh; + int existing, error; + + /* if we come from rename path, we could have the lock already */ + existing = gfs2_glock_is_locked_by_me(ri_gl); + if (!existing) { + error = gfs2_rindex_hold(sdp, &ri_gh); + if (error) + goto out; + } + + /* find the matching rgd */ + error = -EIO; + rgd = gfs2_blk2rgrpd(sdp, ip->i_num.no_addr); + if (!rgd) + goto out_norgrp; + + /* + * Eventually we may want to move rgd(s) to a linked list + * and piggyback the free logic into one of gfs2 daemons + * to gain some performance. + */ + if (!rgd->rd_gl || !gfs2_glock_is_locked_by_me(rgd->rd_gl)) { + error = gfs2_glock_nq_init(rgd->rd_gl, LM_ST_EXCLUSIVE, 0, &rg_gh); + if (error) + goto out_norgrp; + + gfs2_unlink_di(&ip->i_inode); /* mark inode unlinked */ + gfs2_glock_dq_uninit(&rg_gh); + } + +out_norgrp: + if (!existing) + gfs2_glock_dq_uninit(&ri_gh); +out: + return error; +} + /** * gfs2_change_nlink - Change nlink count on inode * @ip: The GFS2 inode @@ -326,50 +370,6 @@ int gfs2_change_nlink(struct gfs2_inode *ip, int diff) return error; } -int gfs2_change_nlink_i(struct gfs2_inode *ip) -{ - struct gfs2_sbd *sdp = ip->i_inode.i_sb->s_fs_info; - struct gfs2_inode *rindex = GFS2_I(sdp->sd_rindex); - struct gfs2_glock *ri_gl = rindex->i_gl; - struct gfs2_rgrpd *rgd; - struct gfs2_holder ri_gh, rg_gh; - int existing, error; - - /* if we come from rename path, we could have the lock already */ - existing = gfs2_glock_is_locked_by_me(ri_gl); - if (!existing) { - error = gfs2_rindex_hold(sdp, &ri_gh); - if (error) - goto out; - } - - /* find the matching rgd */ - error = -EIO; - rgd = gfs2_blk2rgrpd(sdp, ip->i_num.no_addr); - if (!rgd) - goto out_norgrp; - - /* - * Eventually we may want to move rgd(s) to a linked list - * and piggyback the free logic into one of gfs2 daemons - * to gain some performance. - */ - if (!rgd->rd_gl || !gfs2_glock_is_locked_by_me(rgd->rd_gl)) { - error = gfs2_glock_nq_init(rgd->rd_gl, LM_ST_EXCLUSIVE, 0, &rg_gh); - if (error) - goto out_norgrp; - - gfs2_unlink_di(&ip->i_inode); /* mark inode unlinked */ - gfs2_glock_dq_uninit(&rg_gh); - } - -out_norgrp: - if (!existing) - gfs2_glock_dq_uninit(&ri_gh); -out: - return error; -} - struct inode *gfs2_lookup_simple(struct inode *dip, const char *name) { struct qstr qstr; -- cgit v1.2.2 From ddfe0627838ca0c0e8babb0dd2bd7f4b35e25bff Mon Sep 17 00:00:00 2001 From: Eric Sandeen Date: Thu, 18 Jan 2007 16:41:23 -0600 Subject: [GFS2] use CURRENT_TIME_SEC instead of get_seconds in gfs2 I was looking something else up and came across this... I don't honestly have a good reason to change it other than to make it like every other Linux filesystem in this regard. ;-) It doesn't functionally change anything, but makes some lines shorter. :) I'm also curious; why does gfs2 have 64-bits of on-disk timestamps, but not in timespec_t format, and only stores second resolutions? Seems like you're halfway to sub-second resolutions already. I suppose if that gets implemented then all of the below should instead be CURRENT_TIME not CURRENT_TIME_SEC. Signed-off-by: Eric Sandeen Signed-off-by: Steven Whitehouse --- fs/gfs2/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs/gfs2/inode.c') diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index 260316954ad7..f7c8d31ce41a 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -357,7 +357,7 @@ int gfs2_change_nlink(struct gfs2_inode *ip, int diff) else drop_nlink(&ip->i_inode); - ip->i_inode.i_ctime.tv_sec = get_seconds(); + ip->i_inode.i_ctime = CURRENT_TIME_SEC; gfs2_trans_add_bh(ip->i_gl, dibh, 1); gfs2_dinode_out(ip, dibh->b_data); -- cgit v1.2.2 From d7c103d0bd29c94f78155a4538faf314e49d9713 Mon Sep 17 00:00:00 2001 From: Steven Whitehouse Date: Thu, 25 Jan 2007 17:14:59 +0000 Subject: [GFS2] Fix recursive locking attempt with NFS In certain cases, its possible for NFS to call the lookup code while holding the glock (when doing a readdirplus operation) so we need to check for that and not try and lock the glock twice. This also fixes a typo in a previous NFS related GFS2 patch. Signed-off-by: Steven Whitehouse --- fs/gfs2/inode.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) (limited to 'fs/gfs2/inode.c') diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index f7c8d31ce41a..88fcfb4f5c4d 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -395,8 +395,10 @@ struct inode *gfs2_lookup_simple(struct inode *dip, const char *name) * @is_root: If 1, ignore the caller's permissions * @i_gh: An uninitialized holder for the new inode glock * - * There will always be a vnode (Linux VFS inode) for the d_gh inode unless - * @is_root is true. + * This can be called via the VFS filldir function when NFS is doing + * a readdirplus and the inode which its intending to stat isn't + * already in cache. In this case we must not take the directory glock + * again, since the readdir call will have already taken that lock. * * Returns: errno */ @@ -409,8 +411,9 @@ struct inode *gfs2_lookupi(struct inode *dir, const struct qstr *name, struct gfs2_holder d_gh; struct gfs2_inum_host inum; unsigned int type; - int error = 0; + int error; struct inode *inode = NULL; + int unlock = 0; if (!name->len || name->len > GFS2_FNAMESIZE) return ERR_PTR(-ENAMETOOLONG); @@ -422,9 +425,12 @@ struct inode *gfs2_lookupi(struct inode *dir, const struct qstr *name, return dir; } - error = gfs2_glock_nq_init(dip->i_gl, LM_ST_SHARED, 0, &d_gh); - if (error) - return ERR_PTR(error); + if (gfs2_glock_is_locked_by_me(dip->i_gl) == 0) { + error = gfs2_glock_nq_init(dip->i_gl, LM_ST_SHARED, 0, &d_gh); + if (error) + return ERR_PTR(error); + unlock = 1; + } if (!is_root) { error = permission(dir, MAY_EXEC, NULL); @@ -439,10 +445,11 @@ struct inode *gfs2_lookupi(struct inode *dir, const struct qstr *name, inode = gfs2_inode_lookup(sb, &inum, type); out: - gfs2_glock_dq_uninit(&d_gh); + if (unlock) + gfs2_glock_dq_uninit(&d_gh); if (error == -ENOENT) return NULL; - return inode; + return inode ? inode : ERR_PTR(error); } static int pick_formal_ino_1(struct gfs2_sbd *sdp, u64 *formal_ino) -- cgit v1.2.2 From ddee76089cc9bcbd8ae9ec6c26e726a8ab2fe675 Mon Sep 17 00:00:00 2001 From: Russell Cattelan Date: Mon, 29 Jan 2007 17:13:44 -0600 Subject: [GFS2] Fix unlink deadlocks Move the glock acquisition to outside of the transactions. Lock odering must be preserved in order to prevent ABBA deadlocks. The current gfs2_change_nlink code would tries to grab the glock after having started a transaction and thus is holding the log lock. This is inconsistent with other code paths in gfs that grab the resource group glock prior to staring a tranactions. One problem with this fix is that the resource group lock is always grabbed now even if the inode still has ref count and can not be marked for unlink. Signed-off-by: Russell Cattelan Signed-off-by: Steven Whitehouse --- fs/gfs2/inode.c | 46 +--------------------------------------------- 1 file changed, 1 insertion(+), 45 deletions(-) (limited to 'fs/gfs2/inode.c') diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c index 88fcfb4f5c4d..0d6831a40565 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -280,50 +280,6 @@ out: return error; } -static int gfs2_change_nlink_i(struct gfs2_inode *ip) -{ - struct gfs2_sbd *sdp = ip->i_inode.i_sb->s_fs_info; - struct gfs2_inode *rindex = GFS2_I(sdp->sd_rindex); - struct gfs2_glock *ri_gl = rindex->i_gl; - struct gfs2_rgrpd *rgd; - struct gfs2_holder ri_gh, rg_gh; - int existing, error; - - /* if we come from rename path, we could have the lock already */ - existing = gfs2_glock_is_locked_by_me(ri_gl); - if (!existing) { - error = gfs2_rindex_hold(sdp, &ri_gh); - if (error) - goto out; - } - - /* find the matching rgd */ - error = -EIO; - rgd = gfs2_blk2rgrpd(sdp, ip->i_num.no_addr); - if (!rgd) - goto out_norgrp; - - /* - * Eventually we may want to move rgd(s) to a linked list - * and piggyback the free logic into one of gfs2 daemons - * to gain some performance. - */ - if (!rgd->rd_gl || !gfs2_glock_is_locked_by_me(rgd->rd_gl)) { - error = gfs2_glock_nq_init(rgd->rd_gl, LM_ST_EXCLUSIVE, 0, &rg_gh); - if (error) - goto out_norgrp; - - gfs2_unlink_di(&ip->i_inode); /* mark inode unlinked */ - gfs2_glock_dq_uninit(&rg_gh); - } - -out_norgrp: - if (!existing) - gfs2_glock_dq_uninit(&ri_gh); -out: - return error; -} - /** * gfs2_change_nlink - Change nlink count on inode * @ip: The GFS2 inode @@ -365,7 +321,7 @@ int gfs2_change_nlink(struct gfs2_inode *ip, int diff) mark_inode_dirty(&ip->i_inode); if (ip->i_inode.i_nlink == 0) - error = gfs2_change_nlink_i(ip); + gfs2_unlink_di(&ip->i_inode); /* mark inode unlinked */ return error; } -- cgit v1.2.2