* [Cluster-devel] [PATCH GFS2] livelock while reclaiming unlinked dinodes
2010-04-14 15:58 ` [Cluster-devel] [PATCH GFS2] livelock while reclaiming unlinked dinodes Bob Peterson
@ 2010-04-14 16:31 ` Steven Whitehouse
0 siblings, 0 replies; 2+ messages in thread
From: Steven Whitehouse @ 2010-04-14 16:31 UTC (permalink / raw)
To: cluster-devel.redhat.com
Hi,
Now in the -nmw git tree. Thanks,
Steve.
On Wed, 2010-04-14 at 11:58 -0400, Bob Peterson wrote:
> Hi,
>
> Here is a patch for bugzilla bug #570182. Explanation in the
> patch.
>
> Regards,
>
> Bob Peterson
> Red Hat GFS
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> --
> Author: Bob Peterson <bob@krishna.(none)>
> Date: Tue Apr 13 08:49:33 2010 -0500
>
> GFS2: glock livelock
>
> This patch fixes a couple gfs2 problems with the reclaiming of
> unlinked dinodes. First, there were a couple of livelocks where
> everything would come to a halt waiting for a glock that was
> seemingly held by a process that no longer existed. In fact, the
> process did exist, it just had the wrong pid number in the holder
> information. Second, there was a lock ordering problem between
> inode locking and glock locking. Third, glock/inode contention
> could sometimes cause inodes to be improperly marked invalid by
> iget_failed.
>
> rhbz#570182
>
> diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
> index 297d7e5..5f1cc15 100644
> --- a/fs/gfs2/dir.c
> +++ b/fs/gfs2/dir.c
> @@ -1507,7 +1507,7 @@ struct inode *gfs2_dir_search(struct inode *dir, const struct qstr *name)
> inode = gfs2_inode_lookup(dir->i_sb,
> be16_to_cpu(dent->de_type),
> be64_to_cpu(dent->de_inum.no_addr),
> - be64_to_cpu(dent->de_inum.no_formal_ino), 0);
> + be64_to_cpu(dent->de_inum.no_formal_ino));
> brelse(bh);
> return inode;
> }
> diff --git a/fs/gfs2/export.c b/fs/gfs2/export.c
> index d15876e..d81bc7e 100644
> --- a/fs/gfs2/export.c
> +++ b/fs/gfs2/export.c
> @@ -169,7 +169,7 @@ static struct dentry *gfs2_get_dentry(struct super_block *sb,
> if (error)
> goto fail;
>
> - inode = gfs2_inode_lookup(sb, DT_UNKNOWN, inum->no_addr, 0, 0);
> + inode = gfs2_inode_lookup(sb, DT_UNKNOWN, inum->no_addr, 0);
> if (IS_ERR(inode)) {
> error = PTR_ERR(inode);
> goto fail;
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index c69d5fd..847892b 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -857,6 +857,9 @@ void gfs2_holder_reinit(unsigned int state, unsigned flags, struct gfs2_holder *
> gh->gh_flags = flags;
> gh->gh_iflags = 0;
> gh->gh_ip = (unsigned long)__builtin_return_address(0);
> + if (gh->gh_owner_pid)
> + put_pid(gh->gh_owner_pid);
> + gh->gh_owner_pid = get_pid(task_pid(current));
> }
>
> /**
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 6380cd9..40c1ed0 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -160,7 +160,6 @@ void gfs2_set_iop(struct inode *inode)
> * @sb: The super block
> * @no_addr: The inode number
> * @type: The type of the inode
> - * @skip_freeing: set this not return an inode if it is currently being freed.
> *
> * Returns: A VFS inode, or an error
> */
> @@ -168,17 +167,14 @@ void gfs2_set_iop(struct inode *inode)
> struct inode *gfs2_inode_lookup(struct super_block *sb,
> unsigned int type,
> u64 no_addr,
> - u64 no_formal_ino, int skip_freeing)
> + u64 no_formal_ino)
> {
> struct inode *inode;
> struct gfs2_inode *ip;
> struct gfs2_glock *io_gl;
> int error;
>
> - if (skip_freeing)
> - inode = gfs2_iget_skip(sb, no_addr);
> - else
> - inode = gfs2_iget(sb, no_addr);
> + inode = gfs2_iget(sb, no_addr);
> ip = GFS2_I(inode);
>
> if (!inode)
> @@ -236,13 +232,100 @@ fail_glock:
> fail_iopen:
> gfs2_glock_put(io_gl);
> fail_put:
> - ip->i_gl->gl_object = NULL;
> + if (inode->i_state & I_NEW)
> + ip->i_gl->gl_object = NULL;
> gfs2_glock_put(ip->i_gl);
> fail:
> - iget_failed(inode);
> + if (inode->i_state & I_NEW)
> + iget_failed(inode);
> + else
> + iput(inode);
> return ERR_PTR(error);
> }
>
> +/**
> + * gfs2_unlinked_inode_lookup - Lookup an unlinked inode for reclamation
> + * @sb: The super block
> + * no_addr: The inode number
> + * @@inode: A pointer to the inode found, if any
> + *
> + * Returns: 0 and *inode if no errors occurred. If an error occurs,
> + * the resulting *inode may or may not be NULL.
> + */
> +
> +int gfs2_unlinked_inode_lookup(struct super_block *sb, u64 no_addr,
> + struct inode **inode)
> +{
> + struct gfs2_sbd *sdp;
> + struct gfs2_inode *ip;
> + struct gfs2_glock *io_gl;
> + int error;
> + struct gfs2_holder gh;
> +
> + *inode = gfs2_iget_skip(sb, no_addr);
> +
> + if (!(*inode))
> + return -ENOBUFS;
> +
> + if (!((*inode)->i_state & I_NEW))
> + return -ENOBUFS;
> +
> + ip = GFS2_I(*inode);
> + sdp = GFS2_SB(*inode);
> + ip->i_no_formal_ino = -1;
> +
> + error = gfs2_glock_get(sdp, no_addr, &gfs2_inode_glops, CREATE, &ip->i_gl);
> + if (unlikely(error))
> + goto fail;
> + ip->i_gl->gl_object = ip;
> +
> + error = gfs2_glock_get(sdp, no_addr, &gfs2_iopen_glops, CREATE, &io_gl);
> + if (unlikely(error))
> + goto fail_put;
> +
> + set_bit(GIF_INVALID, &ip->i_flags);
> + error = gfs2_glock_nq_init(io_gl, LM_ST_SHARED, LM_FLAG_TRY | GL_EXACT,
> + &ip->i_iopen_gh);
> + if (unlikely(error)) {
> + if (error == GLR_TRYFAILED)
> + error = 0;
> + goto fail_iopen;
> + }
> + ip->i_iopen_gh.gh_gl->gl_object = ip;
> + gfs2_glock_put(io_gl);
> +
> + (*inode)->i_mode = DT2IF(DT_UNKNOWN);
> +
> + /*
> + * We must read the inode in order to work out its type in
> + * this case. Note that this doesn't happen often as we normally
> + * know the type beforehand. This code path only occurs during
> + * unlinked inode recovery (where it is safe to do this glock,
> + * which is not true in the general case).
> + */
> + error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, LM_FLAG_TRY,
> + &gh);
> + if (unlikely(error)) {
> + if (error == GLR_TRYFAILED)
> + error = 0;
> + goto fail_glock;
> + }
> + /* Inode is now uptodate */
> + gfs2_glock_dq_uninit(&gh);
> + gfs2_set_iop(*inode);
> +
> + return 0;
> +fail_glock:
> + gfs2_glock_dq(&ip->i_iopen_gh);
> +fail_iopen:
> + gfs2_glock_put(io_gl);
> +fail_put:
> + ip->i_gl->gl_object = NULL;
> + gfs2_glock_put(ip->i_gl);
> +fail:
> + return error;
> +}
> +
> static int gfs2_dinode_in(struct gfs2_inode *ip, const void *buf)
> {
> const struct gfs2_dinode *str = buf;
> @@ -863,7 +946,7 @@ struct inode *gfs2_createi(struct gfs2_holder *ghs, const struct qstr *name,
> goto fail_gunlock2;
>
> inode = gfs2_inode_lookup(dir->i_sb, IF2DT(mode), inum.no_addr,
> - inum.no_formal_ino, 0);
> + inum.no_formal_ino);
> if (IS_ERR(inode))
> goto fail_gunlock2;
>
> diff --git a/fs/gfs2/inode.h b/fs/gfs2/inode.h
> index c341aaf..e161461 100644
> --- a/fs/gfs2/inode.h
> +++ b/fs/gfs2/inode.h
> @@ -83,8 +83,9 @@ static inline void gfs2_inum_out(const struct gfs2_inode *ip,
>
> extern void gfs2_set_iop(struct inode *inode);
> extern struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned type,
> - u64 no_addr, u64 no_formal_ino,
> - int skip_freeing);
> + u64 no_addr, u64 no_formal_ino);
> +extern int gfs2_unlinked_inode_lookup(struct super_block *sb, u64 no_addr,
> + struct inode **inode);
> extern struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr);
>
> extern int gfs2_inode_refresh(struct gfs2_inode *ip);
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index 8a102f7..9db0be4 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -488,7 +488,7 @@ static int gfs2_lookup_root(struct super_block *sb, struct dentry **dptr,
> struct dentry *dentry;
> struct inode *inode;
>
> - inode = gfs2_inode_lookup(sb, DT_DIR, no_addr, 0, 0);
> + inode = gfs2_inode_lookup(sb, DT_DIR, no_addr, 0);
> if (IS_ERR(inode)) {
> fs_err(sdp, "can't read in %s inode: %ld\n", name, PTR_ERR(inode));
> return PTR_ERR(inode);
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index d87c0f8..ae9a2f7 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -952,18 +952,20 @@ static int try_rgrp_fit(struct gfs2_rgrpd *rgd, struct gfs2_alloc *al)
> * try_rgrp_unlink - Look for any unlinked, allocated, but unused inodes
> * @rgd: The rgrp
> *
> - * Returns: The inode, if one has been found
> + * Returns: 0 if no error
> + * The inode, if one has been found, in inode.
> */
>
> -static struct inode *try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked,
> - u64 skip)
> +static int try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked,
> + u64 skip, struct inode **inode)
> {
> - struct inode *inode;
> u32 goal = 0, block;
> u64 no_addr;
> struct gfs2_sbd *sdp = rgd->rd_sbd;
> unsigned int n;
> + int error = 0;
>
> + *inode = NULL;
> for(;;) {
> if (goal >= rgd->rd_data)
> break;
> @@ -983,14 +985,14 @@ static struct inode *try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked,
> if (no_addr == skip)
> continue;
> *last_unlinked = no_addr;
> - inode = gfs2_inode_lookup(rgd->rd_sbd->sd_vfs, DT_UNKNOWN,
> - no_addr, -1, 1);
> - if (!IS_ERR(inode))
> - return inode;
> + error = gfs2_unlinked_inode_lookup(rgd->rd_sbd->sd_vfs,
> + no_addr, inode);
> + if (*inode || error)
> + return error;
> }
>
> rgd->rd_flags &= ~GFS2_RDF_CHECK;
> - return NULL;
> + return 0;
> }
>
> /**
> @@ -1100,12 +1102,27 @@ static struct inode *get_local_rgrp(struct gfs2_inode *ip, u64 *last_unlinked)
> case 0:
> if (try_rgrp_fit(rgd, al))
> goto out;
> - if (rgd->rd_flags & GFS2_RDF_CHECK)
> - inode = try_rgrp_unlink(rgd, last_unlinked, ip->i_no_addr);
> + /* If the rg came in already locked, there's no
> + way we can recover from a failed try_rgrp_unlink
> + because that would require an iput which can only
> + happen after the rgrp is unlocked. */
> + if (!rg_locked && rgd->rd_flags & GFS2_RDF_CHECK)
> + error = try_rgrp_unlink(rgd, last_unlinked,
> + ip->i_no_addr, &inode);
> if (!rg_locked)
> gfs2_glock_dq_uninit(&al->al_rgd_gh);
> - if (inode)
> + if (inode) {
> + if (error) {
> + if (inode->i_state & I_NEW)
> + iget_failed(inode);
> + else
> + iput(inode);
> + return ERR_PTR(error);
> + }
> return inode;
> + }
> + if (error)
> + return ERR_PTR(error);
> /* fall through */
> case GLR_TRYFAILED:
> rgd = recent_rgrp_next(rgd);
> @@ -1134,12 +1151,23 @@ static struct inode *get_local_rgrp(struct gfs2_inode *ip, u64 *last_unlinked)
> case 0:
> if (try_rgrp_fit(rgd, al))
> goto out;
> - if (rgd->rd_flags & GFS2_RDF_CHECK)
> - inode = try_rgrp_unlink(rgd, last_unlinked, ip->i_no_addr);
> + if (!rg_locked && rgd->rd_flags & GFS2_RDF_CHECK)
> + error = try_rgrp_unlink(rgd, last_unlinked,
> + ip->i_no_addr, &inode);
> if (!rg_locked)
> gfs2_glock_dq_uninit(&al->al_rgd_gh);
> - if (inode)
> + if (inode) {
> + if (error) {
> + if (inode->i_state & I_NEW)
> + iget_failed(inode);
> + else
> + iput(inode);
> + return ERR_PTR(error);
> + }
> return inode;
> + }
> + if (error)
> + return ERR_PTR(error);
> break;
>
> case GLR_TRYFAILED:
>
^ permalink raw reply [flat|nested] 2+ messages in thread