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

[NeilBrown <neilb@suse.com>]

[-- 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 --]