On Mon, 2022-01-10 at 10:59 +0800, Ming Lei wrote:
>
> Hello Martin Wilck,
>
> Can you test the following change and report back the result?
>
> From 480a61a85e9669d3487ebee8db3d387df79279fc Mon Sep 17 00:00:00
> 2001
> From: Ming Lei <ming.lei@xxxxxxxxxx>
> Date: Mon, 10 Jan 2022 10:26:59 +0800
> Subject: [PATCH] scsi: core: reallocate scsi device's budget map if
> default
> queue depth is changed
>
> Martin reported that sdev->queue_depth can often be changed in
> ->slave_configure(), and now we uses ->cmd_per_lun as initial queue
> depth for setting up sdev->budget_map. And some extreme ->cmd_per_lun
> or ->can_queue won't be used at default actually, if we they are used
> to allocate sdev->budget_map, huge memory may be consumed just
> because
> of bad ->cmd_per_lun.
>
> Fix the issue by reallocating sdev->budget_map after -
> >slave_configure()
> returns, at that time, queue_depth should be much more reasonable.
>
> Reported-by: Martin Wilck <martin.wilck@xxxxxxxx>
> Suggested-by: Martin Wilck <martin.wilck@xxxxxxxx>
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
This looks good. I added a few pr_infos, and for the strange mpt3sas
devices I reported, I get this:
# first allocation with depth=7 (cmds_per_lun)
Jan 12 17:05:52 localhost kernel: scsi_realloc_sdev_budget_map: 7 0->0
(these numbers are: depth old_shift->new_shift)
Jan 12 17:05:52 localhost kernel: scsi_realloc_sdev_budget_map: map_nr = 1024
# after slave_alloc() with depth 254
Jan 12 17:05:52 localhost kernel: scsi_realloc_sdev_budget_map: 254 0->5
Jan 12 17:05:52 localhost kernel: scsi_realloc_sdev_budget_map: map_nr = 32
So the depth changed from 7 to 254, the shift from 0 to 5, and the memory size of the
sbitmap was reduced by a factor of 32. Nice!
Tested-by: Martin Wilck <mwilck@xxxxxxxx>
Reviewed-by: Martin Wilck <mwilck@xxxxxxxx>
> ---
> drivers/scsi/scsi_scan.c | 56 ++++++++++++++++++++++++++++++++++++--
> --
> 1 file changed, 51 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 23e1c0acdeae..9593c9111611 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -214,6 +214,48 @@ static void scsi_unlock_floptical(struct
> scsi_device *sdev,
> SCSI_TIMEOUT, 3, NULL);
> }
>
> +static int scsi_realloc_sdev_budget_map(struct scsi_device *sdev,
> + unsigned int depth)
> +{
> + int new_shift = sbitmap_calculate_shift(depth);
> + bool need_alloc = !sdev->budget_map.map;
> + bool need_free = false;
> + int ret;
> + struct sbitmap sb_back;
> +
> + /*
> + * realloc if new shift is calculated, which is caused by
> setting
> + * up one new default queue depth after calling -
> >slave_configure
> + */
> + if (!need_alloc && new_shift != sdev->budget_map.shift)
> + need_alloc = need_free = true;
> +
> + if (!need_alloc)
> + return 0;
> +
> + /*
> + * Request queue has to be freezed for reallocating budget
> map,
> + * and here disk isn't added yet, so freezing is pretty fast
> + */
> + if (need_free) {
> + blk_mq_freeze_queue(sdev->request_queue);
> + sb_back = sdev->budget_map;
> + }
> + ret = sbitmap_init_node(&sdev->budget_map,
> + scsi_device_max_queue_depth(sdev),
> + new_shift, GFP_KERNEL,
> + sdev->request_queue->node, false,
> true);
> + if (need_free) {
> + if (ret)
> + sdev->budget_map = sb_back;
> + else
> + sbitmap_free(&sb_back);
> + ret = 0;
> + blk_mq_unfreeze_queue(sdev->request_queue);
> + }
> + return ret;
> +}
> +
> /**
> * scsi_alloc_sdev - allocate and setup a scsi_Device
> * @starget: which target to allocate a &scsi_device for
> @@ -306,11 +348,7 @@ static struct scsi_device
> *scsi_alloc_sdev(struct scsi_target *starget,
> * default device queue depth to figure out sbitmap shift
> * since we use this queue depth most of times.
> */
> - if (sbitmap_init_node(&sdev->budget_map,
> - scsi_device_max_queue_depth(sdev),
> - sbitmap_calculate_shift(depth),
> - GFP_KERNEL, sdev->request_queue-
> >node,
> - false, true)) {
> + if (scsi_realloc_sdev_budget_map(sdev, depth)) {
> put_device(&starget->dev);
> kfree(sdev);
> goto out;
> @@ -1017,6 +1055,14 @@ static int scsi_add_lun(struct scsi_device
> *sdev, unsigned char *inq_result,
> }
> return SCSI_SCAN_NO_RESPONSE;
> }
> +
> + /*
> + * queue_depth is often changed in ->slave_configure,
> so
> + * setup budget map again for getting better memory
> uses
> + * since memory consumption of the map depends on
> queue
> + * depth heavily
> + */
> + scsi_realloc_sdev_budget_map(sdev, sdev-
> >queue_depth);
> }
>
> if (sdev->scsi_level >= SCSI_3)
> --
> 2.31.1
>
>
>
