On Mon, 2017-10-02 at 16:15 +0900, Damien Le Moal wrote:
> When dispatching write requests to a zoned block device, only allow
> requests targeting an unlocked zone. Requests targeting a locked zone
> are left in the scheduler queue to preserve the initial write order.
> If no write request can be dispatched, allow reads to be dispatched
> even if the write batch is not done.
>
> To ensure that the search for an appropriate write request is atomic
> in deadline_fifo_request() and deadline_next_request() with reagrd to
^^^^^^
regard?
> write requests zone lock state, introduce the spinlock zone_lock.
> Holding this lock while doing the search in these functions as well as
> when unlocking the target zone of a completed write request in
> dd_completed_request() ensure that the search does not pickup a write
> request in the middle of a zone queued write sequence.
Since there is already a spinlock in the mq-deadline scheduler that serializes
most operations, do we really need a new spinlock?
> /*
> * Write unlock the target zone of a write request.
> + * Clearing the target zone write lock bit is done with the scheduler
> zone_lock
> + * spinlock held so that deadline_next_request() and deadline_fifo_request()
> + * cannot see the lock state of a zone change due to a request completion
> during
> + * their eventual search for an appropriate write request. Otherwise, for a
> zone
> + * with multiple write requests queued, a non sequential write request
> + * can be chosen.
> */
> static void deadline_wunlock_zone(struct deadline_data *dd,
> struct request *rq)
> {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dd->zone_lock, flags);
> +
> WARN_ON_ONCE(!test_and_clear_bit(blk_rq_zone_no(rq), dd->zones_wlock));
> deadline_clear_request_zone_wlock(rq);
> +
> + spin_unlock_irqrestore(&dd->zone_lock, flags);
> }
Is a request removed from the sort and fifo lists before being dispatched? If
so,
does that mean that obtaining zone_lock in the above function is not necessary?
> static struct request *
> deadline_fifo_request(struct deadline_data *dd, int data_dir)
> {
> + struct request *rq;
> + unsigned long flags;
> +
> if (WARN_ON_ONCE(data_dir != READ && data_dir != WRITE))
> return NULL;
>
> if (list_empty(&dd->fifo_list[data_dir]))
> return NULL;
>
> - return rq_entry_fifo(dd->fifo_list[data_dir].next);
> + if (!dd->zones_wlock || data_dir == READ)
> + return rq_entry_fifo(dd->fifo_list[data_dir].next);
> +
> + spin_lock_irqsave(&dd->zone_lock, flags);
> +
> + list_for_each_entry(rq, &dd->fifo_list[WRITE], queuelist) {
> + if (deadline_can_dispatch_request(dd, rq))
> + goto out;
> + }
> + rq = NULL;
> +
> +out:
> + spin_unlock_irqrestore(&dd->zone_lock, flags);
> +
> + return rq;
> }
Same question here: is it really necessary to obtain zone_lock?
Thanks,
Bart.
