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

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

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?

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.

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.