Re: [RFC 1/2] fanotify: new event FAN_MODIFY_DIR

[Amir Goldstein]

On Wed, Mar 15, 2017 at 4:05 PM, Jan Kara <jack@xxxxxxx> wrote:
> 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
>

Really? How come?

fsnotify_move() inside vfs_rename() is serialized with lock_rename()

I should use this opportunity to note that fanotify event does not have
a cookie field and that I did not add one to my implementation, so that
is a problem.

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

The addition of filename info is completely optional and the relevant patch
("fanotify: pass filename info for filename events")
Can be yanked out of the series and pushed back to the 'maybe' pile.

Amir.