Re: [PATCH] scsi: scsi_dh_alua: set handler_data before alua_initialize()

[John Garry <john.garry@linux.dev>]

On 11/9/26 20:11, Laurence Oberman wrote:
> Since commit 50759b881e1d ("scsi: device_handler: alua: Call
> scsi_device_put() from non-atomic context"), alua_rtpg_queue() only
> queues an RTPG when sdev->handler_data points at an alua_dh_data whose
> port group matches:
> 
> 	struct alua_dh_data *h = sdev->handler_data;
> 	...
> 	if (h && rcu_dereference(h->pg) == pg) {

Do we still require this check (for h being non-NULL) after the change 
in this patch? I assume that we flush any calls to this function prior 
to unsetting sdev->handler_data.

> 
> However alua_bus_attach() only assigns sdev->handler_data after
> alua_initialize() returns, and alua_initialize() -> alua_check_vpd()
> is where the initial RTPG is queued. At that point handler_data is
> still NULL, so the initial RTPG is silently dropped: no RTPG is sent,
> nothing is logged, and sdev->access_state keeps its zeroed default of
> active/optimized for every path.
> 
> With dm-multipath this goes unnoticed, as pg_init calls
> scsi_dh_activate() once the map is loaded, which queues an RTPG after
> handler_data is set, and multipathd's alua prioritizer issues its own
> RTPG from userspace. Anything relying on sdev->access_state without a
> subsequent activate (sysfs access_state/preferred_path, native SCSI
> multipath path selection) sees every path as active/optimized until a
> Unit Attention or rescan triggers an RTPG.
> 
> Seen on an HP P2000 G3 FC array (implicit ALUA, two port groups, four
> paths per LUN) with dm-multipath not in use: sg_rtpg reports port group
> 0 active/non-optimized and port group 1 active/optimized, but
> /sys/class/scsi_device/H:C:T:L/device/access_state reads
> active/optimized on all four paths after boot, and no
> "alua: port group XX state ..." message is logged. Writing "activate"
> to dh_state sends the RTPG and corrects access_state.
> 
> Kernels without commit 50759b881e1d queue the attach-time RTPG
> unconditionally and are not affected.
> 
> Fix by assigning sdev->handler_data before calling alua_initialize(),
> and clearing it again on failure. sdev->handler is only set once
> scsi_dh_handler_attach() sees ->attach() succeed, so no handler
> callback can observe the partially initialised alua_dh_data. On the
> failure paths of alua_initialize(), h has not been added to a port
> group and no RTPG has been queued, so freeing it is unchanged.
> 
> Fixes: 50759b881e1d ("scsi: device_handler: alua: Call scsi_device_put() from non-atomic context")
> Cc: stable@vger.kernel.org
> Signed-off-by: Laurence Oberman <loberman@redhat.com>
> ---
> 
> Notes:
>      Tested on an HP P2000 G3 FC array (30 LUNs, 4 paths each): with this
>      patch the "alua: port group 00 state N ..." messages are logged during
>      boot and access_state shows two active/optimized and two
>      active/non-optimized paths per LUN without any manual activate.
>      Without it, all paths read active/optimized until an RTPG is triggered.
>      
>      I found this while testing John Garry's native SCSI multipath series
>      (v9) on an HP P2000 G3 FC array (30 LUNs, 4 paths each, dm-multipath
>      not in use). Native multipath uses sdev->access_state for path
>      selection, and every path showed active/optimized, although sg_rtpg
>      and dm-multipath (alua prio 50/10) both report port group 0 as
>      active/non-optimized. No "alua: port group XX state ..." message was
>      logged at boot. Writing "activate" to dh_state corrected access_state,
>      which pointed at the attach-time RTPG never being queued.
>      
>      With this patch, the port group state messages are logged during boot
>      and access_state shows two active/optimized and two
>      active/non-optimized paths per LUN without any manual activate.
>      
>      This fix is independent of the native multipath series. It affects any
>      user of access_state/preferred_path, and ALUA prep_fn fast-fail, on
>      kernels with 50759b881e1d where nothing calls scsi_dh_activate()
>      after attach.
>      
>      v9 series: https://lore.kernel.org/linux-scsi/20261108204733.557995-1-
>      john.garry@linux.dev/
> 
>   drivers/scsi/device_handler/scsi_dh_alua.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 87ac0a6d63e5..84ff3aed2c46 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -1233,13 +1233,20 @@ static int alua_bus_attach(struct scsi_device *sdev)
>   	INIT_LIST_HEAD(&h->node);
>   
>   	mutex_init(&h->init_mutex);
> +	/*
> +	 * alua_rtpg_queue() only queues an RTPG when sdev->handler_data
> +	 * points at this alua_dh_data, so set it before alua_initialize()
> +	 * otherwise the initial RTPG issued from alua_check_vpd() is
> +	 * silently dropped and sdev->access_state is never populated.
> +	 */
> +	sdev->handler_data = h;

Note: I was checking which members of alua_dh_data are not initialized 
prior to publishing sdev->handler_data, and alua_dh_data.group_id is 
never accessed at all AFAICS.

Thanks,
John