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

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On 18/12/15 19:02, 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 assume that this was just intended to be a clean up rather than 
something that fixes a bug? At least I can't see any obvious changes to 
the operations, just the way in which they are carried out. I think it 
would be neater to turn the non_block parameter of gfs2_inode_lookup() 
into a flags argument, rather than adding an additional argument which 
is only a bool.

I'm not sure that factoring out some of the checks from 
gfs2_lookup_by_inum() buys us that much though, since that function 
still has to remain in its current form for NFS anyway,

Steve.

> 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);