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

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

 



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.




[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