The following reply was made to PR kern/130735; it has been noted by GNATS.
From: Eygene Ryabinkin <
rea-fbsd@...>
To: Jaakko Heinonen <
jh@...>
Cc:
bug-followup@...
Subject: Re: kern/130735: [patch] pass M_NOWAIT to the malloc() call inside
cdreaddvdstructure()
Date: Thu, 29 Oct 2009 10:40:27 +0300
Jaakko, good day.
Mon, Oct 26, 2009 at 11:19:20AM +0200, Jaakko Heinonen wrote:
> I have been looking at this. The same problem also exists in
> cdreportkey() and cdsendkey(). I think that it's better to drop periph
> lock while doing M_WAITOK malloc instead of using M_NOWAIT. Could you
> test and look at this patch:
>
>
http://www.saunalahti.fi/~jh3/patches/scsi_cd-M_WAITOK-fixes.diff
It works fine for me. Alhough I am no completely familiar with the CAM
locking (and that't why I had patched with M_NOWAIT rather than with
dropping the locks), so I can't fully judge if dropping the lock inside
the helper is good. As I understand, locking is done to prevent races
with other requests on the same device. Most probably, dropping the
lock inside cdreportkey(), cdsendkey() and cdreaddvdstructure(), is OK,
since all three calls are wrapped by the lock/unlock in the ioctl
handler like this:
-----
cam_periph_lock(periph);
error = cdXXX(ARGS);
cam_periph_unlock(periph);
-----
so dropping the lock at the entry and restoring it after the malloc
call is just equivalent to the moving the lock acquisition/release
to the functions themselves. I mean that these three functions can
be called unlocked and will have the following structure:
-----
int cdXXX(...) {
check for sanity
grab the memory
cam_periph_lock(periph);
do the stuff
cam_periph_unlock(periph);
}
-----
It looks a bit cleaner from the design point of view (and that is what
happens in practice, because the situation when the caller locks us and
we unlock the stuff readily upon the entry to the function, just leads
to the two locking calls that essentially do nothing): one won't think
"heck, why we're dropping the lock here, will it be good?". But this
contradicts with the general stratedy of scsi_cd.c to call all helper
functions that do the actual work locked. Perhaps, the comment on the
top of the cdioctl() that explains that everything, but the ioctl
helpers that need malloc(M_WAIT), will be called locked and the said
functions should grab the locks by themselves.
--
Eygene
_ ___ _.--. #
\`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard
/ ' ` , __.--' # to read the on-line manual
)/' _/ \ `-_, / # while single-stepping the kernel.
`-'" `"\_ ,_.-;_.-\_ ', fsc/as #
_.-'_./ {_.' ; / # -- FreeBSD Developers handbook
{_.-``-' {_/ #
_______________________________________________
freebsd-scsi@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-scsiTo unsubscribe, send any mail to "
freebsd-scsi-unsubscribe@..."
