| Message ID | 20190529132901.27645-3-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: enable reserved commands for LLDDs | expand |
On 5/29/19 6:28 AM, Hannes Reinecke wrote: > + rq = blk_mq_alloc_request(sdev->request_queue, > + REQ_OP_SCSI_OUT | REQ_NOWAIT, > + BLK_MQ_REQ_RESERVED); This looks wrong to me. To avoid that blk_mq_alloc_request() waits I think it should be called as follows: rq = blk_mq_alloc_request(sdev->request_queue, REQ_OP_SCSI_OUT, BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT); Bart.
On 5/29/19 5:12 PM, Bart Van Assche wrote: > On 5/29/19 6:28 AM, Hannes Reinecke wrote: >> + rq = blk_mq_alloc_request(sdev->request_queue, >> + REQ_OP_SCSI_OUT | REQ_NOWAIT, >> + BLK_MQ_REQ_RESERVED); > > This looks wrong to me. To avoid that blk_mq_alloc_request() waits I > think it should be called as follows: > > rq = blk_mq_alloc_request(sdev->request_queue, > REQ_OP_SCSI_OUT, > BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT); > > Bart. Ah. Right. Will be changing it. Cheers, Hannes
On Wed, May 29, 2019 at 03:28:39PM +0200, Hannes Reinecke wrote: > Add helper functions to retrieve SCSI commands from the reserved > tag pool. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > include/scsi/scsi_tcq.h | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/include/scsi/scsi_tcq.h b/include/scsi/scsi_tcq.h > index 6053d46e794e..227f3bd4e974 100644 > --- a/include/scsi/scsi_tcq.h > +++ b/include/scsi/scsi_tcq.h > @@ -39,5 +39,27 @@ static inline struct scsi_cmnd *scsi_host_find_tag(struct Scsi_Host *shost, > return blk_mq_rq_to_pdu(req); > } > > +static inline struct scsi_cmnd *scsi_get_reserved_cmd(struct scsi_device *sdev) > +{ > + struct request *rq; > + struct scsi_cmnd *scmd; > + > + rq = blk_mq_alloc_request(sdev->request_queue, > + REQ_OP_SCSI_OUT | REQ_NOWAIT, > + BLK_MQ_REQ_RESERVED); > + if (IS_ERR(rq)) > + return NULL; > + scmd = blk_mq_rq_to_pdu(rq); > + scmd->request = rq; > + return scmd; > +} Now all these internal commands won't share tags with IO requests, however, your patch switches to reserve slots for internal commands. This way may have performance effect on IO workloads given the reserved tags can't be used by IO at all. Just wondering why not use an new tagset for internal commands? Thanks, Ming
On 5/30/19 8:41 AM, Ming Lei wrote: > On Wed, May 29, 2019 at 03:28:39PM +0200, Hannes Reinecke wrote: >> Add helper functions to retrieve SCSI commands from the reserved >> tag pool. >> >> Signed-off-by: Hannes Reinecke <hare@suse.com> >> --- >> include/scsi/scsi_tcq.h | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/include/scsi/scsi_tcq.h b/include/scsi/scsi_tcq.h >> index 6053d46e794e..227f3bd4e974 100644 >> --- a/include/scsi/scsi_tcq.h >> +++ b/include/scsi/scsi_tcq.h >> @@ -39,5 +39,27 @@ static inline struct scsi_cmnd *scsi_host_find_tag(struct Scsi_Host *shost, >> return blk_mq_rq_to_pdu(req); >> } >> >> +static inline struct scsi_cmnd *scsi_get_reserved_cmd(struct scsi_device *sdev) >> +{ >> + struct request *rq; >> + struct scsi_cmnd *scmd; >> + >> + rq = blk_mq_alloc_request(sdev->request_queue, >> + REQ_OP_SCSI_OUT | REQ_NOWAIT, >> + BLK_MQ_REQ_RESERVED); >> + if (IS_ERR(rq)) >> + return NULL; >> + scmd = blk_mq_rq_to_pdu(rq); >> + scmd->request = rq; >> + return scmd; >> +} > > Now all these internal commands won't share tags with IO requests, > however, your patch switches to reserve slots for internal > commands. > Yes. > This way may have performance effect on IO workloads given the > reserved tags can't be used by IO at all. > Not really. Basically all drivers which have to use tags to send internal commands already set some tags aside to handle internal commands. So all this patchset does is to formalize this behaviour by using private tags. Some drivers (like fnic or snic) does _not_ do this currently; for those I've set one command aside to handle command abort etc. > Just wondering why not use an new tagset for internal commands? > Because it doesn't help. All of these drivers have a common tag pool internally, which every single command is required to use. So using a new tagset doesn't help here; you just would need to split the (hardware) tag pool with no real gain. Cheers, Hannes
On Thu, May 30, 2019 at 10:57 PM Hannes Reinecke <hare@suse.de> wrote: > > On 5/30/19 8:41 AM, Ming Lei wrote: > > On Wed, May 29, 2019 at 03:28:39PM +0200, Hannes Reinecke wrote: > >> Add helper functions to retrieve SCSI commands from the reserved > >> tag pool. > >> > >> Signed-off-by: Hannes Reinecke <hare@suse.com> > >> --- > >> include/scsi/scsi_tcq.h | 22 ++++++++++++++++++++++ > >> 1 file changed, 22 insertions(+) > >> > >> diff --git a/include/scsi/scsi_tcq.h b/include/scsi/scsi_tcq.h > >> index 6053d46e794e..227f3bd4e974 100644 > >> --- a/include/scsi/scsi_tcq.h > >> +++ b/include/scsi/scsi_tcq.h > >> @@ -39,5 +39,27 @@ static inline struct scsi_cmnd *scsi_host_find_tag(struct Scsi_Host *shost, > >> return blk_mq_rq_to_pdu(req); > >> } > >> > >> +static inline struct scsi_cmnd *scsi_get_reserved_cmd(struct scsi_device *sdev) > >> +{ > >> + struct request *rq; > >> + struct scsi_cmnd *scmd; > >> + > >> + rq = blk_mq_alloc_request(sdev->request_queue, > >> + REQ_OP_SCSI_OUT | REQ_NOWAIT, > >> + BLK_MQ_REQ_RESERVED); > >> + if (IS_ERR(rq)) > >> + return NULL; > >> + scmd = blk_mq_rq_to_pdu(rq); > >> + scmd->request = rq; > >> + return scmd; > >> +} > > > > Now all these internal commands won't share tags with IO requests, > > however, your patch switches to reserve slots for internal > > commands. > > > Yes. > > > This way may have performance effect on IO workloads given the > > reserved tags can't be used by IO at all. > > > Not really. Basically all drivers which have to use tags to send > internal commands already set some tags aside to handle internal > commands. So all this patchset does is to formalize this behaviour by > using private tags. > Some drivers (like fnic or snic) does _not_ do this currently; for those > I've set one command aside to handle command abort etc. From hardware view, you might be right, however, the implementation isn't correct: set->queue_depth means number of the total tags, set->reserved_tags is just part of the total tags, see blk_mq_init_bitmap_tags(). So any driver sets .reserved_tags > 0, tags available for IO is reduced by same amount, isn't it? Cause .can_queue isn't increased. Thanks, Ming Lei
On 5/30/19 5:48 PM, Ming Lei wrote: > On Thu, May 30, 2019 at 10:57 PM Hannes Reinecke <hare@suse.de> wrote: >> >> On 5/30/19 8:41 AM, Ming Lei wrote: >>> On Wed, May 29, 2019 at 03:28:39PM +0200, Hannes Reinecke wrote: >>>> Add helper functions to retrieve SCSI commands from the reserved >>>> tag pool. >>>> >>>> Signed-off-by: Hannes Reinecke <hare@suse.com> >>>> --- >>>> include/scsi/scsi_tcq.h | 22 ++++++++++++++++++++++ >>>> 1 file changed, 22 insertions(+) >>>> >>>> diff --git a/include/scsi/scsi_tcq.h b/include/scsi/scsi_tcq.h >>>> index 6053d46e794e..227f3bd4e974 100644 >>>> --- a/include/scsi/scsi_tcq.h >>>> +++ b/include/scsi/scsi_tcq.h >>>> @@ -39,5 +39,27 @@ static inline struct scsi_cmnd *scsi_host_find_tag(struct Scsi_Host *shost, >>>> return blk_mq_rq_to_pdu(req); >>>> } >>>> >>>> +static inline struct scsi_cmnd *scsi_get_reserved_cmd(struct scsi_device *sdev) >>>> +{ >>>> + struct request *rq; >>>> + struct scsi_cmnd *scmd; >>>> + >>>> + rq = blk_mq_alloc_request(sdev->request_queue, >>>> + REQ_OP_SCSI_OUT | REQ_NOWAIT, >>>> + BLK_MQ_REQ_RESERVED); >>>> + if (IS_ERR(rq)) >>>> + return NULL; >>>> + scmd = blk_mq_rq_to_pdu(rq); >>>> + scmd->request = rq; >>>> + return scmd; >>>> +} >>> >>> Now all these internal commands won't share tags with IO requests, >>> however, your patch switches to reserve slots for internal >>> commands. >>> >> Yes. >> >>> This way may have performance effect on IO workloads given the >>> reserved tags can't be used by IO at all. >>> >> Not really. Basically all drivers which have to use tags to send >> internal commands already set some tags aside to handle internal >> commands. So all this patchset does is to formalize this behaviour by >> using private tags. >> Some drivers (like fnic or snic) does _not_ do this currently; for those >> I've set one command aside to handle command abort etc. > > From hardware view, you might be right, however, the implementation > isn't correct: > > set->queue_depth means number of the total tags, set->reserved_tags is just > part of the total tags, see blk_mq_init_bitmap_tags(). > > So any driver sets .reserved_tags > 0, tags available for IO is reduced by > same amount, isn't it? Cause .can_queue isn't increased. > Hmm. I was under the impression that the number of total tags is set->queue_depth + set-?reserved_tags. But reading through blk-mq-tag.c it seems you are right. Okay, will be updating the patchset. Cheers, Hannes
> +static inline struct scsi_cmnd *scsi_get_reserved_cmd(struct scsi_device *sdev) > +{ > + struct request *rq; > + struct scsi_cmnd *scmd; > + > + rq = blk_mq_alloc_request(sdev->request_queue, > + REQ_OP_SCSI_OUT | REQ_NOWAIT, > + BLK_MQ_REQ_RESERVED); REQ_OP_SCSI_OUT is used for data transfers to the device, is that really what you want? REQ_NOWAIT not only seems misplaced, but also generally wrong. Maybe for some callers you don't want to wait, but that really should be passed in. Also why does this take a scsi device? Host reserved command usually would be on a per-host, not a per-LU basis. > + if (IS_ERR(rq)) > + return NULL; > + scmd = blk_mq_rq_to_pdu(rq); > + scmd->request = rq; > + return scmd; > +} > + > +static inline void scsi_put_reserved_cmd(struct scsi_cmnd *scmd) > +{ > + struct request *rq = blk_mq_rq_from_pdu(scmd); > + > + blk_mq_free_request(rq); > +} Also both helpers really should be out of line somewhere.
diff --git a/include/scsi/scsi_tcq.h b/include/scsi/scsi_tcq.h index 6053d46e794e..227f3bd4e974 100644 --- a/include/scsi/scsi_tcq.h +++ b/include/scsi/scsi_tcq.h @@ -39,5 +39,27 @@ static inline struct scsi_cmnd *scsi_host_find_tag(struct Scsi_Host *shost, return blk_mq_rq_to_pdu(req); } +static inline struct scsi_cmnd *scsi_get_reserved_cmd(struct scsi_device *sdev) +{ + struct request *rq; + struct scsi_cmnd *scmd; + + rq = blk_mq_alloc_request(sdev->request_queue, + REQ_OP_SCSI_OUT | REQ_NOWAIT, + BLK_MQ_REQ_RESERVED); + if (IS_ERR(rq)) + return NULL; + scmd = blk_mq_rq_to_pdu(rq); + scmd->request = rq; + return scmd; +} + +static inline void scsi_put_reserved_cmd(struct scsi_cmnd *scmd) +{ + struct request *rq = blk_mq_rq_from_pdu(scmd); + + blk_mq_free_request(rq); +} + #endif /* CONFIG_BLOCK */ #endif /* _SCSI_SCSI_TCQ_H */
Add helper functions to retrieve SCSI commands from the reserved tag pool. Signed-off-by: Hannes Reinecke <hare@suse.com> --- include/scsi/scsi_tcq.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)