On Sat, Nov 04, 2017 at 07:23:36PM -0400, Jeff Layton wrote: > On Fri, 2017-11-03 at 08:22 -0400, Jeff Layton wrote: > > On Thu, 2017-11-02 at 12:15 +0200, Amir Goldstein wrote: > > > Eryu, > > > > > > This series enhances test coverage for generic NFS file handles > > > encode/decode functionality and adds a new gereric/exportfs test. > > > > > > Please note that the new test output includes the temporary test > > > number 500, so don't forget to fix those when renaming the test. > > > > > > The enhanced open_by_handle program is going to be used later on for > > > overlayfs specific exportfs tests [1]. > > > > > > The open_by_handle program is limited to encoding "non-connectable" > > > file handles (used by nfsd on 'no_subtree_check' exports), because there > > > is no user available API (that I know of) to encode a "connecctable" file > > > handle (used by nfsd on 'subtree_check' exports). I used a test patch > > > "test connectable file handles", available on my tree [1] to tun the tests > > > with "connectable" file handles. > > > > > > I verified that the new test passes on xfs, ext4, ext2, btrfs, f2fs. > > > However, the test fails on tmpfs due to: > > > "open_by_handle() returned 116 incorrectly on an unlinked open file!" > > > > > > This happens because tmpfs uses d_find_alias() to get a decoded dentry, > > > but d_find_alias() skips unhashed (deleted with refcount) dentries. > > > > > > I don't know if being able to decode a file handle of a deleted but open > > > file is a requirement for nfsd or just a recommendation, but IMO it is a > > > common case that is worth testing, even if tmpfs (or other file systems) > > > choose not to fix this. > > > > > > Bruce, Jeff, > > > > > > What is your view on this issue? > > > > > > > > > > It seems like something that should be a requirement. > > > > A client could (for instance) send a READ for a filehandle with one of > > the special anonymous stateids. If you can't decode the filehandle, you > > really have no way to know what inode against which to issue the read. Also even if you use a "real" stateid the current implementation will error out if the filehandle lookup fails, and that might be complicated to fix. > That said... > > If an open file is unlinked, and the server reboots you're more or less > back in the same situation anyway. That's generally the reason for > sillyrenaming in NFSv4.x instead of just removing the files outright. > > The main takeaway is that with NFS in general, it's actually rather > difficult to ensure that behavior across all server failure scenarios. > It'd be nice if tmpfs did this like the others, but it's probably not > fatal if it doesn't. I guess. The idea of failing on an unlinked inode still makes me nervous, though. It's supposed to be a look up by *inode*, not dentry. NFSv4 client can hold files open after unlink, even if the possibility of server reboots means applications don't get a real guarantee of that. tmpfs exports aren't much use across server reboots anyway, though. Some day we may fix the reboot problem--but possibly just by doing something like sillyrename on the server side, so that wouldn't affect this case much. I think some users would likely notice if we broke the ability to look up an unlinked file so I'd rather we kept that test in. --b.
