Re: hurd/libdiskfs ChangeLog dir-lookup.c

[Thomas Bushnell BSG]

On Mon, 2008-06-09 at 21:52 +0000, Samuel Thibault wrote:
> CVSROOT:      /cvsroot/hurd
> Module name:  hurd
> Changes by:   Samuel Thibault <sthibaul>      08/06/09 21:52:12
> 
> Modified files:
>       libdiskfs      : ChangeLog dir-lookup.c 
> 
> Log message:
>       2008-06-06  Samuel Thibault  <samuel.thibault@ens-lyon.org>
>       
>               * dir-lookup.c (diskfs_S_dir_lookup): Unlock np in case of 
> errors.

To be clear, as per my previous message, I would like this change
reverted and replaced with something less destabilizing.

Note that the suggested fix I emailed is not complete: it requires
checking to make sure that the relevant code only happens in the right
places.  (for example, if(newpi) is not exactly kosher since the exit
sequence releases a reference without clearing NEWPI. be careful!)

But I want to add a further note that Samuel's process--while
good--suffers from an important flaw in thinking about locking race
conditions.  When there is a deadlock, there is certainly a bug.  But it
is quite incorrect to simply assume that the correct fix is to unlock
something before the relevant routine is called.

Note that in this case the result is a very dangerous destabilization in
the case taht NP = DNP, where Samuel's patch would result in an extra
unlock.  That could either produce one of those panics that was reported
recently (can't remember by whom), or worse yet, unlocking something
that should be locked with the result that we get directory corruption
errors!

The thought process should have been:
  Oooh, releasing the protid is bad if we are the last reference and
holding the lock; indeed, it is not allowed to release the last
reference if you hold the lock....

  So what should we do?  We could (1) release the lock before releasing
the protid, or (2), postpone releasing the protid until later.

Note that (1) changes the way the locking protocol works for possibly
lots of things.  Very destabilizing--even if it happened not to cause a
bug in a particular case.  Option (2) is *guaranteed to be safe*,
because there is no harm in holding a reference longer than you have to.


Thomas