Re: [PATCH] scsi: Update max_hw_sectors on rescan

From: Mike Christie <michael.christie@oracle.com>
To: Brian King <brking@linux.vnet.ibm.com>, linux-scsi@vger.kernel.org
Cc: brking@pobox.com, jejb@linux.ibm.com, martin.petersen@oracle.com
Subject: Re: [PATCH] scsi: Update max_hw_sectors on rescan
Date: Tue, 23 Jan 2024 16:40:00 -0600	[thread overview]
Message-ID: <bc98813f-113e-4074-be85-06455f684856@oracle.com> (raw)
In-Reply-To: <20240117213620.132880-1-brking@linux.vnet.ibm.com>

On 1/17/24 3:36 PM, Brian King wrote:
> This addresses an issue discovered on ibmvfc LUNs. For this driver,
> max_sectors is negotiated with the VIOS. This gets done at initialization
> time, then LUNs get scanned and things generally work fine. However,
> this attribute can be changed on the VIOS, either due to a sysadmin
> change or potentially a VIOS code level change. If this decreases
> to a smaller value, due to one of these reasons, the next time the
> ibmvfc driver performs an NPIV login, it will only be able to use
> the smaller value. In the case of a VIOS reboot, when the VIOS goes
> down, all paths through that VIOS will go to devloss state. When
> the VIOS comes back up, ibmvfc negotiates max_sectors and will only
> be able to get the smaller value and it will update shost->max_sectors.
> However, when LUNs are scanned, the devloss paths will be found
> and brought back online, still using the old max_hw_sectors. This
> change ensures that max_hw_sectors gets updated.
> 
> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
> ---
>  drivers/scsi/scsi_scan.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 44680f65ea14..01f2b38daab3 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1162,6 +1162,7 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
>  	blist_flags_t bflags;
>  	int res = SCSI_SCAN_NO_RESPONSE, result_len = 256;
>  	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
> +	struct request_queue *q;
>  
>  	/*
>  	 * The rescan flag is used as an optimization, the first scan of a
> @@ -1182,6 +1183,10 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
>  				*bflagsp = scsi_get_device_flags(sdev,
>  								 sdev->vendor,
>  								 sdev->model);
> +			q = sdev->request_queue;
> +			if (queue_max_hw_sectors(q) > shost->max_sectors)
> +				blk_queue_max_hw_sectors(q, shost->max_sectors);
> +

What happens if commands that are larger than the new shost->max_sectors get
sent to the driver/device?

For example, if we called fc_remote_port_add and scsi_target_unblock puts the
existing devices into SDEV_RUNNING, then we do the scsi_scan_target call and
hit the code above, could we have commands in the request_queue already (we
relogin before fast_io_fail even fires so the commands never get failed)?
It looks like commands have already passed checks like bio_may_exceed_limit
and will be sent to the driver. Will the driver/device spit out an error?

Is this ok, or do you need some sort of flush and limit re-check/re-split?