Message-ID: <20170315140536.GI12989@quack2.suse.cz>
Date:   Wed, 15 Mar 2017 15:05:36 +0100
From:   Jan Kara <jack@...e.cz>
To:     Amir Goldstein <amir73il@...il.com>
Cc:     Filip Štědronský <r.lklm@...narg.cz>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Jan Kara <jack@...e.cz>,
        Alexander Viro <viro@...iv.linux.org.uk>
Subject: Re: [RFC 1/2] fanotify: new event FAN_MODIFY_DIR

Hello,

On Tue 14-03-17 12:11:40, Amir Goldstein wrote:
> > Add a new fanotify event, FAN_MODIFY_DIR, that is emitted whenever the
> > contents of a directory change (a directory entry is added, removed or
> > renamed). This covers all the currently missing events: rename, unlink,
> > mknod, mkdir, rmdir, etc.
> >
> > Included with the event is a file descriptor to the modified directory
> > but no further info. The userspace application is expected to rescan the
> > whole directory and update its model accordingly. This needs to be done
> > carefully to prevent race conditions. A cross-directory rename generates
> > two FAN_MODIFY_DIR events.
> >
> 
> Your approach is interesting and I am glad you shared it with us.
> I do like it and it gives me an idea, I am going to prepare a branch
> with a subset
> of my patches, so you can try them with your userspace sample program.
> 
> In comments to your patches I am going to argue that, as a matter of fact,
> you can take a small sub set of my patches and get the same functionality
> of FAN_MODIFY_DIR, with code that is *less* complex then what you propose.
> So please treat my comments as technical comments how FAN_MODIFY_DIR
> should be implemented IMO and not as an attempt to reject an opposing proposal.

Yeah, so I have yet to read both your patch sets in detail (good news is
that this is getting towards top of my TODO stack and I think I'll do this
when flying to LSF/MM this weekend). With Filip's design I like the
minimalistic approach of adding just DIR_CHANGED event with (albeit
optional) messing with names. That just seems to fit well with the fanotify
design.

> > This minimalistic approach has several advantages:
> >   * Does not require changes to the fanotify userspace API or the
> >     fsnotify in-kernel framework, apart from adding a new event.
> 
> About the argument of not having to change in-kernel framework,
> I don't think it should be a consideration at all.
> I claim that my 1st fsnotify cleanup series is an improvement of the framework
> (https://github.com/amir73il/linux/commits/fsnotify_dentry)
> regardless of additional functionality.
> So changing the in-kernel framework by adding complexity may be bad,
> but changing it by simplifying and making code more readable should not
> be an argument against the "non-minimalistic" approach.
> 
> About the change to usespace API, if you strip off the *added* functionality
> of super block watch from my patches, your proposal for user API looks
> like this:
> 
>     fanotify_mark(fan_fd, FAN_MARK_ADD|FAN_MARK_MOUNT, \
>                            FAN_MODIFY_DIR|FAN_ONDIR,
> 
> And my proposal for user API looks like this:
> 
>     fanotify_mark(fan_fd, FAN_MARK_ADD|FAN_MARK_MOUNT, \
>                            FAN_MOVE|FAN_CREATE|FAN_DELETE,
> 
> You can see why my proposal for user API is not any less minimalistic
> and it is fully compatible with existing intotify API for the same events.
> 
> I understand why it is hard to see this behind my added functionality,
> so I will post a minimalistic branch and test program as an example.
> 
> >     Especially doesn't complicate it by adding string fields.
> 
> So the string fields in my proposal are optional.
> userspace opts-in to get them by specifying the  FAN_EVENT_INFO_NAME flag:
> 
>     fanotify_init(FAN_CLOEXEC | FAN_CLASS_NOTIF |
>                        FAN_EVENT_INFO_PARENT | FAN_EVENT_INFO_NAME,
> 
> If you don't specify FAN_EVENT_INFO_NAME, you can get filename events
> FAN_MOVE|FAN_CREATE|FAN_DELETE without the name.

So when thinking about this again, I'm again concerned that the names need
not tell userspace what it thinks. I know we already discussed this but
in our last discussion I think I forgot to point out that inotify directory
events (and fanotify would be the same AFAICT) may come out of order compared
to real filesystem changes. E.g. sequence:

Task 1		Task 2

mv a b
		mv b c

may come out of inotify as:

IN_MOVED_FROM "b" COOKIE 1
IN_MOVED_TO   "c" COOKIE 1
IN_MOVED_FROM "a" COOKIE 2
IN_MOVED_TO   "b" COOKIE 2

and if user program just blindly belives this sequence its internal
representation of the filesystem will be different from the real state of
the filesystem.

So for now I'm more inclined towards the trivial approach (possibly using
your patches and stripping additional functionality from them). But I'll
leave final decision to when I'll be able to read everything in detail.

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

Powered by blists - more mailing lists