Messages in this thread | | | From | Dan Williams <> | Date | Fri, 24 Jan 2020 14:36:04 -0800 | Subject | Re: [PATCH v1] driver core: check for dead devices before onlining/offlining |
| |
On Fri, Jan 24, 2020 at 10:58 AM David Hildenbrand <david@redhat.com> wrote: > > > > > Am 24.01.2020 um 19:41 schrieb Dan Williams <dan.j.williams@intel.com>: > > > > On Fri, Jan 24, 2020 at 9:14 AM David Hildenbrand <david@redhat.com> wrote: > >> > >>> On 20.01.20 11:49, David Hildenbrand wrote: > >>> We can have rare cases where the removal of a device races with > >>> somebody trying to online it (esp. via sysfs). We can simply check > >>> if the device is already removed or getting removed under the dev->lock. > >>> > >>> E.g., right now, if memory block devices are removed (remove_memory()), > >>> we do a: > >>> > >>> remove_memory() -> lock_device_hotplug() -> mem_hotplug_begin() -> > >>> lock_device() -> dev->dead = true > >>> > >>> Somebody coming via sysfs (/sys/devices/system/memory/memoryX/online) > >>> triggers a: > >>> > >>> lock_device_hotplug_sysfs() -> device_online() -> lock_device() ... > >>> > >>> So if we made it just before the lock_device_hotplug_sysfs() but get > >>> delayed until remove_memory() released all locks, we will continue > >>> taking locks and trying to online the device - which is then a zombie > >>> device. > >>> > >>> Note that at least the memory onlining path seems to be protected by > >>> checking if all memory sections are still present (something we can then > >>> get rid of). We do have other sysfs attributes > >>> (e.g., /sys/devices/system/memory/memoryX/valid_zones) that don't do any > >>> such locking yet and might race with memory removal in a similar way. For > >>> these users, we can then do a > >>> > >>> device_lock(dev); > >>> if (!device_is_dead(dev)) { > >>> /* magic /* > >>> } > >>> device_unlock(dev); > >>> > >>> Introduce and use device_is_dead() right away. > >>> > >> > >> So, I just added the following: > >> > >> diff --git a/drivers/base/core.c b/drivers/base/core.c > >> index 01cd06eeb513..49c4d8671073 100644 > >> --- a/drivers/base/core.c > >> +++ b/drivers/base/core.c > >> @@ -1567,6 +1567,7 @@ static ssize_t online_store(struct device *dev, > >> struct device_attribute *attr, > >> if (ret < 0) > >> return ret; > >> > >> + msleep(10000); > >> ret = lock_device_hotplug_sysfs(); > >> if (ret) > >> return ret; > >> > >> Then triggered > >> echo 1 > /sys/devices/system/memory/memory51/online > >> And quickly afterwards unplugged the DIMM. > >> > >> Good news is that we get (after 10 seconds) > >> sh: echo: write error: No such device > >> > >> Reason is that unplug will not finish before all sysfs attributes have > >> been exited by other threads. > > > > The unplug thread gets blocked for 10 seconds waiting for this thread > > in online_store() to exit? > > > > Yes, that‘s what I observed.
Nice. This obviously wasn't in my mental model of what sysfs/kernfs guaranteed.
/me digs a bit...
So it's kernfs_drain() that is saving us. That waits for all active nodes to deactivate and that guarantees that removal can be used as a barrier. It also indicates that the lockdep splat I saw can't trigger a deadlock unless mem_hotplug_begin() is being acquired within a memblock-sysfs ops handler that is trying to remove memory. In all cases I can audit it's always a 3rd-party sysfs attribute triggering that removal.
So, simply moving remove_memory_block_devices() outside of mem_hotplug_begin() should be a sufficient fix, no other invalidation data needed. I'll respin that fix.
| |