Page MenuHomeFreeBSD

Avoid ESTALE if directory deleted by same NFS client
ClosedPublic

Authored by cperciva on Dec 29 2016, 7:55 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 27, 10:02 AM
Unknown Object (File)
Sat, Apr 27, 10:01 AM
Unknown Object (File)
Sat, Apr 27, 9:07 AM
Unknown Object (File)
Sat, Apr 27, 8:33 AM
Unknown Object (File)
Dec 20 2023, 7:22 AM
Unknown Object (File)
Dec 13 2023, 4:46 PM
Unknown Object (File)
Nov 7 2023, 2:45 PM
Unknown Object (File)
Aug 19 2023, 4:53 AM
Subscribers

Details

Reviewers
rmacklem
Summary

Add a new NFS node flag, NREMOVED, which is set when a rmdir has been called on a directory. Check this flag in nfs_lookup and return ENOENT in response to attempts to look up paths within a directory which we removed.

This does nothing to prevent ESTALE responses in the event that a directory is removed by a *different* NFS client, but in the case where we are the only NFS client touching a particular tree it should prevent us tripping over ourselves. In particular, it unbreaks the "cleandir" stage of make buildworld -jN with an NFS-mounted /usr/obj/, wherein multiple rm -r commands race to delete each other's working directories.

NOTES FOR REVIEWERS:

  1. It may be necessary to add checks for NREMOVED to other vnode operations. I haven't found any way to trigger the aberrant ESTALE behaviour with this patch so I'm assuming everything I'm trying is hitting the nfs_lookup; but I don't know VFS well enough to know if this is something we can rely upon.
  1. In nfs_rmdir I'm picking up a lock on the nfsnode of the directory being deleted in order to set the NREMOVED flag. I'm not absolutely sure if this is necessary; is there sufficient locking in the VFS layer at this point to protect us?

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

cperciva retitled this revision from to Avoid ESTALE if directory deleted by same NFS client.
cperciva updated this object.
cperciva edited the test plan for this revision. (Show Details)
cperciva added a reviewer: rmacklem.
cperciva set the repository for this revision to rS FreeBSD src repository - subversion.

Just noticed that this breaks things if the rmdir RPC fails. Updated patch coming soon...

rmacklem edited edge metadata.

Looks fine to me. In case you didn't want this behaviour, the NREMOVED flag is
cleared by the NFS VOP_INACTIVE(), which means the "return ENOENT" will only
happen while another reference is held on the vnode (such as a current directory).

To me, this behaviour seems fine.

I might separate the new comment you added at line#1044 from the old one that
applies to the while() loop below your "if" block. For your case, you don't actually wait
for the remove to happen because it has already happened.

And, yes, I think your use of the n_mtx mutex is appropriate.

This revision is now accepted and ready to land.Jan 2 2017, 11:14 PM

Looks fine to me. In case you didn't want this behaviour, the NREMOVED flag is
cleared by the NFS VOP_INACTIVE(), which means the "return ENOENT" will only
happen while another reference is held on the vnode (such as a current directory).

Just to confirm my understanding: Once VOP_INACTIVE is called, that means nobody
is holding a file descriptor open to the deleted directory, right? So any attempt to do
anything with the directory after VOP_INACTIVE is called will result in ENOENT when
we try to find something which no longer exists?

I might separate the new comment you added at line#1044 from the old one that
applies to the while() loop below your "if" block. For your case, you don't actually wait
for the remove to happen because it has already happened.

I'll clarify that comment slightly. I was keeping them together in order to avoid splitting
the lock/unlock pair too far apart.

A new possible issue has come up while testing this: Something is unhappy about getting ENOENT when trying to access '.'. I think it might be getdirentries; on UFS attempting to read a directory which has been deleted acts as if it is an empty directory rather than a non-existent directory. I'm not sure if behaving like UFS in this case is feasible.

For UFS and your simple test example (one process has a current dir that has been
rmdir'd by another thread), getdirentries(2)/getdent(2) returns no error and no data.
(ie. No directory information.)
--> Getting nfs_readdir() to do the same should be feasible. In fact, it might already do so?

(Can't recall if I did the test on an NFS mount as well as UFS.)

If you get to the point where another VOP_xxx() (like VOP_ACCESS()) is happening on ".",
then I think you are stuck. The NFS protocol is not (and cannot be) a POSIX compliant
file system. As such, fixes like these can only help to give it a little more POSIX-compatibility.