On Wed, Aug 26, 2020 at 08:35:42PM -0700, Bart Van Assche wrote:
On 2020-08-25 18:51, Alan Stern wrote:
Ah, perfect. So in blk_queue_enter(), pm should be defined in terms of RQF_PM rather than BLK_MQ_REQ_PREEMPT.
The difficulty is that the flags argument is the wrong type; RQF_PM is defined as req_flags_t, not blk_mq_req_flags_t. It is associated with a particular request after the request has been created, so after blk_queue_enter() has been called.
How can we solve this?
The current code looks a bit weird because my focus when modifying the PM code has been on not breaking any existing code.
scsi_device_quiesce() relies on blk_queue_enter() processing all PREEMPT requests. A difficulty is that scsi_device_quiesce() is used for two separate purposes:
- Runtime power management.
- SCSI domain validation. See e.g. https://lwn.net/Articles/75917/.
I think that modifying blk_queue_enter() such that it only accepts PM requests will require to split scsi_device_quiesce() into two functions: one function that is used by the runtime power management code and another function that is used by the SCSI domain validation code. This may require to introduce new SCSI device states. If new SCSI device states are introduced, that should be done without modifying the state that is reported to user space. See also sdev_states[] and show_state_field in scsi_sysfs.c.
It may not need to be that complicated. what about something like this?
Alan Stern
Index: usb-devel/block/blk-core.c =================================================================== --- usb-devel.orig/block/blk-core.c +++ usb-devel/block/blk-core.c @@ -420,11 +420,11 @@ EXPORT_SYMBOL(blk_cleanup_queue); /** * blk_queue_enter() - try to increase q->q_usage_counter * @q: request queue pointer - * @flags: BLK_MQ_REQ_NOWAIT and/or BLK_MQ_REQ_PREEMPT + * @flags: BLK_MQ_REQ_NOWAIT and/or BLK_MQ_REQ_PM */ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags) { - const bool pm = flags & BLK_MQ_REQ_PREEMPT; + const bool pm = flags & BLK_MQ_REQ_PM;
while (true) { bool success = false; @@ -626,7 +626,8 @@ struct request *blk_get_request(struct r struct request *req;
WARN_ON_ONCE(op & REQ_NOWAIT); - WARN_ON_ONCE(flags & ~(BLK_MQ_REQ_NOWAIT | BLK_MQ_REQ_PREEMPT)); + WARN_ON_ONCE(flags & ~(BLK_MQ_REQ_NOWAIT | BLK_MQ_REQ_PREEMPT | + BLK_MQ_REQ_PM));
req = blk_mq_alloc_request(q, op, flags); if (!IS_ERR(req) && q->mq_ops->initialize_rq_fn) Index: usb-devel/drivers/scsi/scsi_lib.c =================================================================== --- usb-devel.orig/drivers/scsi/scsi_lib.c +++ usb-devel/drivers/scsi/scsi_lib.c @@ -245,11 +245,15 @@ int __scsi_execute(struct scsi_device *s { struct request *req; struct scsi_request *rq; + blk_mq_req_flags_t mq_req_flags; int ret = DRIVER_ERROR << 24;
+ mq_req_flags = BLK_MQ_REQ_PREEMPT; + if (rq_flags & RQF_PM) + mq_req_flags |= BLK_MQ_REQ_PM; req = blk_get_request(sdev->request_queue, data_direction == DMA_TO_DEVICE ? - REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT); + REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, mq_req_flags); if (IS_ERR(req)) return ret; rq = scsi_req(req); Index: usb-devel/include/linux/blk-mq.h =================================================================== --- usb-devel.orig/include/linux/blk-mq.h +++ usb-devel/include/linux/blk-mq.h @@ -435,6 +435,8 @@ enum { BLK_MQ_REQ_RESERVED = (__force blk_mq_req_flags_t)(1 << 1), /* set RQF_PREEMPT */ BLK_MQ_REQ_PREEMPT = (__force blk_mq_req_flags_t)(1 << 3), + /* used for power management */ + BLK_MQ_REQ_PM = (__force blk_mq_req_flags_t)(1 << 4), };
struct request *blk_mq_alloc_request(struct request_queue *q, unsigned int op,