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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jan 9, 2019 at 5:34 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Wed, Jan 9, 2019 at 4:02 PM Jan Kara <jack@xxxxxxx> wrote:
> >
> > On Mon 07-01-19 09:46:37, Amir Goldstein wrote:
> > > On Fri, Jan 4, 2019 at 1:00 PM Jan Kara <jack@xxxxxxx> 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.

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).

The only sane solution I can think of is something along:

static u32 fanotify_group_event_mask(...
...
        if (data_type == FSNOTIFY_EVENT_INODE &&
            !(event_mask & ALL_FSNOTIFY_DIRENT_EVENTS) &&
            S_ISDIR(((struct inode *)data)->i_mode)
            event_mask |= FS_ISDIR;

Thoughts?

Thanks,
Amir.



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux