[systemd-devel] Using *.path units without races?

[Uwe Geuder]

Hi!

On Fri, 20 Mar 2020 at 00:31, Michael Chapman  wrote:
>
> On Fri, 20 Mar 2020, Uwe Geuder wrote:
> [...]
> > The manual page is not very specific about how that is supposed to work
> > IMHO, but I could imagine the following distinction:
> >
> > PathExists=, PathExistsGlob=, and DirectoryNotEmpty= are absolute
> > predicates. When setting the path unit to waiting one can just check
> > whether they are true or not. (After arming inotify of course.) With
> > your patch my limited testing was successful.
> >
> > However, PathChanged= and PathModified= are relative predicates. You
> > cannot just check whether they are true or not. Wouldn't the correct
> > implementation
> >
> > 1. need to store the applicable timestamp of the path involved when the
> >    path unit is set to started and
> >
> > 2. when the path unit is set to waiting again it would need to compare
> >    the stored timestamp with the current timestamp (again after arming
> >    inotify) to catch modifications that happened while the unit was
> >    running/inotify not armed
> >
> > I don't think I see the timestamps stored at all. So how was this
> > supposed to work? Was the intended semantics different?
>
> PathChanged= and PathModified= each map down to a set of inotify
> events. It's the kernel's inotify system that determines whether the
> file changed or modified, not systemd.

My understanding is that since
https://github.com/systemd/systemd/pull/13509/commits/06582e42de65a61d0238a18720a12b6353edb7cd
there are 2 states

1. While the path unit is waiting and the triggered service unit is dead
its indead all inotify's business. When a change happens the kernel will
notify systemd.

2. However, while the triggered service unit is running also the path
unit is running and the inotify fd is closed. So the kernel will not
report any changes to systemd at all during that time.

When thereafter the path unit enters waiting again it will add the
required inotify watches again. However, to find out whether anything
has changed, i. e. been missed while inotify was not watching, it would
need to do it's own checking in user space. In the case of "absolute"
predicates it's a straight-forward check whether such path exists and
obviously that's what the code does with your patch. However, for the
"relative" predicates I don't see another way than comparing timestamps.

How else could it fulfill the promise of the man page of "checking
immediately again". Inotify will report only potential future changes,
not report anything that has already happened while it was inactive /
the path unit running

> When a service unit triggered by a path unit terminates (regardless
> whether it exited successfully or failed), monitored paths are checked
> immediately again, and the service accordingly restarted instantly

I did a quick

$ strace -e inotify_init,inotify_init1,inotify_add_watch,inotify_rm_watch,close -p <pid-of-user-service-mananger>

and got something like this

...
inotify_init1(IN_NONBLOCK|IN_CLOEXEC)   = 12
inotify_add_watch(12, "/", IN_ATTRIB|IN_MOVED_TO|IN_CREATE|IN_DELETE_SELF|IN_MOVE_SELF) = 1
inotify_add_watch(12, "/tmp", IN_ATTRIB|IN_MOVED_TO|IN_CREATE|IN_DELETE_SELF|IN_MOVE_SELF) = 2
inotify_add_watch(12, "/", IN_MOVE_SELF) = 1
inotify_add_watch(12, "/tmp/systemd-path-test", IN_MODIFY|IN_ATTRIB|IN_CLOSE_WRITE|IN_MOVED_FROM|IN_MOVED_TO|IN_CREATE|IN_DELETE|IN_DELETE_SELF|IN_MOVE_SELF) = 3
inotify_add_watch(12, "/tmp", IN_MOVE_SELF) = 2
close(12)
...

for every "wait period" of the path unit. We see that the inotify fd
gets closed when the units enter running state and there is a new
inotify_init1() when the path unit enters watching again.

Instead of timestamps the code does have a

  bool previous_exists

For the "absolute" predicates that could "nearly" be used to implement
different semantics. If the path has triggered once (previous_exists ==
true), don't trigger again on any extra changes unless the predicate was
false (previous_exists == false) in the meantime. But

a.) I don't see any such behaviour really described in the man page

b.) the code doesn't seem to work like that at all, and

c.) it could still not work if the predicate got false for a short
    period while the notify is not watching, because nobody could update
    the boolean appropriately. So even that variant would only "nearly"
    work.

And for the "relative" predicates such a boolean would never be
enough. Modification can have happened in the meantime even if the file
existed before.

So what's the purpose of the boolean?

Regards,

Uwe

Uwe Geuder
Neuro Event Labs Oy
Tampere, Finland
uwe.gxuder at neuroeventlabs.com (bot check: fix 1 obvious typo)