[Cluster-devel] [PATCH 2/4] GFS2: Don't filter out I_FREEING inodes anymore

[Steven Whitehouse]

Hi,


On 18/12/15 19:02, Bob Peterson wrote:
> This patch basically reverts a very old patch from 2008,
> 7a9f53b3c1875bef22ad4588e818bc046ef183da, with the title
> "Alternate gfs2_iget to avoid looking up inodes being freed".
> The original patch was designed to avoid a deadlock caused by lock
> ordering with try_rgrp_unlink. The patch forced the function to not
> find inodes that were being removed by VFS. The problem is, that
> made it impossible for nodes to delete their own unlinked dinodes
> after a certain point in time, because the inode needed was not found
> by this filtering process. There is no longer a need for the patch,
> since function try_rgrp_unlink no longer locks the inode: All it does
> is queue the glock onto the delete work_queue, so there should be no
> more deadlock.
This looks good. For older kernels though I think the issue may still 
exist due to the way workqueues used to work, but for upstream and 
modern kernels this is a nice clean up,

Signed-off-by: Steven Whitehouse <swhiteho at redhat.com>

Steve.
> Signed-off-by: Bob Peterson <rpeterso at redhat.com>
> ---
>   fs/gfs2/export.c |  2 +-
>   fs/gfs2/glock.c  |  2 +-
>   fs/gfs2/inode.c  | 59 ++++----------------------------------------------------
>   fs/gfs2/inode.h  |  2 +-
>   4 files changed, 7 insertions(+), 58 deletions(-)
>
> diff --git a/fs/gfs2/export.c b/fs/gfs2/export.c
> index 5d15e94..d5bda85 100644
> --- a/fs/gfs2/export.c
> +++ b/fs/gfs2/export.c
> @@ -137,7 +137,7 @@ static struct dentry *gfs2_get_dentry(struct super_block *sb,
>   	struct gfs2_sbd *sdp = sb->s_fs_info;
>   	struct inode *inode;
>   
> -	inode = gfs2_ilookup(sb, inum->no_addr, 0);
> +	inode = gfs2_ilookup(sb, inum->no_addr);
>   	if (inode) {
>   		if (GFS2_I(inode)->i_no_formal_ino != inum->no_formal_ino) {
>   			iput(inode);
> diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
> index cb5ed3a..3e16d82 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -582,7 +582,7 @@ static void delete_work_func(struct work_struct *work)
>   	/* Note: Unsafe to dereference ip as we don't hold right refs/locks */
>   
>   	if (ip)
> -		inode = gfs2_ilookup(sdp->sd_vfs, no_addr, 1);
> +		inode = gfs2_ilookup(sdp->sd_vfs, no_addr);
>   	else
>   		inode = gfs2_lookup_by_inum(sdp, no_addr, NULL, GFS2_BLKST_UNLINKED);
>   	if (inode && !IS_ERR(inode)) {
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 6cdafed..2adc1ee 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -37,61 +37,9 @@
>   #include "super.h"
>   #include "glops.h"
>   
> -struct gfs2_skip_data {
> -	u64 no_addr;
> -	int skipped;
> -	int non_block;
> -};
> -
> -static int iget_test(struct inode *inode, void *opaque)
> -{
> -	struct gfs2_inode *ip = GFS2_I(inode);
> -	struct gfs2_skip_data *data = opaque;
> -
> -	if (ip->i_no_addr == data->no_addr) {
> -		if (data->non_block &&
> -		    inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE)) {
> -			data->skipped = 1;
> -			return 0;
> -		}
> -		return 1;
> -	}
> -	return 0;
> -}
> -
> -static int iget_set(struct inode *inode, void *opaque)
> +struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr)
>   {
> -	struct gfs2_inode *ip = GFS2_I(inode);
> -	struct gfs2_skip_data *data = opaque;
> -
> -	if (data->skipped)
> -		return -ENOENT;
> -	inode->i_ino = (unsigned long)(data->no_addr);
> -	ip->i_no_addr = data->no_addr;
> -	return 0;
> -}
> -
> -struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr, int non_block)
> -{
> -	unsigned long hash = (unsigned long)no_addr;
> -	struct gfs2_skip_data data;
> -
> -	data.no_addr = no_addr;
> -	data.skipped = 0;
> -	data.non_block = non_block;
> -	return ilookup5(sb, hash, iget_test, &data);
> -}
> -
> -static struct inode *gfs2_iget(struct super_block *sb, u64 no_addr,
> -			       int non_block)
> -{
> -	struct gfs2_skip_data data;
> -	unsigned long hash = (unsigned long)no_addr;
> -
> -	data.no_addr = no_addr;
> -	data.skipped = 0;
> -	data.non_block = non_block;
> -	return iget5_locked(sb, hash, iget_test, iget_set, &data);
> +	return ilookup(sb, (unsigned long)no_addr);
>   }
>   
>   /**
> @@ -145,8 +93,9 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
>   	struct gfs2_glock *io_gl = NULL;
>   	int error;
>   
> -	inode = gfs2_iget(sb, no_addr, non_block);
> +	inode = iget_locked(sb, (unsigned long)no_addr);
>   	ip = GFS2_I(inode);
> +	ip->i_no_addr = no_addr;
>   
>   	if (!inode)
>   		return ERR_PTR(-ENOMEM);
> diff --git a/fs/gfs2/inode.h b/fs/gfs2/inode.h
> index ba4d949..22c27a8 100644
> --- a/fs/gfs2/inode.h
> +++ b/fs/gfs2/inode.h
> @@ -99,7 +99,7 @@ extern struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned type,
>   extern struct inode *gfs2_lookup_by_inum(struct gfs2_sbd *sdp, u64 no_addr,
>   					 u64 *no_formal_ino,
>   					 unsigned int blktype);
> -extern struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr, int nonblock);
> +extern struct inode *gfs2_ilookup(struct super_block *sb, u64 no_addr);
>   
>   extern int gfs2_inode_refresh(struct gfs2_inode *ip);
>