On Thu, 2015-08-27 at 14:41 +0200, Hannes Reinecke wrote: > The RTPG buffer will only evaluated within alua_rtpg(), > so we can allocate it locally there and avoid having to > put it into the global structure. > > Reviewed-by: Christoph Hellwig <[email protected]> > Signed-off-by: Hannes Reinecke <[email protected]> > --- > drivers/scsi/device_handler/scsi_dh_alua.c | 56 > +++++++++++------------------- > 1 file changed, 21 insertions(+), 35 deletions(-) > > diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c > b/drivers/scsi/device_handler/scsi_dh_alua.c > index d1010dd..4157fe2 100644 > --- a/drivers/scsi/device_handler/scsi_dh_alua.c > +++ b/drivers/scsi/device_handler/scsi_dh_alua.c > @@ -56,7 +56,7 @@ > #define TPGS_MODE_IMPLICIT 0x1 > #define TPGS_MODE_EXPLICIT 0x2 > > -#define ALUA_INQUIRY_SIZE 36 > +#define ALUA_RTPG_SIZE 128 > #define ALUA_FAILOVER_TIMEOUT 60 > #define ALUA_FAILOVER_RETRIES 5 > > @@ -75,9 +75,6 @@ struct alua_port_group { > int state; > int pref; > unsigned flags; /* used for optimizing STPG */ > - unsigned char inq[ALUA_INQUIRY_SIZE]; > - unsigned char *buff; > - int bufflen; > unsigned char transition_tmo; > }; > > @@ -96,21 +93,6 @@ struct alua_dh_data { > static char print_alua_state(int); > static int alua_check_sense(struct scsi_device *, struct scsi_sense_hdr *); > > -static int realloc_buffer(struct alua_port_group *pg, unsigned len) > -{ > - if (pg->buff && pg->buff != pg->inq) > - kfree(pg->buff); > - > - pg->buff = kmalloc(len, GFP_NOIO); > - if (!pg->buff) { > - pg->buff = pg->inq; > - pg->bufflen = ALUA_INQUIRY_SIZE; > - return 1; > - } > - pg->bufflen = len; > - return 0; > -} > - > static void release_port_group(struct kref *kref) > { > struct alua_port_group *pg; > @@ -120,8 +102,6 @@ static void release_port_group(struct kref *kref) > spin_lock(&port_group_lock); > list_del(&pg->node); > spin_unlock(&port_group_lock); > - if (pg->buff && pg->inq != pg->buff) > - kfree(pg->buff); > kfree(pg); > } > > @@ -300,8 +280,6 @@ static int alua_check_vpd(struct scsi_device *sdev, > struct alua_dh_data *h) > return SCSI_DH_DEV_TEMP_BUSY; > } > pg->group_id = group_id; > - pg->buff = pg->inq; > - pg->bufflen = ALUA_INQUIRY_SIZE; > pg->tpgs = h->tpgs; > pg->state = TPGS_STATE_OPTIMIZED; > kref_init(&pg->kref); > @@ -424,8 +402,8 @@ static int alua_check_sense(struct scsi_device *sdev, > static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg, > int wait_for_transition) > { > struct scsi_sense_hdr sense_hdr; > - int len, k, off, valid_states = 0; > - unsigned char *ucp; > + int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE; > + unsigned char *ucp, *buff; > unsigned err, retval; > unsigned long expiry, interval = 0; > unsigned int tpg_desc_tbl_off; > @@ -436,9 +414,12 @@ static int alua_rtpg(struct scsi_device *sdev, struct > alua_port_group *pg, int w > else > expiry = round_jiffies_up(jiffies + pg->transition_tmo * HZ); > > + buff = kzalloc(bufflen, GFP_KERNEL); > + if (!buff) > + return SCSI_DH_DEV_TEMP_BUSY; > + > retry: > - retval = submit_rtpg(sdev, pg->buff, pg->bufflen, > - &sense_hdr, pg->flags); > + retval = submit_rtpg(sdev, buff, bufflen, &sense_hdr, pg->flags); > > if (retval) { > if (!scsi_sense_valid(&sense_hdr)) { > @@ -449,6 +430,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct > alua_port_group *pg, int w > err = SCSI_DH_DEV_TEMP_BUSY; > else > err = SCSI_DH_IO; > + kfree(buff); > return err; > } > > @@ -477,14 +459,18 @@ static int alua_rtpg(struct scsi_device *sdev, struct > alua_port_group *pg, int w > sdev_printk(KERN_ERR, sdev, "%s: rtpg failed\n", > ALUA_DH_NAME); > scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr); > + kfree(buff); > return SCSI_DH_IO; > } > > - len = get_unaligned_be32(&pg->buff[0]) + 4; > + len = get_unaligned_be32(&buff[0]) + 4; > > - if (len > pg->bufflen) { > + if (len > bufflen) { > /* Resubmit with the correct length */ > - if (realloc_buffer(pg, len)) { > + kfree(buff); > + bufflen = len; > + buff = kmalloc(bufflen, GFP_KERNEL); > + if (!buff) { > sdev_printk(KERN_WARNING, sdev, > "%s: kmalloc buffer failed\n",__func__); > /* Temporary failure, bypass */ > @@ -494,9 +480,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct > alua_port_group *pg, int w > } > > orig_transition_tmo = pg->transition_tmo; > - if ((pg->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR && > - pg->buff[5] != 0) > - pg->transition_tmo = pg->buff[5]; > + if ((buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR && buff[5] != 0) > + pg->transition_tmo = buff[5]; > else > pg->transition_tmo = ALUA_FAILOVER_TIMEOUT; > > @@ -508,12 +493,12 @@ static int alua_rtpg(struct scsi_device *sdev, struct > alua_port_group *pg, int w > expiry = jiffies + pg->transition_tmo * HZ; > } > > - if ((pg->buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR) > + if ((buff[4] & RTPG_FMT_MASK) == RTPG_FMT_EXT_HDR) > tpg_desc_tbl_off = 8; > else > tpg_desc_tbl_off = 4; > > - for (k = tpg_desc_tbl_off, ucp = pg->buff + tpg_desc_tbl_off; > + for (k = tpg_desc_tbl_off, ucp = buff + tpg_desc_tbl_off; > k < len; > k += off, ucp += off) { > > @@ -563,6 +548,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct > alua_port_group *pg, int w > err = SCSI_DH_OK; > break; > } > + kfree(buff); > return err; > } >
I guess. Are these buffers so big that we don't want to make it part of a structure that just stays allocated? We should have some validation on the length of the data returned by RTPG in the case where the buffer is re-kmalloc'ed, we can't kmalloc 4GB - 1 + 4 bytes. And if the length is too big, it is not a "temporary" failure. We should maybe only retry once with a larger buffer, rather than potentially loop forever if the device keeps returning different information. (I suppose we have the same potential issue with REPORT LUNS as well.) Reviewed-by: Ewan D. Milne <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
