svn commit: r349243 - head/sys/cam

[Scott Long]

Hi Alexander,

Thanks for the explanation, sorry I didn’t realize sooner that it’s
not used as a copyout buffer anymore.  FWIW, that code is really
hard to read on an 80 column window, would you be ok if it was
refactored for better readability?

Thanks,
Scott


> On Jun 20, 2019, at 6:04 PM, Alexander Motin <mav at FreeBSD.org> wrote:
> 
> Hi Scott,
> 
> You may see this buffer content is not returned directly to user any
> more.  It is used only as temporary storage to pull
> CDAI_TYPE_SCSI_DEVID, which is then converted into readable form,
> returned to user.  And the code is explicitly made to not read outside
> the range that was copied to the buffer with simple and dumb memcpy().
> 
> The effect is clearly visible in profiler when GEOM confxml calls it 3
> times for each disk, taking about 25% of the sysctl's CPU time.  With
> hundreds of disks it is not so small.
> 
> On 20.06.2019 18:59, Scott Long wrote:
>> Hi Alexander,
>> 
>> I’m not a fan of removing the M_ZERO.  I understand your argument that
>> lengths are provided elsewhere, but having the buffer be zero’d is defensive
>> against future bugs that might leak buffer contents.  GETATTR isn’t typically
>> in a performance path, is there any measurable benefit to this?
>> 
>> Thanks,
>> Scott
>> 
>> 
>>> On Jun 20, 2019, at 2:29 PM, Alexander Motin <mav at FreeBSD.org> wrote:
>>> 
>>> Author: mav
>>> Date: Thu Jun 20 20:29:42 2019
>>> New Revision: 349243
>>> URL: https://svnweb.freebsd.org/changeset/base/349243
>>> 
>>> Log:
>>> Optimize xpt_getattr().
>>> 
>>> Do not allocate temporary buffer for attributes we are going to return
>>> as-is, just make sure to NUL-terminate them.  Do not zero temporary 64KB
>>> buffer for CDAI_TYPE_SCSI_DEVID, XPT tells us how much data it filled
>>> and there are also length fields inside the returned data also.
>>> 
>>> MFC after:	2 weeks
>>> Sponsored by:	iXsystems, Inc.
>>> 
>>> Modified:
>>> head/sys/cam/cam_xpt.c
>>> 
>>> Modified: head/sys/cam/cam_xpt.c
>>> ==============================================================================
>>> --- head/sys/cam/cam_xpt.c	Thu Jun 20 20:06:19 2019	(r349242)
>>> +++ head/sys/cam/cam_xpt.c	Thu Jun 20 20:29:42 2019	(r349243)
>>> @@ -1253,6 +1253,7 @@ xpt_getattr(char *buf, size_t len, const char *attr, s
>>> 	cdai.ccb_h.func_code = XPT_DEV_ADVINFO;
>>> 	cdai.flags = CDAI_FLAG_NONE;
>>> 	cdai.bufsiz = len;
>>> +	cdai.buf = buf;
>>> 
>>> 	if (!strcmp(attr, "GEOM::ident"))
>>> 		cdai.buftype = CDAI_TYPE_SERIAL_NUM;
>>> @@ -1262,14 +1263,14 @@ xpt_getattr(char *buf, size_t len, const char *attr, s
>>> 		 strcmp(attr, "GEOM::lunname") == 0) {
>>> 		cdai.buftype = CDAI_TYPE_SCSI_DEVID;
>>> 		cdai.bufsiz = CAM_SCSI_DEVID_MAXLEN;
>>> +		cdai.buf = malloc(cdai.bufsiz, M_CAMXPT, M_NOWAIT);
>>> +		if (cdai.buf == NULL) {
>>> +			ret = ENOMEM;
>>> +			goto out;
>>> +		}
>>> 	} else
>>> 		goto out;
>>> 
>>> -	cdai.buf = malloc(cdai.bufsiz, M_CAMXPT, M_NOWAIT|M_ZERO);
>>> -	if (cdai.buf == NULL) {
>>> -		ret = ENOMEM;
>>> -		goto out;
>>> -	}
>>> 	xpt_action((union ccb *)&cdai); /* can only be synchronous */
>>> 	if ((cdai.ccb_h.status & CAM_DEV_QFRZN) != 0)
>>> 		cam_release_devq(cdai.ccb_h.path, 0, 0, 0, FALSE);
>>> @@ -1334,13 +1335,15 @@ xpt_getattr(char *buf, size_t len, const char *attr, s
>>> 				ret = EFAULT;
>>> 		}
>>> 	} else {
>>> -		ret = 0;
>>> -		if (strlcpy(buf, cdai.buf, len) >= len)
>>> +		if (cdai.provsiz < len) {
>>> +			cdai.buf[cdai.provsiz] = 0;
>>> +			ret = 0;
>>> +		} else
>>> 			ret = EFAULT;
>>> 	}
>>> 
>>> out:
>>> -	if (cdai.buf != NULL)
>>> +	if ((char *)cdai.buf != buf)
>>> 		free(cdai.buf, M_CAMXPT);
>>> 	return ret;
>>> }
>>> 
>> 
> 
> -- 
> Alexander Motin