diff mbox series

[v3] Grow: Split Grow_reshape into helper function

Message ID 20220609074101.14132-1-mateusz.kusiak@intel.com (mailing list archive)
State Mainlined, archived
Headers show
Series [v3] Grow: Split Grow_reshape into helper function | expand

Commit Message

Mateusz Kusiak June 9, 2022, 7:41 a.m. UTC
Grow_reshape should be split into helper functions given its 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 v2:
- removed dot from commit message
- formatted commit description as a list
- got rid of returning -1 in prepare_external_reshape()
- changed "return" section in prepare_external_reshape() description

---
 Grow.c  | 124 ++++++++++++++++++++++++++++++--------------------------
 mdadm.h |   1 +
 util.c  |  14 +++++++
 3 files changed, 81 insertions(+), 58 deletions(-)

Comments

Coly Li June 20, 2022, 5:21 p.m. UTC | #1
> 2022年6月9日 15:41,Mateusz Kusiak <mateusz.kusiak@intel.com> 写道:
> 
> Grow_reshape should be split into helper functions given its size.
> - Add helper function for preparing reshape on external metadata.
> - Close cfd file descriptor.
> 
> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>

Hi Mateusz,

Overall I am fine with this patch. Currently it dose apply on branch 20220621-testing of the mdadm-CI tree. This branch is based on mdadm upstream commit 756a15f32338 (“imsm: Remove possibility for get_imsm_dev to return NULL”) and stacked with other asked patch from for-jes/20220620.

I will response this patch after other queuing patches are handled by Jes. If the change is minor, I will do the patch rebase and inform you.

Thanks.

Coly Li

> ---
> 
> Changes since v2:
> - removed dot from commit message
> - formatted commit description as a list
> - got rid of returning -1 in prepare_external_reshape()
> - changed "return" section in prepare_external_reshape() description
> 
> ---
> Grow.c  | 124 ++++++++++++++++++++++++++++++--------------------------
> mdadm.h |   1 +
> util.c  |  14 +++++++
> 3 files changed, 81 insertions(+), 58 deletions(-)
> 

[snipped]
Coly Li June 28, 2022, 3:56 p.m. UTC | #2
> 2022年6月21日 01:21,Coly Li <colyli@suse.de> 写道:
> 
> 
> 
>> 2022年6月9日 15:41,Mateusz Kusiak <mateusz.kusiak@intel.com> 写道:
>> 
>> Grow_reshape should be split into helper functions given its size.
>> - Add helper function for preparing reshape on external metadata.
>> - Close cfd file descriptor.
>> 
>> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
> 
> Hi Mateusz,
> 
> Overall I am fine with this patch. Currently it dose apply on branch 20220621-testing of the mdadm-CI tree. This branch is based on mdadm upstream commit 756a15f32338 (“imsm: Remove possibility for get_imsm_dev to return NULL”) and stacked with other asked patch from for-jes/20220620.
> 
> I will response this patch after other queuing patches are handled by Jes. If the change is minor, I will do the patch rebase and inform you.

Hi Mateusz,

I just rebased the patch and pushed to mdadm-CI tree into branch 20220621-testing. This is the only patch in this testing branch. It looks fine to me, but can you help to confirm whether the rebase is correctly? The conflict happens in Grow.c:Grow_reshap().

Thanks.

Coly Li
Mateusz Kusiak June 29, 2022, 1:12 p.m. UTC | #3
On 28/06/2022 17:56, Coly Li wrote:
> 
> 
>> 2022年6月21日 01:21,Coly Li <colyli@suse.de> 写道:
>>
>>
>>
>>> 2022年6月9日 15:41,Mateusz Kusiak <mateusz.kusiak@intel.com> 写道:
>>>
>>> Grow_reshape should be split into helper functions given its size.
>>> - Add helper function for preparing reshape on external metadata.
>>> - Close cfd file descriptor.
>>>
>>> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
>>
>> Hi Mateusz,
>>
>> Overall I am fine with this patch. Currently it dose apply on branch 20220621-testing of the mdadm-CI tree. This branch is based on mdadm upstream commit 756a15f32338 (“imsm: Remove possibility for get_imsm_dev to return NULL”) and stacked with other asked patch from for-jes/20220620.
>>
>> I will response this patch after other queuing patches are handled by Jes. If the change is minor, I will do the patch rebase and inform you.
> 
> Hi Mateusz,
> 
> I just rebased the patch and pushed to mdadm-CI tree into branch 20220621-testing. This is the only patch in this testing branch. It looks fine to me, but can you help to confirm whether the rebase is correctly? The conflict happens in Grow.c:Grow_reshap().
> 
> Thanks.
> 
> Coly Li
> 

Hi Coly,
I checked the conflict myself, and the changes on the branch also look
fine for me.

Thanks,
Mateusz
Coly Li June 29, 2022, 1:14 p.m. UTC | #4
> 2022年6月29日 21:12,Kusiak, Mateusz <mateusz.kusiak@linux.intel.com> 写道:
> 
> On 28/06/2022 17:56, Coly Li wrote:
>> 
>> 
>>> 2022年6月21日 01:21,Coly Li <colyli@suse.de> 写道:
>>> 
>>> 
>>> 
>>>> 2022年6月9日 15:41,Mateusz Kusiak <mateusz.kusiak@intel.com> 写道:
>>>> 
>>>> Grow_reshape should be split into helper functions given its size.
>>>> - Add helper function for preparing reshape on external metadata.
>>>> - Close cfd file descriptor.
>>>> 
>>>> Signed-off-by: Mateusz Kusiak <mateusz.kusiak@intel.com>
>>> 
>>> Hi Mateusz,
>>> 
>>> Overall I am fine with this patch. Currently it dose apply on branch 20220621-testing of the mdadm-CI tree. This branch is based on mdadm upstream commit 756a15f32338 (“imsm: Remove possibility for get_imsm_dev to return NULL”) and stacked with other asked patch from for-jes/20220620.
>>> 
>>> I will response this patch after other queuing patches are handled by Jes. If the change is minor, I will do the patch rebase and inform you.
>> 
>> Hi Mateusz,
>> 
>> I just rebased the patch and pushed to mdadm-CI tree into branch 20220621-testing. This is the only patch in this testing branch. It looks fine to me, but can you help to confirm whether the rebase is correctly? The conflict happens in Grow.c:Grow_reshap().
>> 
>> Thanks.
>> 
>> Coly Li
>> 
> 
> Hi Coly,
> I checked the conflict myself, and the changes on the branch also look
> fine for me.


Hi Mateusz,

Copied, I will submit this rebased patch with my Acked-by to Jes wihtin 1 week, with other patches (if there is).

Thanks.

Coly Li
Jes Sorensen Aug. 7, 2022, 8:41 p.m. UTC | #5
On 6/9/22 03:41, Mateusz Kusiak wrote:
> Grow_reshape should be split into helper functions given its 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 v2:
> - removed dot from commit message
> - formatted commit description as a list
> - got rid of returning -1 in prepare_external_reshape()
> - changed "return" section in prepare_external_reshape() description

Hi Mateusz,

Changes look good to me, but it no longer applies. Mind sending an 
updated version?

Thanks,
Jes
Coly Li Aug. 8, 2022, 10:03 a.m. UTC | #6
> 2022年8月8日 04:41,Jes Sorensen <jes@trained-monkey.org> 写道:
> 
> On 6/9/22 03:41, Mateusz Kusiak wrote:
>> Grow_reshape should be split into helper functions given its 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 v2:
>> - removed dot from commit message
>> - formatted commit description as a list
>> - got rid of returning -1 in prepare_external_reshape()
>> - changed "return" section in prepare_external_reshape() description
> 
> Hi Mateusz,
> 
> Changes look good to me, but it no longer applies. Mind sending an updated version?

Hi Jes,

Please check the version I post to you in series “mdadm-CI for-jes/20220728: patches for merge” (Message-Id: <20220728122101.28744-1-colyli@suse.de>), the patch in this series is rebased and confirmed with Mateusz, it could be applied to upstream mdadm.

Thanks.

Coly Li
Jes Sorensen Aug. 15, 2022, 7:18 p.m. UTC | #7
On 8/8/22 06:03, Coly Li wrote:
> 
> 
>> 2022年8月8日 04:41,Jes Sorensen <jes@trained-monkey.org> 写道:
>>
>> On 6/9/22 03:41, Mateusz Kusiak wrote:
>>> Grow_reshape should be split into helper functions given its 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 v2:
>>> - removed dot from commit message
>>> - formatted commit description as a list
>>> - got rid of returning -1 in prepare_external_reshape()
>>> - changed "return" section in prepare_external_reshape() description
>>
>> Hi Mateusz,
>>
>> Changes look good to me, but it no longer applies. Mind sending an updated version?
> 
> Hi Jes,
> 
> Please check the version I post to you in series “mdadm-CI for-jes/20220728: patches for merge” (Message-Id: <20220728122101.28744-1-colyli@suse.de>), the patch in this series is rebased and confirmed with Mateusz, it could be applied to upstream mdadm.

Hi Coly,

It is really painful for me to pull individual patches out of a large
set like that. It is much easier to deal with acks and updated patches
and being able to pull in smaller sets via patchworks.

Thanks,
Jes
Jes Sorensen Aug. 24, 2022, 3:56 p.m. UTC | #8
On 8/8/22 06:03, Coly Li wrote:
> 
> 
>> 2022年8月8日 04:41,Jes Sorensen <jes@trained-monkey.org> 写道:
>>
>> On 6/9/22 03:41, Mateusz Kusiak wrote:
>>> Grow_reshape should be split into helper functions given its 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 v2:
>>> - removed dot from commit message
>>> - formatted commit description as a list
>>> - got rid of returning -1 in prepare_external_reshape()
>>> - changed "return" section in prepare_external_reshape() description
>>
>> Hi Mateusz,
>>
>> Changes look good to me, but it no longer applies. Mind sending an updated version?
> 
> Hi Jes,
> 
> Please check the version I post to you in series “mdadm-CI for-jes/20220728: patches for merge” (Message-Id: <20220728122101.28744-1-colyli@suse.de>), the patch in this series is rebased and confirmed with Mateusz, it could be applied to upstream mdadm.

Hi

I applied this one, but none of the versions applied cleanly. I had to
play formail games to pull it out of your stack, as I am not going to
apply a set of 23 commits in one batch without going through them.

It's really awesome to have your help reviewing patches, much
appreciated, but I would prefer to keep them in the original batches so
I can pull them from patchwork, rather than trying to deal with the
giant stack.

Best regards,
Jes
Coly Li Aug. 24, 2022, 4:09 p.m. UTC | #9
> 2022年8月24日 23:56,Jes Sorensen <jes@trained-monkey.org> 写道:
> 
> On 8/8/22 06:03, Coly Li wrote:
>> 
>> 
>>> 2022年8月8日 04:41,Jes Sorensen <jes@trained-monkey.org> 写道:
>>> 
>>> On 6/9/22 03:41, Mateusz Kusiak wrote:
>>>> Grow_reshape should be split into helper functions given its 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 v2:
>>>> - removed dot from commit message
>>>> - formatted commit description as a list
>>>> - got rid of returning -1 in prepare_external_reshape()
>>>> - changed "return" section in prepare_external_reshape() description
>>> 
>>> Hi Mateusz,
>>> 
>>> Changes look good to me, but it no longer applies. Mind sending an updated version?
>> 
>> Hi Jes,
>> 
>> Please check the version I post to you in series “mdadm-CI for-jes/20220728: patches for merge” (Message-Id: <20220728122101.28744-1-colyli@suse.de>), the patch in this series is rebased and confirmed with Mateusz, it could be applied to upstream mdadm.
> 
> Hi
> 

> I applied this one, but none of the versions applied cleanly. I had to
> play formail games to pull it out of your stack, as I am not going to
> apply a set of 23 commits in one batch without going through them.
> 

These days I was in partner’s office and planed to repost the rebased version soon. If you don’t do the rebase yet, please wait for me to post a v4 version on behavior of Mateusz tomorrow.

> It's really awesome to have your help reviewing patches, much
> appreciated, but I would prefer to keep them in the original batches so
> I can pull them from patchwork, rather than trying to deal with the
> giant stack.

How about we improve the process like this,
1) I will continue to review and response the patches from the original emails, so patch work may track them as they were.
2) For all the reviewed patches are not handled by your after a period, let’s set it as 2 weeks for now, I will post a email with all the patches with their message-IDs to you as a remind.

Thanks.

Coly Li
Jes Sorensen Aug. 24, 2022, 4:33 p.m. UTC | #10
On 8/24/22 12:09, Coly Li wrote:
> 
> 
>> 2022年8月24日 23:56,Jes Sorensen <jes@trained-monkey.org> 写道:
>>> Hi Jes,
>>>
>>> Please check the version I post to you in series “mdadm-CI for-jes/20220728: patches for merge” (Message-Id: <20220728122101.28744-1-colyli@suse.de>), the patch in this series is rebased and confirmed with Mateusz, it could be applied to upstream mdadm.
> 
>> I applied this one, but none of the versions applied cleanly. I had to
>> play formail games to pull it out of your stack, as I am not going to
>> apply a set of 23 commits in one batch without going through them.
> 
> These days I was in partner’s office and planed to repost the rebased version soon. If you don’t do the rebase yet, please wait for me to post a v4 version on behavior of Mateusz tomorrow.

No worries, I already pulled some of it in, but you can check my repo
and see whats there.

>> It's really awesome to have your help reviewing patches, much
>> appreciated, but I would prefer to keep them in the original batches so
>> I can pull them from patchwork, rather than trying to deal with the
>> giant stack.
> 
> How about we improve the process like this,
> 1) I will continue to review and response the patches from the original emails, so patch work may track them as they were.
> 2) For all the reviewed patches are not handled by your after a period, let’s set it as 2 weeks for now, I will post a email with all the patches with their message-IDs to you as a remind.

This sounds good to me! I think it's also fine to have a branch with
everything applied for testing, it's just less easy for me to pull from.

Thanks,
Jes
Coly Li Aug. 24, 2022, 4:41 p.m. UTC | #11
> 2022年8月25日 00:33,Jes Sorensen <jes@trained-monkey.org> 写道:
> 
> On 8/24/22 12:09, Coly Li wrote:
>> 
>> 
>>> 2022年8月24日 23:56,Jes Sorensen <jes@trained-monkey.org> 写道:
>>>> Hi Jes,
>>>> 
>>>> Please check the version I post to you in series “mdadm-CI for-jes/20220728: patches for merge” (Message-Id: <20220728122101.28744-1-colyli@suse.de>), the patch in this series is rebased and confirmed with Mateusz, it could be applied to upstream mdadm.
>> 
>>> I applied this one, but none of the versions applied cleanly. I had to
>>> play formail games to pull it out of your stack, as I am not going to
>>> apply a set of 23 commits in one batch without going through them.
>> 
>> These days I was in partner’s office and planed to repost the rebased version soon. If you don’t do the rebase yet, please wait for me to post a v4 version on behavior of Mateusz tomorrow.
> 
> No worries, I already pulled some of it in, but you can check my repo
> and see whats there.
> 
>>> It's really awesome to have your help reviewing patches, much
>>> appreciated, but I would prefer to keep them in the original batches so
>>> I can pull them from patchwork, rather than trying to deal with the
>>> giant stack.
>> 
>> How about we improve the process like this,
>> 1) I will continue to review and response the patches from the original emails, so patch work may track them as they were.
>> 2) For all the reviewed patches are not handled by your after a period, let’s set it as 2 weeks for now, I will post a email with all the patches with their message-IDs to you as a remind.
> 
> This sounds good to me! I think it's also fine to have a branch with
> everything applied for testing, it's just less easy for me to pull from.

Yes there is already. Every time when I post the reviewed patches to you, there is a branch for all the patches. For example currently the branches in mdadm-CI tree,
  remotes/origin/20220315-testing
  remotes/origin/20220406-testing
  remotes/origin/20220719-testing
  remotes/origin/for-jes/20220402
  remotes/origin/for-jes/20220620
  remotes/origin/for-jes/20220728
  remotes/origin/master

All the branches in for-jes/ are the patches I posted to you as email thread. You may take any one for your convenience. And the latest branch will be rebased against the latest mdadm tree before I post the remind email to you.

BTW, the mdadm-CI tree can be found from git://git.kernel.org/pub/scm/linux/kernel/git/colyli/mdadm.git

Thanks.

Coly Li
diff mbox series

Patch

diff --git a/Grow.c b/Grow.c
index 9c6fc95e..c247e11b 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 1
+ */
+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