On 2023-08-25 13:30, Taylor R Campbell wrote: > Since VOP_READDIR requires vp to be locked, I can infer that the > handle_write caller must already hold vp locked. But that means that > we have ifd_lock -> vnode lock in one path, and vnode lock -> ifd_lock > in another path, which is forbidden (unless there's some reason we can > prove these paths never happen concurrently).
Hmm... I did not realize this, so I don't think NOTE_WRITE disambiguation is (or has ever been) safe. The real pain here is that we inherit a held vp_interlock and vnode lock in the needs_lock=false case from the kevent callback. So this may require a pretty substantial locking revision(?) Anyways, I'll take a closer look later this weekend. > I'm also suspicious of logic like this: > > mutex_enter(&fp->f_lock); > uio.uio_offset = fp->f_offset; > mutex_exit(&fp->f_lock); > ... > mutex_enter(&fp->f_lock); > fp->f_offset = uio.uio_offset; > mutex_exit(&fp->f_lock); > > If fp->f_offset can change concurrently while f_lock is released, this > looks like a problem. But if it can't, why do we need the lock at > all? I suspect this really does need a lock over the whole > transaction (maybe fp->f_lock, maybe something else), which is not > released while I/O is happening -- possibly not with mutex(9) but > something interruptible instead. I think you're right about needing a lock for the whole transaction. Since this is called from get_inotify_dir_entries(), perhaps it would be better for get_inotify_dir_entries() to keep track of the offset and pass it in instead? Theo(dore)
