[Cluster-devel] [GFS2 PATCH v3 08/14] GFS2: Don't filter out I_FREEING inodes anymore

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On 22/10/15 20:30, 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.
I'm not sure I understand. Why would we need to look up and inode in one 
of the
I_FREEING|I_CLEAR|I_WILL_FREE states? If the inode is in one of those 
states then it is already being removed, so we should be able to leave 
it to its own devices and it will be gone, in the normal case. This will 
also likely not be deadlock free for older kernels with the previous 
workqueue implementation, but will probably be ok for upstream,

Steve.

> Signed-off-by: Bob Peterson <rpeterso@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 8aa794d..926652f 100644
> --- a/fs/gfs2/glock.c
> +++ b/fs/gfs2/glock.c
> @@ -576,7 +576,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 833f8fa..06c208b 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);
>