Hi Mateusz,
I reply my comments in line.
> 2022年4月4日 15:17，Mateusz Kusiak <mateusz.kusiak@xxxxxxxxx> 写道：
>
> Grow_reshape should be splitted into helper functions given it's size.
> Add helper function for preparing reshape on external metadata.
> Close cfd file descriptor.
>
> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@xxxxxxxxx>
> ---
> Grow.c | 124 ++++++++++++++++++++++++++++++--------------------------
> mdadm.h | 1 +
> util.c | 14 +++++++
> 3 files changed, 81 insertions(+), 58 deletions(-)
>
> diff --git a/Grow.c b/Grow.c
> index 9c6fc95e..6bb3d388 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1774,6 +1774,65 @@ static int reshape_container(char *container, char *devname,
> char *backup_file, int verbose,
> int forked, int restart, int freeze_reshape);
>
> +/**
> + * prepare_external_reshape() - prepares update on external metadata if supported.
> + * @devname: Device name.
> + * @subarray: Subarray.
> + * @st: Supertype.
> + * @container: Container.
> + * @cfd: Container file descriptor.
> + *
> + * Function checks that the requested reshape is supported on external metadata,
> + * and performs an initial check that the container holds the pre-requisite
> + * spare devices (mdmon owns final validation).
> + *
> + * Return: 0 on success, else error code
> + */
> +static int prepare_external_reshape(char *devname, char *subarray,
> + struct supertype *st, char *container,
> + const int cfd)
> +{
> + struct mdinfo *cc = NULL;
> + struct mdinfo *content = NULL;
> +
> + if (st->ss->load_container(st, cfd, NULL)) {
> + pr_err("Cannot read superblock for %s\n", devname);
> + return 1;
> + }
> +
> + if (!st->ss->container_content)
> + return -1;
> +
> + cc = st->ss->container_content(st, subarray);
> + for (content = cc; content ; content = content->next) {
> + /*
> + * check if reshape is allowed based on metadata
> + * indications stored in content.array.status
> + */
> + if (is_bit_set(content->array_state, MD_SB_BLOCK_VOLUME) ||
> + is_bit_set(content->array_state, MD_SB_BLOCK_CONTAINER_RESHAPE)) {
Content->array_state should be content->array.state, or &content->array.stat if the first parameter in is_bit_set() defined as int *val.
> + pr_err("Cannot reshape arrays in container with unsupported metadata: %s(%s)\n",
> + devname, container);
> + goto error;
> + }
> + if (content->consistency_policy == CONSISTENCY_POLICY_PPL) {
> + pr_err("Operation not supported when ppl consistency policy is enabled\n");
> + goto error;
> + }
> + if (content->consistency_policy == CONSISTENCY_POLICY_BITMAP) {
> + pr_err("Operation not supported when write-intent bitmap consistency policy is enabled\n");
> + goto error;
> + }
> + }
> + sysfs_free(cc);
> + if (mdmon_running(container))
> + st->update_tail = &st->updates;
> + return 0;
> +error:
> + sysfs_free(cc);
> + return 1;
> +}
> +
> int Grow_reshape(char *devname, int fd,
> struct mddev_dev *devlist,
> unsigned long long data_offset,
> @@ -1801,7 +1860,7 @@ int Grow_reshape(char *devname, int fd,
> struct supertype *st;
> char *subarray = NULL;
>
> - int frozen;
> + int frozen = 0;
> int changed = 0;
> char *container = NULL;
> int cfd = -1;
> @@ -1810,7 +1869,7 @@ int Grow_reshape(char *devname, int fd,
> int added_disks;
>
> struct mdinfo info;
> - struct mdinfo *sra;
> + struct mdinfo *sra = NULL;
>
> if (md_get_array_info(fd, &array) < 0) {
> pr_err("%s is not an active md array - aborting\n",
> @@ -1867,13 +1926,7 @@ int Grow_reshape(char *devname, int fd,
> }
> }
>
> - /* in the external case we need to check that the requested reshape is
> - * supported, and perform an initial check that the container holds the
> - * pre-requisite spare devices (mdmon owns final validation)
> - */
> if (st->ss->external) {
> - int retval;
> -
> if (subarray) {
> container = st->container_devnm;
> cfd = open_dev_excl(st->container_devnm);
> @@ -1889,58 +1942,13 @@ int Grow_reshape(char *devname, int fd,
> return 1;
> }
>
> - retval = st->ss->load_container(st, cfd, NULL);
> -
> - if (retval) {
> - pr_err("Cannot read superblock for %s\n", devname);
> + rv = prepare_external_reshape(devname, subarray, st,
> + container, cfd);
> + if (rv > 0) {
> free(subarray);
> - return 1;
> - }
> -
> - /* check if operation is supported for metadata handler */
> - if (st->ss->container_content) {
> - struct mdinfo *cc = NULL;
> - struct mdinfo *content = NULL;
> -
> - cc = st->ss->container_content(st, subarray);
> - for (content = cc; content ; content = content->next) {
> - int allow_reshape = 1;
> -
> - /* check if reshape is allowed based on metadata
> - * indications stored in content.array.status
> - */
> - if (content->array.state &
> - (1 << MD_SB_BLOCK_VOLUME))
> - allow_reshape = 0;
> - if (content->array.state &
> - (1 << MD_SB_BLOCK_CONTAINER_RESHAPE))
> - allow_reshape = 0;
> - if (!allow_reshape) {
> - pr_err("cannot reshape arrays in container with unsupported metadata: %s(%s)\n",
> - devname, container);
> - sysfs_free(cc);
> - free(subarray);
> - return 1;
> - }
> - if (content->consistency_policy ==
> - CONSISTENCY_POLICY_PPL) {
> - pr_err("Operation not supported when ppl consistency policy is enabled\n");
> - sysfs_free(cc);
> - free(subarray);
> - return 1;
> - }
> - if (content->consistency_policy ==
> - CONSISTENCY_POLICY_BITMAP) {
> - pr_err("Operation not supported when write-intent bitmap is enabled\n");
> - sysfs_free(cc);
> - free(subarray);
> - return 1;
> - }
> - }
> - sysfs_free(cc);
> + close(cfd);
> + goto release;
> }
> - if (mdmon_running(container))
> - st->update_tail = &st->updates;
> }
>
> added_disks = 0;
> diff --git a/mdadm.h b/mdadm.h
> index c7268a71..6478a399 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1528,6 +1528,7 @@ extern int stat_is_blkdev(char *devname, dev_t *rdev);
> extern bool is_dev_alive(char *path);
> extern int get_mdp_major(void);
> extern int get_maj_min(char *dev, int *major, int *minor);
> +extern bool is_bit_set(int val, int index);
> extern int dev_open(char *dev, int flags);
> extern int open_dev(char *devnm);
> extern void reopen_mddev(int mdfd);
> diff --git a/util.c b/util.c
> index 3d05d074..3ebb48a1 100644
> --- a/util.c
> +++ b/util.c
> @@ -1028,6 +1028,20 @@ int get_maj_min(char *dev, int *major, int *minor)
> *e == 0);
> }
>
> +/**
> + * is_bit_set() - get bit value by index.
> + * @val: value.
> + * @index: index of the bit (LSB numering).
> + *
> + * Return: bit value.
> + */
> +bool is_bit_set(int val, int index)
> +{
> + if (val & (1 << index))
> + return true;
> + return false;
> +}
I suggest to define the first parameter to int *val, to avoid unnecessary memory copy. And for the index parameter, 32bit value for bit position index might be too large, unsigned char should be enough already (which indicates 255 bits offset).
For the rested part, they look fine with me.
Thanks.
Coly Li
> +
> int dev_open(char *dev, int flags)
> {
> /* like 'open', but if 'dev' matches %d:%d, create a temp
> --
> 2.26.2
>
