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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux