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@xxxxxxxxxxxxxxxxxx>
> ---
> 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.
