nfsrvd_readdirplus: for some errors, skip an entry instead of failing the request

[avg]

Summary

This change consist of two primary changes.

A failure to vget or lookup an entry is considered to be a result of a concurrent removal,
which is the only resonable explanation given that the filesystem is busied.
So, the entry would be silently skipped.

In the case of a failure to get attributes of an entry for an NFSv3 request, the entry
would be silently skipped. There can be legitimate reasons for the failure, but NFSv3
does not provide any means to report the error, so we have two options: either fail the
whole request or ignore the failed entry. Traditionally, the old NFS server used the latter option,
so the code is reverted to it. Making the whole directory unreadable because of a single entry
seems to be unpractical.

Additionally, some bits of code are slightly re-arranged to account for the new control flow and
to honor style(9).

Sponsored by Panzura.

[avg]

Just for the context, here is what old NFS server did: https://reviews.freebsd.org/source/src/browse/stable/10/sys/nfsserver/nfs_serv.c$3094

[julian]

only one thought really.

sys/fs/nfsserver/nfs_nfsdport.c

2175 ↗

(On Diff #42514)

become, not became.. it's in this case would be short for "it has".
Possibly Best to actually spell it that way.

2246 ↗

(On Diff #42514)

You are really sure about nvp never being NULL right? (e.g. line 2162)

[avg]

word smithing

[avg]

sys/fs/nfsserver/nfs_nfsdport.c

2246 ↗

(On Diff #42514)

Yes, I am. With this change, the code detects much earlier if it failed to get nvp. So, past that point nvp cannot be NULL. Modulo bugs, of course.

[rmacklem]

See the first inline comment. I think you need the vput(nvp)... before "goto invalid;" for the
"if ((nd->nd_flag & ND_NFSV3) != 0)".

Or you could move this "if..goto" down into the "if" block that follows it to just before
the "nd->nd_repstat = r;" line. This will work, but may be less clear because you need
to know that NFSATTRBIT_RDATTRERROR is never set for NFSv3.

sys/fs/nfsserver/nfs_nfsdport.c

2182 ↗

(On Diff #42564)

Maybe you could stick a
KASSERT(nvp != NULL. ...);
here as a asafety belt against future changes that might result in the code
getting down here without a valid nvp?
(I agree that it should be non-NULL when you get here after your changes.)

2218 ↗

(On Diff #42564)

Shouldn't there be a
{

vput(nvp);
if (needs_unbusy != 0)
      vfs_unbusy(new_mp);
goto invalid;

}
in this "if" as well as the code that follows, since "invalid:" is after code that does this,
instead of just a "goto invalid;"?

[avg]

sys/fs/nfsserver/nfs_nfsdport.c

2182 ↗

(On Diff #42564)

The only reason I didn't add such an assertion is that NULL pointer dereference is a very obvious kind of bug. But I agree that the assertion can be helpful to readers and maintainers of the code, so I am adding it now.
Also, I overlooked the case where we get a non-NULL refp for NFSv4, so we do not get nvp at all.

2218 ↗

(On Diff #42564)

Good catch! I think that needs_unbusy is for NFSv4 only, but nvp must be released.

2246 ↗

(On Diff #42514)

Actually, I was very wrong! In the case of NFSv4, if all attribute bits are zero or if the entry has a referral, then we do not look up nvp at all, so it remains NULL.
Thank you for alerting me to this.

[avg]

fix bugs pointed out by the reviewers

[avg]

• restore formatting of a block that has no functional changes
• re-arrange the code that handles attribute errors per Rick's suggestion

[rmacklem]

This version looks fine to me. Thanks, rick