* [PATCHv2] block/genhd: use atomic_t for disk_event->block
@ 2021-06-02 6:20 Hannes Reinecke
2021-06-02 6:47 ` Ming Lei
0 siblings, 1 reply; 2+ messages in thread
From: Hannes Reinecke @ 2021-06-02 6:20 UTC (permalink / raw)
To: Jens Axboe
Cc: Christoph Hellwig, Ming Lei, linux-block,
Linux Kernel Mailinglist, Hannes Reinecke
__disk_unblock_events() will call queue_delayed_work() with a '0' argument
under a spin lock. This might cause the queue_work item to be executed
immediately, and run into a deadlock in disk_check_events() waiting for
the lock to be released.
This patch converts the 'blocked' counter into an atomic variable, so we don't
need to hold a spinlock anymore when scheduling the workqueue function.
Changes to v1:
- Fixup wrong mutex_unlock detected by 0-day robot
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
block/genhd.c | 35 ++++++++++++-----------------------
1 file changed, 12 insertions(+), 23 deletions(-)
diff --git a/block/genhd.c b/block/genhd.c
index 9f8cb7beaad1..a18a3058728b 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1379,7 +1379,7 @@ struct disk_events {
spinlock_t lock;
struct mutex block_mutex; /* protects blocking */
- int block; /* event blocking depth */
+ atomic_t block; /* event blocking depth */
unsigned int pending; /* events already sent out */
unsigned int clearing; /* events being cleared */
@@ -1439,8 +1439,6 @@ static unsigned long disk_events_poll_jiffies(struct gendisk *disk)
void disk_block_events(struct gendisk *disk)
{
struct disk_events *ev = disk->ev;
- unsigned long flags;
- bool cancel;
if (!ev)
return;
@@ -1451,11 +1449,7 @@ void disk_block_events(struct gendisk *disk)
*/
mutex_lock(&ev->block_mutex);
- spin_lock_irqsave(&ev->lock, flags);
- cancel = !ev->block++;
- spin_unlock_irqrestore(&ev->lock, flags);
-
- if (cancel)
+ if (atomic_inc_return(&ev->block) == 1)
cancel_delayed_work_sync(&disk->ev->dwork);
mutex_unlock(&ev->block_mutex);
@@ -1467,23 +1461,18 @@ static void __disk_unblock_events(struct gendisk *disk, bool check_now)
unsigned long intv;
unsigned long flags;
- spin_lock_irqsave(&ev->lock, flags);
-
- if (WARN_ON_ONCE(ev->block <= 0))
- goto out_unlock;
-
- if (--ev->block)
- goto out_unlock;
+ if (atomic_dec_return(&ev->block) <= 0)
+ return;
+ spin_lock_irqsave(&ev->lock, flags);
intv = disk_events_poll_jiffies(disk);
+ spin_unlock_irqrestore(&ev->lock, flags);
if (check_now)
queue_delayed_work(system_freezable_power_efficient_wq,
&ev->dwork, 0);
else if (intv)
queue_delayed_work(system_freezable_power_efficient_wq,
&ev->dwork, intv);
-out_unlock:
- spin_unlock_irqrestore(&ev->lock, flags);
}
/**
@@ -1523,10 +1512,10 @@ void disk_flush_events(struct gendisk *disk, unsigned int mask)
spin_lock_irq(&ev->lock);
ev->clearing |= mask;
- if (!ev->block)
+ spin_unlock_irq(&ev->lock);
+ if (!atomic_read(&ev->block))
mod_delayed_work(system_freezable_power_efficient_wq,
&ev->dwork, 0);
- spin_unlock_irq(&ev->lock);
}
/**
@@ -1638,11 +1627,11 @@ static void disk_check_events(struct disk_events *ev,
*clearing_ptr &= ~clearing;
intv = disk_events_poll_jiffies(disk);
- if (!ev->block && intv)
+ spin_unlock_irq(&ev->lock);
+ if (!atomic_read(&ev->block) && intv)
queue_delayed_work(system_freezable_power_efficient_wq,
&ev->dwork, intv);
- spin_unlock_irq(&ev->lock);
/*
* Tell userland about new events. Only the events listed in
@@ -1807,7 +1796,7 @@ static void disk_alloc_events(struct gendisk *disk)
ev->disk = disk;
spin_lock_init(&ev->lock);
mutex_init(&ev->block_mutex);
- ev->block = 1;
+ atomic_set(&ev->block, 1);
ev->poll_msecs = -1;
INIT_DELAYED_WORK(&ev->dwork, disk_events_workfn);
@@ -1851,6 +1840,6 @@ static void disk_del_events(struct gendisk *disk)
static void disk_release_events(struct gendisk *disk)
{
/* the block count should be 1 from disk_del_events() */
- WARN_ON_ONCE(disk->ev && disk->ev->block != 1);
+ WARN_ON_ONCE(disk->ev && atomic_read(&disk->ev->block) != 1);
kfree(disk->ev);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCHv2] block/genhd: use atomic_t for disk_event->block
2021-06-02 6:20 [PATCHv2] block/genhd: use atomic_t for disk_event->block Hannes Reinecke
@ 2021-06-02 6:47 ` Ming Lei
0 siblings, 0 replies; 2+ messages in thread
From: Ming Lei @ 2021-06-02 6:47 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Jens Axboe, Christoph Hellwig, linux-block, Linux Kernel Mailinglist
On Wed, Jun 02, 2021 at 08:20:15AM +0200, Hannes Reinecke wrote:
> __disk_unblock_events() will call queue_delayed_work() with a '0' argument
> under a spin lock. This might cause the queue_work item to be executed
> immediately, and run into a deadlock in disk_check_events() waiting for
> the lock to be released.
The above commit log isn't correct, and the current usage shouldn't cause such
deadlock, so can you root cause the task hang report[1] on 5.3.18?
[1] https://lore.kernel.org/linux-block/73783f6f-a3ec-907f-ea19-966e9d1457dc@suse.de/T/#mcde381522bdb95740e04ced555ec017a25c993cc
Thanks,
Ming
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-06-02 6:47 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-02 6:20 [PATCHv2] block/genhd: use atomic_t for disk_event->block Hannes Reinecke
2021-06-02 6:47 ` Ming Lei
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.