| Message ID | 20220601092840.19986-1-mateusz.kusiak@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jes Sorensen |
| Headers | show |
| Series | [v2] Grow: Split Grow_reshape into helper function. | expand |
Dear Mateusz, Am 01.06.22 um 11:28 schrieb Mateusz Kusiak: Could you please remove the dot/period at the end of the commit message summary [1]? > Grow_reshape should be splitted into helper functions given it's size. 1. be split 2. s/it's/its/ > Add helper function for preparing reshape on external metadata. > Close cfd file descriptor. Maybe format this as a list. > Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com> > --- > > Changes since v1: > - changed int val to int *val and int index to unsigned char index in > is_bit_set() > - fixed typo in is_bit_set() description > - changed context->array_state to &context->array.state > > --- > 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..6b8daf4a 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 -1 and 1 are returned. What is the difference? > + */ > +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)) { > + 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..7bf9147d 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, unsigned char 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..c13c81d7 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 numbering). > + * > + * Return: bit value. > + */ > +bool is_bit_set(int *val, unsigned char index) > +{ > + if ((*val) & (1 << index)) > + return true; > + return false; Maybe: return (*val) & (1 << index) > +} > + > int dev_open(char *dev, int flags) > { > /* like 'open', but if 'dev' matches %d:%d, create a temp Kind regards, Paul [1]: https://cbea.ms/git-commit/
> 2022年6月1日 19:22，Paul Menzel <pmenzel@molgen.mpg.de> 写道： >> [snipped] >> +/** >> + * is_bit_set() - get bit value by index. >> + * @val: value. >> + * @index: index of the bit (LSB numbering). >> + * >> + * Return: bit value. >> + */ >> +bool is_bit_set(int *val, unsigned char index) >> +{ >> + if ((*val) & (1 << index)) >> + return true; >> + return false; > > Maybe: > > return (*val) & (1 << index) The above line doesn’t return bool value range, otherwise it returns !!((*val) & (1 << index). I am OK with either of them, what the patch does is fine to me. Coly Li [snipped]
diff --git a/Grow.c b/Grow.c index 9c6fc95e..6b8daf4a 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)) { + 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..7bf9147d 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, unsigned char 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..c13c81d7 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 numbering). + * + * Return: bit value. + */ +bool is_bit_set(int *val, unsigned char index) +{ + if ((*val) & (1 << index)) + return true; + return false; +} + int dev_open(char *dev, int flags) { /* like 'open', but if 'dev' matches %d:%d, create a temp
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@intel.com> --- Changes since v1: - changed int val to int *val and int index to unsigned char index in is_bit_set() - fixed typo in is_bit_set() description - changed context->array_state to &context->array.state --- Grow.c | 124 ++++++++++++++++++++++++++++++-------------------------- mdadm.h | 1 + util.c | 14 +++++++ 3 files changed, 81 insertions(+), 58 deletions(-)