Page MenuHomeFreeBSD

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

Authored by avg on May 14 2018, 12:48 PM.



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.

Diff Detail

rS FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

avg created this revision.May 14 2018, 12:48 PM

only one thought really.

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 updated this revision to Diff 42564.May 15 2018, 6:04 AM

word smithing

avg marked an inline comment as done.May 15 2018, 6:07 AM
avg added inline comments.
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 requested changes to this revision.May 15 2018, 7:48 PM

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.

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

if (needs_unbusy != 0)
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;"?

This revision now requires changes to proceed.May 15 2018, 7:48 PM
avg added inline comments.May 16 2018, 7:24 AM
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.

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.

avg updated this revision to Diff 42614.May 16 2018, 7:32 AM

fix bugs pointed out by the reviewers

avg updated this revision to Diff 42616.May 16 2018, 7:54 AM
  • restore formatting of a block that has no functional changes
  • re-arrange the code that handles attribute errors per Rick's suggestion
avg marked 7 inline comments as done.May 16 2018, 7:56 AM
rmacklem accepted this revision.May 16 2018, 12:02 PM

This version looks fine to me. Thanks, rick

This revision is now accepted and ready to land.May 16 2018, 12:02 PM
This revision was automatically updated to reflect the committed changes.