Re: [PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

["Tkaczyk, Mariusz" <mariusz.tkaczyk@linux.intel.com>]

On 18.10.2021 11:58, Xiao Ni wrote:
>>> The test case is:
>>> 1. create one imsm container
>>> 2. create a raid5 device from the container
>>> 3. unplug two disks
>>> 4. mdadm --detail /dev/md126
>>> [root@rhel85 ~]# mdadm -D /dev/md126
>>> /dev/md126:
>>>            Container : ��, member 0
>>>
>>> The Detail function first gets container name by function
>>> map_dev_preferred. Then it tries to find which disks are
>>> available. In patch db5377883fef(It should be FAILED..)
>>> uses map_dev_preferred to find which disks are under /dev.
>>>
>>> But now, the major/minor information comes from kernel space.
>>> map_dev_preferred malloc memory and init a device list when
>>> first be called by Detail. It can't find the device in the
>>> list by the major/minor. It free the memory and reinit the
>>> list.
>>>   > The container name now points to an area tha has been freed.
>>> So the containt is a mess.
>>>
>>
>> Container name is collected with 'create' flag set, so it's
>> name is additionally copied to static memory to prevent
>> overwrites. Could you verify?
> 
> Hi Mariusz
> 
> The chapter above you mentioned is talking about the creation process?
> The container name mentioned from the patch is a temporary variable in
> Detail function.
> 
> You want to say we can use the container name from the "static memory" in
> Detail function, so we don't get the container name again? And where is the
> static memory?
> 

There is a code:
	if (create && !regular && !preferred) {
		static char buf[30]; <- this variable will survive retry.
		snprintf(buf, sizeof(buf), "%d:%d", major, minor);
		regular = buf;
	}
but seems that it is not a case for this scenario. I suspected that
this was used because when gathering container name:

		container = map_dev_preferred(major(devid), minor(devid),
					      1, c->prefer);

'create' is explicitly set to 1. That is why I expect to have 'container'
declared in static area. Make sense?


>>> This patch replaces map_dev_preferred with devid2kname. If
>>> the name is NULL, it means the disk is unplug.
>>>
>> Your patch fixes only one place. Please go forward and analyze all
>> map_dev_preffered() calls (which looks safe to me). Maybe this
>> function can be replaced at all and we can drop this code in
>> flavor of devid2kname() or other.
> 
> At first, I just wanted to fix this bug. We can check all the places which
> are using map_dev_preffered. But it looks like a big project. It needs to
> understand all codes related to this function. It needs more time. Do you
> agree with fixing this bug first, then we can try to fix the hidden bugs.
> 
Sure, we are in rc2, so bigger changes should be merged after release.
It need to wait for now. Seems to be good future improvement.

>>
>>> Fixes: db5377883fef (It should be FAILED when raid has)
>>> Signed-off-by: Xiao Ni <xni@redhat.com>
>>> Reported-by: Fine Fan <ffan@redhat.com>
>>> ---
>>>    Detail.c | 12 +++++++-----
>>>    1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Detail.c b/Detail.c
>>> index d3af0ab..2164de3 100644
>>> --- a/Detail.c
>>> +++ b/Detail.c
>>> @@ -351,11 +351,13 @@ int Detail(char *dev, struct context *c)
>>>        avail = xcalloc(array.raid_disks, 1);
>>>
>>>        for (d = 0; d < array.raid_disks; d++) {
>>> -             char *dv, *dv_rep;
>>> -             dv = map_dev_preferred(disks[d*2].major,
>>> -                             disks[d*2].minor, 0, c->prefer);
>>> -             dv_rep = map_dev_preferred(disks[d*2+1].major,
>>> -                             disks[d*2+1].minor, 0, c->prefer);
>>> +             char *dv, *dv_rep = NULL;
>>> +
>>> +             if (!disks[d*2].major && !disks[d*2].minor)
>>> +                     continue; > +
>>> +             dv = devid2kname(makedev(disks[d*2].major, disks[d*2].minor));
>>> +             dv_rep = devid2kname(makedev(disks[d*2+1].major, disks[d*2+1].minor));
>>>
>>>                if ((dv && (disks[d*2].state & (1<<MD_DISK_SYNC))) ||
>>>                    (dv_rep && (disks[d*2+1].state & (1<<MD_DISK_SYNC)))) {
>>>
>>
>> Yeah, I know that it is used in Detail this way, but please  determine
>> way to replace this ugly [d*2] and [d*2+1].
> 
> Yes, it takes me much time to understand how to calculate avail[]. We
> can focus on
> fixing this bug first, then we prepare some patches for improving the codes that
> related to the function map_dev_preferred and the [d*2] format codes.
> It might be
> a big change.
> 
Agree.

>>
>> This whole block should be moved from Detail() code to separate
>> function, which determines if device or replacement is in sync.
> 
> A good suggestion. Put it into the change I mentioned above, is it ok?
> 
Agree. So, will you take care about all improvements later (after release)?

Thanks,
Mariusz