On 08/17/2017 09:34 AM, Steven Rostedt wrote: > On Wed, 16 Aug 2017 16:40:40 -0400 > Waiman Long <longman@xxxxxxxxxx> wrote: > >> The lockdep code had reported the following unsafe locking scenario: >> >> CPU0 CPU1 >> ---- ---- >> lock(s_active#228); >> lock(&bdev->bd_mutex/1); >> lock(s_active#228); >> lock(&bdev->bd_mutex); > Can you show the exact locations of these locks. I have no idea where > this "s_active" is. The s_active isn't an actual lock. It is a reference count (kn->count) on the sysfs (kernfs) file. Removal of a sysfs file, however, require a wait until all the references are gone. The reference count is treated like a rwsem using lockdep instrumentation code. >> *** DEADLOCK *** >> >> The deadlock may happen when one task (CPU1) is trying to delete >> a partition in a block device and another task (CPU0) is accessing >> tracing sysfs file in that partition. >> >> To avoid that, accessing tracing sysfs file will now use a mutex >> trylock loop and the operation will fail if a delete operation is >> in progress. >> >> Signed-off-by: Waiman Long <longman@xxxxxxxxxx> >> --- >> >> v2: >> - Use READ_ONCE() and smp_store_mb() to read and write bd_deleting. >> - Check for signal in the mutex_trylock loops. >> - Use usleep() instead of schedule() for RT tasks. > I'm sorry but I really do hate this patch. Any suggestion on how to make it better? >> block/ioctl.c | 3 +++ >> include/linux/fs.h | 1 + >> kernel/trace/blktrace.c | 39 +++++++++++++++++++++++++++++++++++++-- >> 3 files changed, 41 insertions(+), 2 deletions(-) >> >> diff --git a/block/ioctl.c b/block/ioctl.c >> index 0de02ee..b920329 100644 >> --- a/block/ioctl.c >> +++ b/block/ioctl.c >> @@ -86,12 +86,15 @@ static int blkpg_ioctl(struct block_device *bdev, struct blkpg_ioctl_arg __user >> return -EBUSY; >> } >> /* all seems OK */ >> + smp_store_mb(bdev->bd_deleting, 1); > No comment to explain what is happening here, and why. I am going to add a comment block here. >> fsync_bdev(bdevp); >> invalidate_bdev(bdevp); >> >> mutex_lock_nested(&bdev->bd_mutex, 1); >> delete_partition(disk, partno); >> mutex_unlock(&bdev->bd_mutex); >> + smp_store_mb(bdev->bd_deleting, 0); >> + > ditto. > >> mutex_unlock(&bdevp->bd_mutex); >> bdput(bdevp); >> >> diff --git a/include/linux/fs.h b/include/linux/fs.h >> index 6e1fd5d..c2ba35e 100644 >> --- a/include/linux/fs.h >> +++ b/include/linux/fs.h >> @@ -427,6 +427,7 @@ struct block_device { >> #endif >> struct block_device * bd_contains; >> unsigned bd_block_size; >> + int bd_deleting; >> struct hd_struct * bd_part; >> /* number of times partitions within this device have been opened. */ >> unsigned bd_part_count; >> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c >> index bc364f8..b2dffa9 100644 >> --- a/kernel/trace/blktrace.c >> +++ b/kernel/trace/blktrace.c >> @@ -27,6 +27,8 @@ >> #include <linux/time.h> >> #include <linux/uaccess.h> >> #include <linux/list.h> >> +#include <linux/delay.h> >> +#include <linux/sched/rt.h> >> >> #include "../../block/blk.h" >> >> @@ -1605,6 +1607,23 @@ static struct request_queue *blk_trace_get_queue(struct block_device *bdev) >> return bdev_get_queue(bdev); >> } >> >> +/* >> + * Read/write to the tracing sysfs file requires taking references to the > What's the "tracing sysfs" file? tracefs? I am referring to blk_trace sysfs files which are used for enable tracing of block operations. >> + * sysfs file and then acquiring the bd_mutex. Deleting a block device >> + * requires acquiring the bd_mutex and then waiting for all the sysfs >> + * references to be gone. This can lead to deadlock if both operations >> + * happen simultaneously. To avoid this problem, read/write to the >> + * the tracing sysfs files can now fail if the bd_mutex cannot be >> + * acquired while a deletion operation is in progress. >> + * >> + * A mutex trylock loop is used assuming that tracing sysfs operations > A mutex trylock loop is not enough to stop a deadlock. But I'm guessing > the undocumented bd_deleting may prevent that. Yes, that is what the bd_deleting flag is for. I was thinking about failing the sysfs operation after a certain number of trylock attempts, but then it will require changes to user space code to handle the occasional failures. Finally I decided to fail it only when a delete operation is in progress as all hopes are lost in this case. >> + * aren't frequently enough to cause any contention problem. >> + * >> + * For RT tasks, a running high priority task will prevent any lower >> + * priority RT tasks from being run. So they do need to actually sleep >> + * when the trylock fails to allow lower priority tasks to make forward >> + * progress. >> + */ >> static ssize_t sysfs_blk_trace_attr_show(struct device *dev, >> struct device_attribute *attr, >> char *buf) >> @@ -1622,7 +1641,15 @@ static ssize_t sysfs_blk_trace_attr_show(struct device *dev, >> if (q == NULL) >> goto out_bdput; >> >> - mutex_lock(&bdev->bd_mutex); >> + while (!mutex_trylock(&bdev->bd_mutex)) { >> + if (READ_ONCE(bdev->bd_deleting)) >> + goto out_bdput; >> + if (signal_pending(current)) { >> + ret = -EINTR; >> + goto out_bdput; >> + } >> + rt_task(current) ? usleep_range(10, 10) : schedule(); > We need to come up with a better solution. This is just a hack that > circumvents a lot of the lockdep infrastructure. > > -- Steve The root cause is the lock inversion under this circumstance. I think modifying the blk_trace code has the least impact overall. I agree that the code is ugly. If you have a better suggestion, I will certainly like to hear it. Cheers, Longman