From: Derek Basehore <dbasehore@chromium.org>
To: linux-kernel@vger.kernel.org
Cc: axboe@kernel.dk, msb@chromium.orq, bleung@chromium.org,
taysom@chromium.org, Derek Basehore <dbasehore@chromium.org>
Subject: [PATCH v2 2/2] block: prevent race/cleanup
Date: Mon, 19 Nov 2012 18:07:02 -0800 [thread overview]
Message-ID: <1353377222-19541-2-git-send-email-dbasehore@chromium.org> (raw)
In-Reply-To: <1353377222-19541-1-git-send-email-dbasehore@chromium.org>
Remove a race condition which causes a warning in disk_clear_events. This is a
race between disk_clear_events and disk_flush_events. ev->clearing will be
altered by disk_flush_events (even though we are blocking event checking through
disk_flush_events). If this happens after ev->clearing was cleared for
disk_clear_events, this can cause the WARN_ON_ONCE in that function to be
triggered.
This change also has disk_clear_events not go through a workqueue. Since we have
to wait for the work to complete, we should just call the function directly.
Also, since this work cannot be put on a freezable workqueue, it will have to
contend with increased demand, so calling the function directly avoids this.
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
block/genhd.c | 46 +++++++++++++++++++++++++++++++---------------
1 files changed, 31 insertions(+), 15 deletions(-)
diff --git a/block/genhd.c b/block/genhd.c
index 4e700ee..768371f 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -35,6 +35,8 @@ static DEFINE_IDR(ext_devt_idr);
static struct device_type disk_type;
+static void disk_check_events(struct disk_events *ev,
+ unsigned int *clearing_ptr);
static void disk_alloc_events(struct gendisk *disk);
static void disk_add_events(struct gendisk *disk);
static void disk_del_events(struct gendisk *disk);
@@ -1555,6 +1557,7 @@ unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask)
const struct block_device_operations *bdops = disk->fops;
struct disk_events *ev = disk->ev;
unsigned int pending;
+ unsigned int clearing = mask;
if (!ev) {
/* for drivers still using the old ->media_changed method */
@@ -1564,40 +1567,53 @@ unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask)
return 0;
}
- /* tell the workfn about the events being cleared */
+ disk_block_events(disk);
+
+ /*
+ * store the union of mask and ev->clearing on the stack so that the
+ * race with disk_flush_events does not cause ambiguity (ev->clearing
+ * can still be modified even if events are blocked).
+ */
spin_lock_irq(&ev->lock);
- ev->clearing |= mask;
+ clearing |= ev->clearing;
+ ev->clearing = 0;
spin_unlock_irq(&ev->lock);
- /* uncondtionally schedule event check and wait for it to finish */
- disk_block_events(disk);
- /* We need to put the work on system_nrt_wq here since there is a
- * deadlock that happens while probing a usb device while suspending. If
- * we put work on a freezable worqueue here, a usb probe will wait here
- * until the workqueue is unfrozen during suspend. Since suspend waits
- * on all probes to complete, we have a deadlock
+ disk_check_events(ev, &clearing);
+ /*
+ * if ev->clearing is not 0, the disk_flush_events got called in the
+ * middle of this function, so we want to run the workfn without delay.
*/
- queue_delayed_work(system_nrt_wq, &ev->dwork, 0);
- flush_delayed_work(&ev->dwork);
- __disk_unblock_events(disk, false);
+ __disk_unblock_events(disk, ev->clearing ? true : false);
/* then, fetch and clear pending events */
spin_lock_irq(&ev->lock);
- WARN_ON_ONCE(ev->clearing & mask); /* cleared by workfn */
pending = ev->pending & mask;
ev->pending &= ~mask;
spin_unlock_irq(&ev->lock);
+ WARN_ON_ONCE(clearing & mask);
return pending;
}
+/*
+ * Seperate this part out so that a different pointer for clearing_ptr can be
+ * passed in for disk_clear_events.
+ */
static void disk_events_workfn(struct work_struct *work)
{
struct delayed_work *dwork = to_delayed_work(work);
struct disk_events *ev = container_of(dwork, struct disk_events, dwork);
+
+ disk_check_events(ev, &ev->clearing);
+}
+
+static void disk_check_events(struct disk_events *ev,
+ unsigned int *clearing_ptr)
+{
struct gendisk *disk = ev->disk;
char *envp[ARRAY_SIZE(disk_uevents) + 1] = { };
- unsigned int clearing = ev->clearing;
+ unsigned int clearing = *clearing_ptr;
unsigned int events;
unsigned long intv;
int nr_events = 0, i;
@@ -1610,7 +1626,7 @@ static void disk_events_workfn(struct work_struct *work)
events &= ~ev->pending;
ev->pending |= events;
- ev->clearing &= ~clearing;
+ *clearing_ptr &= ~clearing;
intv = disk_events_poll_jiffies(disk);
if (!ev->block && intv)
--
1.7.7.3
next prev parent reply other threads:[~2012-11-20 2:07 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-11-20 2:07 [PATCH v2 1/2] block: Remove deadlock in disk_clear_events Derek Basehore
2012-11-20 2:07 ` Derek Basehore [this message]
2012-11-21 8:31 ` Andrew Morton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1353377222-19541-2-git-send-email-dbasehore@chromium.org \
--to=dbasehore@chromium.org \
--cc=axboe@kernel.dk \
--cc=bleung@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=msb@chromium.orq \
--cc=taysom@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox