* [PATCH] mdadm: handle super == NULL case in avail_size1
@ 2017-08-31 17:37 Song Liu
2017-08-31 23:36 ` NeilBrown
0 siblings, 1 reply; 4+ messages in thread
From: Song Liu @ 2017-08-31 17:37 UTC (permalink / raw)
To: linux-raid
Cc: Song Liu, shli, neilb, kernel-team, dan.j.williams, hch, jes.sorensen
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] mdadm: handle super == NULL case in avail_size1
2017-08-31 17:37 [PATCH] mdadm: handle super == NULL case in avail_size1 Song Liu
@ 2017-08-31 23:36 ` NeilBrown
2017-08-31 23:46 ` Song Liu
0 siblings, 1 reply; 4+ messages in thread
From: NeilBrown @ 2017-08-31 23:36 UTC (permalink / raw)
To: linux-raid; +Cc: Song Liu, shli, kernel-team, dan.j.williams, hch, jes.sorensen
[-- 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 --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mdadm: handle super == NULL case in avail_size1
2017-08-31 23:36 ` NeilBrown
@ 2017-08-31 23:46 ` Song Liu
2017-09-01 0:21 ` NeilBrown
0 siblings, 1 reply; 4+ messages in thread
From: Song Liu @ 2017-08-31 23:46 UTC (permalink / raw)
To: NeilBrown
Cc: linux-raid, Shaohua Li, Kernel Team, Dan Williams,
Christoph Hellwig, jes.sorensen
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mdadm: handle super == NULL case in avail_size1
2017-08-31 23:46 ` Song Liu
@ 2017-09-01 0:21 ` NeilBrown
0 siblings, 0 replies; 4+ messages in thread
From: NeilBrown @ 2017-09-01 0:21 UTC (permalink / raw)
To: Song Liu
Cc: linux-raid, Shaohua Li, Kernel Team, Dan Williams,
Christoph Hellwig, jes.sorensen
[-- 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 --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-09-01 0:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-31 17:37 [PATCH] mdadm: handle super == NULL case in avail_size1 Song Liu
2017-08-31 23:36 ` NeilBrown
2017-08-31 23:46 ` Song Liu
2017-09-01 0:21 ` NeilBrown
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.