Date: Fri, 21 Jun 2019 14:02:01 -0600 From: Scott Long <scottl@samsco.org> To: Alexander Motin <mav@FreeBSD.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r349243 - head/sys/cam Message-ID: <D9890FB4-7BD4-432C-B3A1-DDEF3BEED6EC@samsco.org> In-Reply-To: <42dbbc7f-51ea-bc3d-0432-5cc9b872d0e7@FreeBSD.org> References: <201906202029.x5KKThjn017867@repo.freebsd.org> <6B266DFC-87FF-4742-ACEE-0D37EABEFD7A@samsco.org> <42dbbc7f-51ea-bc3d-0432-5cc9b872d0e7@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Alexander, Thanks for the explanation, sorry I didn=E2=80=99t realize sooner that = it=E2=80=99s 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@FreeBSD.org> wrote: >=20 > Hi Scott, >=20 > 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(). >=20 > 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. >=20 > On 20.06.2019 18:59, Scott Long wrote: >> Hi Alexander, >>=20 >> I=E2=80=99m not a fan of removing the M_ZERO. I understand your = argument that >> lengths are provided elsewhere, but having the buffer be zero=E2=80=99d= is defensive >> against future bugs that might leak buffer contents. GETATTR isn=E2=80= =99t typically >> in a performance path, is there any measurable benefit to this? >>=20 >> Thanks, >> Scott >>=20 >>=20 >>> On Jun 20, 2019, at 2:29 PM, Alexander Motin <mav@FreeBSD.org> = wrote: >>>=20 >>> Author: mav >>> Date: Thu Jun 20 20:29:42 2019 >>> New Revision: 349243 >>> URL: https://svnweb.freebsd.org/changeset/base/349243 >>>=20 >>> Log: >>> Optimize xpt_getattr(). >>>=20 >>> 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. >>>=20 >>> MFC after: 2 weeks >>> Sponsored by: iXsystems, Inc. >>>=20 >>> Modified: >>> head/sys/cam/cam_xpt.c >>>=20 >>> Modified: head/sys/cam/cam_xpt.c >>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>> --- 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 =3D XPT_DEV_ADVINFO; >>> cdai.flags =3D CDAI_FLAG_NONE; >>> cdai.bufsiz =3D len; >>> + cdai.buf =3D buf; >>>=20 >>> if (!strcmp(attr, "GEOM::ident")) >>> cdai.buftype =3D CDAI_TYPE_SERIAL_NUM; >>> @@ -1262,14 +1263,14 @@ xpt_getattr(char *buf, size_t len, const = char *attr, s >>> strcmp(attr, "GEOM::lunname") =3D=3D 0) { >>> cdai.buftype =3D CDAI_TYPE_SCSI_DEVID; >>> cdai.bufsiz =3D CAM_SCSI_DEVID_MAXLEN; >>> + cdai.buf =3D malloc(cdai.bufsiz, M_CAMXPT, M_NOWAIT); >>> + if (cdai.buf =3D=3D NULL) { >>> + ret =3D ENOMEM; >>> + goto out; >>> + } >>> } else >>> goto out; >>>=20 >>> - cdai.buf =3D malloc(cdai.bufsiz, M_CAMXPT, M_NOWAIT|M_ZERO); >>> - if (cdai.buf =3D=3D NULL) { >>> - ret =3D ENOMEM; >>> - goto out; >>> - } >>> xpt_action((union ccb *)&cdai); /* can only be synchronous */ >>> if ((cdai.ccb_h.status & CAM_DEV_QFRZN) !=3D 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 =3D EFAULT; >>> } >>> } else { >>> - ret =3D 0; >>> - if (strlcpy(buf, cdai.buf, len) >=3D len) >>> + if (cdai.provsiz < len) { >>> + cdai.buf[cdai.provsiz] =3D 0; >>> + ret =3D 0; >>> + } else >>> ret =3D EFAULT; >>> } >>>=20 >>> out: >>> - if (cdai.buf !=3D NULL) >>> + if ((char *)cdai.buf !=3D buf) >>> free(cdai.buf, M_CAMXPT); >>> return ret; >>> } >>>=20 >>=20 >=20 > --=20 > Alexander Motin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D9890FB4-7BD4-432C-B3A1-DDEF3BEED6EC>