On Thu, Jul 16, 2020 at 10:39 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Thu, Jul 16, 2020 at 9:38 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > On Wed, Jul 15, 2020 at 8:42 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > > > On Wed, Jul 15, 2020 at 8:09 PM Jan Kara <jack@xxxxxxx> wrote: > > > > > > > > On Tue 14-07-20 14:54:44, Amir Goldstein wrote: > > > > > On Tue, Jul 14, 2020 at 1:34 PM Jan Kara <jack@xxxxxxx> wrote: > > > > > > > > > > > > On Thu 02-07-20 15:57:37, Amir Goldstein wrote: > > > > > > > Instead of calling fsnotify() twice, once with parent inode and once > > > > > > > with child inode, if event should be sent to parent inode, send it > > > > > > > with both parent and child inodes marks in object type iterator and call > > > > > > > the backend handle_event() callback only once. > > > > > > > > > > > > > > The parent inode is assigned to the standard "inode" iterator type and > > > > > > > the child inode is assigned to the special "child" iterator type. > > > > > > > > > > > > > > In that case, the bit FS_EVENT_ON_CHILD will be set in the event mask, > > > > > > > the dir argment to handle_event will be the parent inode, the file_name > > > > > > > argument to handle_event is non NULL and refers to the name of the child > > > > > > > and the child inode can be accessed with fsnotify_data_inode(). > > > > > > > > > > > > > > This will allow fanotify to make decisions based on child or parent's > > > > > > > ignored mask. For example, when a parent is interested in a specific > > > > > > > event on its children, but a specific child wishes to ignore this event, > > > > > > > the event will not be reported. This is not what happens with current > > > > > > > code, but according to man page, it is the expected behavior. > > > > > > > > > > > > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > > > > > > > > > > > I like the direction where this is going. But can't we push it even a bit > > > > > > further? I like the fact that we now have "one fs event" -> "one fsnotify() > > > > > > call". Ideally I'd like to get rid of FS_EVENT_ON_CHILD in the event mask > > > > > > because it's purpose seems very weak now and it complicates code (and now > > > > > > > > > > Can you give an example where it complicates the code? > > > > > Don't confuse this with the code in fanotify_user.c that subscribes for > > > > > events on child/with name. > > > > > > > > I refer mostly to the stuff like: > > > > > > > > /* An event "on child" is not intended for a mount/sb mark */ > > > > if (mask & FS_EVENT_ON_CHILD) > > > > ... > > > > > > > > I need to explain something that was not an obvious decision for me. > > > > When sending the same event on two inodes marks I considered a few options: > > > > 1. TYPE_INODE is the mark on the object referred to in data > > TYPE_PARENT is the mark on the parent if event is sent to a watching > > parent or to sb/mnt/child with parent/name info > > 2. TYPE_CHILD is the mark on the object referred to in data > > TYPE_INODE is the mark on the fsnotify to_tell inode if not same as data > > 3. TYPE_INODE is the mark on the fsnotify to_tell inode > > TYPE_CHILD is the mark on the object referred to in data if it is > > not to_tell > > > > The first option with TYPE_PARENT would require changing audit > > and dnotify to look at TYPE_PARENT mark in addition to TYPE_INODE > > mark, so it adds more friction and I ruled it out. > > > > I think you had option #2 in mind when reading the code, but I went > > for option #3. > > There is a minor difference between them related to how we deal with the case > > that the parent is watching and the case that only the child is watching. > > > > If the parent is not watching (and child/sb/mnt not interested in name) we do > > not snapshot the name and do not set the ON_CHILD flag in the mask. > > In that case, should we add the child mark as TYPE_INODE or TYPE_CHILD? > > > > I chose TYPE_INODE because this meant I did not have to change audit/dnotify > > for that case. I didn't even care to look if they needed to be changed or not, > > just wanted to keep things as they were. > > > > Looking now, I see that dnotify would have needed to check TYPE_CHILD to > > get FS_ATTRIB event on self. > > > > It looks like audit would not have needed to change because although they set > > FS_EVENT_ON_CHILD in mask, none of the events they care about are > > "possible on child": > > #define AUDIT_FS_WATCH (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\ > > FS_MOVE_SELF | FS_EVENT_ON_CHILD | FS_UNMOUNT) > > #define AUDIT_FS_EVENTS (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\ > > FS_MOVE_SELF | FS_EVENT_ON_CHILD) > > > > Having written that decision process down made me realize there is a bug in > > my unified inotify event handler implementation - it does not clear > > FS_EVENT_ON_CHILD when reporting without name. > > > > It is interesting to note that the result of sending FS_OPEN only to a watching > > child to inotify_handle_event() is the same for design choices #2 and #3 above. > > But the bug fix of clearing FS_EVENT_ON_CHILD when reporting without name > > would look different depending on said choice. > > > > Since I had to change inotify handler anyway, I prefer to stick with my choice > > and fix inotify handler using goto notify_child which is a bit uglier, > > instead of > > having to adapt dnotify to choice #2. > > > > It turns out it's the other way around. > inotify handler has no bug (FS_EVENT_ON_CHILD is not exposed to the user) > just a confusing comment, so I will fix that. > But dnotify does have a bug - it also needs to be taught about the unified event > so that DN_ATTRIB event can be reported twice on both parent dir and child > subdir if both are watching. > Alas, we have no test coverage for dnotify in LTP... FYI I verified this dnotify regression and fix manually after adding DN_ATTRIB to tools/testing/selftests/filesystems/dnotify_test.c: $ cd dir/ $ dnotify_test & $ cd subdir/ $ dnotify_test & $ chmod 777 . Got event on fd=3 Got event on fd=3 I will write an LTP test to cover this and see if we have similar tests for inotify and fanotify. Thanks, Amir.
