----- Original Message -----
> From: "David Lehman" <dlehman(a)redhat.com>
> To: anaconda-patches(a)lists.fedorahosted.org
> Sent: Friday, June 20, 2014 10:12:35 AM
> Subject: Re: [blivet:master 3/8] Omit special check for md devices
in addUdevDevice().
>
> On 06/20/2014 06:47 AM, Anne Mulhern wrote:
>>
>>
>>
>>
>> ----- Original Message -----
>>> From: "David Lehman" <dlehman(a)redhat.com>
>>> To: anaconda-patches(a)lists.fedorahosted.org
>>> Sent: Thursday, June 19, 2014 1:05:22 PM
>>> Subject: Re: [blivet:master 3/8] Omit special check for md devices in
>>> addUdevDevice().
>>>
>>> On 06/18/2014 02:05 PM, mulhern wrote:
>>>> If the device appears to be an md device, just call addUdevMDDevice
>>>> directly.
>>>>
>>>> Use udev_device_get_md_uuid() to get the mduuid of the array, but catch
>>>> a key error. This has the same effect as the previous code, but is a bit
>>>> more proper, even if more wordy.
>>>>
>>>> Signed-off-by: mulhern <amulhern(a)redhat.com>
>>>> ---
>>>> blivet/devicetree.py | 18 +++++++-----------
>>>> 1 file changed, 7 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/blivet/devicetree.py b/blivet/devicetree.py
>>>> index b4e5b00..36b8b05 100644
>>>> --- a/blivet/devicetree.py
>>>> +++ b/blivet/devicetree.py
>>>> @@ -891,7 +891,12 @@ class DeviceTree(object):
>>>> device = self.getDeviceByName(name)
>>>>
>>>> if device is None:
>>>> - device =
self.getDeviceByUuid(info.get("MD_UUID"))
>>>> + try:
>>>> + uuid = udev.udev_device_get_md_uuid(info)
>>>> + except KeyError:
>>>> + log.warning("failed to obtain uuid for mdraid
device")
>>>> + else:
>>>> + device = self.getDeviceByUuid(uuid)
>>>>
>>>> # if we get here, we found all of the slave devices and
>>>> # something must be wrong -- if all of the slaves are in
>>>> @@ -1188,16 +1193,7 @@ class DeviceTree(object):
>>>> device = self.addUdevDMDevice(info)
>>>> elif udev.udev_device_is_md(info) and not
>>>> udev.udev_device_get_md_container(info):
>>>> log.info("%s is an md device", name)
>>>> - try:
>>>> - md_uuid = udev.udev_device_get_md_uuid(info)
>>>> - except KeyError:
>>>> - pass
>>>> - else:
>>>> - # try to find the device by uuid
>>>> - device = self.getDeviceByUuid(md_uuid)
>>>> -
>>>> - if device is None:
>>>> - device = self.addUdevMDDevice(info)
>>>> + device = self.addUdevMDDevice(info)
>>>> elif udev.udev_device_is_cdrom(info):
>>>> log.info("%s is a cdrom", name)
>>>> device = self.addUdevOpticalDevice(info)
>>>>
>>>
>>> It looks okay to me except that now we'll always run that code to add
>>> all of the member devices, which takes/wastes time. In all normal
>>> circumstances the array will already be active and most likely already
>>> in the tree by the time we get to it, so the expectation is that the
>>> short-circuit lookup will succeed. Maybe you could add an attempt to
>>> look up the device to the top of addUdevMDDevice if you want to clean up
>>> addUDevDevice.
>>>
>>> David
>>> _______________________________________________
>>> anaconda-patches mailing list
>>> anaconda-patches(a)lists.fedorahosted.org
>>>
https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
>>>
>>
>> Is there any possibility that the device would have already been
>> added (because one of its members was added) but some of its
>> slaves have not been added and should be added in addUdevMDDevice?
>
> The members should all get added the same way, so no -- we don't need to
> count on adding member devices via addUdevMDDevice unless by coincidence
> due to some bug.
>
> David
>
I see that the array device is added to the device tree the first time a member device
is
encountered (in handleUdevMDMemberFormat).
So, being able to find the device in the device tree means that one of the
member devices has been discovered, not all.
Do we expect that all member devices will have been added when addUdevMDDevice()
is called? It seems like the only way that can be ensured is if the udev db gets
filled up in an order which means that the array itself comes after all its
members (I'm looking at DeviceTree._populate() to come up with that notion). If we
expect that, then there is no point in looking for slave devices in
addUdevMDDevice() at all, because they will certainly all have been added.
The relevant anaconda commits I found are: d20417940f803410f3c668b2599400394d6a84ab
and e95be86ccde6c4997fd87729cc655c9072f865fd.
Based on all this it looks to me like we should either get rid of the
slave related code entirely, or execute it unconditionally. Either it's
never necessary because all slaves will have been processed, or it's always
necessary because it's impossible to be sure that all slaves have been
processed.
Or, I'm missing something.
It used to be necessary because of an old patch that randomized the
ordering of udev's device list. It shouldn't be necessary anymore, but
let's not rush to remove it just yet without making sure. I am not as
worried now that I see we only call addUdevDevice after trying to look
up the member device by name. addUdevDevice adds a lot of noise to the
logs, but it looks like we only call it as needed.
David
- mulhern
>>
>> - mulhern
>>
>> _______________________________________________
>> anaconda-patches mailing list
>> anaconda-patches(a)lists.fedorahosted.org
>>
https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
>>
>
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
>
_______________________________________________
anaconda-patches mailing list
anaconda-patches(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches