[Cluster-devel] [PATCH 4/4] GFS2: Make deletes use regular inode_lookup with special code path

[Benjamin Marzinski <bmarzins@redhat.com>]

On Fri, Dec 18, 2015 at 01:02:30PM -0600, Bob Peterson wrote:
> This patch changes the inode reclaiming code, delete_work_func, use
> the normal gfs2_inode_lookup, but with a special parameter, for_del
> which causes additional checks to be made.

I get what you're doing here, and I can't see anything really wrong with
it, but I haven't gone through all the delete code as carefully as I
could.  I do have two questions.

1. I don't see any reason why, even if it's just for paranoias sake, we
don't avoid dereferencing the gfs2_inode in verify_unlinked before
locking and checking it. It seems like a simple fix that avoids any
chance that there is a problem due to not holding the directory glock
when we're getting the inode.

2. gfs2_inode_lookup calls iget_locked, which creates an inode if one
doesn't already exist.  If there's any chance of racing and calling
delete_work_func for an inode that has already been removed, won't we
just end up creating a new one here?

-Ben

> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  fs/gfs2/dir.c        |  2 +-
>  fs/gfs2/glock.c      |  9 +--------
>  fs/gfs2/inode.c      | 32 ++++++++++++++++++++++++++++----
>  fs/gfs2/inode.h      |  3 ++-
>  fs/gfs2/ops_fstype.c |  2 +-
>  5 files changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
> index 2bef023..6a92592 100644
> --- a/fs/gfs2/dir.c
> +++ b/fs/gfs2/dir.c
> @@ -1660,7 +1660,7 @@ struct inode *gfs2_dir_search(struct inode *dir, const struct qstr *name,
>  		brelse(bh);
>  		if (fail_on_exist)
>  			return ERR_PTR(-EEXIST);
> -		inode = gfs2_inode_lookup(dir->i_sb, dtype, addr, formal_ino);
> +		inode = gfs2_inode_lookup(dir->i_sb, dtype, addr, formal_ino, 0);
>  		if (!IS_ERR(inode))
>  			GFS2_I(inode)->i_rahead = rahead;
>  		return inode;
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index 3e16d82..795c2f3 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -568,7 +568,6 @@ static void delete_work_func(struct work_struct *work)
>  {
>  	struct gfs2_glock *gl = container_of(work, struct gfs2_glock, gl_delete);
>  	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
> -	struct gfs2_inode *ip;
>  	struct inode *inode;
>  	u64 no_addr = gl->gl_name.ln_number;
>  
> @@ -578,13 +577,7 @@ static void delete_work_func(struct work_struct *work)
>  	if (test_bit(GLF_INODE_CREATING, &gl->gl_flags))
>  		goto out;
>  
> -	ip = gl->gl_object;
> -	/* Note: Unsafe to dereference ip as we don't hold right refs/locks */
> -
> -	if (ip)
> -		inode = gfs2_ilookup(sdp->sd_vfs, no_addr);
> -	else
> -		inode = gfs2_lookup_by_inum(sdp, no_addr, NULL, GFS2_BLKST_UNLINKED);
> +	inode = gfs2_inode_lookup(sdp->sd_vfs, DT_UNKNOWN, no_addr, 0, 1);
>  	if (inode && !IS_ERR(inode)) {
>  		d_prune_aliases(inode);
>  		iput(inode);
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 01b6d1a..0357862 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -75,17 +75,37 @@ static void gfs2_set_iop(struct inode *inode)
>  	}
>  }
>  
> +static int verify_unlinked(struct gfs2_inode *ip)
> +{
> +	struct gfs2_holder i_gh;
> +	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
> +	int error;
> +
> +	error = gfs2_glock_nq_init(ip->i_gl, LM_ST_EXCLUSIVE, GL_SKIP, &i_gh);
> +	if (error)
> +		return error;
> +
> +	error = gfs2_check_blk_type(sdp, ip->i_no_addr, GFS2_BLKST_UNLINKED);
> +	if (error == 0)
> +		error = gfs2_inode_refresh(ip);
> +
> +	gfs2_glock_dq_uninit(&i_gh);
> +	return error;
> +}
> +
>  /**
>   * gfs2_inode_lookup - Lookup an inode
>   * @sb: The super block
> - * @no_addr: The inode number
>   * @type: The type of the inode
> + * @no_addr: The inode number
> + * @no_formal_ino: The formal inode number
> + * @for_del: Is this lookup for deleting purposes?
>   *
>   * Returns: A VFS inode, or an error
>   */
>  
>  struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
> -				u64 no_addr, u64 no_formal_ino)
> +				u64 no_addr, u64 no_formal_ino, int for_del)
>  {
>  	struct inode *inode;
>  	struct gfs2_inode *ip;
> @@ -121,7 +141,11 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
>  		gfs2_glock_put(io_gl);
>  		io_gl = NULL;
>  
> -		if (type == DT_UNKNOWN) {
> +		if (for_del) {
> +			error = verify_unlinked(ip);
> +			if (error)
> +				goto fail_refresh;
> +		} else if (type == DT_UNKNOWN) {
>  			/* Inode glock must be locked already */
>  			error = gfs2_inode_refresh(GFS2_I(inode));
>  			if (error)
> @@ -169,7 +193,7 @@ struct inode *gfs2_lookup_by_inum(struct gfs2_sbd *sdp, u64 no_addr,
>  	if (error)
>  		goto fail;
>  
> -	inode = gfs2_inode_lookup(sb, DT_UNKNOWN, no_addr, 0);
> +	inode = gfs2_inode_lookup(sb, DT_UNKNOWN, no_addr, 0, 0);
>  	if (IS_ERR(inode))
>  		goto fail;
>  
> diff --git a/fs/gfs2/inode.h b/fs/gfs2/inode.h
> index e1af0d4..d6233e9 100644
> --- a/fs/gfs2/inode.h
> +++ b/fs/gfs2/inode.h
> @@ -94,7 +94,8 @@ err:
>  }
>  
>  extern struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned type, 
> -				       u64 no_addr, u64 no_formal_ino);
> +				       u64 no_addr, u64 no_formal_ino,
> +				       int for_del);
>  extern struct inode *gfs2_lookup_by_inum(struct gfs2_sbd *sdp, u64 no_addr,
>  					 u64 *no_formal_ino,
>  					 unsigned int blktype);
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index 9ed522c..7aacdf2 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -454,7 +454,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);
> +	inode = gfs2_inode_lookup(sb, DT_DIR, no_addr, 0, 0);
>  	if (IS_ERR(inode)) {
>  		fs_err(sdp, "can't read in %s inode: %ld\n", name, PTR_ERR(inode));
>  		return PTR_ERR(inode);
> -- 
> 2.5.0