Skip to contentSkip to conversation

block: non-atomic queue_flags prep

RFC PATCH In review Nick Piggin 2 participants 6 messages Dec 15, 2007 – Dec 19, 2007

Subsystems (4)
SCSI Block layer MD RAID Device mapper
lore
Share & tools
lore

Nick Piggin

author
#0 lore raw

RFC PATCH 1/3 block: non-atomic queue_flags prep

Hi,

This is just an idea I had, which might make request processing a little bit cheaper depending on queue behaviour. For example if it is getting plugged unplugged frequently (as I think is the case for some database workloads), then we might save one or two atomic operations per request.

Anyway, I'm not completely sure if I have ensured all queue_flags users are safe (I think md may need a bit of help). But overall it seems quite doable.

Comments?
---
Prep queue_flags to be non-atomic and taking protection from queue_lock. Do this by ensuring the queue_lock is held everywhere that queue_flags are changed. Locking additions are confined to slowpaths.

block/elevator.c +7 -0
Index: linux-2.6/block/elevator.c
===================================================================
@@ -1066,7 +1066,10 @@ static int elevator_switch(struct reques
 	 * finally exit old elevator and turn off BYPASS.
 	 */
 	elevator_exit(old_elevator);
+	spin_lock_irq(q->queue_lock);
 	clear_bit(QUEUE_FLAG_ELVSWITCH, &q->queue_flags);
+	spin_unlock_irq(q->queue_lock);
+
 	return 1;
 
 fail_register:

@@ -1077,7 +1080,11 @@ fail_register:
 	elevator_exit(e);
 	q->elevator = old_elevator;
 	elv_register_queue(q);
+
+	spin_lock_irq(q->queue_lock);
 	clear_bit(QUEUE_FLAG_ELVSWITCH, &q->queue_flags);
+	spin_unlock_irq(q->queue_lock);
+
 	return 0;
 }
block/ll_rw_blk.c +16 -5
Index: linux-2.6/block/ll_rw_blk.c
===================================================================
@@ -1732,14 +1732,11 @@ void blk_sync_queue(struct request_queue
 EXPORT_SYMBOL(blk_sync_queue);
 
 /**
- * blk_run_queue - run a single device queue
+ * __blk_run_queue - run a single device queue. queue_lock must be held.
  * @q:	The queue to run
  */
-void blk_run_queue(struct request_queue *q)
+void __blk_run_queue(struct request_queue *q)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(q->queue_lock, flags);
 	blk_remove_plug(q);
 
 	/*

@@ -1755,7 +1752,19 @@ void blk_run_queue(struct request_queue 
 			kblockd_schedule_work(&q->unplug_work);
 		}
 	}
+}
+EXPORT_SYMBOL(__blk_run_queue);
 
+/**
+ * blk_run_queue - run a single device queue
+ * @q:	The queue to run
+ */
+void blk_run_queue(struct request_queue *q)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	__blk_run_queue(q);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 }
 EXPORT_SYMBOL(blk_run_queue);

@@ -1804,7 +1813,9 @@ EXPORT_SYMBOL(blk_put_queue);
 void blk_cleanup_queue(struct request_queue * q)
 {
 	mutex_lock(&q->sysfs_lock);
+	spin_lock_irq(q->queue_lock);
 	set_bit(QUEUE_FLAG_DEAD, &q->queue_flags);
+	spin_unlock_irq(q->queue_lock);
 	mutex_unlock(&q->sysfs_lock);
 
 	if (q->elevator)
drivers/scsi/scsi_lib.c +12 -9
Index: linux-2.6/drivers/scsi/scsi_lib.c
===================================================================
@@ -532,6 +532,9 @@ static void scsi_run_queue(struct reques
 	       !shost->host_blocked && !shost->host_self_blocked &&
 		!((shost->can_queue > 0) &&
 		  (shost->host_busy >= shost->can_queue))) {
+
+		int flagset;
+
 		/*
 		 * As long as shost is accepting commands and we have
 		 * starved queues, call blk_run_queue. scsi_request_fn

@@ -547,17 +550,17 @@ static void scsi_run_queue(struct reques
 		list_del_init(&sdev->starved_entry);
 		spin_unlock_irqrestore(shost->host_lock, flags);
 
-
-		if (test_bit(QUEUE_FLAG_REENTER, &q->queue_flags) &&
-		    !test_and_set_bit(QUEUE_FLAG_REENTER,
-				      &sdev->request_queue->queue_flags)) {
-			blk_run_queue(sdev->request_queue);
+		spin_lock(sdev->request_queue->queue_lock);
+		flagset = test_bit(QUEUE_FLAG_REENTER, &q->queue_flags) &&
+				!test_and_set_bit(QUEUE_FLAG_REENTER,
+					&sdev->request_queue->queue_flags);
+		__blk_run_queue(sdev->request_queue);
+		if (flagset)
 			clear_bit(QUEUE_FLAG_REENTER,
-				  &sdev->request_queue->queue_flags);
-		} else
-			blk_run_queue(sdev->request_queue);
+					&sdev->request_queue->queue_flags);
+		spin_unlock(sdev->request_queue->queue_lock);
 
-		spin_lock_irqsave(shost->host_lock, flags);
+		spin_lock(shost->host_lock);
 		if (unlikely(!list_empty(&sdev->starved_entry)))
 			/*
 			 * sdev lost a race, and was put back on the
drivers/block/loop.c +3 -0
Index: linux-2.6/drivers/block/loop.c
===================================================================
@@ -541,9 +541,12 @@ out:
  */
 static void loop_unplug(struct request_queue *q)
 {
+	unsigned long flags;
 	struct loop_device *lo = q->queuedata;
 
+	spin_lock_irqsave(q->queue_lock, flags);
 	clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags);
+	spin_unlock_irqrestore(q->queue_lock, flags);
 	blk_run_address_space(lo->lo_backing_file->f_mapping);
 }
include/linux/blkdev.h +1 -0
Index: linux-2.6/include/linux/blkdev.h
===================================================================
@@ -685,6 +685,7 @@ extern void blk_start_queue(struct reque
 extern void blk_stop_queue(struct request_queue *q);
 extern void blk_sync_queue(struct request_queue *q);
 extern void __blk_stop_queue(struct request_queue *q);
+extern void __blk_run_queue(struct request_queue *);
 extern void blk_run_queue(struct request_queue *);
 extern void blk_start_queueing(struct request_queue *);
 extern int blk_rq_map_user(struct request_queue *, struct request *, void __user *, unsigned long);
author
#1 lore raw

RFC PATCH 2/3 block: non-atomic queue_flags

All queue_flag manipulations are performed under queue_lock (or eg. during allocation-time where parallelism isn't a problem). So we can use non-atomic bitops for these.

block/elevator.c +3 -3
Index: linux-2.6/block/elevator.c
===================================================================
@@ -1032,7 +1032,7 @@ static int elevator_switch(struct reques
 	 */
 	spin_lock_irq(q->queue_lock);
 
-	set_bit(QUEUE_FLAG_ELVSWITCH, &q->queue_flags);
+	__set_bit(QUEUE_FLAG_ELVSWITCH, &q->queue_flags);
 
 	elv_drain_elevator(q);
 

@@ -1067,7 +1067,7 @@ static int elevator_switch(struct reques
 	 */
 	elevator_exit(old_elevator);
 	spin_lock_irq(q->queue_lock);
-	clear_bit(QUEUE_FLAG_ELVSWITCH, &q->queue_flags);
+	__clear_bit(QUEUE_FLAG_ELVSWITCH, &q->queue_flags);
 	spin_unlock_irq(q->queue_lock);
 
 	return 1;

@@ -1082,7 +1082,7 @@ fail_register:
 	elv_register_queue(q);
 
 	spin_lock_irq(q->queue_lock);
-	clear_bit(QUEUE_FLAG_ELVSWITCH, &q->queue_flags);
+	__clear_bit(QUEUE_FLAG_ELVSWITCH, &q->queue_flags);
 	spin_unlock_irq(q->queue_lock);
 
 	return 0;
block/ll_rw_blk.c +21 -17
Index: linux-2.6/block/ll_rw_blk.c
===================================================================
@@ -720,7 +720,7 @@ void blk_queue_stack_limits(struct reque
 	t->max_segment_size = min(t->max_segment_size,b->max_segment_size);
 	t->hardsect_size = max(t->hardsect_size,b->hardsect_size);
 	if (!test_bit(QUEUE_FLAG_CLUSTER, &b->queue_flags))
-		clear_bit(QUEUE_FLAG_CLUSTER, &t->queue_flags);
+		__clear_bit(QUEUE_FLAG_CLUSTER, &t->queue_flags);
 }
 
 EXPORT_SYMBOL(blk_queue_stack_limits);

@@ -823,7 +823,7 @@ static void __blk_queue_free_tags(struct
 	__blk_free_tags(bqt);
 
 	q->queue_tags = NULL;
-	q->queue_flags &= ~(1 << QUEUE_FLAG_QUEUED);
+	__clear_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
 }
 
 

@@ -852,7 +852,7 @@ EXPORT_SYMBOL(blk_free_tags);
  **/
 void blk_queue_free_tags(struct request_queue *q)
 {
-	clear_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
+	__clear_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
 }
 
 EXPORT_SYMBOL(blk_queue_free_tags);

@@ -942,7 +942,7 @@ int blk_queue_init_tags(struct request_q
 	} else if (q->queue_tags) {
 		if ((rc = blk_queue_resize_tags(q, depth)))
 			return rc;
-		set_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
+		__set_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
 		return 0;
 	} else
 		atomic_inc(&tags->refcnt);

@@ -951,7 +951,7 @@ int blk_queue_init_tags(struct request_q
 	 * assign it, all done
 	 */
 	q->queue_tags = tags;
-	q->queue_flags |= (1 << QUEUE_FLAG_QUEUED);
+	__set_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
 	INIT_LIST_HEAD(&q->tag_busy_list);
 	return 0;
 fail:

@@ -1205,7 +1205,7 @@ static void blk_recalc_rq_segments(struc
 	if (!rq->bio)
 		return;
 
-	cluster = q->queue_flags & (1 << QUEUE_FLAG_CLUSTER);
+	cluster = test_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
 	hw_seg_size = seg_size = 0;
 	phys_size = hw_size = nr_phys_segs = nr_hw_segs = 0;
 	rq_for_each_segment(bv, rq, iter) {

@@ -1263,7 +1263,7 @@ new_hw_segment:
 static int blk_phys_contig_segment(struct request_queue *q, struct bio *bio,
 				   struct bio *nxt)
 {
-	if (!(q->queue_flags & (1 << QUEUE_FLAG_CLUSTER)))
+	if (!test_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags))
 		return 0;
 
 	if (!BIOVEC_PHYS_MERGEABLE(__BVEC_END(bio), __BVEC_START(nxt)))

@@ -1310,7 +1310,7 @@ int blk_rq_map_sg(struct request_queue *
 	int nsegs, cluster;
 
 	nsegs = 0;
-	cluster = q->queue_flags & (1 << QUEUE_FLAG_CLUSTER);
+	cluster = test_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
 
 	/*
 	 * for each bio in rq

@@ -1557,7 +1557,8 @@ void blk_plug_device(struct request_queu
 	if (blk_queue_stopped(q))
 		return;
 
-	if (!test_and_set_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags)) {
+	if (!test_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags)) {
+		__set_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags);
 		mod_timer(&q->unplug_timer, jiffies + q->unplug_delay);
 		blk_add_trace_generic(q, NULL, 0, BLK_TA_PLUG);
 	}

@@ -1573,8 +1574,9 @@ int blk_remove_plug(struct request_queue
 {
 	WARN_ON(!irqs_disabled());
 
-	if (!test_and_clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags))
+	if (!test_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags))
 		return 0;
+	__clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags);
 
 	del_timer(&q->unplug_timer);
 	return 1;

@@ -1672,15 +1674,16 @@ void blk_start_queue(struct request_queu
 {
 	WARN_ON(!irqs_disabled());
 
-	clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
+	__clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
 
 	/*
 	 * one level of recursion is ok and is much faster than kicking
 	 * the unplug handling
 	 */
-	if (!test_and_set_bit(QUEUE_FLAG_REENTER, &q->queue_flags)) {
+	if (!test_bit(QUEUE_FLAG_REENTER, &q->queue_flags)) {
+		__set_bit(QUEUE_FLAG_REENTER, &q->queue_flags);
 		q->request_fn(q);
-		clear_bit(QUEUE_FLAG_REENTER, &q->queue_flags);
+		__clear_bit(QUEUE_FLAG_REENTER, &q->queue_flags);
 	} else {
 		blk_plug_device(q);
 		kblockd_schedule_work(&q->unplug_work);

@@ -1706,7 +1709,7 @@ EXPORT_SYMBOL(blk_start_queue);
 void blk_stop_queue(struct request_queue *q)
 {
 	blk_remove_plug(q);
-	set_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
+	__set_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
 }
 EXPORT_SYMBOL(blk_stop_queue);
 

@@ -1744,9 +1747,10 @@ void __blk_run_queue(struct request_queu
 	 * handling reinvoke the handler shortly if we already got there.
 	 */
 	if (!elv_queue_empty(q)) {
-		if (!test_and_set_bit(QUEUE_FLAG_REENTER, &q->queue_flags)) {
+		if (!test_bit(QUEUE_FLAG_REENTER, &q->queue_flags)) {
+			__set_bit(QUEUE_FLAG_REENTER, &q->queue_flags);
 			q->request_fn(q);
-			clear_bit(QUEUE_FLAG_REENTER, &q->queue_flags);
+			__clear_bit(QUEUE_FLAG_REENTER, &q->queue_flags);
 		} else {
 			blk_plug_device(q);
 			kblockd_schedule_work(&q->unplug_work);

@@ -1814,7 +1818,7 @@ void blk_cleanup_queue(struct request_qu
 {
 	mutex_lock(&q->sysfs_lock);
 	spin_lock_irq(q->queue_lock);
-	set_bit(QUEUE_FLAG_DEAD, &q->queue_flags);
+	__set_bit(QUEUE_FLAG_DEAD, &q->queue_flags);
 	spin_unlock_irq(q->queue_lock);
 	mutex_unlock(&q->sysfs_lock);
drivers/block/loop.c +1 -1
Index: linux-2.6/drivers/block/loop.c
===================================================================
@@ -545,7 +545,7 @@ static void loop_unplug(struct request_q
 	struct loop_device *lo = q->queuedata;
 
 	spin_lock_irqsave(q->queue_lock, flags);
-	clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags);
+	__clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 	blk_run_address_space(lo->lo_backing_file->f_mapping);
 }
drivers/block/ub.c +1 -1
Index: linux-2.6/drivers/block/ub.c
===================================================================
@@ -2402,7 +2402,7 @@ static void ub_disconnect(struct usb_int
 		del_gendisk(lun->disk);
 		/*
 		 * I wish I could do:
-		 *    set_bit(QUEUE_FLAG_DEAD, &q->queue_flags);
+		 *    __set_bit(QUEUE_FLAG_DEAD, &q->queue_flags);
 		 * As it is, we rely on our internal poisoning and let
 		 * the upper levels to spin furiously failing all the I/O.
 		 */
drivers/md/dm-table.c +2 -2
Index: linux-2.6/drivers/md/dm-table.c
===================================================================
@@ -899,9 +899,9 @@ void dm_table_set_restrictions(struct dm
 	q->seg_boundary_mask = t->limits.seg_boundary_mask;
 	q->bounce_pfn = t->limits.bounce_pfn;
 	if (t->limits.no_cluster)
-		q->queue_flags &= ~(1 << QUEUE_FLAG_CLUSTER);
+		__clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
 	else
-		q->queue_flags |= (1 << QUEUE_FLAG_CLUSTER);
+		__set_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
 
 }
drivers/md/md.c +1 -1
Index: linux-2.6/drivers/md/md.c
===================================================================
@@ -281,7 +281,7 @@ static mddev_t * mddev_find(dev_t unit)
 		kfree(new);
 		return NULL;
 	}
-	set_bit(QUEUE_FLAG_CLUSTER, &new->queue->queue_flags);
+	__set_bit(QUEUE_FLAG_CLUSTER, &new->queue->queue_flags);
 
 	blk_queue_make_request(new->queue, md_fail_request);
drivers/scsi/scsi_lib.c +6 -3
Index: linux-2.6/drivers/scsi/scsi_lib.c
===================================================================
@@ -552,11 +552,14 @@ static void scsi_run_queue(struct reques
 
 		spin_lock(sdev->request_queue->queue_lock);
 		flagset = test_bit(QUEUE_FLAG_REENTER, &q->queue_flags) &&
-				!test_and_set_bit(QUEUE_FLAG_REENTER,
+				!test_bit(QUEUE_FLAG_REENTER,
+					&sdev->request_queue->queue_flags);
+		if (flagset)
+			__set_bit(QUEUE_FLAG_REENTER,
 					&sdev->request_queue->queue_flags);
 		__blk_run_queue(sdev->request_queue);
 		if (flagset)
-			clear_bit(QUEUE_FLAG_REENTER,
+			__clear_bit(QUEUE_FLAG_REENTER,
 					&sdev->request_queue->queue_flags);
 		spin_unlock(sdev->request_queue->queue_lock);
 

@@ -1677,7 +1680,7 @@ struct request_queue *__scsi_alloc_queue
 	blk_queue_segment_boundary(q, shost->dma_boundary);
 
 	if (!shost->use_clustering)
-		clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
+		__clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
 	return q;
 }
 EXPORT_SYMBOL(__scsi_alloc_queue);
drivers/scsi/scsi_transport_sas.c +1 -1
Index: linux-2.6/drivers/scsi/scsi_transport_sas.c
===================================================================
@@ -234,7 +234,7 @@ static int sas_bsg_initialize(struct Scs
 	else
 		q->queuedata = shost;
 
-	set_bit(QUEUE_FLAG_BIDI, &q->queue_flags);
+	__set_bit(QUEUE_FLAG_BIDI, &q->queue_flags);
 
 	return 0;
 }
include/linux/blkdev.h +4 -4
Index: linux-2.6/include/linux/blkdev.h
===================================================================
@@ -560,17 +560,17 @@ static inline int blk_queue_full(struct 
 static inline void blk_set_queue_full(struct request_queue *q, int rw)
 {
 	if (rw == READ)
-		set_bit(QUEUE_FLAG_READFULL, &q->queue_flags);
+		__set_bit(QUEUE_FLAG_READFULL, &q->queue_flags);
 	else
-		set_bit(QUEUE_FLAG_WRITEFULL, &q->queue_flags);
+		__set_bit(QUEUE_FLAG_WRITEFULL, &q->queue_flags);
 }
 
 static inline void blk_clear_queue_full(struct request_queue *q, int rw)
 {
 	if (rw == READ)
-		clear_bit(QUEUE_FLAG_READFULL, &q->queue_flags);
+		__clear_bit(QUEUE_FLAG_READFULL, &q->queue_flags);
 	else
-		clear_bit(QUEUE_FLAG_WRITEFULL, &q->queue_flags);
+		__clear_bit(QUEUE_FLAG_WRITEFULL, &q->queue_flags);
 }
author
#2 lore raw

RFC PATCH 3/3 block: non-atomic queue_flags accessors

In reply to Nick Piggin

Introduce queue_ accessors to set and clear queue_flags, which include debug checks to ensure queue_lock is held. Non-checking versions are provided where it is known that there can be no parallelism on queue_flags.

block/elevator.c +3 -3
Index: linux-2.6/block/elevator.c
===================================================================
@@ -1032,7 +1032,7 @@ static int elevator_switch(struct reques
 	 */
 	spin_lock_irq(q->queue_lock);
 
-	__set_bit(QUEUE_FLAG_ELVSWITCH, &q->queue_flags);
+	queue_flag_set(QUEUE_FLAG_ELVSWITCH, q);
 
 	elv_drain_elevator(q);
 

@@ -1067,7 +1067,7 @@ static int elevator_switch(struct reques
 	 */
 	elevator_exit(old_elevator);
 	spin_lock_irq(q->queue_lock);
-	__clear_bit(QUEUE_FLAG_ELVSWITCH, &q->queue_flags);
+	queue_flag_clear(QUEUE_FLAG_ELVSWITCH, q);
 	spin_unlock_irq(q->queue_lock);
 
 	return 1;

@@ -1082,7 +1082,7 @@ fail_register:
 	elv_register_queue(q);
 
 	spin_lock_irq(q->queue_lock);
-	__clear_bit(QUEUE_FLAG_ELVSWITCH, &q->queue_flags);
+	queue_flag_clear(QUEUE_FLAG_ELVSWITCH, q);
 	spin_unlock_irq(q->queue_lock);
 
 	return 0;
block/ll_rw_blk.c +14 -14
Index: linux-2.6/block/ll_rw_blk.c
===================================================================
@@ -720,7 +720,7 @@ void blk_queue_stack_limits(struct reque
 	t->max_segment_size = min(t->max_segment_size,b->max_segment_size);
 	t->hardsect_size = max(t->hardsect_size,b->hardsect_size);
 	if (!test_bit(QUEUE_FLAG_CLUSTER, &b->queue_flags))
-		__clear_bit(QUEUE_FLAG_CLUSTER, &t->queue_flags);
+		queue_flag_clear(QUEUE_FLAG_CLUSTER, t);
 }
 
 EXPORT_SYMBOL(blk_queue_stack_limits);

@@ -823,7 +823,7 @@ static void __blk_queue_free_tags(struct
 	__blk_free_tags(bqt);
 
 	q->queue_tags = NULL;
-	__clear_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
+	queue_flag_clear(QUEUE_FLAG_QUEUED, q);
 }
 
 

@@ -852,7 +852,7 @@ EXPORT_SYMBOL(blk_free_tags);
  **/
 void blk_queue_free_tags(struct request_queue *q)
 {
-	__clear_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
+	queue_flag_clear(QUEUE_FLAG_QUEUED, q);
 }
 
 EXPORT_SYMBOL(blk_queue_free_tags);

@@ -942,7 +942,7 @@ int blk_queue_init_tags(struct request_q
 	} else if (q->queue_tags) {
 		if ((rc = blk_queue_resize_tags(q, depth)))
 			return rc;
-		__set_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
+		queue_flag_set(QUEUE_FLAG_QUEUED, q);
 		return 0;
 	} else
 		atomic_inc(&tags->refcnt);

@@ -951,7 +951,7 @@ int blk_queue_init_tags(struct request_q
 	 * assign it, all done
 	 */
 	q->queue_tags = tags;
-	__set_bit(QUEUE_FLAG_QUEUED, &q->queue_flags);
+	queue_flag_set(QUEUE_FLAG_QUEUED, q);
 	INIT_LIST_HEAD(&q->tag_busy_list);
 	return 0;
 fail:

@@ -1558,7 +1558,7 @@ void blk_plug_device(struct request_queu
 		return;
 
 	if (!test_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags)) {
-		__set_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags);
+		queue_flag_set(QUEUE_FLAG_PLUGGED, q);
 		mod_timer(&q->unplug_timer, jiffies + q->unplug_delay);
 		blk_add_trace_generic(q, NULL, 0, BLK_TA_PLUG);
 	}

@@ -1576,7 +1576,7 @@ int blk_remove_plug(struct request_queue
 
 	if (!test_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags))
 		return 0;
-	__clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags);
+	queue_flag_clear(QUEUE_FLAG_PLUGGED, q);
 
 	del_timer(&q->unplug_timer);
 	return 1;

@@ -1674,16 +1674,16 @@ void blk_start_queue(struct request_queu
 {
 	WARN_ON(!irqs_disabled());
 
-	__clear_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
+	queue_flag_clear(QUEUE_FLAG_STOPPED, q);
 
 	/*
 	 * one level of recursion is ok and is much faster than kicking
 	 * the unplug handling
 	 */
 	if (!test_bit(QUEUE_FLAG_REENTER, &q->queue_flags)) {
-		__set_bit(QUEUE_FLAG_REENTER, &q->queue_flags);
+		queue_flag_set(QUEUE_FLAG_REENTER, q);
 		q->request_fn(q);
-		__clear_bit(QUEUE_FLAG_REENTER, &q->queue_flags);
+		queue_flag_clear(QUEUE_FLAG_REENTER, q);
 	} else {
 		blk_plug_device(q);
 		kblockd_schedule_work(&q->unplug_work);

@@ -1709,7 +1709,7 @@ EXPORT_SYMBOL(blk_start_queue);
 void blk_stop_queue(struct request_queue *q)
 {
 	blk_remove_plug(q);
-	__set_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
+	queue_flag_set(QUEUE_FLAG_STOPPED, q);
 }
 EXPORT_SYMBOL(blk_stop_queue);
 

@@ -1748,9 +1748,9 @@ void __blk_run_queue(struct request_queu
 	 */
 	if (!elv_queue_empty(q)) {
 		if (!test_bit(QUEUE_FLAG_REENTER, &q->queue_flags)) {
-			__set_bit(QUEUE_FLAG_REENTER, &q->queue_flags);
+			queue_flag_set(QUEUE_FLAG_REENTER, q);
 			q->request_fn(q);
-			__clear_bit(QUEUE_FLAG_REENTER, &q->queue_flags);
+			queue_flag_clear(QUEUE_FLAG_REENTER, q);
 		} else {
 			blk_plug_device(q);
 			kblockd_schedule_work(&q->unplug_work);

@@ -1818,7 +1818,7 @@ void blk_cleanup_queue(struct request_qu
 {
 	mutex_lock(&q->sysfs_lock);
 	spin_lock_irq(q->queue_lock);
-	__set_bit(QUEUE_FLAG_DEAD, &q->queue_flags);
+	queue_flag_set(QUEUE_FLAG_DEAD, q);
 	spin_unlock_irq(q->queue_lock);
 	mutex_unlock(&q->sysfs_lock);
drivers/block/loop.c +1 -4
Index: linux-2.6/drivers/block/loop.c
===================================================================
@@ -541,12 +541,9 @@ out:
  */
 static void loop_unplug(struct request_queue *q)
 {
-	unsigned long flags;
 	struct loop_device *lo = q->queuedata;
 
-	spin_lock_irqsave(q->queue_lock, flags);
-	__clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags);
-	spin_unlock_irqrestore(q->queue_lock, flags);
+	queue_flag_clear_unlocked(QUEUE_FLAG_PLUGGED, q);
 	blk_run_address_space(lo->lo_backing_file->f_mapping);
 }
drivers/block/ub.c +1 -1
Index: linux-2.6/drivers/block/ub.c
===================================================================
@@ -2402,7 +2402,7 @@ static void ub_disconnect(struct usb_int
 		del_gendisk(lun->disk);
 		/*
 		 * I wish I could do:
-		 *    __set_bit(QUEUE_FLAG_DEAD, &q->queue_flags);
+		 *    queue_flag_set(QUEUE_FLAG_DEAD, q);
 		 * As it is, we rely on our internal poisoning and let
 		 * the upper levels to spin furiously failing all the I/O.
 		 */
drivers/md/dm-table.c +5 -2
Index: linux-2.6/drivers/md/dm-table.c
===================================================================
@@ -898,10 +898,13 @@ void dm_table_set_restrictions(struct dm
 	q->max_segment_size = t->limits.max_segment_size;
 	q->seg_boundary_mask = t->limits.seg_boundary_mask;
 	q->bounce_pfn = t->limits.bounce_pfn;
+	/* XXX: the below will probably go bug. must ensure there can be no
+	 * concurrency on queue_flags, and use the unlocked versions...
+	 */
 	if (t->limits.no_cluster)
-		__clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
+		queue_flag_clear(QUEUE_FLAG_CLUSTER, q);
 	else
-		__set_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
+		queue_flag_set(QUEUE_FLAG_CLUSTER, q);
 
 }
drivers/md/md.c +2 -1
Index: linux-2.6/drivers/md/md.c
===================================================================
@@ -281,7 +281,8 @@ static mddev_t * mddev_find(dev_t unit)
 		kfree(new);
 		return NULL;
 	}
-	__set_bit(QUEUE_FLAG_CLUSTER, &new->queue->queue_flags);
+	/* Can be unlocked because the queue is new: no concurrency */
+	queue_flag_set_unlocked(QUEUE_FLAG_CLUSTER, new->queue);
 
 	blk_queue_make_request(new->queue, md_fail_request);
drivers/scsi/scsi_lib.c +4 -5
Index: linux-2.6/drivers/scsi/scsi_lib.c
===================================================================
@@ -555,12 +555,10 @@ static void scsi_run_queue(struct reques
 				!test_bit(QUEUE_FLAG_REENTER,
 					&sdev->request_queue->queue_flags);
 		if (flagset)
-			__set_bit(QUEUE_FLAG_REENTER,
-					&sdev->request_queue->queue_flags);
+			queue_flag_set(QUEUE_FLAG_REENTER, sdev->request_queue);
 		__blk_run_queue(sdev->request_queue);
 		if (flagset)
-			__clear_bit(QUEUE_FLAG_REENTER,
-					&sdev->request_queue->queue_flags);
+			queue_flag_clear(QUEUE_FLAG_REENTER, sdev->request_queue);
 		spin_unlock(sdev->request_queue->queue_lock);
 
 		spin_lock(shost->host_lock);

@@ -1679,8 +1677,9 @@ struct request_queue *__scsi_alloc_queue
 	blk_queue_bounce_limit(q, scsi_calculate_bounce_limit(shost));
 	blk_queue_segment_boundary(q, shost->dma_boundary);
 
+	/* New queue, no concurrency on queue_flags */
 	if (!shost->use_clustering)
-		__clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
+		queue_flag_clear_unlocked(QUEUE_FLAG_CLUSTER, q);
 	return q;
 }
 EXPORT_SYMBOL(__scsi_alloc_queue);
drivers/scsi/scsi_transport_sas.c +2 -2
Index: linux-2.6/drivers/scsi/scsi_transport_sas.c
===================================================================
@@ -218,6 +218,8 @@ static int sas_bsg_initialize(struct Scs
 	if (!q)
 		return -ENOMEM;
 
+	queue_flag_set_unlocked(QUEUE_FLAG_BIDI, q);
+
 	error = bsg_register_queue(q, dev, name);
 	if (error) {
 		blk_cleanup_queue(q);

@@ -234,8 +236,6 @@ static int sas_bsg_initialize(struct Scs
 	else
 		q->queuedata = shost;
 
-	__set_bit(QUEUE_FLAG_BIDI, &q->queue_flags);
-
 	return 0;
 }
include/linux/blkdev.h +26 -4
Index: linux-2.6/include/linux/blkdev.h
===================================================================
@@ -474,6 +474,28 @@ struct request_queue
 #define QUEUE_FLAG_ELVSWITCH	8	/* don't use elevator, just do FIFO */
 #define QUEUE_FLAG_BIDI		9	/* queue supports bidi requests */
 
+static inline void queue_flag_set_unlocked(unsigned int flag, struct request_queue *q)
+{
+	__set_bit(flag, &q->queue_flags);
+}
+
+static inline void queue_flag_set(unsigned int flag, struct request_queue *q)
+{
+	BUG_ON(!spin_is_locked(q->queue_lock));
+	__set_bit(flag, &q->queue_flags);
+}
+
+static inline void queue_flag_clear_unlocked(unsigned int flag, struct request_queue *q)
+{
+	__clear_bit(flag, &q->queue_flags);
+}
+
+static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
+{
+	BUG_ON(!spin_is_locked(q->queue_lock));
+	__clear_bit(flag, &q->queue_flags);
+}
+
 enum {
 	/*
 	 * Hardbarrier is supported with one of the following methods.

@@ -560,17 +582,17 @@ static inline int blk_queue_full(struct 
 static inline void blk_set_queue_full(struct request_queue *q, int rw)
 {
 	if (rw == READ)
-		__set_bit(QUEUE_FLAG_READFULL, &q->queue_flags);
+		queue_flag_set(QUEUE_FLAG_READFULL, q);
 	else
-		__set_bit(QUEUE_FLAG_WRITEFULL, &q->queue_flags);
+		queue_flag_set(QUEUE_FLAG_WRITEFULL, q);
 }
 
 static inline void blk_clear_queue_full(struct request_queue *q, int rw)
 {
 	if (rw == READ)
-		__clear_bit(QUEUE_FLAG_READFULL, &q->queue_flags);
+		queue_flag_clear(QUEUE_FLAG_READFULL, q);
 	else
-		__clear_bit(QUEUE_FLAG_WRITEFULL, &q->queue_flags);
+		queue_flag_clear(QUEUE_FLAG_WRITEFULL, q);
 }
#3 lore raw
In reply to Nick Piggin
Quoted 9 lines · Nick Piggin

Hi,

This is just an idea I had, which might make request processing a little bit cheaper depending on queue behaviour. For example if it is getting plugged unplugged frequently (as I think is the case for some database workloads), then we might save one or two atomic operations per request.

Anyway, I'm not completely sure if I have ensured all queue_flags users are safe (I think md may need a bit of help). But overall it seems quite doable.

Looks ok to me, I'll throw it into the testing mix. Thanks Nick!

Signature · 1 line
Jens Axboe
author
#4 lore raw
Quoted 8 lines · Jens Axboe
Quoted 7 lines · Nick Piggin

Hi,

This is just an idea I had, which might make request processing a little bit cheaper depending on queue behaviour. For example if it is getting plugged unplugged frequently (as I think is the case for some database workloads), then we might save one or two atomic operations per request.

Quoted 4 lines
Quoted 2 lines

Anyway, I'm not completely sure if I have ensured all queue_flags users are safe (I think md may need a bit of help). But overall it seems quite doable.

Looks ok to me, I'll throw it into the testing mix. Thanks Nick!

OK... actually if you are expecting it to be widely tested, can you change the BUG_ONs in queue_flag_set / queue_flag_clear into WARN_ON?

That way it's less likely to take down people's systems...

Thanks!

#5 lore raw
Quoted 12 lines · Nick Piggin
Quoted 11 lines · Jens Axboe
Quoted 9 lines · Nick Piggin

Hi,

This is just an idea I had, which might make request processing a little bit cheaper depending on queue behaviour. For example if it is getting plugged unplugged frequently (as I think is the case for some database workloads), then we might save one or two atomic operations per request.

Anyway, I'm not completely sure if I have ensured all queue_flags users are safe (I think md may need a bit of help). But overall it seems quite doable.

Quoted 6 lines
Quoted 1 line

Looks ok to me, I'll throw it into the testing mix. Thanks Nick!

OK... actually if you are expecting it to be widely tested, can you change the BUG_ONs in queue_flag_set / queue_flag_clear into WARN_ON?

That way it's less likely to take down people's systems...

Agree, will do so.

Signature · 1 line
Jens Axboe