Re: [PATCH 3/4] scsi: scsi_dh_alua: print changes to RTPG state of other PGs too

[Bart Van Assche <Bart.VanAssche@sandisk.com>]

On Mon, 2017-04-10 at 22:18 -0300, Mauricio Faria de Oliveira wrote:
> Currently, alua_rtpg() can change the 'state' and 'preferred'
> values for the current port group _and_ of other port groups
> present in the response buffer/descriptors.
> 
> However, it reports such changes _only_ for the current port
> group (i.e., only for 'pg' but not for 'tmp_pg').
> 
> This might cause uncertainty and confusion when going through
> the kernel logs for analyzing/debugging scsi_dh_alua behavior,
> which is not helpful during support and development scenarios.
> 
> So, print such changes for other port groups than the current
> one.
> 
> This requires finding a scsi_device to call sdev_printk() with
> for that other port group. Using 'tmp_pg->rtpg_sdev' is not an
> option because in that 'if' conditional the 'tmp_pg' is not in
> the ALUA_PG_RUNNING state, so that pointer may be NULL. So the
> for-loop over the tmp->pg device_handler structures is used to
> find a valid scsi_device that is associated to this port group.
> 
> Signed-off-by: Mauricio Faria de Oliveira <mauricfo@linux.vnet.ibm.com>
> ---
>  drivers/scsi/device_handler/scsi_dh_alua.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 0d3508f2e285..c2c9173fd883 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -688,6 +688,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  				if ((tmp_pg == pg) ||
>  				    !(tmp_pg->flags & ALUA_PG_RUNNING)) {
>  					struct alua_dh_data *h;
> +					struct scsi_device *tmp_pg_sdev = NULL;
>  
>  					tmp_pg->state = desc[0] & 0x0f;
>  					tmp_pg->pref = desc[0] >> 7;
> @@ -697,6 +698,17 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  						/* h->sdev should always be valid */
>  						BUG_ON(!h->sdev);
>  						h->sdev->access_state = desc[0];
> +
> +						/*
> +						 * If tmp_pg is not the running pg, and its worker
> +						 * is not running, tmp_pg->rtpg_sdev might be NULL.
> +						 * Use another/one associated scsi_device to print
> +						 * its RTPG information.
> +						 */
> +						if ((tmp_pg != pg) && !tmp_pg_sdev) {
> +							tmp_pg_sdev = h->sdev;
> +							alua_rtpg_print(tmp_pg_sdev, tmp_pg, NULL);
> +						}
>  					}
>  					rcu_read_unlock();
>  				}

Hello Mauricio,

Please consider moving the alua_rtpg_print() call out of the loop. That will
not only allow to eliminate the tmp_pg_sdev variable but will also reduce the
nesting depth of the code. E.g. something like the following (untested) code:

					list_for_each_entry_rcu(h,
						&tmp_pg->dh_list, node) {
						[ ... ]
					}
+ 					h = list_first_or_null_rcu(&tmp_pg->dh_list, typeof(*h), node);
+ 					if (tmp_pg != pg && h)
+ 						alua_rtpg_print(h->sdev, tmp_pg, NULL);
 					rcu_read_unlock();

Thanks,

Bart.