kern/130735: [patch] pass M_NOWAIT to the malloc() call inside cdreaddvdstructure()

[Eygene Ryabinkin]

The following reply was made to PR kern/130735; it has been noted by GNATS.

From: Eygene Ryabinkin <rea-fbsd at codelabs.ru>
To: Jaakko Heinonen <jh at FreeBSD.org>
Cc: bug-followup at 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
     {_.-``-'         {_/            #