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
