Re: SEEK_DATA/SEEK_HOLE with vnode locked

On Wed, Aug 10, 2022 at 10:17:36PM +0000, Rick Macklem wrote:
> Hi,
> 
> When implementing what is called Read_Plus for the NFSv4.2
> server, I need to do SEEK_DATA/SEEK_HOLE. However, if I
> use VOP_IOCTL(), I need to unlock/relock the vnode.
> 
> This can result in problems if another RPC changes the size
> of the file or allocates/deallocates data in the file while the
> vnode is unlocked.
> 
> >From what I can see, UFS does SEEK_DATA/SEEK_HOLE with
> the vnode locked and ZFS doesn't seem to care/notice if it
> is locked.
> (Actually, ZFS looks like it might be unsafe, since it seems to
>  assume it can use the unlocked vnode that might be doomed,
>  but I do not know ZFS.)
The statement about UFS needs more precision.  UFS VOP_IOCTL() takes
unlocked but references vnode, and the first thing vn_bmap_seekhole/data()
and ufs_vmap_seekdata() do is vn_lock(vp, LK_SHARED).  Since LK_RETRY flag
is not specified, doomed vnodes result in ENOENT.

I believe this interface was done because I wrote bmap-based seekhold/data
while ZFS had its implementation already in place.  It avoided taking the
vnode lock, so UFS needed to do the same.

I am not sure was the ZFS interface decision due to some internal consistency
guarantees already present, or because internal locking causing ordering
issue with the vnode lock.

> 
> Anyhow, does implementing a new vnode op VOP_SEEK(), which
> takes a locked vnode and does SEEK_DATA/SEEK_HOLE sound
> reasonable?
You need to make some decision about ZFS first, I believe.  UFS and generic
buffer cache consumers fs are trivial to adopt, of course.