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.
