Re: Fileassoc locking

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

Elad Efrat <elad%NetBSD.org@localhost> wrote:
> So... attached is a second attempt at fileassoc locking. (Entire source
> file to make reading easier.) This time there are no rw locks, and I'm
> using mutexes and condition variables. I've tested this version as well,
> but once again, I'd appreciate a review for the changes.

I do not have time for a full review, but from a quick glance:

- Code should be re-structured to allocate/create objects before taking any
  locks, and free/destroy after last locks.  For example, operations which
  might block or take a lot of time should be outside the locked paths.

- There are deadlocks (e.g. fileassoc_deregister).  If lock order is
  fileassoc_list_lock -> assoc_mtx, then you cannot lock in other way round.
  Apart from comments, please add KASSERT()s for locking and ref-counting
  assumptions.

- fileassoc_deregister() should remove entry from the list first, wait while
  for references to exit, and then destroy.

- fileassoc_table_lookup() or its uses are missing locking (reference does
  not provide exclusion).  Thus, fileassoc_table does not need ref-counting,
  unless for workarounds in fileassoc_table_run(), see below.  Use of RW-lock
  would fit in this case.

- fileassoc_file_lookup() is wrong: fileassoc_table should be locked when
  performing lookup, it does not need ref-counting.  Also, no need to hold
  a reference on each fileassoc_file entry, reference must be held and kept
  for caller on entry which was found.

- fileassoc_table_clear() and fileassoc_table_run() lack locking.  Callback
  execution without locks held will be problematic here.

- Why fileassoc_file_add() and fileassoc_file_delete() are waiting for zero
  references?  It does not look to me that fileassoc_file requires condvar,
  e.g. last reference holder can free it.   Also, fileassoc_file_add() checks
  for duplicates - I think caller (of API) should do that.

- I think fileassoc_init() should be called manually, instead of spreading
  RUN_ONCE() across API routines.

-- 
Mindaugas