| Message ID | 20201116090737.50989-12-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | blk-mq/scsi: tracking device queue depth via sbitmap | expand |
On 11/16/20 10:07 AM, Ming Lei wrote: > Limit scsi device's queue depth is less than max(host->can_queue, 1024) > in scsi_change_queue_depth(), and 1024 is big enough for saturating > current fast SCSI LUN(SSD, or raid volume on multiple SSDs). > > We need this patch for replacing sdev->device_busy with sbitmap which > has to be pre-allocated with reasonable max depth. > > Cc: Omar Sandoval <osandov@fb.com> > Cc: Kashyap Desai <kashyap.desai@broadcom.com> > Cc: Sumanesh Samanta <sumanesh.samanta@broadcom.com> > Cc: Ewan D. Milne <emilne@redhat.com> > Tested-by: Sumanesh Samanta <sumanesh.samanta@broadcom.com> > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > drivers/scsi/scsi.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > index 24619c3bebd5..a28d48c850cf 100644 > --- a/drivers/scsi/scsi.c > +++ b/drivers/scsi/scsi.c > @@ -214,6 +214,15 @@ void scsi_finish_command(struct scsi_cmnd *cmd) > scsi_io_completion(cmd, good_bytes); > } > > + > +/* > + * 1024 is big enough for saturating the fast scsi LUN now > + */ > +static int scsi_device_max_queue_depth(struct scsi_device *sdev) > +{ > + return max_t(int, sdev->host->can_queue, 1024); > +} > + Shouldn't this rather be initialized with scsi_host->can_queue? These 'should be enough' settings inevitable turn out to be not enough in the long run ... > /** > * scsi_change_queue_depth - change a device's queue depth > * @sdev: SCSI Device in question > @@ -223,6 +232,8 @@ void scsi_finish_command(struct scsi_cmnd *cmd) > */ > int scsi_change_queue_depth(struct scsi_device *sdev, int depth) > { > + depth = min_t(int, depth, scsi_device_max_queue_depth(sdev)); > + > if (depth > 0) { > sdev->queue_depth = depth; > wmb(); > Cheers, Hannes
On Mon, Nov 16, 2020 at 10:44:54AM +0100, Hannes Reinecke wrote: > On 11/16/20 10:07 AM, Ming Lei wrote: > > Limit scsi device's queue depth is less than max(host->can_queue, 1024) > > in scsi_change_queue_depth(), and 1024 is big enough for saturating > > current fast SCSI LUN(SSD, or raid volume on multiple SSDs). > > > > We need this patch for replacing sdev->device_busy with sbitmap which > > has to be pre-allocated with reasonable max depth. > > > > Cc: Omar Sandoval <osandov@fb.com> > > Cc: Kashyap Desai <kashyap.desai@broadcom.com> > > Cc: Sumanesh Samanta <sumanesh.samanta@broadcom.com> > > Cc: Ewan D. Milne <emilne@redhat.com> > > Tested-by: Sumanesh Samanta <sumanesh.samanta@broadcom.com> > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > drivers/scsi/scsi.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > > index 24619c3bebd5..a28d48c850cf 100644 > > --- a/drivers/scsi/scsi.c > > +++ b/drivers/scsi/scsi.c > > @@ -214,6 +214,15 @@ void scsi_finish_command(struct scsi_cmnd *cmd) > > scsi_io_completion(cmd, good_bytes); > > } > > + > > +/* > > + * 1024 is big enough for saturating the fast scsi LUN now > > + */ > > +static int scsi_device_max_queue_depth(struct scsi_device *sdev) > > +{ > > + return max_t(int, sdev->host->can_queue, 1024); > > +} > > + > > Shouldn't this rather be initialized with scsi_host->can_queue? Multiple queues may be used for one single LUN, so in theory we should return max queue depth as host->can_queue * host->nr_hw_queues, but this number can be too big for the sbitmap's pre-allocation. That is why this patch introduces one reasonable limit on this value of max(sdev->host->can_queue, 1024). Suppose single SSD can be saturated by ~128 requests, we still can saturate one LUN with 8 SSDs behind if the hw queue depth is set as too low. > These 'should be enough' settings inevitable turn out to be not enough in > the long run ... I have provided the theory behind this idea, not just simple 'should be enough'. Thanks, Ming
On 11/17/20 3:18 AM, Ming Lei wrote: > On Mon, Nov 16, 2020 at 10:44:54AM +0100, Hannes Reinecke wrote: >> On 11/16/20 10:07 AM, Ming Lei wrote: >>> Limit scsi device's queue depth is less than max(host->can_queue, 1024) >>> in scsi_change_queue_depth(), and 1024 is big enough for saturating >>> current fast SCSI LUN(SSD, or raid volume on multiple SSDs). >>> >>> We need this patch for replacing sdev->device_busy with sbitmap which >>> has to be pre-allocated with reasonable max depth. >>> >>> Cc: Omar Sandoval <osandov@fb.com> >>> Cc: Kashyap Desai <kashyap.desai@broadcom.com> >>> Cc: Sumanesh Samanta <sumanesh.samanta@broadcom.com> >>> Cc: Ewan D. Milne <emilne@redhat.com> >>> Tested-by: Sumanesh Samanta <sumanesh.samanta@broadcom.com> >>> Signed-off-by: Ming Lei <ming.lei@redhat.com> >>> --- >>> drivers/scsi/scsi.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c >>> index 24619c3bebd5..a28d48c850cf 100644 >>> --- a/drivers/scsi/scsi.c >>> +++ b/drivers/scsi/scsi.c >>> @@ -214,6 +214,15 @@ void scsi_finish_command(struct scsi_cmnd *cmd) >>> scsi_io_completion(cmd, good_bytes); >>> } >>> + >>> +/* >>> + * 1024 is big enough for saturating the fast scsi LUN now >>> + */ >>> +static int scsi_device_max_queue_depth(struct scsi_device *sdev) >>> +{ >>> + return max_t(int, sdev->host->can_queue, 1024); >>> +} >>> + >> >> Shouldn't this rather be initialized with scsi_host->can_queue? > > Multiple queues may be used for one single LUN, so in theory we should > return max queue depth as host->can_queue * host->nr_hw_queues, but > this number can be too big for the sbitmap's pre-allocation. > Ah, so that's the problem here. > That is why this patch introduces one reasonable limit on this value > of max(sdev->host->can_queue, 1024). Suppose single SSD can be saturated > by ~128 requests, we still can saturate one LUN with 8 SSDs behind if > the hw queue depth is set as too low. > >> These 'should be enough' settings inevitable turn out to be not enough in >> the long run ... > > I have provided the theory behind this idea, not just simple 'should be > enough'. > No, it's okay now. I wasn't aware that we had a limitation on the sbitmap pre-allocation. You can add my Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index 24619c3bebd5..a28d48c850cf 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -214,6 +214,15 @@ void scsi_finish_command(struct scsi_cmnd *cmd) scsi_io_completion(cmd, good_bytes); } + +/* + * 1024 is big enough for saturating the fast scsi LUN now + */ +static int scsi_device_max_queue_depth(struct scsi_device *sdev) +{ + return max_t(int, sdev->host->can_queue, 1024); +} + /** * scsi_change_queue_depth - change a device's queue depth * @sdev: SCSI Device in question @@ -223,6 +232,8 @@ void scsi_finish_command(struct scsi_cmnd *cmd) */ int scsi_change_queue_depth(struct scsi_device *sdev, int depth) { + depth = min_t(int, depth, scsi_device_max_queue_depth(sdev)); + if (depth > 0) { sdev->queue_depth = depth; wmb();