Re: [PATCH 2/4] md-cluster: remove capabilities

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 5181 bytes --]

On Wed, 8 Apr 2015 14:22:47 -0500 Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:

> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Hi Goldwyn,
 where really need to be more words here.  What is the purpose of this
 "remove capabilities"? How are they used?


> ---
>  drivers/md/md-cluster.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/md/md-cluster.h |  1 +
>  drivers/md/md.c         | 24 +++++++++++++++---------
>  drivers/md/md.h         |  1 +
>  4 files changed, 64 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 96679b2..d036c83 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -72,6 +72,7 @@ enum msg_type {
>  	METADATA_UPDATED = 0,
>  	RESYNCING,
>  	NEWDISK,
> +	REMOVE,
>  };
>  
>  struct cluster_msg {
> @@ -186,6 +187,20 @@ static char *pretty_uuid(char *dest, char *src)
>  	return dest;
>  }
>  
> +static struct md_rdev *find_rdev_uuid(struct mddev *mddev, char *uuid)
> +{
> +	struct md_rdev *rdev;
> +	struct mdp_superblock_1 *sb;
> +
> +	rdev_for_each_rcu(rdev, mddev) {
> +		sb = page_address(rdev->sb_page);
> +		if (!strncmp(uuid, sb->device_uuid, 16)) {
> +			return rdev;
> +		}
> +	}
> +	return NULL;
> +}

I'm not at all comfortable about this.

Any code that "knows" about a particular metadata format should be accessed
through the super_types[] array.

I think I would rather you used 'desc_nr' to identify the device to be
removed.  This number is determined from the metadata so every node will see
the same number for the same device.  And it is independent of metadata type.

> +
>  static void add_resync_info(struct mddev *mddev, struct dlm_lock_resource *lockres,
>  		sector_t lo, sector_t hi)
>  {
> @@ -401,6 +416,17 @@ static void process_metadata_update(struct mddev *mddev, struct cluster_msg *msg
>  	dlm_lock_sync(cinfo->no_new_dev_lockres, DLM_LOCK_CR);
>  }
>  
> +static void process_remove_disk(struct mddev *mddev, struct cluster_msg *msg)
> +{
> +	struct md_rdev *rdev = find_rdev_uuid(mddev, msg->uuid);
> +	char uuid[32];
> +
> +	if (rdev)
> +		md_kick_rdev_from_array(rdev);
> +	else
> +		pr_warn("%s: %d Could not find disk with uuid: %s", __func__, __LINE__, pretty_uuid(uuid, msg->uuid));
> +}
> +
>  static void process_recvd_msg(struct mddev *mddev, struct cluster_msg *msg)
>  {
>  	switch (msg->type) {
> @@ -419,6 +445,15 @@ static void process_recvd_msg(struct mddev *mddev, struct cluster_msg *msg)
>  		pr_info("%s: %d Received message: NEWDISK from %d\n",
>  			__func__, __LINE__, msg->slot);
>  		process_add_new_disk(mddev, msg);
> +		break;
> +	case REMOVE:
> +		pr_info("%s: %d Received REMOVE from %d\n",
> +			__func__, __LINE__, msg->slot);
> +		process_remove_disk(mddev, msg);
> +		break;
> +	default:
> +		pr_warn("%s:%d Received unknown message from %d\n",
> +			__func__, __LINE__, msg->slot);
>  	};
>  }
>  
> @@ -854,6 +889,17 @@ static int new_disk_ack(struct mddev *mddev, bool ack)
>  	return 0;
>  }
>  
> +static int remove_disk(struct mddev *mddev, struct md_rdev *rdev)
> +{
> +	struct cluster_msg cmsg;
> +	struct md_cluster_info *cinfo = mddev->cluster_info;
> +	struct mdp_superblock_1 *sb = page_address(rdev->sb_page);
> +	char *uuid = sb->device_uuid;
> +	cmsg.type = REMOVE;
> +	memcpy(cmsg.uuid, uuid, 16);
> +	return __sendmsg(cinfo, &cmsg);
> +}
> +
>  static struct md_cluster_operations cluster_ops = {
>  	.join   = join,
>  	.leave  = leave,
> @@ -868,6 +914,7 @@ static struct md_cluster_operations cluster_ops = {
>  	.add_new_disk_start = add_new_disk_start,
>  	.add_new_disk_finish = add_new_disk_finish,
>  	.new_disk_ack = new_disk_ack,
> +	.remove_disk = remove_disk,
>  };
>  
>  static int __init cluster_init(void)
> diff --git a/drivers/md/md-cluster.h b/drivers/md/md-cluster.h
> index 7417133..71e5143 100644
> --- a/drivers/md/md-cluster.h
> +++ b/drivers/md/md-cluster.h
> @@ -22,6 +22,7 @@ struct md_cluster_operations {
>  	int (*add_new_disk_start)(struct mddev *mddev, struct md_rdev *rdev);
>  	int (*add_new_disk_finish)(struct mddev *mddev);
>  	int (*new_disk_ack)(struct mddev *mddev, bool ack);
> +	int (*remove_disk)(struct mddev *mddev, struct md_rdev *rdev);
>  };
>  
>  #endif /* _MD_CLUSTER_H */
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index bc11551..0c65e51 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -2291,11 +2291,12 @@ static void export_rdev(struct md_rdev * rdev)
>  	kobject_put(&rdev->kobj);
>  }
>  
> -static void kick_rdev_from_array(struct md_rdev * rdev)
> +void md_kick_rdev_from_array(struct md_rdev * rdev)
>  {
>  	unbind_rdev_from_array(rdev);
>  	export_rdev(rdev);
>  }
> +EXPORT_SYMBOL_GPL(md_kick_rdev_from_array);

You didn't create this patch against upstream code did you?  The space
between '*' and 'rdev' disappeared in 3.18.
I think I would prefer a separate patch which renames and exports
kick_rdev_from_array, and then a much smaller (easier to review) patch which
adds the 'remove capabilities'.

Thanks,
NeilBrown



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]