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/ops_inode.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) (limited to 'fs/gfs2/ops_inode.c') diff --git a/fs/gfs2/ops_inode.c b/fs/gfs2/ops_inode.c index 636dda4c7d38..919e8947e710 100644 --- a/fs/gfs2/ops_inode.c +++ b/fs/gfs2/ops_inode.c @@ -553,6 +553,7 @@ static int gfs2_rename(struct inode *odir, struct dentry *odentry, int alloc_required; unsigned int x; int error; + struct gfs2_rgrpd *rgd; if (ndentry->d_inode) { nip = GFS2_I(ndentry->d_inode); @@ -684,12 +685,12 @@ static int gfs2_rename(struct inode *odir, struct dentry *odentry, error = gfs2_trans_begin(sdp, sdp->sd_max_dirres + al->al_rgd->rd_ri.ri_length + 4 * RES_DINODE + 4 * RES_LEAF + - RES_STATFS + RES_QUOTA, 0); + RES_STATFS + RES_QUOTA + 1, 0); if (error) goto out_ipreserv; } else { error = gfs2_trans_begin(sdp, 4 * RES_DINODE + - 5 * RES_LEAF, 0); + 5 * RES_LEAF + 1, 0); if (error) goto out_gunlock; } @@ -703,7 +704,25 @@ static int gfs2_rename(struct inode *odir, struct dentry *odentry, error = gfs2_dir_del(ndip, &ndentry->d_name); if (error) goto out_end_trans; - error = gfs2_change_nlink(nip, -1); + error = gfs2_change_nlink_i(nip, -1); + if ((!error) && (nip->i_inode.i_nlink == 0)) { + error = -EIO; + rgd = gfs2_blk2rgrpd(sdp, nip->i_num.no_addr); + if (rgd) { + struct gfs2_holder nlink_rg_gh; + if (rgd != nip->i_alloc.al_rgd) + error = gfs2_glock_nq_init( + rgd->rd_gl, LM_ST_EXCLUSIVE, + 0, &nlink_rg_gh); + else + error = 0; + if (!error) { + gfs2_unlink_di(&nip->i_inode); + if (rgd != nip->i_alloc.al_rgd) + gfs2_glock_dq_uninit(&nlink_rg_gh); + } + } + } } if (error) goto out_end_trans; -- 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/ops_inode.c | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) (limited to 'fs/gfs2/ops_inode.c') diff --git a/fs/gfs2/ops_inode.c b/fs/gfs2/ops_inode.c index 919e8947e710..b2a12f44f59d 100644 --- a/fs/gfs2/ops_inode.c +++ b/fs/gfs2/ops_inode.c @@ -553,7 +553,6 @@ static int gfs2_rename(struct inode *odir, struct dentry *odentry, int alloc_required; unsigned int x; int error; - struct gfs2_rgrpd *rgd; if (ndentry->d_inode) { nip = GFS2_I(ndentry->d_inode); @@ -685,12 +684,12 @@ static int gfs2_rename(struct inode *odir, struct dentry *odentry, error = gfs2_trans_begin(sdp, sdp->sd_max_dirres + al->al_rgd->rd_ri.ri_length + 4 * RES_DINODE + 4 * RES_LEAF + - RES_STATFS + RES_QUOTA + 1, 0); + RES_STATFS + RES_QUOTA + 4, 0); if (error) goto out_ipreserv; } else { error = gfs2_trans_begin(sdp, 4 * RES_DINODE + - 5 * RES_LEAF + 1, 0); + 5 * RES_LEAF + 4, 0); if (error) goto out_gunlock; } @@ -704,25 +703,7 @@ static int gfs2_rename(struct inode *odir, struct dentry *odentry, error = gfs2_dir_del(ndip, &ndentry->d_name); if (error) goto out_end_trans; - error = gfs2_change_nlink_i(nip, -1); - if ((!error) && (nip->i_inode.i_nlink == 0)) { - error = -EIO; - rgd = gfs2_blk2rgrpd(sdp, nip->i_num.no_addr); - if (rgd) { - struct gfs2_holder nlink_rg_gh; - if (rgd != nip->i_alloc.al_rgd) - error = gfs2_glock_nq_init( - rgd->rd_gl, LM_ST_EXCLUSIVE, - 0, &nlink_rg_gh); - else - error = 0; - if (!error) { - gfs2_unlink_di(&nip->i_inode); - if (rgd != nip->i_alloc.al_rgd) - gfs2_glock_dq_uninit(&nlink_rg_gh); - } - } - } + error = gfs2_change_nlink(nip, -1); } if (error) goto out_end_trans; -- 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/ops_inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs/gfs2/ops_inode.c') diff --git a/fs/gfs2/ops_inode.c b/fs/gfs2/ops_inode.c index b2a12f44f59d..747c7316f5cc 100644 --- a/fs/gfs2/ops_inode.c +++ b/fs/gfs2/ops_inode.c @@ -728,7 +728,7 @@ static int gfs2_rename(struct inode *odir, struct dentry *odentry, error = gfs2_meta_inode_buffer(ip, &dibh); if (error) goto out_end_trans; - 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); brelse(dibh); -- 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/ops_inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs/gfs2/ops_inode.c') diff --git a/fs/gfs2/ops_inode.c b/fs/gfs2/ops_inode.c index 747c7316f5cc..5591f8905cf7 100644 --- a/fs/gfs2/ops_inode.c +++ b/fs/gfs2/ops_inode.c @@ -1018,7 +1018,7 @@ static int gfs2_getattr(struct vfsmount *mnt, struct dentry *dentry, } generic_fillattr(inode, stat); - if (unlock); + if (unlock) gfs2_glock_dq_uninit(&gh); return 0; -- 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/ops_inode.c | 47 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 8 deletions(-) (limited to 'fs/gfs2/ops_inode.c') diff --git a/fs/gfs2/ops_inode.c b/fs/gfs2/ops_inode.c index 5591f8905cf7..f40a84807d75 100644 --- a/fs/gfs2/ops_inode.c +++ b/fs/gfs2/ops_inode.c @@ -264,13 +264,23 @@ static int gfs2_unlink(struct inode *dir, struct dentry *dentry) struct gfs2_inode *dip = GFS2_I(dir); struct gfs2_sbd *sdp = GFS2_SB(dir); struct gfs2_inode *ip = GFS2_I(dentry->d_inode); - struct gfs2_holder ghs[2]; + struct gfs2_holder ghs[3]; + struct gfs2_rgrpd *rgd; + struct gfs2_holder ri_gh; int error; + error = gfs2_rindex_hold(sdp, &ri_gh); + if (error) + return error; + gfs2_holder_init(dip->i_gl, LM_ST_EXCLUSIVE, 0, ghs); - gfs2_holder_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, ghs + 1); + gfs2_holder_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, ghs + 1); - error = gfs2_glock_nq_m(2, ghs); + rgd = gfs2_blk2rgrpd(sdp, ip->i_num.no_addr); + gfs2_holder_init(rgd->rd_gl, LM_ST_EXCLUSIVE, 0, ghs + 2); + + + error = gfs2_glock_nq_m(3, ghs); if (error) goto out; @@ -291,10 +301,12 @@ static int gfs2_unlink(struct inode *dir, struct dentry *dentry) out_end_trans: gfs2_trans_end(sdp); out_gunlock: - gfs2_glock_dq_m(2, ghs); + gfs2_glock_dq_m(3, ghs); out: gfs2_holder_uninit(ghs); gfs2_holder_uninit(ghs + 1); + gfs2_holder_uninit(ghs + 2); + gfs2_glock_dq_uninit(&ri_gh); return error; } @@ -449,13 +461,22 @@ static int gfs2_rmdir(struct inode *dir, struct dentry *dentry) struct gfs2_inode *dip = GFS2_I(dir); struct gfs2_sbd *sdp = GFS2_SB(dir); struct gfs2_inode *ip = GFS2_I(dentry->d_inode); - struct gfs2_holder ghs[2]; + struct gfs2_holder ghs[3]; + struct gfs2_rgrpd *rgd; + struct gfs2_holder ri_gh; int error; + + error = gfs2_rindex_hold(sdp, &ri_gh); + if (error) + return error; gfs2_holder_init(dip->i_gl, LM_ST_EXCLUSIVE, 0, ghs); gfs2_holder_init(ip->i_gl, LM_ST_EXCLUSIVE, 0, ghs + 1); - error = gfs2_glock_nq_m(2, ghs); + rgd = gfs2_blk2rgrpd(sdp, ip->i_num.no_addr); + gfs2_holder_init(rgd->rd_gl, LM_ST_EXCLUSIVE, 0, ghs + 2); + + error = gfs2_glock_nq_m(3, ghs); if (error) goto out; @@ -483,10 +504,12 @@ static int gfs2_rmdir(struct inode *dir, struct dentry *dentry) gfs2_trans_end(sdp); out_gunlock: - gfs2_glock_dq_m(2, ghs); + gfs2_glock_dq_m(3, ghs); out: gfs2_holder_uninit(ghs); gfs2_holder_uninit(ghs + 1); + gfs2_holder_uninit(ghs + 2); + gfs2_glock_dq_uninit(&ri_gh); return error; } @@ -547,7 +570,8 @@ static int gfs2_rename(struct inode *odir, struct dentry *odentry, struct gfs2_inode *ip = GFS2_I(odentry->d_inode); struct gfs2_inode *nip = NULL; struct gfs2_sbd *sdp = GFS2_SB(odir); - struct gfs2_holder ghs[4], r_gh; + struct gfs2_holder ghs[5], r_gh; + struct gfs2_rgrpd *nrgd; unsigned int num_gh; int dir_rename = 0; int alloc_required; @@ -587,6 +611,13 @@ static int gfs2_rename(struct inode *odir, struct dentry *odentry, if (nip) { gfs2_holder_init(nip->i_gl, LM_ST_EXCLUSIVE, 0, ghs + num_gh); num_gh++; + /* grab the resource lock for unlink flag twiddling + * this is the case of the target file already existing + * so we unlink before doing the rename + */ + nrgd = gfs2_blk2rgrpd(sdp, nip->i_num.no_addr); + if (nrgd) + gfs2_holder_init(nrgd->rd_gl, LM_ST_EXCLUSIVE, 0, ghs + num_gh++); } error = gfs2_glock_nq_m(num_gh, ghs); -- cgit v1.2.2