From: "Eric Van Hensbergen" <eric.vanhensbergen@linux.dev>
To: asmadeus@codewreck.org, v9fs@lists.linux.dev
Subject: Anomolies
Date: Wed, 10 Apr 2024 15:38:34 +0000 [thread overview]
Message-ID: <1296bceb87d9da7c58f7677260592683b3abdb3e@linux.dev> (raw)
Dominique - check my line of thought on this...
Okay, I'm going down lots of ratholes in my search for where the bug might be since I can't reproduce it.
One bit that I picked up that I think might be wrong is there is v9fs_fid_add(dentry, &fid) at the end of v9fs_vfs_mknod_dotl (which ends up being the proxy for all non-atomic create events). This was there only for cached modes previously, but when I "cleaned up" the code I propagated it. I think its wrong in both cases because we v9fs_fid_put at the end of function which should clunk and invalidate the fid, but it never gets removed from the dentry. Of course, if I want parity with the old code, I could only add it when we are cached, but I'm concerned its just a latent bug.
The other element here that is concerning is why the entire code block is even necessary -- if ACLs are enabled, I guess it makes sense because if the host file system isn't using ACLs this lets us stash our ACLs in the appropraite xattrs. But that says it should be bounded by CONFIG_9P_FS_POSIX_ACL and its not clear why we deinstantiate(dentry,inode) since in non-cached mode we are just gonna clean those up immediately.
----
While I haven't been able to reproduce, I'm concerned that the race condition we are chasing is based on the assumption that we can keep inode numbers and hence qid.paths unique and synchronized between server and client. If the underlying file system (or server) is reusing qid.paths we'll get collisions and if it does it quickly we'll see lots of problems. So maybe we have to just break that assumption and keep our own inode numbers on the client -- from what I can tell there are two places we need to lookup inode by qid.path/inode_number:
a) in vfs_lookup to find existing inodes so we don't duplicate the structure unnecessarily -- in uncached mode before we were always creating new inodes which hid any underlying problems.
b) in create routines (like mknod) where we have to walk to get a fid in order to set acls. Better protocol design (by me) would have been to have the dotL creation all work like atomic create/open so we get a fid back and don't take the extra protocol steps -- but that ship has sailed for now.
For (b) we don't have to really intantiate the inode if we are going to discard it, we just need the fid to set the acl -- the current v9fs_set_create_acl takes inode as an argument so it can cache the acls in it, but in non-cached mode that inode should be discarded anyways so it feels like wasted effort.
For (a) I'm less certain of what to do -- we could go back to the deeper iget5 comparison which checks a number of other variables other than inode number (like i_generation if available or qid.vers) to match - but I feel like this is still maybe gambling with the inode number uniqueness unless i_generation is covering us, but I thought I remember something about it not being universally used by underlying file systems.
-eric
reply other threads:[~2024-04-10 15:38 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1296bceb87d9da7c58f7677260592683b3abdb3e@linux.dev \
--to=eric.vanhensbergen@linux.dev \
--cc=asmadeus@codewreck.org \
--cc=v9fs@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).