Date: Thu, 29 Oct 2009 07:50:03 GMT From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> To: freebsd-scsi@FreeBSD.org Subject: Re: kern/130735: [patch] pass M_NOWAIT to the malloc() call inside cdreaddvdstructure() Message-ID: <200910290750.n9T7o28e011964@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/130735; it has been noted by GNATS. From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> To: Jaakko Heinonen <jh@FreeBSD.org> Cc: bug-followup@FreeBSD.org 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 {_.-``-' {_/ #
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200910290750.n9T7o28e011964>