Christos, On Sat, Apr 11, 2009 at 09:35:03PM +0000, Christos Zoulas wrote:
> In article <20090411210833.go22...@hairylemon.org>, > Andrew Doran <a...@netbsd.org> wrote: > >On Sat, Apr 11, 2009 at 11:47:34AM -0400, Christos Zoulas wrote: > > > >> Module Name: src > >> Committed By: christos > >> Date: Sat Apr 11 15:47:34 UTC 2009 > >> > >> Modified Files: > >> src/sys/dev/dmover: dmover_io.c > >> src/sys/dev/putter: putter.c > >> src/sys/kern: kern_drvctl.c sys_mqueue.c > >> src/sys/net: bpf.c bpfdesc.h if_tap.c > >> src/sys/opencrypto: cryptodev.c > >> src/sys/sys: mqueue.h > >> > >> Log Message: > >> Fix PR/37878 and PR/37550: Provide stat(2) for all devices and don't use > >> fbadop_stat. > > > >The locking is all screwed up or missing. stat() can be expected to report > >bad times for these files. I wrote the below to you in private and forgot to > >copy the list. > > > > I added locking as you suggested, and if it is screwed up or missing > it is screwed up the same way in all the existing copies of fop_stat() > which were there before I added mine. Prior to your change: soo_stat: good kqueue_stat: good (single read of a <=uintptr_t sized value is atomic and is the only value we are after that can change at runtime, so we get an atomic snapshot of everything asked for via stat(). no need to synchronize with other threads since the caller does not know for sure when the stat() will take place in relation to other operations) vn_statfile: broken, does not lock vnode pipe_stat: broken, does not lock pipe > As for invalid timestamps, all set proper timestamps except drvctl and you > did not mention that. I was speaking about concurrency and not about other aspects of how timestamps are managed. I haven't looked at the functional aspects of your change. I said that >uintptr_t sized updates are unlikely to be atomic. One example: if your stat and update race on a 'struct timespec', stat can return a timestamp that is in the future, because only one chunk of the timespec has been updated and made globally visible when stat executes. > It is much easier and saves time to explain what the mistakes are rather > than sending out messages saying this is wrong or that is wrong. I did not have the time when I wrote the message to you and I expected that you would be able to examine the code and see the problems. Anyway: dmover: looks good. putter: + getnanotime(&pi->pi_atime); KERNEL_LOCK(1, NULL); the getnanotime needs to be within KERNEL_LOCK for reason outlined above. drvctl: looks good. mqueue: mqueue runs without kernel_lock, so we need mq_mtx and not kernel_lock for stat. i haven't checked to see if the getnanotime calls are within a block where mq_mtx is held (into an existing block would of course be best). bpf_stat: looks good. tap: looks good. cryptodev: crypto runs without kernel_lock. a lot of the timespec assignments in the diff are done ouside crypto_mtx. on the other side, stat needs crypto_mtx and not kernel_lock.