Re: [PATCH v4 00/15] fanotify: add support for more event types

[Jan Kara <jack@suse.cz>]

On Thu 10-01-19 11:50:30, Amir Goldstein wrote:
> On Thu, Jan 10, 2019 at 11:22 AM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 10-01-19 09:49:24, Amir Goldstein wrote:
> > > On Wed, Jan 9, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > On Wed, Jan 9, 2019 at 4:02 PM Jan Kara <jack@suse.cz> wrote:
> > > > >
> > > > > On Mon 07-01-19 09:46:37, Amir Goldstein wrote:
> > > > > > On Fri, Jan 4, 2019 at 1:00 PM Jan Kara <jack@suse.cz> wrote:
> > > > > > > On Sun 02-12-18 13:38:11, Amir Goldstein wrote:
> > > > > > > > This is the 4th revision of patch series to add support for filesystem
> > > > > > > > change monitoring to fanotify.
> > > > > > > > It incorporates the changes you requested in review of v3 FAN_REPORT_FID
> > > > > > > > patches.
> > > > > > > > The complete work is available on fanotify_dirent branch [1] on my tree.
> > > > > > > >
> > > > > > > > The combined functionality of FAN_MARK_FILESYSTEM, FAN_REPORT_FID and
> > > > > > > > dirent modification events is demonstrated with a prototype of global
> > > > > > > > filesystem monitor based on inotify-tools [2].
> > > > > > > >
> > > > > > > > In your review of v3 patched you only got as far as patch v3 9/13.
> > > > > > > > Because this patch marks the end of the FAN_REPORT_FID sub series,
> > > > > > > > I found it best to re-post the entire series with the changes you
> > > > > > > > requested thus far. For convenience of review, I pushed branches
> > > > > > > > fanotify_fid-v3 [3] and fanotify_fid-v4 [4] with the work you
> > > > > > > > reviewed so far and its re-worked version.
> > > > > > > >
> > > > > > > > One thing that we discussed and I did NOT do is move struct file_handle
> > > > > > > > to uapi headers. This got complicated due to existing definitions in
> > > > > > > > glibc header files and I realized we could do without it.
> > > > > > > >
> > > > > > > > I have added the vfs_get_fsid() helper as you requested, but since it
> > > > > > > > wasn't required by the patch set, I added it as two new cleanup patches
> > > > > > > > at the end of the FAN_REPORT_FID series, so you will be able to stage
> > > > > > > > the feature with or without the VFS change.
> > > > > > >
> > > > > > > So overall the series looks very good. I've had only some smaller comments
> > > > > > > / disagreements. So once we settle those please resend the series and I'll
> > > > > > > pick it up to my tree.
> > > > > > >
> > > > > >
> > > > > > Jan,
> > > > > >
> > > > > > I addressed all your comments, rebased on v5.0-rc1 and pushed to:
> > > > > > https://github.com/amir73il/linux/commits/fanotify_dirent
> > > > > >
> > > > > > When I get an ACK from you on the FAN_ONDIR issue, I will post the v5 series.
> > > > > > The gist is that FAN_ONDIR an input flag is required to produce mkdir/rmdir
> > > > > > events (as you suggested) and FAN_ONDIR is reported as output flag
> > > > > > for FAN_REPORT_FID group for all events that occur on directories
> > > > > > and never reported to non FAN_REPORT_FID group.
> > > > >
> > > > > Agreed. But thinking about it a bit more and looking at your patch on
> > > > > Github I think your solution doesn't quite work in presence of event
> > > > > merging since an event with FAN_ONDIR can get merged with event without
> > > > > FAN_ONDIR, can't it? That seems to be generally a problem with
> > > > > should_merge() for direntry events that it doesn't take ISDIR flag into
> > > > > account...
> > > > >
> > > >
> > > > Right... good catch! will fix should_merge().
> > > > Do you want me to re-post the series or just push to Github?
> > > >
> > >
> > > In the mean while, I pushed the fix to branch fanotify_dirent.
> > > A branch with the same name in LTP tree has a WIP dirent events
> > > test that includes the test for expected merge behavior.
> > > Matthew intends to work this test into shape.
> >
> > OK, thanks!
> >
> > > I did notice another wrinkle.
> > > There is a group of events that this patch set adds support to
> > > that are not dirent events, namely: DELETE_SELF, MOVE_SELF
> > > and ATTRIB.
> > >
> > > The first two never carry the ISDIR flag (in inotify as well).
> > > ATTRIB does usually carry the ISDIR flag, except for the odd case
> > > of fsnotify_link_count(target) being called on rename over an empty
> > > directory.
> > >
> > > If we add FS_ISDIR in those hooks, we risk breaking inotify apps.
> > > If we don't do anything, we will get inconsistent behavior w.r.t
> > > FAN_ONDIR (especially relevant for XXX_SELF).
> >
> > I see. So how about adding ISDIR flag properly to all events and just mask
> > it out in inotify_handle_event() for bug-to-bug compatibility? I think we
> > can start sending ISDIR flag properly for fsnotify_link_count() from because
> > that is unlikely to surprise anybody given other ATTRIB events can have it.
> > Because the fact that ISDIR flag is missing looks like a bug to me.
> >
> 
> Agreed. I will work this into the series and post v5.
> 
> TBH, I think that the ATTRIB event just before renaming over an empty
> dir is grossly uninteresting. Not to mention that there is no such event
> on rmdir() nor on unlink(), even if nlink is decremented to non zero.

But you won't get other event telling you that some dir entry got actually
overwritten by the rename, will you? Note that the inode need not get
deleted if it has other links to it. Actually sending fsnotify_nameremove()
would look like a more sensible choice but that ship has already sailed...

> So I would rather if we just removed the fsnotify_link_count(target)
> call altogether or add the missing calls, but I won't do that now.

I would not remove existing calls - possibility of breaking compatibility
just because we don't find the calls very useful simply is not worth it.
Adding new calls is possible but let's leave that for later once things
settle down a bit.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR