[PATCH v2 0/2] mdadm: Fix array size mismatch after grow

[PATCH v2 0/2] mdadm: Fix array size mismatch after grow

[Lukasz Florczak]

Changes in v2:
- split commit into two,
- explain dead code part,
- reformat commit body to wrap text after 72 characters.

Lukasz Florczak (2):
  mdadm: Fix array size mismatch after grow
  mdadm: Remove dead code in imsm_fix_size_mismatch

 super-intel.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

-- 
2.27.0

[PATCH 1/2] mdadm: Fix array size mismatch after grow

[Lukasz Florczak]

imsm_fix_size_mismatch() is invoked to fix the problem, but it couldn't
proceed due to migration check. This patch allows for intended behavior.

Signed-off-by: Lukasz Florczak <lukasz.florczak@linux.intel.com>
---
 super-intel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/super-intel.c b/super-intel.c
index d5fad102..102689bc 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -11757,7 +11757,7 @@ static int imsm_fix_size_mismatch(struct supertype *st, int subarray_index)
 		unsigned long long d_size = imsm_dev_size(dev);
 		int u_size;
 
-		if (calc_size == d_size || dev->vol.migr_type == MIGR_GEN_MIGR)
+		if (calc_size == d_size)
 			continue;
 
 		/* There is a difference, confirm that imsm_dev_size is
-- 
2.27.0

[PATCH 2/2] mdadm: Remove dead code in imsm_fix_size_mismatch

[Lukasz Florczak]

imsm_create_metadata_update_for_size_change() that returns u_size value
could return 0 in the past. As its behavior changed, and returned value
is always the size of imsm_update_size_change structure, check for
u_size is no longer needed.

Signed-off-by: Lukasz Florczak <lukasz.florczak@linux.intel.com>
---
 super-intel.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 102689bc..cb5292e1 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -11772,10 +11772,6 @@ static int imsm_fix_size_mismatch(struct supertype *st, int subarray_index)
 		geo.size = d_size;
 		u_size = imsm_create_metadata_update_for_size_change(st, &geo,
 								     &update);
-		if (u_size < 1) {
-			dprintf("imsm: Cannot prepare size change update\n");
-			goto exit;
-		}
 		imsm_update_metadata_locally(st, update, u_size);
 		if (st->update_tail) {
 			append_metadata_update(st, update, u_size);
-- 
2.27.0

Re: [PATCH 1/2] mdadm: Fix array size mismatch after grow

[Coly Li]

Hi Lukasz,


> 2022年4月7日 22:27，Lukasz Florczak <lukasz.florczak@linux.intel.com> 写道：
> 
> imsm_fix_size_mismatch() is invoked to fix the problem, but it couldn't
> proceed due to migration check. This patch allows for intended behavior.


Could you please explain a bit more about why “it couldn’t proceed due to migration”, and what is the “intended behavior”?
It may help me to understand your change and response faster.

Thank you in advance.

Coly Li

> 
> Signed-off-by: Lukasz Florczak <lukasz.florczak@linux.intel.com>
> ---
> super-intel.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/super-intel.c b/super-intel.c
> index d5fad102..102689bc 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -11757,7 +11757,7 @@ static int imsm_fix_size_mismatch(struct supertype *st, int subarray_index)
> 		unsigned long long d_size = imsm_dev_size(dev);
> 		int u_size;
> 
> -		if (calc_size == d_size || dev->vol.migr_type == MIGR_GEN_MIGR)
> +		if (calc_size == d_size)
> 			continue;
> 
> 		/* There is a difference, confirm that imsm_dev_size is
> -- 
> 2.27.0
> 

Re: [PATCH 2/2] mdadm: Remove dead code in imsm_fix_size_mismatch

[Coly Li]

> 2022年4月7日 22:27，Lukasz Florczak <lukasz.florczak@linux.intel.com> 写道：
> 
> imsm_create_metadata_update_for_size_change() that returns u_size value
> could return 0 in the past. As its behavior changed, and returned value
> is always the size of imsm_update_size_change structure, check for
> u_size is no longer needed.
> 
> Signed-off-by: Lukasz Florczak <lukasz.florczak@linux.intel.com>

It looks good to me.

Acked-by: Coly Li <colyli@suse.de>

Thanks.

Coly Li

> ---
> super-intel.c | 4 ----
> 1 file changed, 4 deletions(-)
> 
> diff --git a/super-intel.c b/super-intel.c
> index 102689bc..cb5292e1 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -11772,10 +11772,6 @@ static int imsm_fix_size_mismatch(struct supertype *st, int subarray_index)
> 		geo.size = d_size;
> 		u_size = imsm_create_metadata_update_for_size_change(st, &geo,
> 								     &update);
> -		if (u_size < 1) {
> -			dprintf("imsm: Cannot prepare size change update\n");
> -			goto exit;
> -		}
> 		imsm_update_metadata_locally(st, update, u_size);
> 		if (st->update_tail) {
> 			append_metadata_update(st, update, u_size);
> -- 
> 2.27.0
> 

Re: [PATCH 1/2] mdadm: Fix array size mismatch after grow

[Lukasz Florczak]

On Mon, 30 May 2022 18:01:05 +0800, Coly Li <colyli@suse.de> wrote:

Hi Coly,
> Hi Lukasz,
> 
> 
> > 2022年4月7日 22:27，Lukasz Florczak
> > <lukasz.florczak@linux.intel.com> 写道：
> > 
> > imsm_fix_size_mismatch() is invoked to fix the problem, but it
> > couldn't proceed due to migration check. This patch allows for
> > intended behavior.
> 
> 
> Could you please explain a bit more about why “it couldn’t proceed
> due to migration”, and what is the “intended behavior”? It may help
> me to understand your change and response faster.

The intended behavior here is to fix the array size after grow that is
displayed in mdadm detail, since there can be a mismatch if the raid
was created in EFI [1]. That is the array size is not consistent with
the formula: 
Array_size * block_size = Num_stripes * Chunk_size * Num_of_data_drives 

That fix couldn't happen as the metadata update part was efficiently
omitted with continue statement after the migration type condition was
met. 

About migration I didn't go that much into detail, but it was an issue
that dev->vol.migr_type was still in MIGR_GEN_MIGR state even though
imsm_fix_size_mismatch() was called after migration has been finished,
at least from the mdadm's point of view. That happens because this
value is changed later, afaik, by mdmon. The initial idea here must've
been not to change the array size during migration, but that is not
valid since its state is just not updated yet.

[1]
https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=895ffd992954069e4ea67efb8a85bb0fd72c3707

Thanks,
Lukasz

> 
> Thank you in advance.
> 
> Coly Li
> 
> > 
> > Signed-off-by: Lukasz Florczak <lukasz.florczak@linux.intel.com>
> > ---
> > super-intel.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/super-intel.c b/super-intel.c
> > index d5fad102..102689bc 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -11757,7 +11757,7 @@ static int imsm_fix_size_mismatch(struct
> > supertype *st, int subarray_index) unsigned long long d_size =
> > imsm_dev_size(dev); int u_size;
> > 
> > -		if (calc_size == d_size || dev->vol.migr_type ==
> > MIGR_GEN_MIGR)
> > +		if (calc_size == d_size)
> > 			continue;
> > 
> > 		/* There is a difference, confirm that
> > imsm_dev_size is -- 
> > 2.27.0
> > 
> 

Re: [PATCH 1/2] mdadm: Fix array size mismatch after grow

[Coly Li]

> 2022年6月3日 22:30，Lukasz Florczak <lukasz.florczak@linux.intel.com> 写道：
> 
> On Mon, 30 May 2022 18:01:05 +0800, Coly Li <colyli@suse.de> wrote:
> 
> Hi Coly,
>> Hi Lukasz,
>> 
>> 
>>> 2022年4月7日 22:27，Lukasz Florczak
>>> <lukasz.florczak@linux.intel.com> 写道：
>>> 
>>> imsm_fix_size_mismatch() is invoked to fix the problem, but it
>>> couldn't proceed due to migration check. This patch allows for
>>> intended behavior.
>> 
>> 
>> Could you please explain a bit more about why “it couldn’t proceed
>> due to migration”, and what is the “intended behavior”? It may help
>> me to understand your change and response faster.
> 
> The intended behavior here is to fix the array size after grow that is
> displayed in mdadm detail, since there can be a mismatch if the raid
> was created in EFI [1]. That is the array size is not consistent with
> the formula: 
> Array_size * block_size = Num_stripes * Chunk_size * Num_of_data_drives 
> 
> That fix couldn't happen as the metadata update part was efficiently
> omitted with continue statement after the migration type condition was
> met. 
> 
> About migration I didn't go that much into detail, but it was an issue
> that dev->vol.migr_type was still in MIGR_GEN_MIGR state even though
> imsm_fix_size_mismatch() was called after migration has been finished,
> at least from the mdadm's point of view. That happens because this
> value is changed later, afaik, by mdmon. The initial idea here must've
> been not to change the array size during migration, but that is not
> valid since its state is just not updated yet.
> 
> [1]
> https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=895ffd992954069e4ea67efb8a85bb0fd72c3707

Copied, thanks for the hint.

BTW, now I do the imsm related test with IMSM_NO_PLATFORM=1 and IMSM_DEVNAME_AS_SERIAL=1. To test situation as the above text mentioned, do I have to find a real hardware with VROC supported?

Coly Li

Re: [PATCH 1/2] mdadm: Fix array size mismatch after grow

[Lukasz Florczak]

On Tue, 21 Jun 2022 00:28:18 +0800, Coly Li <colyli@suse.de> wrote:

> > 2022年6月3日 22:30，Lukasz Florczak
> > <lukasz.florczak@linux.intel.com> 写道：
> > 
> > On Mon, 30 May 2022 18:01:05 +0800, Coly Li <colyli@suse.de> wrote:
> > 
> > Hi Coly,  
> >> Hi Lukasz,
> >> 
> >>   
> >>> 2022年4月7日 22:27，Lukasz Florczak
> >>> <lukasz.florczak@linux.intel.com> 写道：
> >>> 
> >>> imsm_fix_size_mismatch() is invoked to fix the problem, but it
> >>> couldn't proceed due to migration check. This patch allows for
> >>> intended behavior.  
> >> 
> >> 
> >> Could you please explain a bit more about why “it couldn’t proceed
> >> due to migration”, and what is the “intended behavior”? It may help
> >> me to understand your change and response faster.  
> > 
> > The intended behavior here is to fix the array size after grow that
> > is displayed in mdadm detail, since there can be a mismatch if the
> > raid was created in EFI [1]. That is the array size is not
> > consistent with the formula: 
> > Array_size * block_size = Num_stripes * Chunk_size *
> > Num_of_data_drives 
> > 
> > That fix couldn't happen as the metadata update part was efficiently
> > omitted with continue statement after the migration type condition
> > was met. 
> > 
> > About migration I didn't go that much into detail, but it was an
> > issue that dev->vol.migr_type was still in MIGR_GEN_MIGR state even
> > though imsm_fix_size_mismatch() was called after migration has been
> > finished, at least from the mdadm's point of view. That happens
> > because this value is changed later, afaik, by mdmon. The initial
> > idea here must've been not to change the array size during
> > migration, but that is not valid since its state is just not
> > updated yet.
> > 
> > [1]
> > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=895ffd992954069e4ea67efb8a85bb0fd72c3707
> >  
> 
> Copied, thanks for the hint.
> 
> BTW, now I do the imsm related test with IMSM_NO_PLATFORM=1 and
> IMSM_DEVNAME_AS_SERIAL=1. To test situation as the above text
> mentioned, do I have to find a real hardware with VROC supported?

Hi Coly,

Having a real hardware with VROC support would be the most convenient
solution here. However, you could do a quick hack to overcome this
situation. That would be commenting out the line:
array_size = round_size_to_mb(array_blocks, data_disks);
in init_super_imsm_volume(). Then creating RAID in OS should have the
same size mismatch issues as size per drive won't be aligned to 1MiB
(considering you create raid with size not aligned to 1MiB) - raid
size created in EFI only has to be multiple of sector size and chunk
size.
Hope this helps you.

Thanks,
Lukasz
> 
> Coly Li
>