fsmonitor: use an opaque token for the last update #225
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kewillford
commented
Dec 4, 2019
kewillford
commented
Dec 4, 2019
kewillford
commented
Dec 4, 2019
kewillford
commented
Dec 4, 2019
kewillford
commented
Dec 4, 2019
kewillford
commented
Dec 4, 2019
kewillford
commented
Dec 4, 2019
kewillford
commented
Dec 4, 2019
kewillford
commented
Dec 4, 2019
kewillford
force-pushed
the
fsmonitor-use-clockid
branch
from
870e47b to
a04d76c
Compare
kewillford
force-pushed
the
fsmonitor-use-clockid
branch
from
a04d76c to
e465554
Compare
kewillford
changed the title
kewillford
requested review from
derrickstolee,
dscho,
garimasi514 and
jeffhostetler
derrickstolee
approved these changes
Dec 16, 2019
derrickstolee
left a comment
derrickstolee
left a comment
There was a problem hiding this comment.
I don't really have any complaints. This looks like a good approach.
Have you started building installers so you can test this paired with the C# layer?
dscho
reviewed
Dec 16, 2019
Member
dscho
left a comment
dscho
left a comment
There was a problem hiding this comment.
I left a couple of comments/suggestions; Hopefully you find them useful!
Also, I'd like to see the fixup! "rebased away".
kewillford
commented
Dec 17, 2019
fsmonitor.c
Outdated
There was a problem hiding this comment.
Just a thought: getnanotime() has a bunch of slop in it. For example, the first call to it within the process always gets a value ending with 000 on Windows (observed when I started trace2). And there's some other mysterious magic in there that I don't completely understand.
We might want to consider just getting the whole seconds since epoch, subtract 1 or 2, and then convert that to nanoseconds for storage in the header. And err on the side of caution.
jeffhostetler
approved these changes
Dec 17, 2019
…token Some file system monitors might not use or take a timestamp for processing and in the case of watchman could have race conditions with using a timestamp. Change the fsmonitor_last_update from being a uint64_t to a char pointer so that any arbitrary data can be stored in it. Signed-off-by: Kevin Willford <[email protected]>
kewillford
force-pushed
the
fsmonitor-use-clockid
branch
from
1b80381 to
a3b5397
Compare
kewillford
force-pushed
the
fsmonitor-use-clockid
branch
2 times, most recently
from
d2a0316 to
2ce4d82
Compare
Some file monitors like watchman will use a clock id to keep better track of what changes happen in between calls to query the monitor. Update the hook version handle the clock id so that it can be stored with the index and the dirty flags. Because the version of the fsmonitor hook will need to be updated to support a clockid from watchman, create a config value for the hook version that can be set. This will help be able to stay backwards compatible with version 1 of the hook but allow for a new version. Signed-off-by: Kevin Willford <[email protected]>
kewillford
force-pushed
the
fsmonitor-use-clockid
branch
3 times, most recently
from
f778723 to
8acf258
Compare
Need scripts used in testing fsmonitor using version 2. Signed-off-by: Kevin Willford <[email protected]>
A new config value for core.fsmonitorHookVersion was added to be able to force the version of the fsmonitor hook. A version has been added to the fsmonitor hook to be able to handle clock ids that are used with watchman. Signed-off-by: Kevin Willford <[email protected]>
kewillford
force-pushed
the
fsmonitor-use-clockid
branch
from
8acf258 to
49a3ae1
Compare
derrickstolee
added a commit
to microsoft/scalar
that referenced
this pull request
Jan 22, 2020
Resolves #291. Mostly a code move from `CloneVerb` to `ConfigStep`. We lose some output during clone, like "Watchman configured!" as we don't have access to `Output` during the `ConfigStep`. The `CloneVerb` runs the `ConfigStep` directly, but so does the `RegisterStep` and background maintenance. There is a benefit, though: we will try configuring Watchman on every upgrade and once a day. If a user installs Watchman after cloning, then we will configure it automatically. One remaining thing to think about: we are currently _copying_ the existing hook template from the hooks directory. As this changes (see microsoft/git#225 and gitgitgadget/git#510), we will need a different way to populate the hook. Perhaps we should store the hook contents in the Scalar codebase to avoid using a stale (or malicious) hook. Cc @kewillford for thoughts on this.
|
I'm thrilled that this is in v2.26.0-rc0, so will be in our fork as part of vfs-2.26.0. When I get installers working for #251, then we can test microsoft/scalar#255 with that build. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Applications that are used for monitoring the file system might not use a timestamp for querying. Watchman for example uses a clock id to remove race conditions that come from using a timestamp.
Switch to using an opaque token in the fsmonitor code so that the clock id from watchman or any other token from a file monitor can be stored.
Update the watchman scripts to get the clock id and send that back to the caller. This is sent back as the first NUL terminated entry in the output stream.
Still want to clean up the watchman scripts because it is not very well organized or understandable for me at least.