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.
