Comment 3 for bug 1336794

On Sun, Apr 12, 2015 at 9:09 AM, Al Viro <email address hidden> wrote:

> On Sun, Apr 12, 2015 at 12:42:35PM -0000, Eric Van Hensbergen wrote:
>
> > In other words, it only uses the open fd to derrive a path and then
> > executes the getattr off of that path. If that path no longer exists
> > (because of unlink or remove) then you are hosed. In my understanding,
> the
> > "work around" I suppose is the so-called 'silly renaming' where
> > remove/unlink simply does a rename until all open instances are closed.
>
> What do you mean, "no longer exists"? Don't confuse path with pathname -
> it's a mount,dentry pair. And dentry in question bloody well ought to
> still
> have the FID associated with it - you shouldn't use the same FID for
> TREMOVE and for TREAD/TWRITE.

Quite right, the fid is still there, I just don't have an easy way to get
at it. vfs_file operations have a direct notion of the fid because they
cache it in the struct file private data. dentries have several fids
associated with them and stored in thier private data, so I've got to
"guess" the right one. In most cases this probably won't cause a problem,
but it just feels a bit off.

There was a thread a few years back where Miklos was arguing for fstat to
pass through struct file information since the the fd given the fstat had a
file structure associated with it (which in 9p's case has a direct pointer
to the "right" fid):
    http://lwn.net/Articles/251228/

In any case, I've drafted a quick patch which takes the approach of
searching the dentry fid list for the fid, but it doesn't feel like the
right answer and I'm fairly certain I need to iterate on it a few times to
make sure I haven't hosed something up.

> TREMOVE clunks the FID passed to it; on
> some servers you really have no choice - server discards the file
> completely
> and on any FID that used to refer to it you get an error from that point
> on.
> On those you'd really have to do something like sillyrename - the only
> way to keep IO going for a file sitting on such server is to have it
> visible somewhere. Normal fs(4) is that way; e.g. u9fs(4) isn't - there
> FID maps to opened file descriptor on host and TREMOVE on another FID
> doesn't break it, as long as host supports IO on opened-but-unlinked files.
> I don't remember where qemu 9pfs falls in that respect, but I'd expect it
> to be more like u9fs...
>
>
Sort of, the servers in kvmtool and qemu (and diod) have a fid with the
open handle. However, all of them seem to implement getattr assuming they
have to re-walk to the file. In order to test my aforementioned changes to
the client, I also did a quick patch to kvmtool which checks and sees if
the fid it receives has an fd and just uses fstat instead of lstat. Patch
is here at the moment, I'll send upstream once I'm happy with the client
side changes and look into creating a patch for qemu/diod:

https://github.com/ericvh/linux-kvm/commit/2fa2f7e728ac08a7d9006516870db1a986aa6acc

> Which FID are you passing to server on unlink()?
>
>
Unlink/remove look to be getting a proper fid (in other words, not using
the one that is open). The problem is that getattr is using a reference
fid (an open fid that's already walked to the name). From a protocol
semantics perspective the fixes mentioned above probably don't help that we
may have some dangling unopen fids pointing at a name that is no longer
valid, but that is just a consequence of the imperfect nature of the
mapping of dentries to fids and I'm not sure it does any harm. A trace
from the original problem on diod (which appears to not implement unlink
and is falling back to remove).

diod: P9_TWALK tag 1 fid 1 newfid 2 nwname 1 'foobar'

diod: P9_RLERROR tag 1 ecode 2

diod: P9_TWALK tag 1 fid 1 newfid 2 nwname 0

diod: P9_RWALK tag 1 nwqid 0e

diod: P9_TLCREATE tag 1 fid 2 name 'foobar' flags 0x8042 mode 0100644 gid 0

diod: P9_RLCREATE tag 1 qid (000000000012524b 0 '') iounit 0

diod: P9_TWALK tag 1 fid 1 newfid 3 nwname 1 'foobar'

diod: P9_RWALK tag 1 nwqid 1 (000000000012524b 0 '')

diod: P9_TGETATTR tag 1 fid 3 request_mask 0x17ff

diod: P9_RGETATTR tag 1 valid 0x17ff qid (000000000012524b 0 '') mode
0100644 uid 0 gid 0 nlink 1 rdev 0 size 0 blksize 4096 blocks 0 atime Mon
Apr 6 11:11:08 2015 mtime Mon Apr 6 11:11:08 2015 ctime Mon Apr 6
11:11:08 2015 btime X gen 0 data_version X

diod: P9_TUNLINKAT tag 1 dirfid 1 name 'foobar' flags 0

diod: P9_RLERROR tag 1 ecode 95

diod: P9_TWALK tag 1 fid 3 newfid 4 nwname 0

diod: P9_RWALK tag 1 nwqid 0

diod: P9_TREMOVE tag 1 fid 4

diod: P9_RREMOVE tag 1

diod: P9_TGETATTR tag 1 fid 3 request_mask 0x3fff

diod: P9_RLERROR tag 1 ecode 2

diod: P9_TCLUNK tag 1 fid 2

diod: P9_RCLUNK tag 1

diod: P9_TCLUNK tag 1 fid 3

diod: P9_RCLUNK tag 1

The client cloning 3 to 4 before the remove seems a bit unnecessary, but is
probably done in the case that the remove fails on the server so that we
still have a dentry reference.