Re: [PATCH/WIP v2 00/14] inotify support

[Thomas Rast <tr@thomasrast.ch>]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>   read-cache: save trailing sha-1
>   read-cache: new extension to mark what file is watched
>   read-cache: connect to file watcher
>   read-cache: ask file watcher to watch files
>   read-cache: put some limits on file watching
>   read-cache: get modified file list from file watcher
>   read-cache: add config to start file watcher automatically
>   read-cache: add GIT_TEST_FORCE_WATCHER for testing
>   file-watcher: add --shutdown and --log options
>   file-watcher: automatically quit
>   file-watcher: support inotify
>   file-watcher: exit when cwd is gone
>   pkt-line.c: increase buffer size to 8192
>   t1301: exclude sockets from file permission check

I never got the last three patches, did you send them?

Also, this doesn't cleanly apply anywhere at my end.  Can you push it
somewhere for easier experimentation?

> This is getting in better shape. Still wondering if the design is
> right, so documentation, tests and some error cases are still
> neglected. I have not addressed Jonathan's and Jeff's comments in this
> reroll, but I haven't forgotten them yet. The test suite seems to be
> fine when file-watcher is forced on with GIT_TEST_FORCE_WATCHER set..

I tried to figure out whether there were any corners you are painting it
into, but doing so without a clear overview of the protocol makes my
head spin.

So here's what I gather from the patches (I would have started from the
final result, but see above).

The slow path, before the watcher is ready, works like:

  spawn watcher
  send "hello $index_sha1"
  receive "hello "
  mark most files CE_WATCHED
  send "watch $paths" for each CE_WATCHED
    the paths are actually a pkt-line-encoded series of paths
    get back "fine $n" (n = number of watches processed)
  do work as usual (including lstat())

The fast path:

  load CE_WATCHED from index
  send "hello $index_sha1"
  receive "hello $index_sha1"
  send "status"
    get back "new $path" for each changed file
    again aggregate
  mark files that are "new" as ~CE_WATCHED
  files that are CE_WATCHED can skip their lstat(), since they are unchanged

On index writing (both paths):

  save CE_WATCHED in index extension
  send "bye $index_sha1"
  send "forget $index_sha1 $path" for every path that is known to be changed

So I think the watcher protocol can be specified roughly as:

  Definitions: The watcher is a separate process that maintains a set of
  _watched paths_.  Git uses the commands below to add or remove paths
  from this set.

  In addition it maintains a set of _changed paths_, which is a subset
  of the watched paths.  Git occasionally queries the changed paths.  If
  at any time after a path P was added to the "watched" set, P has or
  may have changed, it MUST be added to the "changed" set.

  Note that a path being "unchanged" under this definition does NOT mean
  that it is unchanged in the index as per `git diff`.  It may have been
  changed before the watch was issued!  Therefore, Git MUST test whether
  the path started out unchanged using lstat() AFTER it was added to the
  "watched" set.

  Handshake::
    On connecting, git sends "hello <magic>".  <magic> is an opaque
    string that identifies the state of the index.  The watcher MUST
    respond with "hello <previous_magic>", where previous_magic is
    obtained from the "bye" command (below).  If <magic> !=
    <previous_magic>, the watcher MUST reset state for this repository.

  Watch::
    Git sends "watch <pathlist>" where the <pathlist> consists of a
    series of 4-digit lengths and literal pathnames (as in the pkt-line
    format) for each path it is interested in.  The watcher MUST respond
    with "fine <n>" after processing a message that contained <n> paths.
    The watcher MUST add each path to the "watched" set and remove it
    from the "changed" set (no error if it was already watched, or not
    changed).

  Status::
    Git sends "status".  The watcher MUST respond with its current
    "changed" set.  This uses the format "new <pathlist>", with
    <pathlist> formatted as for the "watch" command.

  Bye::
    Git sends "bye <magic>" to indicate it has finished writing the
    index.  The watcher must store <magic> so as to use it as the
    <previous_magic> in a "hello" response.

  Forget::
    Git sends "forget <pathlist>", with the <pathlist> formatted as for
    the "watch" command.  The watcher SHOULD remove each path in
    <pathlist> from its watched set.

Did I get that approximately straight?  Perhaps you can fix up as needed
and then turn it into the documentation for the protocol.

There are several points about this that I don't like:

* What does the datagram socket buy us?  You already do a lot of
  pkt-line work for the really big messages, so why not just use
  pkt-line everywhere and a streaming socket?

* The handshake should have room for capabilities, so that we can extend
  the protocol.

* The hello/hello handshake is confusing, and probably would allow you
  to point two watchers at each other without them noticing.  Make it
  hello/ready, or some other unambiguous choice.

* I took some liberty and tried to fully define the transitions between
  the sets.  So even though I'm not sure this is currently handled, I
  made it well-defined to issue "watch" for a path that is in the
  changed set.

* "bye" is confusing, because in practice git issues "forget"s after
  "bye".  The best I can come up with is "setstate", I'm sure you have
  better ideas.

There's also the problem of ordering guarantees between the socket and
inotify.  I haven't found any, so I would conservatively assume that the
socket messages may in fact arrive before inotify, which is a race in
the current code.  E.g., in the sequence 'touch foo; git status' the
daemon may see

  socket                    inotify                  
  < hello...
  < status
  > new <empty list>
                            touch foo

I think a clever way to handle this would be to add a new command:

  Wait::
    This command serves synchronization.  Git creates a file of its
    choice in $GIT_DIR/watch (say, `.git/watch/wait.<random>`).  Then it
    sends "wait <path>".  The watcher MUST block until it has processed
    all change notifications up to and including <path>.

This assumes the FS notification API to be ordered, which appears to be
the case for inotify; from inotify(7):

  The events returned by reading from an inotify file descriptor form an
  ordered queue.  Thus, for example, it is guaranteed that when renaming
  from one directory to another, events will be produced in the correct
  order on the inotify file descriptor.

As a corollary, from watching .git/watch you get free notification of
'rm .git/watch/socket' as a termination signal.

We also need to think about how other OS's APIs work with this.  From
what I've heard about the Windows API, it should work well, so that's
great.  I suspect the *bsd/darwin API corresponding to inotify won't be
too different, but it's better to work this out now.

> Thomas, you were a proponent of per-user daemon last time. I agree
> that is a better solution when you need to support submodules. So if
> you have time, have a look and see if anything I did may prevent
> per-user daemon changes later (hint, I have a few unfriendly exit() in
> file-watcher.c). You also worked with inotify before maybe you can
> help spot some mishandling too as I'm totally new to inotify.

As you note, the protocol for "help, I don't know what's true any more"
(i.e., it got an inotify buffer overflow event) needs to be a special
"reset" message, not exit().

The per-repo approach does require keeping open an extra FD per repo
though, regardless of whether the daemon is actually for all of the
user's repos.  The linuxen I run as desktop systems usually default
ulimit -n to 1024, so that's not a massive restriction, but still.

As a reminder to other reviewers, the reason I wanted a per-user watcher
is that /proc/sys/fs/inotify/max_user_instances defaults to 128 on my
systems.  We need one inotify FD per watcher process, and given that a
full android tree had something on the order of 300 repos last I looked,
that just won't fly.

As far as inotify corner-cases go, the only one I'm aware of is
directory renames.  I suspect we'll have to watch directories all the
way up to the repository root to reliably detect when this happens.  Not
sure how to best handle this.  Perhaps we should declare Git completely
agnostic wrt such issues, and behind the scenes issue all watches up to
the root even if we don't need them for anything other than directory
renames.


Ok, that's probably a confused sum of rambles.  Let me know if you can
make any sense of it.

-- 
Thomas Rast
tr@thomasrast.ch