[PATCH] mdadm: handle super == NULL case in avail_size1

[PATCH] mdadm: handle super == NULL case in avail_size1

[Song Liu]

Summary:

Handling super == NULL case in avail_size1() was removed a while
back. However, it is still useful in the following stack:

avail_size1() with st->sb == NULL
array_try_spare() with st == NULL
try_spare() with st == NULL
Incremental() with st == NULL

This patch adds the handling of super == NULL back to avail_size1().

Signed-off-by: Song Liu <songliubraving@fb.com>
---
 super1.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/super1.c b/super1.c
index f6a1045..5e8d967 100644
--- a/super1.c
+++ b/super1.c
@@ -2340,7 +2340,9 @@ static __u64 avail_size1(struct supertype *st, __u64 devsize,
 	if (devsize < 24)
 		return 0;
 
-	if (__le32_to_cpu(super->feature_map) & MD_FEATURE_BITMAP_OFFSET) {
+	if (!super)
+		bmspace = choose_bm_space(devsize);
+	else if (__le32_to_cpu(super->feature_map) & MD_FEATURE_BITMAP_OFFSET) {
 		/* hot-add. allow for actual size of bitmap */
 		struct bitmap_super_s *bsb;
 		bsb = (struct bitmap_super_s *)(((char*)super)+MAX_SB_SIZE);
@@ -2350,7 +2352,7 @@ static __u64 avail_size1(struct supertype *st, __u64 devsize,
 	}
 
 	/* Allow space for bad block log */
-	if (super->bblog_size)
+	if (super && super->bblog_size)
 		bbspace = __le16_to_cpu(super->bblog_size);
 
 	if (st->minor_version < 0)
-- 
2.9.5

Re: [PATCH] mdadm: handle super == NULL case in avail_size1

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1936 bytes --]

On Thu, Aug 31 2017, Song Liu wrote:

> Summary:
>
> Handling super == NULL case in avail_size1() was removed a while
> back. However, it is still useful in the following stack:
>
> avail_size1() with st->sb == NULL
> array_try_spare() with st == NULL
> try_spare() with st == NULL
> Incremental() with st == NULL

I assume you mean "st->sb == NULL" in each case here?  What you have
written doesn't make sense.


>
> This patch adds the handling of super == NULL back to avail_size1().

I doubt this is the best thing to do.
If we don't have st->sb, calling avail_size doesn't make any sense.
Maybe we should be calling validate_geometry.

NeilBrown


>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>  super1.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/super1.c b/super1.c
> index f6a1045..5e8d967 100644
> --- a/super1.c
> +++ b/super1.c
> @@ -2340,7 +2340,9 @@ static __u64 avail_size1(struct supertype *st, __u64 devsize,
>  	if (devsize < 24)
>  		return 0;
>  
> -	if (__le32_to_cpu(super->feature_map) & MD_FEATURE_BITMAP_OFFSET) {
> +	if (!super)
> +		bmspace = choose_bm_space(devsize);
> +	else if (__le32_to_cpu(super->feature_map) & MD_FEATURE_BITMAP_OFFSET) {
>  		/* hot-add. allow for actual size of bitmap */
>  		struct bitmap_super_s *bsb;
>  		bsb = (struct bitmap_super_s *)(((char*)super)+MAX_SB_SIZE);
> @@ -2350,7 +2352,7 @@ static __u64 avail_size1(struct supertype *st, __u64 devsize,
>  	}
>  
>  	/* Allow space for bad block log */
> -	if (super->bblog_size)
> +	if (super && super->bblog_size)
>  		bbspace = __le16_to_cpu(super->bblog_size);
>  
>  	if (st->minor_version < 0)
> -- 
> 2.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] mdadm: handle super == NULL case in avail_size1

[Song Liu]

> On Aug 31, 2017, at 4:36 PM, NeilBrown <neilb@suse.com> wrote:
> 
> On Thu, Aug 31 2017, Song Liu wrote:
> 
>> Summary:
>> 
>> Handling super == NULL case in avail_size1() was removed a while
>> back. However, it is still useful in the following stack:
>> 
>> avail_size1() with st->sb == NULL
>> array_try_spare() with st == NULL
>> try_spare() with st == NULL
>> Incremental() with st == NULL
> 
> I assume you mean "st->sb == NULL" in each case here?  What you have
> written doesn't make sense.
> 

I did mean st == NULL. In array_try_spare(), if st is NULL, st2 is 
allocated with match_metadata_desc(), and avail_size() is called with
st2. 

>> 
>> This patch adds the handling of super == NULL back to avail_size1().
> 
> I doubt this is the best thing to do.
> If we don't have st->sb, calling avail_size doesn't make any sense.
> Maybe we should be calling validate_geometry.
> 

This was the same logic that got removed a few years ago. So far it 
works for my use cases. And I think it makes (some) sense. 
Is validate_geometry() really better than this approach?

Thanks,
Song

Re: [PATCH] mdadm: handle super == NULL case in avail_size1

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1847 bytes --]

On Thu, Aug 31 2017, Song Liu wrote:

>> On Aug 31, 2017, at 4:36 PM, NeilBrown <neilb@suse.com> wrote:
>> 
>> On Thu, Aug 31 2017, Song Liu wrote:
>> 
>>> Summary:
>>> 
>>> Handling super == NULL case in avail_size1() was removed a while
>>> back. However, it is still useful in the following stack:
>>> 
>>> avail_size1() with st->sb == NULL
>>> array_try_spare() with st == NULL
>>> try_spare() with st == NULL
>>> Incremental() with st == NULL
>> 
>> I assume you mean "st->sb == NULL" in each case here?  What you have
>> written doesn't make sense.
>> 
>
> I did mean st == NULL. In array_try_spare(), if st is NULL, st2 is 
> allocated with match_metadata_desc(), and avail_size() is called with
> st2. 

Ahhh, OK.  But if st isn't NULL I think the problem still exists as when
an st is passed to try_spare() it doesn't have an st->sb.

>
>>> 
>>> This patch adds the handling of super == NULL back to avail_size1().
>> 
>> I doubt this is the best thing to do.
>> If we don't have st->sb, calling avail_size doesn't make any sense.
>> Maybe we should be calling validate_geometry.
>> 
>
> This was the same logic that got removed a few years ago. So far it 
> works for my use cases. And I think it makes (some) sense. 
> Is validate_geometry() really better than this approach?

validate_geometry is better because it doesn't expect 'sb' to be set.
The documentation for avail_size says:

	 * 'mdadm' only calls this for existing arrays where a possible
	 * spare is being added.

In the try_spare() case there is no existing array.  Rather, try_spare()
is checking if a particular geometry is valid.

It is rather sad that this auto-sparing functionality has been broken
for 4 years and no-one noticed until now.  I really should have created
some self-tests....

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]