On Tue, Oct 19, 2021 at 09:02:53AM +0200, Sebastien Marie wrote:
> Hi,
>
> The following diff is a bit large. it could be splitted for easy
> review, but I tought having the full view would be good too.
>
> It moves the current vnode lock mecanism, implemented inside FS
> specific struct, to `struct vnode`.
>
> I used `vop_generic_lock`, `vop_generic_unlock` and
> `vop_generic_islocked` functions for this generic implementation (see
> kern/vfs_default.c for implementation and kern/vfs_subr.c for
> initialisation).
>
> As usual, `struct vops` controls which functions are called when using
> VOP wrappers. It makes possible to move FS to generic implementation
> slowly.
>
> FS implementation part is mostly about removing the current code
> (rrw_init, lock/unlock functions, vop_{lock,unlock} setting).
>
> As part of the diff, ntfs and mfs are using the generic vnode lock
> (set vop_{lock,unlock,islocked} in vops).
>
>
> There is one tricky part is vget() FS implementation for special
> devices. The aliases mecanism is a bit tricky as it replaces the
> v_data from one vnode to another.
>
> With new code, the lock isn't moved anymore (as it is part of vnode,
> and is not inside v_data) and it should be reacquired. It makes me to
> unlock old vnode, and lock the new vnode. I think this part might be
> wrong and/or racy (but I am unsure the current version would be
> right), but we don't have a way to properly "move" the lock (WITNESS
> is tracking pointers, so just copying the data doesn't work with
> WITNESS).
I think it is not correct to unlock the inode like this. The inode
is already in the inode hash table, so other threads can in principle
get a hold of it before the vinit routine manages to lock the alias
and update the inode. The code has relied on the fact that the lock
gets transferred from a vnode to another.
checkalias() and the related vnode swapping is difficult to understand.
I have been tinkering with a patch that replaces checkalias().
Unfortunately, it is not ready yet.
