Page MenuHomeFreeBSD

nfs server: improve use if the VFS KPI
ClosedPublic

Authored by kib on Jan 1 2021, 3:40 PM.

Details

Summary

In particular, do not assume that vn_start_write() returns the same mp as it was passed in.
Also be more accurate to return NULL vp and mp when error occured, to catch wrong control flow easier.
Stop checking for NULL mp before calling vn_finished_write(), NULL mp is handled transparently by VFS.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib requested review of this revision.Jan 1 2021, 3:40 PM
kib created this revision.
kib edited the summary of this revision. (Show Details)

Comments inline.

sys/fs/nfsserver/nfs_nfsdport.c
3250

This causes crashes. I think you meant..

if (mpp != NULL)
3266

It might be better to set ESTALE here.
It is the error that tells the NFS client
that the file handle did not work.

3266

I think that ESTALE might be better here.
It is what NFS clients expect when translation
of a file handle fails.

Having said that, it does not appear that
vn_start_write(NULL, &mpw, V_WAIT)
will ever return an error, for the current code.
As such, it is hard to say what it should be
if/when the function can fail with these arguments.

kib marked 3 inline comments as done.Jan 2 2021, 12:43 AM
kib added inline comments.
sys/fs/nfsserver/nfs_nfsdport.c
3266

This is not a translation of the file handle, but attempt to prepare for operation. Anyway, I changed the error to ESTALE.

I cannot guarantee that vn_start_write() would not return an error for V_WAIT forever. I see no reason to not use the VFS KPI in a way that is more future-proof.

kib marked an inline comment as done.

Fix silly typo, *mpp != NULL -> mpp != NULL.
Change error code from EIO to ESTALE for failed vn_start_write().

Looks fine to me now.
With these changes my test run works fine.

sys/fs/nfsserver/nfs_nfsdport.c
3265

Sure. But at least for NFSv4, the recognized errors
for this operation (PutFH) are in nfsv4err_putfh
and they are:
NFSERR_SERVERFAULT
NFSERR_BADHANDLE
NFSERR_BADXDR
NFSERR_MOVED
NFSERR_RESOURCE
NFSERR_STALE
NFSERR_WRONGSEC

The only one of these that is recognized by
NFSv3 is NFSERR_STALE. So it is either return
it or do different returns for NFSv3 vs NFSv4.
NFSERR_BADHANDLE might make more
sense for NFSv4, but it is not obviously better
than NFSERR_STALE and the others would
depend upon why vn_start_write() fails.

This revision is now accepted and ready to land.Jan 2 2021, 1:55 AM
This revision was automatically updated to reflect the committed changes.