public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Vivek Goyal <vgoyal@redhat.com>
To: linux-kernel@vger.kernel.org, jaxboe@fusionio.com
Cc: vgoyal@redhat.com, neilb@suse.de
Subject: [PATCH 1/3] block: Initialize ->queue_lock to internal lock at queue allocation time
Date: Mon, 28 Feb 2011 14:25:28 -0500	[thread overview]
Message-ID: <1298921130-17533-2-git-send-email-vgoyal@redhat.com> (raw)
In-Reply-To: <1298921130-17533-1-git-send-email-vgoyal@redhat.com>

o There does not seem to be a clear convention whether q->queue_lock is
  initialized or not when blk_cleanup_queue() is called. In the past
  it was not necessary but now blk_throtl_exit() takes up queue lock
  by default and needs queue lock to be available.

  In fact elevator_exit() code also has similar requirement just that
  it is less stringent in the sense that elevator_exit() is called only
  if elevator is initialized.

o Two problems have been noticed because of ambiguity about spin lock
  status.

	- If a driver calls blk_alloc_queue() and then soon calls
	  blk_cleanup_queue() almost immediately, (because some other driver
	  structure allocation failed or some other error happened) then
	  blk_throtl_exit() will run into issues as queue lock is not
	  initialized. Loop driver ran into this issue recently and I noticed
	  error paths in md driver too. Similar error paths should exist in
	  other drivers too.

	- If some driver provided external spin lock and zapped the lock
	  before blk_cleanup_queue(), then it can lead to issues.

o So this patch initializes the default queue lock at queue allocation time.
  block throttling code is one of the users of queue lock and it is
  initialized at the queue allocation time, so it makes sense to
  initialize ->queue_lock also to internal lock. A driver can overide that
  lock later. This will take care of the issue where a driver does not have
  to worry about initializing the queue lock to default before calling
  blk_cleanup_queue()

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 block/blk-core.c     |   16 +++++++++++++++-
 block/blk-settings.c |    7 -------
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 3cc17e6..bc2b7c5 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -446,6 +446,11 @@ void blk_put_queue(struct request_queue *q)
 	kobject_put(&q->kobj);
 }
 
+/*
+ * Note: If a driver supplied the queue lock, it should not zap that lock
+ * unexpectedly as some queue cleanup components like elevator_exit() and
+ * blk_throtl_exit() need queue lock.
+ */
 void blk_cleanup_queue(struct request_queue *q)
 {
 	/*
@@ -540,6 +545,12 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
 	mutex_init(&q->sysfs_lock);
 	spin_lock_init(&q->__queue_lock);
 
+	/*
+	 * By default initialize queue_lock to internal lock and driver can
+	 * override it later if need be.
+	 */
+	q->queue_lock = &q->__queue_lock;
+
 	return q;
 }
 EXPORT_SYMBOL(blk_alloc_queue_node);
@@ -624,7 +635,10 @@ blk_init_allocated_queue_node(struct request_queue *q, request_fn_proc *rfn,
 	q->unprep_rq_fn		= NULL;
 	q->unplug_fn		= generic_unplug_device;
 	q->queue_flags		= QUEUE_FLAG_DEFAULT;
-	q->queue_lock		= lock;
+
+	/* Override internal queue lock with supplied lock pointer */
+	if (lock)
+		q->queue_lock		= lock;
 
 	/*
 	 * This also sets hw/phys segments, boundary and size
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 36c8c1f..df649fa 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -176,13 +176,6 @@ void blk_queue_make_request(struct request_queue *q, make_request_fn *mfn)
 	blk_queue_max_hw_sectors(q, BLK_SAFE_MAX_SECTORS);
 
 	/*
-	 * If the caller didn't supply a lock, fall back to our embedded
-	 * per-queue locks
-	 */
-	if (!q->queue_lock)
-		q->queue_lock = &q->__queue_lock;
-
-	/*
 	 * by default assume old behaviour and bounce for any highmem page
 	 */
 	blk_queue_bounce_limit(q, BLK_BOUNCE_HIGH);
-- 
1.7.2.3


  reply	other threads:[~2011-02-28 19:25 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-02-28 19:25 [PATCH 0/3] block: Few fixes for throttle and blk_cleanup_queue() Vivek Goyal
2011-02-28 19:25 ` Vivek Goyal [this message]
2011-02-28 19:25 ` [PATCH 2/3] loop: No need to initialize ->queue_lock explicitly before calling blk_cleanup_queue() Vivek Goyal
2011-02-28 19:25 ` [PATCH 3/3] block: Move blk_throtl_exit() call to blk_cleanup_queue() Vivek Goyal
2011-03-02 23:57 ` [PATCH 0/3] block: Few fixes for throttle and blk_cleanup_queue() Jens Axboe
2011-03-03  0:09   ` Vivek Goyal
2011-03-03  0:11     ` Jens Axboe
  -- strict thread matches above, loose matches on Subject: below --
2011-02-22  3:53 [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during call to blk_cleanup_queue() Vivek Goyal
2011-02-22  3:53 ` [PATCH 1/3] block: Initialize ->queue_lock to internal lock at queue allocation time Vivek Goyal

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=1298921130-17533-2-git-send-email-vgoyal@redhat.com \
    --to=vgoyal@redhat.com \
    --cc=jaxboe@fusionio.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=neilb@suse.de \
    /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