| Message ID | 1440679281-13234-22-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 27, 2015 at 02:41:19PM +0200, Hannes Reinecke wrote: > When we read in the target port group state we should be > updating all affected port groups, otherwise we risk > running out of sync. Looks fine, Reviewed-by: Christoph Hellwig <hch@lst.de> -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-08-27 at 14:41 +0200, Hannes Reinecke wrote: > When we read in the target port group state we should be > updating all affected port groups, otherwise we risk > running out of sync. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/scsi/device_handler/scsi_dh_alua.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c > index 8717fd3..2fa985e 100644 > --- a/drivers/scsi/device_handler/scsi_dh_alua.c > +++ b/drivers/scsi/device_handler/scsi_dh_alua.c > @@ -477,11 +477,13 @@ static int alua_check_sense(struct scsi_device *sdev, > static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg) > { > struct scsi_sense_hdr sense_hdr; > + struct alua_port_group *tmp_pg; > int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE; > unsigned char *ucp, *buff; > unsigned err, retval; > unsigned int tpg_desc_tbl_off; > unsigned char orig_transition_tmo; > + unsigned long flags; > > if (!pg->expiry) { > if (!pg->transition_tmo) > @@ -584,17 +586,28 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg) > else > tpg_desc_tbl_off = 4; > > + spin_lock_irqsave(&port_group_lock, flags); > for (k = tpg_desc_tbl_off, ucp = buff + tpg_desc_tbl_off; > k < len; > k += off, ucp += off) { "ucp" is not really a good variable name, it matches nothing in the T10 spec. The structure is a "Target Port Group Descriptor". I realize it was already there, but maybe this is a good opportunity to change it? > > - if (pg->group_id == get_unaligned_be16(&ucp[2])) { > - pg->state = ucp[0] & 0x0f; > - pg->pref = ucp[0] >> 7; > - valid_states = ucp[1]; > + list_for_each_entry(tmp_pg, &port_group_list, node) { > + u16 group_id = get_unaligned_be16(&ucp[2]); This is invariant and should be outside the loop. > + if (tmp_pg->group_id != group_id) > + continue; > + if (tmp_pg->device_id_size != pg->device_id_size) > + continue; > + if (strncmp(tmp_pg->device_id_str, pg->device_id_str, > + tmp_pg->device_id_size)) > + continue; > + tmp_pg->state = ucp[0] & 0x0f; > + tmp_pg->pref = ucp[0] >> 7; > + if (tmp_pg == pg) > + valid_states = ucp[1]; > } > off = 8 + (ucp[7] * 4); > } > + spin_unlock_irqrestore(&port_group_lock, flags); > > sdev_printk(KERN_INFO, sdev, > "%s: port group %02x state %c %s supports %c%c%c%c%c%c%c\n", Reviewed-by: Ewan D. Milne <emilne@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/22/2015 10:04 PM, Ewan Milne wrote: > On Thu, 2015-08-27 at 14:41 +0200, Hannes Reinecke wrote: >> When we read in the target port group state we should be >> updating all affected port groups, otherwise we risk >> running out of sync. >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> drivers/scsi/device_handler/scsi_dh_alua.c | 21 +++++++++++++++++---- >> 1 file changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c >> index 8717fd3..2fa985e 100644 >> --- a/drivers/scsi/device_handler/scsi_dh_alua.c >> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c >> @@ -477,11 +477,13 @@ static int alua_check_sense(struct scsi_device *sdev, >> static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg) >> { >> struct scsi_sense_hdr sense_hdr; >> + struct alua_port_group *tmp_pg; >> int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE; >> unsigned char *ucp, *buff; >> unsigned err, retval; >> unsigned int tpg_desc_tbl_off; >> unsigned char orig_transition_tmo; >> + unsigned long flags; >> >> if (!pg->expiry) { >> if (!pg->transition_tmo) >> @@ -584,17 +586,28 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg) >> else >> tpg_desc_tbl_off = 4; >> >> + spin_lock_irqsave(&port_group_lock, flags); >> for (k = tpg_desc_tbl_off, ucp = buff + tpg_desc_tbl_off; >> k < len; >> k += off, ucp += off) { > > "ucp" is not really a good variable name, it matches nothing in the T10 > spec. The structure is a "Target Port Group Descriptor". I realize it > was already there, but maybe this is a good opportunity to change it? > Doug Gilbert is to blame; I've copied this section over from sg3_utils :-) But yeah, I can set it to eg 'desc'. >> >> - if (pg->group_id == get_unaligned_be16(&ucp[2])) { >> - pg->state = ucp[0] & 0x0f; >> - pg->pref = ucp[0] >> 7; >> - valid_states = ucp[1]; >> + list_for_each_entry(tmp_pg, &port_group_list, node) { >> + u16 group_id = get_unaligned_be16(&ucp[2]); > > This is invariant and should be outside the loop. > True. Cheers, Hannes
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c index 8717fd3..2fa985e 100644 --- a/drivers/scsi/device_handler/scsi_dh_alua.c +++ b/drivers/scsi/device_handler/scsi_dh_alua.c @@ -477,11 +477,13 @@ static int alua_check_sense(struct scsi_device *sdev, static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg) { struct scsi_sense_hdr sense_hdr; + struct alua_port_group *tmp_pg; int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE; unsigned char *ucp, *buff; unsigned err, retval; unsigned int tpg_desc_tbl_off; unsigned char orig_transition_tmo; + unsigned long flags; if (!pg->expiry) { if (!pg->transition_tmo) @@ -584,17 +586,28 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg) else tpg_desc_tbl_off = 4; + spin_lock_irqsave(&port_group_lock, flags); for (k = tpg_desc_tbl_off, ucp = buff + tpg_desc_tbl_off; k < len; k += off, ucp += off) { - if (pg->group_id == get_unaligned_be16(&ucp[2])) { - pg->state = ucp[0] & 0x0f; - pg->pref = ucp[0] >> 7; - valid_states = ucp[1]; + list_for_each_entry(tmp_pg, &port_group_list, node) { + u16 group_id = get_unaligned_be16(&ucp[2]); + if (tmp_pg->group_id != group_id) + continue; + if (tmp_pg->device_id_size != pg->device_id_size) + continue; + if (strncmp(tmp_pg->device_id_str, pg->device_id_str, + tmp_pg->device_id_size)) + continue; + tmp_pg->state = ucp[0] & 0x0f; + tmp_pg->pref = ucp[0] >> 7; + if (tmp_pg == pg) + valid_states = ucp[1]; } off = 8 + (ucp[7] * 4); } + spin_unlock_irqrestore(&port_group_lock, flags); sdev_printk(KERN_INFO, sdev, "%s: port group %02x state %c %s supports %c%c%c%c%c%c%c\n",
When we read in the target port group state we should be updating all affected port groups, otherwise we risk running out of sync. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/device_handler/scsi_dh_alua.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)