| Message ID | 1647340746-17600-1-git-send-email-john.garry@huawei.com |
|---|---|
| Headers | show |
| Series | scsi/libata: A potential tagging fix and improvement | expand |
On Tue, Mar 15, 2022 at 06:39:05PM +0800, John Garry wrote: > In commit edb854a3680b ("scsi: core: Reallocate device's budget map on > queue depth change"), the sbitmap for the device budget map may be > reallocated after the slave device depth is configured. > > When the sbitmap is reallocated we use the result from > scsi_device_max_queue_depth() for the sbitmap size, but don't resize to > match the actual device queue depth. > > Fix by resizing the sbitmap after reallocating the budget sbitmap. > > Signed-off-by: John Garry <john.garry@huawei.com> > --- > drivers/scsi/scsi_scan.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c > index f4e6c68ac99e..2ef78083f1ef 100644 > --- a/drivers/scsi/scsi_scan.c > +++ b/drivers/scsi/scsi_scan.c > @@ -223,6 +223,8 @@ static int scsi_realloc_sdev_budget_map(struct scsi_device *sdev, > int ret; > struct sbitmap sb_backup; > > + depth = min_t(unsigned int, depth, scsi_device_max_queue_depth(sdev)); > + > /* > * realloc if new shift is calculated, which is caused by setting > * up one new default queue depth after calling ->slave_configure > @@ -245,6 +247,9 @@ static int scsi_realloc_sdev_budget_map(struct scsi_device *sdev, > scsi_device_max_queue_depth(sdev), > new_shift, GFP_KERNEL, > sdev->request_queue->node, false, true); > + if (!ret) > + sbitmap_resize(&sdev->budget_map, depth); > + Looks fine, Reviewed-by: Ming Lei <ming.lei@redhat.com> Thanks, Ming
On 15/03/2022 14:33, Bart Van Assche wrote: >> sbitmap sb_backup; >> + depth = min_t(unsigned int, depth, >> scsi_device_max_queue_depth(sdev)); >> + >> /* >> * realloc if new shift is calculated, which is caused by setting >> * up one new default queue depth after calling ->slave_configure >> @@ -245,6 +247,9 @@ static int scsi_realloc_sdev_budget_map(struct >> scsi_device *sdev, >> scsi_device_max_queue_depth(sdev), >> new_shift, GFP_KERNEL, >> sdev->request_queue->node, false, true); >> + if (!ret) >> + sbitmap_resize(&sdev->budget_map, depth); > > Hmm ... why to call both sbitmap_init_node() and sbitmap_resize() > instead of combining both calls into a single call with the proper depth? Hi Bart, Is the user wants to change the queue depth later via sysfs we do not reallocate the sbitmap then. So we need to ensure that the size we reallocate here will satisfy the scsi device max depth. I'm referencing scsi_change_queue_depth() for this. Thanks, John
On 3/15/22 19:39, John Garry wrote: > Two loosely related patches are included: > > - Fix for scsi_realloc_sdev_budget_map(). I noticed that the budget token > for scsi commands was way in excess of the device queue depth, so I > think we need to fix the sbitmap depth. I need to test this more. > > - libata change to use scsi command budget token for qc tag for SAS host. > I marked this as RFC as for SAS hosts I don't see anything which > guarantees that the budget size is <= 32 always. > For libsas hosts we resize the device depth to 32 in the slave configure > callback, but this seems an unreliable approach since not all hosts may > call this. > In addition, I am worried that even if we resize the device depth > properly in the slave config callback, we may still try to alloc qc tag > prior to this - in lun scan, for example. > So we need a way to guarantee that the device queue depth is <= 32 > always, which I would be open to suggestions for. > > John Garry (2): > scsi: core: Fix sbitmap depth in scsi_realloc_sdev_budget_map() > libata: Use scsi cmnd budget token for qc tag for SAS host > > drivers/ata/libata-core.c | 5 +++-- > drivers/ata/libata-sata.c | 21 ++++----------------- > drivers/ata/libata-scsi.c | 2 +- > drivers/ata/libata.h | 4 ++-- > drivers/scsi/scsi_scan.c | 5 +++++ > include/linux/libata.h | 1 - > 6 files changed, 15 insertions(+), 23 deletions(-) > I tested this and it is working fine for me. This actually solves the QD not changing problem I had detected with the pm80xx driver. Now, doing this: # cat /sys/block/sde/device/queue_depth 32 # echo 16 > /sys/block/sde/device/queue_depth # cat /sys/block/sde/device/queue_depth 16 is working as expected. See my comments on patch 2 for getting final ack and tested tags :)
On 16/03/2022 03:25, Damien Le Moal wrote:s Hi Damien, > I tested this and it is working fine for me. This actually solves the QD > not changing problem I had detected with the pm80xx driver. > Now, doing this: > > # cat /sys/block/sde/device/queue_depth > 32 > # echo 16 > /sys/block/sde/device/queue_depth > # cat /sys/block/sde/device/queue_depth > 16 > Having this working is down to the first patch. So I will resend that patch today separately so that we may look to have it included in 5.18, even though we're so late in the cycle... > is working as expected. > > See my comments on patch 2 for getting final ack and tested tags:) OK, Thanks, John