Re: CVS commit: src/sys

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

Hello,

> Module Name:    src
> Committed By:   reinoud
> Date:           Sat Sep 27 13:01:07 UTC 2008
> 
> Modified Files:
>         src/sys/conf: files
>         src/sys/kern: vfs_init.c
> Added Files:
>         src/sys/kern: vfs_dirhash.c
>         src/sys/sys: dirhash.h
> 
> Log Message:
> Add generic FS agnostic directory hashing support. Currently only in use by
> UDF. Future users could be msdosfs, ufs, nilfs2 (when ready), cd9660 etc.

It would be nice to see UFS using this in future! I have few comments:

- Locking in vfs_dirhash.c is not clear, it does not describe which and who
  protects the structures, nor how synchronisation is done. It looks like some
  parts are protected by caller, some by dirhashmutex. KASSERTs do not explain
  that either.

- In dirhash_purge_entries() memory is freed (pool_put() called) while lock
  is held (dirhashmutex) - that reduces the concurrency. Also, if caller
  holds some lock, API might need to be adjusted to free the structures later.

- It looks like hash(9) and hashinit(9) interfaces could be used for hashing
  part, instead of re-implementation of them.

- These could be static, thought: dirhashmutex, maxdirhashsize, dirhashsize,
  dirhash_queue.

- It would be good to add prefixes for the members of dirhash_entry and
  dirhash structures, like dh_* and dhe_*. No need to name arguments in
  function prototypes. Also, header needs #ifndef _SYS_DIRHASH_H_ macros
  to avoid double inclusion.

- Please create sysctl()s dynamically, that is, using CTL_CREATE, instead of
  CURDIRHASHSIZE_SYSCTLOPT and MAXDIRHASHSIZE_SYSCTLOPT.

- Having minimal comments about functions or dirhash(9) man page would help
  to understand the interface better too. :)

Thanks.

-- 
Best regards,
Mindaugas
www.NetBSD.org