diff mbox

[V5,12/14] block: mq-deadline: Introduce zone locking support

Message ID 20170925061454.5533-13-damien.lemoal@wdc.com (mailing list archive)
State Superseded
Headers show

Commit Message

Damien Le Moal Sept. 25, 2017, 6:14 a.m. UTC
For a write request to a zoned block device, lock the request target
zone upon request displatch. The zone is unlocked either when the
request completes or when the request is requeued (inserted).

To indicate that a request has locked its target zone, use the first
pointer of the request elevator private data to store the value
RQ_ZONE_WLOCKED. Testing for this value allows quick decision in
dd_insert_request() and dd_completed_request() regarding the need for
unlocking the target zone of a request.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 block/mq-deadline.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)

Comments

Bart Van Assche Sept. 25, 2017, 10 p.m. UTC | #1
On Mon, 2017-09-25 at 15:14 +0900, Damien Le Moal wrote:
> +static inline bool deadline_request_needs_zone_wlock(struct deadline_data *dd,

> +						     struct request *rq)

> +{

> +

> +	if (!dd->zones_wlock)

> +		return false;

> +

> +	if (blk_rq_is_passthrough(rq))

> +		return false;

> +

> +	switch (req_op(rq)) {

> +	case REQ_OP_WRITE_ZEROES:

> +	case REQ_OP_WRITE_SAME:

> +	case REQ_OP_WRITE:

> +		return blk_rq_zone_is_seq(rq);

> +	default:

> +		return false;

> +	}


If anyone ever adds a new write request type it will be easy to overlook this
function. Should the 'default' case be left out and should all request types
be mentioned in the switch/case statement such that the compiler will issue a
warning if a new request operation type is added to enum req_opf?

> +/*

> + * Abuse the elv.priv[0] pointer to indicate if a request has write

> + * locked its target zone. Only write request to a zoned block device

> + * can own a zone write lock.

> + */

> +#define RQ_ZONE_WLOCKED		((void *)1UL)

> +static inline void deadline_set_request_zone_wlock(struct request *rq)

> +{

> +	rq->elv.priv[0] = RQ_ZONE_WLOCKED;

> +}

> +

> +#define RQ_ZONE_NO_WLOCK	((void *)0UL)

> +static inline void deadline_clear_request_zone_wlock(struct request *rq)

> +{

> +	rq->elv.priv[0] = RQ_ZONE_NO_WLOCK;

> +}


Should an enumeration type be introduced for RQ_ZONE_WLOCKED and RQ_ZONE_NO_WLOCK?

> +/*

> + * Write lock the target zone of a write request.

> + */

> +static void deadline_wlock_zone(struct deadline_data *dd,

> +				struct request *rq)

> +{

> +	unsigned int zno = blk_rq_zone_no(rq);

> +

> +	WARN_ON_ONCE(deadline_request_has_zone_wlock(rq));

> +	WARN_ON_ONCE(test_and_set_bit(zno, dd->zones_wlock));

> +	deadline_set_request_zone_wlock(rq);

> +}

> +

> +/*

> + * Write unlock the target zone of a write request.

> + */

> +static void deadline_wunlock_zone(struct deadline_data *dd,

> +				  struct request *rq)

> +{

> +	unsigned int zno = blk_rq_zone_no(rq);

> +	unsigned long flags;

> +

> +	spin_lock_irqsave(&dd->zone_lock, flags);

> +

> +	WARN_ON_ONCE(!test_and_clear_bit(zno, dd->zones_wlock));

> +	deadline_clear_request_zone_wlock(rq);

> +

> +	spin_unlock_irqrestore(&dd->zone_lock, flags);

> +}


Why does deadline_wunlock_zone() protect modifications with dd->zone_lock but
deadline_wlock_zone() not? If this code is correct, please add a
lockdep_assert_held() statement in the first function.

> +/*

> + * Test the write lock state of the target zone of a write request.

> + */

> +static inline bool deadline_zone_is_wlocked(struct deadline_data *dd,

> +					    struct request *rq)

> +{

> +	unsigned int zno = blk_rq_zone_no(rq);

> +

> +	return test_bit(zno, dd->zones_wlock);

> +}


Do we really need the local variable 'zno'?

> +/*

> + * For zoned block devices, write unlock the target zone of

> + * completed write requests.

> + */

> +static void dd_completed_request(struct request *rq)

> +{

> +


Please leave out the blank line at the start of this function.

Thanks,

Bart.
Damien Le Moal Oct. 2, 2017, 4:36 a.m. UTC | #2
Bart,

On Mon, 2017-09-25 at 22:00 +0000, Bart Van Assche wrote:
> On Mon, 2017-09-25 at 15:14 +0900, Damien Le Moal wrote:

> > +static inline bool deadline_request_needs_zone_wlock(struct deadline_data

> > *dd,

> > +						     struct request *rq)

> > +{

> > +

> > +	if (!dd->zones_wlock)

> > +		return false;

> > +

> > +	if (blk_rq_is_passthrough(rq))

> > +		return false;

> > +

> > +	switch (req_op(rq)) {

> > +	case REQ_OP_WRITE_ZEROES:

> > +	case REQ_OP_WRITE_SAME:

> > +	case REQ_OP_WRITE:

> > +		return blk_rq_zone_is_seq(rq);

> > +	default:

> > +		return false;

> > +	}

> 

> If anyone ever adds a new write request type it will be easy to overlook

> this

> function. Should the 'default' case be left out and should all request types

> be mentioned in the switch/case statement such that the compiler will issue

> a

> warning if a new request operation type is added to enum req_opf?


I tried, but that does not work. The switch-case needs either a default case
or a return after it. Otherwise I get a compilation warning (reached end of
non-void function).

> > +/*

> > + * Abuse the elv.priv[0] pointer to indicate if a request has write

> > + * locked its target zone. Only write request to a zoned block device

> > + * can own a zone write lock.

> > + */

> > +#define RQ_ZONE_WLOCKED		((void *)1UL)

> > +static inline void deadline_set_request_zone_wlock(struct request *rq)

> > +{

> > +	rq->elv.priv[0] = RQ_ZONE_WLOCKED;

> > +}

> > +

> > +#define RQ_ZONE_NO_WLOCK	((void *)0UL)

> > +static inline void deadline_clear_request_zone_wlock(struct request *rq)

> > +{

> > +	rq->elv.priv[0] = RQ_ZONE_NO_WLOCK;

> > +}

> 

> Should an enumeration type be introduced for RQ_ZONE_WLOCKED and

> RQ_ZONE_NO_WLOCK?


Sure. Added in V6.

> > +/*

> > + * Write lock the target zone of a write request.

> > + */

> > +static void deadline_wlock_zone(struct deadline_data *dd,

> > +				struct request *rq)

> > +{

> > +	unsigned int zno = blk_rq_zone_no(rq);

> > +

> > +	WARN_ON_ONCE(deadline_request_has_zone_wlock(rq));

> > +	WARN_ON_ONCE(test_and_set_bit(zno, dd->zones_wlock));

> > +	deadline_set_request_zone_wlock(rq);

> > +}

> > +

> > +/*

> > + * Write unlock the target zone of a write request.

> > + */

> > +static void deadline_wunlock_zone(struct deadline_data *dd,

> > +				  struct request *rq)

> > +{

> > +	unsigned int zno = blk_rq_zone_no(rq);

> > +	unsigned long flags;

> > +

> > +	spin_lock_irqsave(&dd->zone_lock, flags);

> > +

> > +	WARN_ON_ONCE(!test_and_clear_bit(zno, dd->zones_wlock));

> > +	deadline_clear_request_zone_wlock(rq);

> > +

> > +	spin_unlock_irqrestore(&dd->zone_lock, flags);

> > +}

> 

> Why does deadline_wunlock_zone() protect modifications with dd->zone_lock

> but

> deadline_wlock_zone() not? If this code is correct, please add a

> lockdep_assert_held() statement in the first function.


Yes, that was a little confusing. In V6, I move the introduction of the
zone_lock spinlock to when it is actually needed, that is the patch following
this one. And I added more comments in both the commit message and in the code
to explain why the spinlock is needed.

> > +/*

> > + * Test the write lock state of the target zone of a write request.

> > + */

> > +static inline bool deadline_zone_is_wlocked(struct deadline_data *dd,

> > +					    struct request *rq)

> > +{

> > +	unsigned int zno = blk_rq_zone_no(rq);

> > +

> > +	return test_bit(zno, dd->zones_wlock);

> > +}

> 

> Do we really need the local variable 'zno'?


No we don't. Fixed.

Best regards.

-- 
Damien Le Moal
Western Digital
diff mbox

Patch

diff --git a/block/mq-deadline.c b/block/mq-deadline.c
index 296880e2471f..186c32099845 100644
--- a/block/mq-deadline.c
+++ b/block/mq-deadline.c
@@ -178,6 +178,93 @@  deadline_move_request(struct deadline_data *dd, struct request *rq)
 }
 
 /*
+ * Return true if a request is a write requests that needs zone
+ * write locking.
+ */
+static inline bool deadline_request_needs_zone_wlock(struct deadline_data *dd,
+						     struct request *rq)
+{
+
+	if (!dd->zones_wlock)
+		return false;
+
+	if (blk_rq_is_passthrough(rq))
+		return false;
+
+	switch (req_op(rq)) {
+	case REQ_OP_WRITE_ZEROES:
+	case REQ_OP_WRITE_SAME:
+	case REQ_OP_WRITE:
+		return blk_rq_zone_is_seq(rq);
+	default:
+		return false;
+	}
+}
+
+/*
+ * Abuse the elv.priv[0] pointer to indicate if a request has write
+ * locked its target zone. Only write request to a zoned block device
+ * can own a zone write lock.
+ */
+#define RQ_ZONE_WLOCKED		((void *)1UL)
+static inline void deadline_set_request_zone_wlock(struct request *rq)
+{
+	rq->elv.priv[0] = RQ_ZONE_WLOCKED;
+}
+
+#define RQ_ZONE_NO_WLOCK	((void *)0UL)
+static inline void deadline_clear_request_zone_wlock(struct request *rq)
+{
+	rq->elv.priv[0] = RQ_ZONE_NO_WLOCK;
+}
+
+static inline bool deadline_request_has_zone_wlock(struct request *rq)
+{
+	return rq->elv.priv[0] == RQ_ZONE_WLOCKED;
+}
+
+/*
+ * Write lock the target zone of a write request.
+ */
+static void deadline_wlock_zone(struct deadline_data *dd,
+				struct request *rq)
+{
+	unsigned int zno = blk_rq_zone_no(rq);
+
+	WARN_ON_ONCE(deadline_request_has_zone_wlock(rq));
+	WARN_ON_ONCE(test_and_set_bit(zno, dd->zones_wlock));
+	deadline_set_request_zone_wlock(rq);
+}
+
+/*
+ * Write unlock the target zone of a write request.
+ */
+static void deadline_wunlock_zone(struct deadline_data *dd,
+				  struct request *rq)
+{
+	unsigned int zno = blk_rq_zone_no(rq);
+	unsigned long flags;
+
+	spin_lock_irqsave(&dd->zone_lock, flags);
+
+	WARN_ON_ONCE(!test_and_clear_bit(zno, dd->zones_wlock));
+	deadline_clear_request_zone_wlock(rq);
+
+	spin_unlock_irqrestore(&dd->zone_lock, flags);
+}
+
+/*
+ * Test the write lock state of the target zone of a write request.
+ */
+static inline bool deadline_zone_is_wlocked(struct deadline_data *dd,
+					    struct request *rq)
+{
+	unsigned int zno = blk_rq_zone_no(rq);
+
+	return test_bit(zno, dd->zones_wlock);
+}
+
+/*
  * deadline_check_fifo returns 0 if there are no expired requests on the fifo,
  * 1 otherwise. Requires !list_empty(&dd->fifo_list[data_dir])
  */
@@ -316,6 +403,11 @@  static struct request *__dd_dispatch_request(struct blk_mq_hw_ctx *hctx)
 	dd->batching++;
 	deadline_move_request(dd, rq);
 done:
+	/*
+	 * If the request needs its target zone locked, do it.
+	 */
+	if (deadline_request_needs_zone_wlock(dd, rq))
+		deadline_wlock_zone(dd, rq);
 	rq->rq_flags |= RQF_STARTED;
 	return rq;
 }
@@ -466,6 +558,13 @@  static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
 	struct deadline_data *dd = q->elevator->elevator_data;
 	const int data_dir = rq_data_dir(rq);
 
+	/*
+	 * This may be a requeue of a request that has locked its
+	 * target zone. If this is the case, release the request zone lock.
+	 */
+	if (deadline_request_has_zone_wlock(rq))
+		deadline_wunlock_zone(dd, rq);
+
 	if (blk_mq_sched_try_insert_merge(q, rq))
 		return;
 
@@ -510,6 +609,20 @@  static void dd_insert_requests(struct blk_mq_hw_ctx *hctx,
 	spin_unlock(&dd->lock);
 }
 
+/*
+ * For zoned block devices, write unlock the target zone of
+ * completed write requests.
+ */
+static void dd_completed_request(struct request *rq)
+{
+
+	if (deadline_request_has_zone_wlock(rq)) {
+		struct deadline_data *dd = rq->q->elevator->elevator_data;
+
+		deadline_wunlock_zone(dd, rq);
+	}
+}
+
 static bool dd_has_work(struct blk_mq_hw_ctx *hctx)
 {
 	struct deadline_data *dd = hctx->queue->elevator->elevator_data;
@@ -711,6 +824,7 @@  static struct elevator_type mq_deadline = {
 	.ops.mq = {
 		.insert_requests	= dd_insert_requests,
 		.dispatch_request	= dd_dispatch_request,
+		.completed_request	= dd_completed_request,
 		.next_request		= elv_rb_latter_request,
 		.former_request		= elv_rb_former_request,
 		.bio_merge		= dd_bio_merge,