Re: SEEK_DATA/SEEK_HOLE with vnode locked

Just to summarize this...
I was able to do a VOP_SEEK() which would be called with a
LK_SHARED locked vnode and it seemed to work fine.

However, ReadPlus (which is like Read, but allows for
holes to be represented as <offset, length> in the reply
instead of a stream of 0 bytes) seems to be a performance
dud.

I was surprised how poorly it performed compares to ordinary
Read. Typically it would take 60% longer to read a file. I tried
sparse and non-sparse files of various sizes and they always
took longer. (If I disabled SEEK_DATA/SEEK_HOLE in the server
code, so it never actually did holes, it worked comparably to
regular Read, so somehow the overhead of doing SEEK_DATA/SEEK_HOLE
was a big performance hit. It was using LK_SHARED locks, so
it wasn't serializing the reads, but I don't really know why it
performed so poorly?)

Anyhow, unless the performance issue gets resolved, there is
no reason to commit the code to FreeBSD's main.
(NFSv4.2 operations, like ReadPlus, are all optional and are not
 required for an RFC conformant implementation.)

rick

________________________________________
From: Konstantin Belousov <kostikbel@gmail.com>
Sent: Wednesday, August 10, 2022 7:11 PM
To: Rick Macklem
Cc: FreeBSD Filesystems
Subject: Re: SEEK_DATA/SEEK_HOLE with vnode locked

CAUTION: This email originated from outside of the University of Guelph. Do not click links or open attachments unless you recognize the sender and know the content is safe. If in doubt, forward suspicious emails to IThelp@uoguelph.ca


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.