race between nvme device creation and discovery?

[Hannes Reinecke]

On 2/5/24 15:47, Daniel Wagner wrote:
> On Mon, Feb 05, 2024 at 02:02:04PM +0900, Hannes Reinecke wrote:
>> Hehe. Good old sysfs.
>> This is a common issue with sysfs, and we've even had a retry loop in udev
>> back in them days to avoid these kind of things.
>>
>> Point is, uevent will be sent out with device_add(), causing udev to run,
>> running udev rules, and eventually call into libnvme to scan the device. But
>> as you rightly pointed out, the sysfs link is only created
>> _after_ the event has been sent, so there's a race window during which
>> libnvme will fails to read the link, landing us with the scenario above.
>>
>> While we could add a retry logic to libnvme, I'm not really convinced
>> this is a good idea; in the end, who's to tell how long we should wait?
>> A second? Several seconds? A minute? Several minutes?
>> Also not that sysfs_create_link() has a return code, so the link might
>> not be created at all ...
> 
> Yep, retry logics have always a smell too it. What about the idea to
> add some sort of ready attribute to the controller sysfs entry?
> 
>> A possibly better way here would be to suppress uevents on device_add(),
>> and only send out events once the device is fully set up, ie just before
>> the 'return 0'.
> 
> I don't think this will address the problem. The blktests runs completely
> independent to what udev does and it's blktests which observes the
> missing link not udev.

I thought something along these lines:

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 85ab0fcf9e88..3ed706f595fe 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2845,6 +2845,7 @@ static int nvme_init_subsystem(struct nvme_ctrl 
*ctrl, struct nvme_id_ctrl *id)
                         goto out_put_subsystem;
                 }
         } else {
+               dev_set_uevent_suppress(&subsys->dev, 1);
                 ret = device_add(&subsys->dev);
                 if (ret) {
                         dev_err(ctrl->device,
@@ -2869,6 +2870,10 @@ static int nvme_init_subsystem(struct nvme_ctrl 
*ctrl, struct nvme_id_ctrl *id)
         ctrl->subsys = subsys;
         list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
         mutex_unlock(&nvme_subsystems_lock);
+       if (!found) {
+               dev_set_uevent_suppress(&subsys->dev, 0);
+               kobject_uevent(&subsys->dev.kobj, KOBJ_ADD);
+       }
         return 0;


Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare at suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Ivo Totev, Andrew McDonald,
Werner Knoblich