Commit b3b454f6 authored by Dave Jiang's avatar Dave Jiang Committed by Dan Williams

libnvdimm: fix clear poison locking with spinlock and GFP_NOWAIT allocation

The following warning results from holding a lane spinlock,
preempt_disable(), or the btt map spinlock and then trying to take the
reconfig_mutex to walk the poison list and potentially add new entries.

BUG: sleeping function called from invalid context at kernel/locking/mutex.
c:747
in_atomic(): 1, irqs_disabled(): 0, pid: 17159, name: dd
[..]
Call Trace:
dump_stack+0x85/0xc8
___might_sleep+0x184/0x250
__might_sleep+0x4a/0x90
__mutex_lock+0x58/0x9b0
? nvdimm_bus_lock+0x21/0x30 [libnvdimm]
? __nvdimm_bus_badblocks_clear+0x2f/0x60 [libnvdimm]
? acpi_nfit_forget_poison+0x79/0x80 [nfit]
? _raw_spin_unlock+0x27/0x40
mutex_lock_nested+0x1b/0x20
nvdimm_bus_lock+0x21/0x30 [libnvdimm]
nvdimm_forget_poison+0x25/0x50 [libnvdimm]
nvdimm_clear_poison+0x106/0x140 [libnvdimm]
nsio_rw_bytes+0x164/0x270 [libnvdimm]
btt_write_pg+0x1de/0x3e0 [nd_btt]
? blk_queue_enter+0x30/0x290
btt_make_request+0x11a/0x310 [nd_btt]
? blk_queue_enter+0xb7/0x290
? blk_queue_enter+0x30/0x290
generic_make_request+0x118/0x3b0

A spinlock is introduced to protect the poison list. This allows us to not
having to acquire the reconfig_mutex for touching the poison list. The
add_poison() function has been broken out into two helper functions. One to
allocate the poison entry and the other to apppend the entry. This allows us
to unlock the poison_lock in non-I/O path and continue to be able to allocate
the poison entry with GFP_KERNEL. We will use GFP_NOWAIT in the I/O path in
order to satisfy being in atomic context.
Reviewed-by: default avatarVishal Verma <vishal.l.verma@intel.com>
Signed-off-by: default avatarDave Jiang <dave.jiang@intel.com>
Signed-off-by: default avatarDan Williams <dan.j.williams@intel.com>
parent efebc711
...@@ -296,6 +296,7 @@ struct nvdimm_bus *nvdimm_bus_register(struct device *parent, ...@@ -296,6 +296,7 @@ struct nvdimm_bus *nvdimm_bus_register(struct device *parent,
init_waitqueue_head(&nvdimm_bus->probe_wait); init_waitqueue_head(&nvdimm_bus->probe_wait);
nvdimm_bus->id = ida_simple_get(&nd_ida, 0, 0, GFP_KERNEL); nvdimm_bus->id = ida_simple_get(&nd_ida, 0, 0, GFP_KERNEL);
mutex_init(&nvdimm_bus->reconfig_mutex); mutex_init(&nvdimm_bus->reconfig_mutex);
spin_lock_init(&nvdimm_bus->poison_lock);
if (nvdimm_bus->id < 0) { if (nvdimm_bus->id < 0) {
kfree(nvdimm_bus); kfree(nvdimm_bus);
return NULL; return NULL;
...@@ -364,9 +365,9 @@ static int nd_bus_remove(struct device *dev) ...@@ -364,9 +365,9 @@ static int nd_bus_remove(struct device *dev)
nd_synchronize(); nd_synchronize();
device_for_each_child(&nvdimm_bus->dev, NULL, child_unregister); device_for_each_child(&nvdimm_bus->dev, NULL, child_unregister);
nvdimm_bus_lock(&nvdimm_bus->dev); spin_lock(&nvdimm_bus->poison_lock);
free_poison_list(&nvdimm_bus->poison_list); free_poison_list(&nvdimm_bus->poison_list);
nvdimm_bus_unlock(&nvdimm_bus->dev); spin_unlock(&nvdimm_bus->poison_lock);
nvdimm_bus_destroy_ndctl(nvdimm_bus); nvdimm_bus_destroy_ndctl(nvdimm_bus);
...@@ -990,7 +991,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, ...@@ -990,7 +991,7 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm,
if (clear_err->cleared) { if (clear_err->cleared) {
/* clearing the poison list we keep track of */ /* clearing the poison list we keep track of */
__nvdimm_forget_poison(nvdimm_bus, clear_err->address, nvdimm_forget_poison(nvdimm_bus, clear_err->address,
clear_err->cleared); clear_err->cleared);
/* now sync the badblocks lists */ /* now sync the badblocks lists */
......
...@@ -518,6 +518,15 @@ void nvdimm_badblocks_populate(struct nd_region *nd_region, ...@@ -518,6 +518,15 @@ void nvdimm_badblocks_populate(struct nd_region *nd_region,
} }
EXPORT_SYMBOL_GPL(nvdimm_badblocks_populate); EXPORT_SYMBOL_GPL(nvdimm_badblocks_populate);
static void append_poison_entry(struct nvdimm_bus *nvdimm_bus,
struct nd_poison *pl, u64 addr, u64 length)
{
lockdep_assert_held(&nvdimm_bus->poison_lock);
pl->start = addr;
pl->length = length;
list_add_tail(&pl->list, &nvdimm_bus->poison_list);
}
static int add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length, static int add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length,
gfp_t flags) gfp_t flags)
{ {
...@@ -527,19 +536,24 @@ static int add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length, ...@@ -527,19 +536,24 @@ static int add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length,
if (!pl) if (!pl)
return -ENOMEM; return -ENOMEM;
pl->start = addr; append_poison_entry(nvdimm_bus, pl, addr, length);
pl->length = length;
list_add_tail(&pl->list, &nvdimm_bus->poison_list);
return 0; return 0;
} }
static int bus_add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length) static int bus_add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length)
{ {
struct nd_poison *pl; struct nd_poison *pl, *pl_new;
if (list_empty(&nvdimm_bus->poison_list)) spin_unlock(&nvdimm_bus->poison_lock);
return add_poison(nvdimm_bus, addr, length, GFP_KERNEL); pl_new = kzalloc(sizeof(*pl_new), GFP_KERNEL);
spin_lock(&nvdimm_bus->poison_lock);
if (list_empty(&nvdimm_bus->poison_list)) {
if (!pl_new)
return -ENOMEM;
append_poison_entry(nvdimm_bus, pl_new, addr, length);
return 0;
}
/* /*
* There is a chance this is a duplicate, check for those first. * There is a chance this is a duplicate, check for those first.
...@@ -551,6 +565,7 @@ static int bus_add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length) ...@@ -551,6 +565,7 @@ static int bus_add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length)
/* If length has changed, update this list entry */ /* If length has changed, update this list entry */
if (pl->length != length) if (pl->length != length)
pl->length = length; pl->length = length;
kfree(pl_new);
return 0; return 0;
} }
...@@ -559,30 +574,33 @@ static int bus_add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length) ...@@ -559,30 +574,33 @@ static int bus_add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length)
* as any overlapping ranges will get resolved when the list is consumed * as any overlapping ranges will get resolved when the list is consumed
* and converted to badblocks * and converted to badblocks
*/ */
return add_poison(nvdimm_bus, addr, length, GFP_KERNEL); if (!pl_new)
return -ENOMEM;
append_poison_entry(nvdimm_bus, pl_new, addr, length);
return 0;
} }
int nvdimm_bus_add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length) int nvdimm_bus_add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length)
{ {
int rc; int rc;
nvdimm_bus_lock(&nvdimm_bus->dev); spin_lock(&nvdimm_bus->poison_lock);
rc = bus_add_poison(nvdimm_bus, addr, length); rc = bus_add_poison(nvdimm_bus, addr, length);
nvdimm_bus_unlock(&nvdimm_bus->dev); spin_unlock(&nvdimm_bus->poison_lock);
return rc; return rc;
} }
EXPORT_SYMBOL_GPL(nvdimm_bus_add_poison); EXPORT_SYMBOL_GPL(nvdimm_bus_add_poison);
void __nvdimm_forget_poison(struct nvdimm_bus *nvdimm_bus, phys_addr_t start, void nvdimm_forget_poison(struct nvdimm_bus *nvdimm_bus, phys_addr_t start,
unsigned int len) unsigned int len)
{ {
struct list_head *poison_list = &nvdimm_bus->poison_list; struct list_head *poison_list = &nvdimm_bus->poison_list;
u64 clr_end = start + len - 1; u64 clr_end = start + len - 1;
struct nd_poison *pl, *next; struct nd_poison *pl, *next;
lockdep_assert_held(&nvdimm_bus->reconfig_mutex); spin_lock(&nvdimm_bus->poison_lock);
WARN_ON_ONCE(list_empty(poison_list)); WARN_ON_ONCE(list_empty(poison_list));
/* /*
...@@ -629,21 +647,13 @@ void __nvdimm_forget_poison(struct nvdimm_bus *nvdimm_bus, phys_addr_t start, ...@@ -629,21 +647,13 @@ void __nvdimm_forget_poison(struct nvdimm_bus *nvdimm_bus, phys_addr_t start,
u64 new_len = pl_end - new_start + 1; u64 new_len = pl_end - new_start + 1;
/* Add new entry covering the right half */ /* Add new entry covering the right half */
add_poison(nvdimm_bus, new_start, new_len, GFP_NOIO); add_poison(nvdimm_bus, new_start, new_len, GFP_NOWAIT);
/* Adjust this entry to cover the left half */ /* Adjust this entry to cover the left half */
pl->length = start - pl->start; pl->length = start - pl->start;
continue; continue;
} }
} }
} spin_unlock(&nvdimm_bus->poison_lock);
EXPORT_SYMBOL_GPL(__nvdimm_forget_poison);
void nvdimm_forget_poison(struct nvdimm_bus *nvdimm_bus,
phys_addr_t start, unsigned int len)
{
nvdimm_bus_lock(&nvdimm_bus->dev);
__nvdimm_forget_poison(nvdimm_bus, start, len);
nvdimm_bus_unlock(&nvdimm_bus->dev);
} }
EXPORT_SYMBOL_GPL(nvdimm_forget_poison); EXPORT_SYMBOL_GPL(nvdimm_forget_poison);
......
...@@ -32,6 +32,7 @@ struct nvdimm_bus { ...@@ -32,6 +32,7 @@ struct nvdimm_bus {
struct list_head poison_list; struct list_head poison_list;
struct list_head mapping_list; struct list_head mapping_list;
struct mutex reconfig_mutex; struct mutex reconfig_mutex;
spinlock_t poison_lock;
}; };
struct nvdimm { struct nvdimm {
......
...@@ -122,8 +122,6 @@ static inline struct nd_blk_region_desc *to_blk_region_desc( ...@@ -122,8 +122,6 @@ static inline struct nd_blk_region_desc *to_blk_region_desc(
int nvdimm_bus_add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length); int nvdimm_bus_add_poison(struct nvdimm_bus *nvdimm_bus, u64 addr, u64 length);
void nvdimm_forget_poison(struct nvdimm_bus *nvdimm_bus, void nvdimm_forget_poison(struct nvdimm_bus *nvdimm_bus,
phys_addr_t start, unsigned int len); phys_addr_t start, unsigned int len);
void __nvdimm_forget_poison(struct nvdimm_bus *nvdimm_bus,
phys_addr_t start, unsigned int len);
struct nvdimm_bus *nvdimm_bus_register(struct device *parent, struct nvdimm_bus *nvdimm_bus_register(struct device *parent,
struct nvdimm_bus_descriptor *nfit_desc); struct nvdimm_bus_descriptor *nfit_desc);
void nvdimm_bus_unregister(struct nvdimm_bus *nvdimm_bus); void nvdimm_bus_unregister(struct nvdimm_bus *nvdimm_bus);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment