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

From: Jan Kara <jack@suse.cz>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>,
	Matthew Bobrowski <mbobrowski@mbobrowski.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	linux-api@vger.kernel.org
Subject: Re: [PATCH v4 00/15] fanotify: add support for more event types
Date: Thu, 10 Jan 2019 09:53:30 +0100	[thread overview]
Message-ID: <20190110085330.GA15790@quack2.suse.cz> (raw)
In-Reply-To: <CAOQ4uxg91KFoQ_EQCDNPkcJorbue1fjr=EL-_0YgEVXtR6PK8g@mail.gmail.com>

On Wed 09-01-19 17:34:36, Amir Goldstein 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?

Please post the series now since there were quite some changes (although
smaller ones) since the last posting. I'll go through it and push it to my
tree. Do we already have LTP tests for the new functionality? I'd like to
do some testing before pushing things out...

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