Date: Tue, 26 Oct 2010 14:40:57 +0000 From: Mark Johnston <mjohnston@sandvine.com> To: Scott Long <scottl@samsco.org> Cc: "freebsd-scsi@freebsd.org" <freebsd-scsi@freebsd.org> Subject: RE: [Patch] camcontrol - determine the defect list length Message-ID: <649630C24D5E884F85DD13645FE903A7640B50C2@wtl-exch-1.sandvine.com> In-Reply-To: <DD4C1CD8-FE86-429A-AF9A-26DD81DF7D49@samsco.org> References: <649630C24D5E884F85DD13645FE903A7640B244F@wtl-exch-2.sandvine.com> <DD4C1CD8-FE86-429A-AF9A-26DD81DF7D49@samsco.org>
next in thread | previous in thread | raw e-mail | index | archive | help
>> Hi,
>>=20
>> Below is a patch for camcontrol from the tree at my work. It was introdu=
ced several years ago to resolve some issues that we were seeing in Adaptec=
's RAID controller firmware. The change is to the readdefects() function - =
when sending the request to read the defect list, instead of using a 65000-=
entry buffer, we wrote a new function to query the drive and determine the =
size of the defect list before allocating any memory. Apparently, if we sen=
d a maximum length longer than the number of bytes in the defect list, the =
cam call returns an overflow error due to what is probably a firmware bug.
>>=20
>> It's probable that this issue has been fixed in Adaptec's firmware for a=
while now, and that this change is no longer necessary to us. However, it'=
s conceivable that other devices could have the same problem, and as far as=
I can see, this patch shouldn't introduce any regressions - it essentially=
consists of sending an additional CAM request before getting the defect li=
st.
>>=20
>> I'd like to know whether this patch is worth trying to get committed int=
o FreeBSD. If anyone has some comments or suggestions on the patch, I'd hig=
hly appreciate it.
>>=20
> Should the whole operation of reading the defect list abort if readdefect=
len() fails, or should it just fall back to a modest length and try the ope=
ration anyways? > Also, if readdefectlen() succeeds, how do the defect_lis=
t and ccb get freed?
> Scott
Ah, thanks for pointing that out. The patch below fixes the missing frees.
I would argue that there's not much point in retrying the operation if read=
defectlen() fails since
readdefectlen() performs essentially the same operations as readdefects() -=
readdefectlen() will fail if:
1. malloc() returns NULL somewhere,
2. the defect list format isn't specified
3. cam_send_ccb() fails
4. the request defect list format isn't supported
5. there was an error when reading the defect list data
Of these, I don't see any reason that readdefects() would succeed if readde=
fectlen() fails. cam_send_ccb()
performs a cam ioctl which in turn fails if functions like xpt_find_bus() a=
nd xpt_find_target() do. It doesn't
seem likely to me that a second call would succeed if the first failed.
Thanks,
-Mark
--- camcontrol.c@@/main/RELENG_8/RELENG_8_1/svos_8_1/1 2010-10-21 15:39:04=
.000000000 -0400
+++ camcontrol.c 2010-10-26 10:12:12.000000000 -0400
@@ -2168,7 +2168,7 @@ static int readdefectlen(struct cam_devi
}
=20
*ret_len =3D returned_length;
- return 0;
+
defect_len_bailout:
=20
if (defect_list !=3D NULL)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?649630C24D5E884F85DD13645FE903A7640B50C2>