[PATCH 1/1] mdadm/super1: Add MD_FEATURE_RAID0_LAYOUT if sb->layout is set

[PATCH 1/1] mdadm/super1: Add MD_FEATURE_RAID0_LAYOUT if sb->layout is set

[Xiao Ni]

In kernel space super_1_validate sets mddev->layout to -1 if MD_FEATURE_RAID0_LAYOUT
is not set. MD_FEATURE_RAID0_LAYOUT is set in mdadm write_init_super1. Now only raid
with more than one zone can set this bit. But for raid0 with same size member disks,
it doesn't set this bit. The layout is *unknown* when running mdadm -D command. In
fact it should be RAID0_ORIG_LAYOUT which gets from default_layout.

So set MD_FEATURE_RAID0_LAYOUT when sb->layout has value.

Fixes: 329dfc28debb ('Create: add support for RAID0 layouts.')
Signed-off-by: Xiao Ni <xni@redhat.com>
---
 super1.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/super1.c b/super1.c
index 856b02082662..f29751b4a5c7 100644
--- a/super1.c
+++ b/super1.c
@@ -1978,26 +1978,10 @@ static int write_init_super1(struct supertype *st)
 	unsigned long long sb_offset;
 	unsigned long long data_offset;
 	long bm_offset;
-	int raid0_need_layout = 0;
 
-	for (di = st->info; di; di = di->next) {
+	for (di = st->info; di; di = di->next)
 		if (di->disk.state & (1 << MD_DISK_JOURNAL))
 			sb->feature_map |= __cpu_to_le32(MD_FEATURE_JOURNAL);
-		if (sb->level == 0 && sb->layout != 0) {
-			struct devinfo *di2 = st->info;
-			unsigned long long s1, s2;
-			s1 = di->dev_size;
-			if (di->data_offset != INVALID_SECTORS)
-				s1 -= di->data_offset;
-			s1 /= __le32_to_cpu(sb->chunksize);
-			s2 = di2->dev_size;
-			if (di2->data_offset != INVALID_SECTORS)
-				s2 -= di2->data_offset;
-			s2 /= __le32_to_cpu(sb->chunksize);
-			if (s1 != s2)
-				raid0_need_layout = 1;
-		}
-	}
 
 	for (di = st->info; di; di = di->next) {
 		if (di->disk.state & (1 << MD_DISK_FAULTY))
@@ -2139,8 +2123,7 @@ static int write_init_super1(struct supertype *st)
 			sb->bblog_offset = 0;
 		}
 
-		/* RAID0 needs a layout if devices aren't all the same size */
-		if (raid0_need_layout)
+		if (sb->level == 0 && sb->layout)
 			sb->feature_map |= __cpu_to_le32(MD_FEATURE_RAID0_LAYOUT);
 
 		sb->sb_csum = calc_sb_1_csum(sb);
-- 
2.32.0 (Apple Git-132)

Re: [PATCH 1/1] mdadm/super1: Add MD_FEATURE_RAID0_LAYOUT if sb->layout is set

[Mariusz Tkaczyk]

On Wed, 11 Oct 2023 21:05:22 +0800
Xiao Ni <xni@redhat.com> wrote:

> In kernel space super_1_validate sets mddev->layout to -1 if
> MD_FEATURE_RAID0_LAYOUT is not set. MD_FEATURE_RAID0_LAYOUT is set in mdadm
> write_init_super1. Now only raid with more than one zone can set this bit.
> But for raid0 with same size member disks, it doesn't set this bit. The
> layout is *unknown* when running mdadm -D command. In fact it should be
> RAID0_ORIG_LAYOUT which gets from default_layout.
> 
> So set MD_FEATURE_RAID0_LAYOUT when sb->layout has value.
> 
> Fixes: 329dfc28debb ('Create: add support for RAID0 layouts.')
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  super1.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/super1.c b/super1.c
> index 856b02082662..f29751b4a5c7 100644
> --- a/super1.c
> +++ b/super1.c
> @@ -1978,26 +1978,10 @@ static int write_init_super1(struct supertype *st)
>  	unsigned long long sb_offset;
>  	unsigned long long data_offset;
>  	long bm_offset;
> -	int raid0_need_layout = 0;
>  
> -	for (di = st->info; di; di = di->next) {
> +	for (di = st->info; di; di = di->next)
>  		if (di->disk.state & (1 << MD_DISK_JOURNAL))
>  			sb->feature_map |= __cpu_to_le32(MD_FEATURE_JOURNAL);
> -		if (sb->level == 0 && sb->layout != 0) {
> -			struct devinfo *di2 = st->info;
> -			unsigned long long s1, s2;
> -			s1 = di->dev_size;
> -			if (di->data_offset != INVALID_SECTORS)
> -				s1 -= di->data_offset;
> -			s1 /= __le32_to_cpu(sb->chunksize);
> -			s2 = di2->dev_size;
> -			if (di2->data_offset != INVALID_SECTORS)
> -				s2 -= di2->data_offset;
> -			s2 /= __le32_to_cpu(sb->chunksize);
> -			if (s1 != s2)
> -				raid0_need_layout = 1;
> -		}
> -	}
>  
>  	for (di = st->info; di; di = di->next) {
>  		if (di->disk.state & (1 << MD_DISK_FAULTY))
> @@ -2139,8 +2123,7 @@ static int write_init_super1(struct supertype *st)
>  			sb->bblog_offset = 0;
>  		}
>  
> -		/* RAID0 needs a layout if devices aren't all the same size
> */
> -		if (raid0_need_layout)
> +		if (sb->level == 0 && sb->layout)
>  			sb->feature_map |=
> __cpu_to_le32(MD_FEATURE_RAID0_LAYOUT); 
>  		sb->sb_csum = calc_sb_1_csum(sb);
Hi Xiao,

I read Neil patch:
https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=329dfc28de

For sure Neil has a purpose to make it this way. I think that because it breaks
creation when layout is not supported by kernel. Neil wanted to keep possible
largest compatibility so it sets layout feature only if it is necessary.
Your change forces layout bit to be always used. Can you test this change on
kernel without raid0_layout support? I expect regression for same dev size raid
arrays.

I think that before we will set layout bit we should check kernel
version, it must be higher than 5.4. In the future we would remove this check.
So, it forces the calculations made by Neil back but I think that we can simply
compare dev_size and data_offset between members.

Thanks,
Mariusz

Re: [PATCH 1/1] mdadm/super1: Add MD_FEATURE_RAID0_LAYOUT if sb->layout is set

[Xiao Ni]

On Fri, Oct 13, 2023 at 5:31 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> On Wed, 11 Oct 2023 21:05:22 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
> > In kernel space super_1_validate sets mddev->layout to -1 if
> > MD_FEATURE_RAID0_LAYOUT is not set. MD_FEATURE_RAID0_LAYOUT is set in mdadm
> > write_init_super1. Now only raid with more than one zone can set this bit.
> > But for raid0 with same size member disks, it doesn't set this bit. The
> > layout is *unknown* when running mdadm -D command. In fact it should be
> > RAID0_ORIG_LAYOUT which gets from default_layout.
> >
> > So set MD_FEATURE_RAID0_LAYOUT when sb->layout has value.
> >
> > Fixes: 329dfc28debb ('Create: add support for RAID0 layouts.')
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >  super1.c | 21 ++-------------------
> >  1 file changed, 2 insertions(+), 19 deletions(-)
> >
> > diff --git a/super1.c b/super1.c
> > index 856b02082662..f29751b4a5c7 100644
> > --- a/super1.c
> > +++ b/super1.c
> > @@ -1978,26 +1978,10 @@ static int write_init_super1(struct supertype *st)
> >       unsigned long long sb_offset;
> >       unsigned long long data_offset;
> >       long bm_offset;
> > -     int raid0_need_layout = 0;
> >
> > -     for (di = st->info; di; di = di->next) {
> > +     for (di = st->info; di; di = di->next)
> >               if (di->disk.state & (1 << MD_DISK_JOURNAL))
> >                       sb->feature_map |= __cpu_to_le32(MD_FEATURE_JOURNAL);
> > -             if (sb->level == 0 && sb->layout != 0) {
> > -                     struct devinfo *di2 = st->info;
> > -                     unsigned long long s1, s2;
> > -                     s1 = di->dev_size;
> > -                     if (di->data_offset != INVALID_SECTORS)
> > -                             s1 -= di->data_offset;
> > -                     s1 /= __le32_to_cpu(sb->chunksize);
> > -                     s2 = di2->dev_size;
> > -                     if (di2->data_offset != INVALID_SECTORS)
> > -                             s2 -= di2->data_offset;
> > -                     s2 /= __le32_to_cpu(sb->chunksize);
> > -                     if (s1 != s2)
> > -                             raid0_need_layout = 1;
> > -             }
> > -     }
> >
> >       for (di = st->info; di; di = di->next) {
> >               if (di->disk.state & (1 << MD_DISK_FAULTY))
> > @@ -2139,8 +2123,7 @@ static int write_init_super1(struct supertype *st)
> >                       sb->bblog_offset = 0;
> >               }
> >
> > -             /* RAID0 needs a layout if devices aren't all the same size
> > */
> > -             if (raid0_need_layout)
> > +             if (sb->level == 0 && sb->layout)
> >                       sb->feature_map |=
> > __cpu_to_le32(MD_FEATURE_RAID0_LAYOUT);
> >               sb->sb_csum = calc_sb_1_csum(sb);
> Hi Xiao,
>
> I read Neil patch:
> https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=329dfc28de
>
> For sure Neil has a purpose to make it this way. I think that because it breaks
> creation when layout is not supported by kernel. Neil wanted to keep possible
> largest compatibility so it sets layout feature only if it is necessary.
> Your change forces layout bit to be always used. Can you test this change on
> kernel without raid0_layout support? I expect regression for same dev size raid
> arrays.

Hi Mariusz

Thanks for pointing out this. I only think the kernel which supports
MD_FEATURE_RAID0_LAYOUT

>
> I think that before we will set layout bit we should check kernel
> version, it must be higher than 5.4. In the future we would remove this check.

Let me check if I understand right:

It needs to check raid0_need_layout when <= kernel 5.4. It can set
MD_FEATURE_RAID0_LAYOUT for all raid0 > kernel 5.4


> So, it forces the calculations made by Neil back but I think that we can simply
> compare dev_size and data_offset between members.

We don't need to consider the compatibility anymore in future?

Best Regards
Xiao
>
> Thanks,
> Mariusz
>

Re: [PATCH 1/1] mdadm/super1: Add MD_FEATURE_RAID0_LAYOUT if sb->layout is set

[Mariusz Tkaczyk]

On Fri, 13 Oct 2023 18:59:21 +0800
Xiao Ni <xni@redhat.com> wrote:

> On Fri, Oct 13, 2023 at 5:31 PM Mariusz Tkaczyk
> <mariusz.tkaczyk@linux.intel.com> wrote:
> >
> > On Wed, 11 Oct 2023 21:05:22 +0800
> > Xiao Ni <xni@redhat.com> wrote:
> >  
> > > In kernel space super_1_validate sets mddev->layout to -1 if
> > > MD_FEATURE_RAID0_LAYOUT is not set. MD_FEATURE_RAID0_LAYOUT is set in
> > > mdadm write_init_super1. Now only raid with more than one zone can set
> > > this bit. But for raid0 with same size member disks, it doesn't set this
> > > bit. The layout is *unknown* when running mdadm -D command. In fact it
> > > should be RAID0_ORIG_LAYOUT which gets from default_layout.
> > >
> > > So set MD_FEATURE_RAID0_LAYOUT when sb->layout has value.
> > >
> > > Fixes: 329dfc28debb ('Create: add support for RAID0 layouts.')
> > > Signed-off-by: Xiao Ni <xni@redhat.com>
> > > ---
> > >  super1.c | 21 ++-------------------
> > >  1 file changed, 2 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/super1.c b/super1.c
> > > index 856b02082662..f29751b4a5c7 100644
> > > --- a/super1.c
> > > +++ b/super1.c
> > > @@ -1978,26 +1978,10 @@ static int write_init_super1(struct supertype *st)
> > >       unsigned long long sb_offset;
> > >       unsigned long long data_offset;
> > >       long bm_offset;
> > > -     int raid0_need_layout = 0;
> > >
> > > -     for (di = st->info; di; di = di->next) {
> > > +     for (di = st->info; di; di = di->next)
> > >               if (di->disk.state & (1 << MD_DISK_JOURNAL))
> > >                       sb->feature_map |=
> > > __cpu_to_le32(MD_FEATURE_JOURNAL);
> > > -             if (sb->level == 0 && sb->layout != 0) {
> > > -                     struct devinfo *di2 = st->info;
> > > -                     unsigned long long s1, s2;
> > > -                     s1 = di->dev_size;
> > > -                     if (di->data_offset != INVALID_SECTORS)
> > > -                             s1 -= di->data_offset;
> > > -                     s1 /= __le32_to_cpu(sb->chunksize);
> > > -                     s2 = di2->dev_size;
> > > -                     if (di2->data_offset != INVALID_SECTORS)
> > > -                             s2 -= di2->data_offset;
> > > -                     s2 /= __le32_to_cpu(sb->chunksize);
> > > -                     if (s1 != s2)
> > > -                             raid0_need_layout = 1;
> > > -             }
> > > -     }
> > >
> > >       for (di = st->info; di; di = di->next) {
> > >               if (di->disk.state & (1 << MD_DISK_FAULTY))
> > > @@ -2139,8 +2123,7 @@ static int write_init_super1(struct supertype *st)
> > >                       sb->bblog_offset = 0;
> > >               }
> > >
> > > -             /* RAID0 needs a layout if devices aren't all the same size
> > > */
> > > -             if (raid0_need_layout)
> > > +             if (sb->level == 0 && sb->layout)
> > >                       sb->feature_map |=
> > > __cpu_to_le32(MD_FEATURE_RAID0_LAYOUT);
> > >               sb->sb_csum = calc_sb_1_csum(sb);  
> > Hi Xiao,
> >
> > I read Neil patch:
> > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=329dfc28de
> >
> > For sure Neil has a purpose to make it this way. I think that because it
> > breaks creation when layout is not supported by kernel. Neil wanted to keep
> > possible largest compatibility so it sets layout feature only if it is
> > necessary. Your change forces layout bit to be always used. Can you test
> > this change on kernel without raid0_layout support? I expect regression for
> > same dev size raid arrays.  
> 
> Hi Mariusz
> 
> Thanks for pointing out this. I only think the kernel which supports
> MD_FEATURE_RAID0_LAYOUT
> 
> >
> > I think that before we will set layout bit we should check kernel
> > version, it must be higher than 5.4. In the future we would remove this
> > check.  

> 
> Let me check if I understand right:
> 
> It needs to check raid0_need_layout when <= kernel 5.4. It can set
> MD_FEATURE_RAID0_LAYOUT for all raid0 > kernel 5.4

Correct! I'm accepting risk in extraordinary cases. In general, kernel is bumped
not downgraded.

> 
> 
> > So, it forces the calculations made by Neil back but I think that we can
> > simply compare dev_size and data_offset between members.  
> 
> We don't need to consider the compatibility anymore in future?
> 
Not sure if I get your question correctly. This property is supported now so
why we should? It is already there so we are safe to set it.

This comment is about code optimization here:
> > > -                     struct devinfo *di2 = st->info;
> > > -                     unsigned long long s1, s2;
> > > -                     s1 = di->dev_size;
> > > -                     if (di->data_offset != INVALID_SECTORS)
> > > -                             s1 -= di->data_offset;
> > > -                     s1 /= __le32_to_cpu(sb->chunksize);
> > > -                     s2 = di2->dev_size;
> > > -                     if (di2->data_offset != INVALID_SECTORS)
> > > -                             s2 -= di2->data_offset;
> > > -                     s2 /= __le32_to_cpu(sb->chunksize);
> > > -                     if (s1 != s2)
> > > -                             raid0_need_layout = 1;

I think that we can check:
if (di->dev_size != di2->dev_size || di->data_offset != di2->data_offset)
    raid0_need_layout = 1;

but I could be wrong here, it is zoned raid, I don't have experience in this
area. It is existing code so you don't need to dig into. You can left it as is.

Isn't the size of members saved somewhere, do we need to count it?

Mariusz

Re: [PATCH 1/1] mdadm/super1: Add MD_FEATURE_RAID0_LAYOUT if sb->layout is set

[Xiao Ni]

On Fri, Oct 13, 2023 at 7:59 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> On Fri, 13 Oct 2023 18:59:21 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
> > On Fri, Oct 13, 2023 at 5:31 PM Mariusz Tkaczyk
> > <mariusz.tkaczyk@linux.intel.com> wrote:
> > >
> > > On Wed, 11 Oct 2023 21:05:22 +0800
> > > Xiao Ni <xni@redhat.com> wrote:
> > >
> > > > In kernel space super_1_validate sets mddev->layout to -1 if
> > > > MD_FEATURE_RAID0_LAYOUT is not set. MD_FEATURE_RAID0_LAYOUT is set in
> > > > mdadm write_init_super1. Now only raid with more than one zone can set
> > > > this bit. But for raid0 with same size member disks, it doesn't set this
> > > > bit. The layout is *unknown* when running mdadm -D command. In fact it
> > > > should be RAID0_ORIG_LAYOUT which gets from default_layout.
> > > >
> > > > So set MD_FEATURE_RAID0_LAYOUT when sb->layout has value.
> > > >
> > > > Fixes: 329dfc28debb ('Create: add support for RAID0 layouts.')
> > > > Signed-off-by: Xiao Ni <xni@redhat.com>
> > > > ---
> > > >  super1.c | 21 ++-------------------
> > > >  1 file changed, 2 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/super1.c b/super1.c
> > > > index 856b02082662..f29751b4a5c7 100644
> > > > --- a/super1.c
> > > > +++ b/super1.c
> > > > @@ -1978,26 +1978,10 @@ static int write_init_super1(struct supertype *st)
> > > >       unsigned long long sb_offset;
> > > >       unsigned long long data_offset;
> > > >       long bm_offset;
> > > > -     int raid0_need_layout = 0;
> > > >
> > > > -     for (di = st->info; di; di = di->next) {
> > > > +     for (di = st->info; di; di = di->next)
> > > >               if (di->disk.state & (1 << MD_DISK_JOURNAL))
> > > >                       sb->feature_map |=
> > > > __cpu_to_le32(MD_FEATURE_JOURNAL);
> > > > -             if (sb->level == 0 && sb->layout != 0) {
> > > > -                     struct devinfo *di2 = st->info;
> > > > -                     unsigned long long s1, s2;
> > > > -                     s1 = di->dev_size;
> > > > -                     if (di->data_offset != INVALID_SECTORS)
> > > > -                             s1 -= di->data_offset;
> > > > -                     s1 /= __le32_to_cpu(sb->chunksize);
> > > > -                     s2 = di2->dev_size;
> > > > -                     if (di2->data_offset != INVALID_SECTORS)
> > > > -                             s2 -= di2->data_offset;
> > > > -                     s2 /= __le32_to_cpu(sb->chunksize);
> > > > -                     if (s1 != s2)
> > > > -                             raid0_need_layout = 1;
> > > > -             }
> > > > -     }
> > > >
> > > >       for (di = st->info; di; di = di->next) {
> > > >               if (di->disk.state & (1 << MD_DISK_FAULTY))
> > > > @@ -2139,8 +2123,7 @@ static int write_init_super1(struct supertype *st)
> > > >                       sb->bblog_offset = 0;
> > > >               }
> > > >
> > > > -             /* RAID0 needs a layout if devices aren't all the same size
> > > > */
> > > > -             if (raid0_need_layout)
> > > > +             if (sb->level == 0 && sb->layout)
> > > >                       sb->feature_map |=
> > > > __cpu_to_le32(MD_FEATURE_RAID0_LAYOUT);
> > > >               sb->sb_csum = calc_sb_1_csum(sb);
> > > Hi Xiao,
> > >
> > > I read Neil patch:
> > > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=329dfc28de
> > >
> > > For sure Neil has a purpose to make it this way. I think that because it
> > > breaks creation when layout is not supported by kernel. Neil wanted to keep
> > > possible largest compatibility so it sets layout feature only if it is
> > > necessary. Your change forces layout bit to be always used. Can you test
> > > this change on kernel without raid0_layout support? I expect regression for
> > > same dev size raid arrays.
> >
> > Hi Mariusz
> >
> > Thanks for pointing out this. I only think the kernel which supports
> > MD_FEATURE_RAID0_LAYOUT
> >
> > >
> > > I think that before we will set layout bit we should check kernel
> > > version, it must be higher than 5.4. In the future we would remove this
> > > check.
>
> >
> > Let me check if I understand right:
> >
> > It needs to check raid0_need_layout when <= kernel 5.4. It can set
> > MD_FEATURE_RAID0_LAYOUT for all raid0 > kernel 5.4
>
> Correct! I'm accepting risk in extraordinary cases. In general, kernel is bumped
> not downgraded.

Thanks. I'll send a new version :)

>
> >
> >
> > > So, it forces the calculations made by Neil back but I think that we can
> > > simply compare dev_size and data_offset between members.
> >
> > We don't need to consider the compatibility anymore in future?
> >
> Not sure if I get your question correctly. This property is supported now so
> why we should? It is already there so we are safe to set it.

I asked because you said we can remove the check in future. So I don't
know why we don't need the check in future. The check here should be
the kernel version check, right?

>
> This comment is about code optimization here:
> > > > -                     struct devinfo *di2 = st->info;
> > > > -                     unsigned long long s1, s2;
> > > > -                     s1 = di->dev_size;
> > > > -                     if (di->data_offset != INVALID_SECTORS)
> > > > -                             s1 -= di->data_offset;
> > > > -                     s1 /= __le32_to_cpu(sb->chunksize);
> > > > -                     s2 = di2->dev_size;
> > > > -                     if (di2->data_offset != INVALID_SECTORS)
> > > > -                             s2 -= di2->data_offset;
> > > > -                     s2 /= __le32_to_cpu(sb->chunksize);
> > > > -                     if (s1 != s2)
> > > > -                             raid0_need_layout = 1;
>
> I think that we can check:
> if (di->dev_size != di2->dev_size || di->data_offset != di2->data_offset)
>     raid0_need_layout = 1;
>
> but I could be wrong here, it is zoned raid, I don't have experience in this
> area. It is existing code so you don't need to dig into. You can left it as is.
>
> Isn't the size of members saved somewhere, do we need to count it?

In fact, I have the same question as you. It looks like it can do as
your code mentioned above. We only store the member disk size in
superblock. I'll seft it as is.

Best Regards
Xiao

Re: [PATCH 1/1] mdadm/super1: Add MD_FEATURE_RAID0_LAYOUT if sb->layout is set

[Mariusz Tkaczyk]

On Fri, 13 Oct 2023 20:12:38 +0800
Xiao Ni <xni@redhat.com> wrote:

> > > > So, it forces the calculations made by Neil back but I think that we can
> > > > simply compare dev_size and data_offset between members.  
> > >
> > > We don't need to consider the compatibility anymore in future?
> > >  
> > Not sure if I get your question correctly. This property is supported now so
> > why we should? It is already there so we are safe to set it.  
> 
> I asked because you said we can remove the check in future. So I don't
> know why we don't need the check in future. The check here should be
> the kernel version check, right?


We are not supporting old kernels forever. At some point of time, we would
decide that kernels older than 5.5 are no longer a valid case and then we will
free to remove verification. If we are not supporting something older than the
version where it was added, we can assume that MD_RAID0_LAYOUT is always
available and we don't need to care anymore, right?

Here a recent example:
https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=f8d2c4286a

Thanks,
Mariusz

Re: [PATCH 1/1] mdadm/super1: Add MD_FEATURE_RAID0_LAYOUT if sb->layout is set

[Coly Li]

> 2023年10月13日 21:44，Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> 写道：
> 
> On Fri, 13 Oct 2023 20:12:38 +0800
> Xiao Ni <xni@redhat.com> wrote:
> 
>>>>> So, it forces the calculations made by Neil back but I think that we can
>>>>> simply compare dev_size and data_offset between members.  
>>>> 
>>>> We don't need to consider the compatibility anymore in future?
>>>> 
>>> Not sure if I get your question correctly. This property is supported now so
>>> why we should? It is already there so we are safe to set it.  
>> 
>> I asked because you said we can remove the check in future. So I don't
>> know why we don't need the check in future. The check here should be
>> the kernel version check, right?
> 
> 
> We are not supporting old kernels forever. At some point of time, we would
> decide that kernels older than 5.5 are no longer a valid case and then we will
> free to remove verification. If we are not supporting something older than the
> version where it was added, we can assume that MD_RAID0_LAYOUT is always
> available and we don't need to care anymore, right?
> 
> Here a recent example:
> https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=f8d2c4286a

Just FYI, we still support Linux v4.12 based kernel for SLES12-SP5.

Coly Li

Re: [PATCH 1/1] mdadm/super1: Add MD_FEATURE_RAID0_LAYOUT if sb->layout is set

[Xiao Ni]

On Fri, Oct 13, 2023 at 7:59 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> On Fri, 13 Oct 2023 18:59:21 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
> > On Fri, Oct 13, 2023 at 5:31 PM Mariusz Tkaczyk
> > <mariusz.tkaczyk@linux.intel.com> wrote:
> > >
> > > On Wed, 11 Oct 2023 21:05:22 +0800
> > > Xiao Ni <xni@redhat.com> wrote:
> > >
> > > > In kernel space super_1_validate sets mddev->layout to -1 if
> > > > MD_FEATURE_RAID0_LAYOUT is not set. MD_FEATURE_RAID0_LAYOUT is set in
> > > > mdadm write_init_super1. Now only raid with more than one zone can set
> > > > this bit. But for raid0 with same size member disks, it doesn't set this
> > > > bit. The layout is *unknown* when running mdadm -D command. In fact it
> > > > should be RAID0_ORIG_LAYOUT which gets from default_layout.
> > > >
> > > > So set MD_FEATURE_RAID0_LAYOUT when sb->layout has value.
> > > >
> > > > Fixes: 329dfc28debb ('Create: add support for RAID0 layouts.')
> > > > Signed-off-by: Xiao Ni <xni@redhat.com>
> > > > ---
> > > >  super1.c | 21 ++-------------------
> > > >  1 file changed, 2 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/super1.c b/super1.c
> > > > index 856b02082662..f29751b4a5c7 100644
> > > > --- a/super1.c
> > > > +++ b/super1.c
> > > > @@ -1978,26 +1978,10 @@ static int write_init_super1(struct supertype *st)
> > > >       unsigned long long sb_offset;
> > > >       unsigned long long data_offset;
> > > >       long bm_offset;
> > > > -     int raid0_need_layout = 0;
> > > >
> > > > -     for (di = st->info; di; di = di->next) {
> > > > +     for (di = st->info; di; di = di->next)
> > > >               if (di->disk.state & (1 << MD_DISK_JOURNAL))
> > > >                       sb->feature_map |=
> > > > __cpu_to_le32(MD_FEATURE_JOURNAL);
> > > > -             if (sb->level == 0 && sb->layout != 0) {
> > > > -                     struct devinfo *di2 = st->info;
> > > > -                     unsigned long long s1, s2;
> > > > -                     s1 = di->dev_size;
> > > > -                     if (di->data_offset != INVALID_SECTORS)
> > > > -                             s1 -= di->data_offset;
> > > > -                     s1 /= __le32_to_cpu(sb->chunksize);
> > > > -                     s2 = di2->dev_size;
> > > > -                     if (di2->data_offset != INVALID_SECTORS)
> > > > -                             s2 -= di2->data_offset;
> > > > -                     s2 /= __le32_to_cpu(sb->chunksize);
> > > > -                     if (s1 != s2)
> > > > -                             raid0_need_layout = 1;
> > > > -             }
> > > > -     }
> > > >
> > > >       for (di = st->info; di; di = di->next) {
> > > >               if (di->disk.state & (1 << MD_DISK_FAULTY))
> > > > @@ -2139,8 +2123,7 @@ static int write_init_super1(struct supertype *st)
> > > >                       sb->bblog_offset = 0;
> > > >               }
> > > >
> > > > -             /* RAID0 needs a layout if devices aren't all the same size
> > > > */
> > > > -             if (raid0_need_layout)
> > > > +             if (sb->level == 0 && sb->layout)
> > > >                       sb->feature_map |=
> > > > __cpu_to_le32(MD_FEATURE_RAID0_LAYOUT);
> > > >               sb->sb_csum = calc_sb_1_csum(sb);
> > > Hi Xiao,
> > >
> > > I read Neil patch:
> > > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=329dfc28de
> > >
> > > For sure Neil has a purpose to make it this way. I think that because it
> > > breaks creation when layout is not supported by kernel. Neil wanted to keep
> > > possible largest compatibility so it sets layout feature only if it is
> > > necessary. Your change forces layout bit to be always used. Can you test
> > > this change on kernel without raid0_layout support? I expect regression for
> > > same dev size raid arrays.
> >
> > Hi Mariusz
> >
> > Thanks for pointing out this. I only think the kernel which supports
> > MD_FEATURE_RAID0_LAYOUT
> >
> > >
> > > I think that before we will set layout bit we should check kernel
> > > version, it must be higher than 5.4. In the future we would remove this
> > > check.

Hi Mariusz

I just noticed the kernel version should be 3.14 rather than 5.4. In
kernel 3.14 (20d0189b1012 block: Introduce new bio_split()) introduces
this problem. So 5.4 is a typo error?

Regards
Xiao

Re: [PATCH 1/1] mdadm/super1: Add MD_FEATURE_RAID0_LAYOUT if sb->layout is set

[Xiao Ni]

On Fri, Oct 13, 2023 at 11:54 PM Coly Li <colyli@suse.de> wrote:
>
>
>
> > 2023年10月13日 21:44，Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> 写道：
> >
> > On Fri, 13 Oct 2023 20:12:38 +0800
> > Xiao Ni <xni@redhat.com> wrote:
> >
> >>>>> So, it forces the calculations made by Neil back but I think that we can
> >>>>> simply compare dev_size and data_offset between members.
> >>>>
> >>>> We don't need to consider the compatibility anymore in future?
> >>>>
> >>> Not sure if I get your question correctly. This property is supported now so
> >>> why we should? It is already there so we are safe to set it.
> >>
> >> I asked because you said we can remove the check in future. So I don't
> >> know why we don't need the check in future. The check here should be
> >> the kernel version check, right?
> >
> >
> > We are not supporting old kernels forever. At some point of time, we would
> > decide that kernels older than 5.5 are no longer a valid case and then we will
> > free to remove verification. If we are not supporting something older than the
> > version where it was added, we can assume that MD_RAID0_LAYOUT is always
> > available and we don't need to care anymore, right?
> >
> > Here a recent example:
> > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=f8d2c4286a
>
> Just FYI, we still support Linux v4.12 based kernel for SLES12-SP5.
>
> Coly Li
>

Hi all

Thanks for the explanation.

Best Regards
Xiao

Re: [PATCH 1/1] mdadm/super1: Add MD_FEATURE_RAID0_LAYOUT if sb->layout is set

[Mariusz Tkaczyk]

On Mon, 16 Oct 2023 16:13:16 +0800
Xiao Ni <xni@redhat.com> wrote:

> On Fri, Oct 13, 2023 at 7:59 PM Mariusz Tkaczyk
> <mariusz.tkaczyk@linux.intel.com> wrote:
> >
> > On Fri, 13 Oct 2023 18:59:21 +0800
> > Xiao Ni <xni@redhat.com> wrote:
> >  
> > > On Fri, Oct 13, 2023 at 5:31 PM Mariusz Tkaczyk
> > > <mariusz.tkaczyk@linux.intel.com> wrote:  
> > > >
> > > > On Wed, 11 Oct 2023 21:05:22 +0800
> > > > Xiao Ni <xni@redhat.com> wrote:
> > > >  
> > > > > In kernel space super_1_validate sets mddev->layout to -1 if
> > > > > MD_FEATURE_RAID0_LAYOUT is not set. MD_FEATURE_RAID0_LAYOUT is set in
> > > > > mdadm write_init_super1. Now only raid with more than one zone can set
> > > > > this bit. But for raid0 with same size member disks, it doesn't set
> > > > > this bit. The layout is *unknown* when running mdadm -D command. In
> > > > > fact it should be RAID0_ORIG_LAYOUT which gets from default_layout.
> > > > >
> > > > > So set MD_FEATURE_RAID0_LAYOUT when sb->layout has value.
> > > > >
> > > > > Fixes: 329dfc28debb ('Create: add support for RAID0 layouts.')
> > > > > Signed-off-by: Xiao Ni <xni@redhat.com>
> > > > > ---
> > > > >  super1.c | 21 ++-------------------
> > > > >  1 file changed, 2 insertions(+), 19 deletions(-)
> > > > >
> > > > > diff --git a/super1.c b/super1.c
> > > > > index 856b02082662..f29751b4a5c7 100644
> > > > > --- a/super1.c
> > > > > +++ b/super1.c
> > > > > @@ -1978,26 +1978,10 @@ static int write_init_super1(struct supertype
> > > > > *st) unsigned long long sb_offset;
> > > > >       unsigned long long data_offset;
> > > > >       long bm_offset;
> > > > > -     int raid0_need_layout = 0;
> > > > >
> > > > > -     for (di = st->info; di; di = di->next) {
> > > > > +     for (di = st->info; di; di = di->next)
> > > > >               if (di->disk.state & (1 << MD_DISK_JOURNAL))
> > > > >                       sb->feature_map |=
> > > > > __cpu_to_le32(MD_FEATURE_JOURNAL);
> > > > > -             if (sb->level == 0 && sb->layout != 0) {
> > > > > -                     struct devinfo *di2 = st->info;
> > > > > -                     unsigned long long s1, s2;
> > > > > -                     s1 = di->dev_size;
> > > > > -                     if (di->data_offset != INVALID_SECTORS)
> > > > > -                             s1 -= di->data_offset;
> > > > > -                     s1 /= __le32_to_cpu(sb->chunksize);
> > > > > -                     s2 = di2->dev_size;
> > > > > -                     if (di2->data_offset != INVALID_SECTORS)
> > > > > -                             s2 -= di2->data_offset;
> > > > > -                     s2 /= __le32_to_cpu(sb->chunksize);
> > > > > -                     if (s1 != s2)
> > > > > -                             raid0_need_layout = 1;
> > > > > -             }
> > > > > -     }
> > > > >
> > > > >       for (di = st->info; di; di = di->next) {
> > > > >               if (di->disk.state & (1 << MD_DISK_FAULTY))
> > > > > @@ -2139,8 +2123,7 @@ static int write_init_super1(struct supertype
> > > > > *st) sb->bblog_offset = 0;
> > > > >               }
> > > > >
> > > > > -             /* RAID0 needs a layout if devices aren't all the same
> > > > > size */
> > > > > -             if (raid0_need_layout)
> > > > > +             if (sb->level == 0 && sb->layout)
> > > > >                       sb->feature_map |=
> > > > > __cpu_to_le32(MD_FEATURE_RAID0_LAYOUT);
> > > > >               sb->sb_csum = calc_sb_1_csum(sb);  
> > > > Hi Xiao,
> > > >
> > > > I read Neil patch:
> > > > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=329dfc28de
> > > >
> > > > For sure Neil has a purpose to make it this way. I think that because it
> > > > breaks creation when layout is not supported by kernel. Neil wanted to
> > > > keep possible largest compatibility so it sets layout feature only if
> > > > it is necessary. Your change forces layout bit to be always used. Can
> > > > you test this change on kernel without raid0_layout support? I expect
> > > > regression for same dev size raid arrays.  
> > >
> > > Hi Mariusz
> > >
> > > Thanks for pointing out this. I only think the kernel which supports
> > > MD_FEATURE_RAID0_LAYOUT
> > >  
> > > >
> > > > I think that before we will set layout bit we should check kernel
> > > > version, it must be higher than 5.4. In the future we would remove this
> > > > check.  
> 
> Hi Mariusz
> 
> I just noticed the kernel version should be 3.14 rather than 5.4. In
> kernel 3.14 (20d0189b1012 block: Introduce new bio_split()) introduces
> this problem. So 5.4 is a typo error?
> 
> Regards
> Xiao
> 
Hi Xiao,
5.4 is a kernel where Neil introduced RAID0_LAYOUT_SUPPORT:
"Since Linux 5.4 a layout is needed for RAID0 arrays with
varying device sizes."

3.14 is a kernel when regression came but it seems that we fixed it in
5.4. I think that we can set it safely starting from 5.4.

Thanks,
Mariusz