| Message ID | 1440679281-13234-15-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <hch@lst.de> > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > 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 <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 09:04 PM, Ewan Milne wrote: > 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 <hch@lst.de> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> 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? > The point is that the buffer scales with the size of the installation, so we cannot allocate it statically. > 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. Well, it's hard to distinguish between ENOMEM because of a (temporary) memory shortage and ENOMEM because kmalloc cannot allocate that much memory in general. Mind you, FC-AL can only have up to 128M bytes worth of data here (64k possible loop IDs with 8 bytes port IDs each). However, during every testing I've done (and this particular code is running in SLES for some years now) I've never ever seen an issue with kmalloc failing to allocate memory. > 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.) > Well, the size shouldn't really change (unless someone does an array reconfiguration). So at most we'll be retrying once (plus any sense code retries, of course). Cheers, Hannes
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; }