[Cluster-devel] [GFS Patch] GFS: Don't free superblock until last kobject user is done

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

Looks good to me,

Steve.

On Tue, 2012-06-26 at 09:27 -0400, Bob Peterson wrote:
> Hi,
> 
> This patch rearranges the sysfs interface to more closely match
> GFS2, plus it adds an object release mechanism, as described in
> kernel/Documentation/kobject.txt
> 
> rhbz#788694
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
> ---
>  gfs-kernel/src/gfs/ops_fstype.c |   27 +++++++++++++++++++--------
>  gfs-kernel/src/gfs/ops_fstype.h |    1 -
>  gfs-kernel/src/gfs/ops_super.c  |    6 +-----
>  gfs-kernel/src/gfs/sys.c        |   14 ++++++++++++++
>  4 files changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/gfs-kernel/src/gfs/ops_fstype.c b/gfs-kernel/src/gfs/ops_fstype.c
> index 34d9fa4..7104a2d 100644
> --- a/gfs-kernel/src/gfs/ops_fstype.c
> +++ b/gfs-kernel/src/gfs/ops_fstype.c
> @@ -682,12 +682,24 @@ static int fill_super(struct super_block *sb, void *data, int silent)
>  		goto fail;
>  	}
>  	error = init_names(sdp, silent);
> -	if (error)
> -		goto fail;
> +	if (error) {
> +		/* In this case, we haven't initialized sysfs, so we have to
> +		   manually free the sdp. */
> +		vfree(sdp);
> +		sb->s_fs_info = NULL;
> +		return error;
> +	}
>  
>  	error = gfs_sys_fs_add(sdp);
> +	/*
> +	 * If we hit an error here, gfs2_sys_fs_add will have called function
> +	 * kobject_put which causes the sysfs usage count to go to zero, which
> +	 * causes sysfs to call function gfs2_sbd_release, which frees sdp.
> +	 * Subsequent error paths here will call gfs_sys_fs_del, which also
> +	 * kobject_put to free sdp.
> +	 */
>  	if (error)
> -		goto fail;
> +		return error;
>  
>  	/*  Mount an inter-node lock module, check for local optimizations */
>  
> @@ -746,17 +758,16 @@ fail_locking:
>  fail_lm:
>  	gfs_gl_hash_clear(sdp, TRUE);
>  	gfs_lm_unmount(sdp);
> +fail_sys:
>  	gfs_clear_dirty_j(sdp);
>  	while (invalidate_inodes(sb))
>  		yield();
>  
> -fail_sys:
> -	gfs_sys_fs_del(sdp);
> -
>  fail:
> -	vfree(sdp);
> +	/* gfs_sys_fs_del must be the last thing we do, since it causes
> +	 * sysfs to call function gfs2_sbd_release, which frees sdp. */
> +	gfs_sys_fs_del(sdp);
>  	sb->s_fs_info = NULL;
> -
>  	return error;
>  }
>  
> diff --git a/gfs-kernel/src/gfs/ops_fstype.h b/gfs-kernel/src/gfs/ops_fstype.h
> index 8f6f0f7..d31225e 100644
> --- a/gfs-kernel/src/gfs/ops_fstype.h
> +++ b/gfs-kernel/src/gfs/ops_fstype.h
> @@ -16,7 +16,6 @@
>  
>  int gfs_sys_init(void);
>  void gfs_sys_uninit(void);
> -void gfs_sys_fs_del(struct gfs_sbd *sdp);
>  int gfs_test_bdev_super(struct super_block *sb, void *data);
>  int gfs_set_bdev_super(struct super_block *sb, void *data);
>  int init_names(struct gfs_sbd *sdp, int silent);
> diff --git a/gfs-kernel/src/gfs/ops_super.c b/gfs-kernel/src/gfs/ops_super.c
> index 1fa7faf..c111a2e 100644
> --- a/gfs-kernel/src/gfs/ops_super.c
> +++ b/gfs-kernel/src/gfs/ops_super.c
> @@ -167,17 +167,13 @@ gfs_put_super(struct super_block *sb)
>  
>  	/*  At this point, we're through participating in the lockspace  */
>  
> -	gfs_sys_fs_del(sdp);
> -
>  	gfs_clear_dirty_j(sdp);
>  
>  	/*  Get rid of any extra inodes  */
>  	while (invalidate_inodes(sb))
>  		yield();
>  
> -	vfree(sdp);
> -
> -	set_v2sdp(sb, NULL);
> +	gfs_sys_fs_del(sdp);
>  }
>  
>  /**
> diff --git a/gfs-kernel/src/gfs/sys.c b/gfs-kernel/src/gfs/sys.c
> index 81fe82b..3079cae 100644
> --- a/gfs-kernel/src/gfs/sys.c
> +++ b/gfs-kernel/src/gfs/sys.c
> @@ -20,6 +20,7 @@
>  #include <linux/buffer_head.h>
>  #include <linux/proc_fs.h>
>  #include <linux/module.h>
> +#include <linux/vmalloc.h>
>  #include <asm/uaccess.h>
>  
>  #include "gfs.h"
> @@ -75,12 +76,20 @@ static ssize_t gfs_attr_store(struct kobject *kobj, struct attribute *attr,
>  	return a->store ? a->store(sdp, buf, len) : len;
>  }
>  
> +static void gfs_sbd_release(struct kobject *kobj)
> +{
> +       struct gfs_sbd *sdp = container_of(kobj, struct gfs_sbd, sd_kobj);
> +
> +       vfree(sdp);
> +}
> +
>  static struct sysfs_ops gfs_attr_ops = {
>  	.show  = gfs_attr_show,
>  	.store = gfs_attr_store,
>  };
>  
>  static struct kobj_type gfs_ktype = {
> +	.release = gfs_sbd_release,
>  	.default_attrs = gfs_attrs,
>  	.sysfs_ops     = &gfs_attr_ops,
>  };
> @@ -93,6 +102,7 @@ static struct kset gfs_kset = {
>  
>  int gfs_sys_fs_add(struct gfs_sbd *sdp)
>  {
> +	struct super_block *sb;
>  	int error;
>  
>  	sdp->sd_kobj.kset = &gfs_kset;
> @@ -109,6 +119,10 @@ int gfs_sys_fs_add(struct gfs_sbd *sdp)
>  	return 0;
>  
>   fail:
> +	printk("GFS: error %d adding sysfs files", error);
> +	sb = sdp->sd_vfs;
> +	vfree(sdp);
> +	sb->s_fs_info = NULL;
>  	return error;
>  }
>  
>