Cathy Zhou wrote:
>> I don't understand why dladm would need to call delete_linkid if you're
>> temporarily deleting a link. I think if you're temporarily deleting a
>> link, you'd want to keep the link ID to name mapping around so that the
>> link ID is not reused. Even if that wasn't a concern (say if guaranteed
>> that we would never reuse link IDs even after the mapping was
>> deleted), I still don't see why we couldn't bring the link down without
>> deleting the mapping.
>>
> Well. If this linkid was created for a temporary link only, it should be
> deleted when that link is destroyed.
>
Ahh, because if we didn't, we could potentially run out of link IDs.
I see.
>> If knowing the state of the link was important, I would say we should
>> track that using the configuration structures. We could have a field
>> indicating if a link is active. When the system reboots, the link
>> becomes active and the field would be changed to true.
>>
> But this configuration structure is only used for persist data, right?
> and if the link is purely temporary, the configuration structure is not
> involved at all.
>
You're right, I'm going down the wrong path here.
Let me think about the affects of the flag you suggested.
>>> Therefore, I'd suggest get_new_linkid() and delete_linkid() to have a
>>> flag to indicate to create or delete linkids for which one (or both,
>>> active and persistent links).
>>>
>>
>> I could do that, but I think it would be better to track that
>> information elsewhere.
>>
> Where is the elsewhere? How?
>
No, you're right. This won't work.
>>>> walk_confs()
>>>>
>>> What exactly information it iterates? The linkids? Then it can be
>>> achieved by the walkers in the linkid management API.
>>>
>>
>> It walks the configuration data structure for each persistent link. I
>> don't think it can be achieved by the walkers in the linkid management
>> API because at boot time we need some way of loading the persistent
>> link ID to name mappings before we can use the link ID management API
>> walker.
>>
>> I.e.
>>
>> in devfsadmd at boot
>> walk_confs()
>> for each conf
>> get_conf_field(conf, "id", &id);
>> get_conf_field(conf, "name", &name);
>> add_map(id, name); /* this function is part of devfsadmd and
>> not any of the API's discussed in this mail */
>>
> I see.
>
>>>> Return a configuration struct for each persistent link. I expect
>>>> that devfsadmd would use this on boot to determine what link ID to
>>>> name mappings have been persisted.
>>>>
>>> In theory, as we are separating linkid management and configuration
>>> persistence APIs, the devfsadmd doesn't need to be informed of all
>>> the persistent link attributes at all, and the libdladm can simply do
>>> the persistence work by itself, and not through devfsadmd. But I can
>>> see two benefit that devfsadmd stores persistent link configuration
>>> information:
>>>
>>> a. those configuration are looked up from both libdladm and the
>>> kernel, so have the configuration in memory will helps the performance.
>>>
>>> b. the devfdadmd will need to be used to store and query the
>>> persistent configuration anyway, as requested from the door-upcall
>>> from the kernel, so it might be easy if the all persistence/query
>>> requests to be processed in the same place.
>>>
>>
>> I agree with everything you say here, but, there's one problem. How do
>> you initialize devfsadmd's internal structures at boot? That's what
>> this walker is for.
>>
> Yes, you are right. But what I tried to discuss with you here is whether
> devfdadmd needs to be involved as part of the implementation of
> configuration persistence API, and it seems you agreed it should.
>
Yes, devfsadmd will need to call into the API so that it can load the
persistent link ID to name mappings.
>>>> conf = get_new_conf();
>>>> set_conf_field(conf, "class", class);
>>>> set_conf_field(conf, "name", name);
>>>> set_conf_field(conf, "id", id);
>>>> commit_conf(conf);
>>>> free_conf(conf);
>>>>
>>> I think it is better make "id" to be the identifier of a persistent
>>> link configuration entry, and unique from other conf_field, so that
>>> when you do the commit_conf(), you know whether it is a new link or
>>> it is to update an existing link.
>>>
>>
>> I'm not sure that is necessary because the ID is in the conf
>> structure, and I could extract it internally.
>>
> Right, so that your internal processes is different for "key fields"
> even users use the same API to set them. I don't see the benefit.
>
After spending some time going over meem's comments, I decided to
include the key fields on the create call, and then make all of them,
except for the name field, read-only.
>>>> If the API user only does the delete_linkid, then the link ID to
>>>> name mapping is removed, but it still exists in persistent storage.
>>>> For example:
>>>>
>>>> delete_linkid("net6");
>>>> id = get_new_linkid("net10", class); /* Could return net6's link
>>>> ID! */
>>>>
>>>> If the user persisted this new link ID, we'd have a link ID
>>>> collision on the next reboot.
>>>>
>>>> This also implies that there is no way to safely and temporarily
>>>> delete the link ID to name mapping for a persistent link. Since
>>>> this is an API, I think we could document this limitation.
>>>>
>>> I don't quite understand why.
>>>
>>
>> If a link always has the same ID, even across reboots, then we will
>> have a collision in this case. At the next reboot, net6 is restored
>> (because it was persistent and was never removed form persistent
>> storage) and net10 comes up (because it was persisted) and both have
>> the same link ID.
>>
> I would think that When "net10" was persisted, the commit_conf(conf)
> function should return failures.
>
Take a look at my response to meem. I spent some time with pen and
paper going over different scenarios, I couldn't see a way to tell the
difference between this case and the rename case.
thanks,
Dan
