| Message ID | 20181202113826.32133-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fanotify: add support for more event types | expand |
Hi, 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. Honza
On Fri, Jan 4, 2019 at 1:00 PM Jan Kara <jack@suse.cz> wrote: > > Hi, > > 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. Thanks, Amir.
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... Honza
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? Thanks, Amir.
On Wed, Jan 9, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> 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? > 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.
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
On Thu 10-01-19 09:49:24, Amir Goldstein wrote: > On Wed, Jan 9, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> 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? > > > > 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. OK, thanks! > 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). I see. So how about adding ISDIR flag properly to all events and just mask it out in inotify_handle_event() for bug-to-bug compatibility? I think we can start sending ISDIR flag properly for fsnotify_link_count() from because that is unlikely to surprise anybody given other ATTRIB events can have it. Because the fact that ISDIR flag is missing looks like a bug to me. Generally the ISDIR flag handling might deserve a larger cleanup but I don't want to mix it into your series as that's big enough as is. Honza
On Thu, Jan 10, 2019 at 11:22 AM Jan Kara <jack@suse.cz> wrote: > > On Thu 10-01-19 09:49:24, Amir Goldstein wrote: > > On Wed, Jan 9, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> 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? > > > > > > > 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. > > OK, thanks! > > > 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). > > I see. So how about adding ISDIR flag properly to all events and just mask > it out in inotify_handle_event() for bug-to-bug compatibility? I think we > can start sending ISDIR flag properly for fsnotify_link_count() from because > that is unlikely to surprise anybody given other ATTRIB events can have it. > Because the fact that ISDIR flag is missing looks like a bug to me. > Agreed. I will work this into the series and post v5. TBH, I think that the ATTRIB event just before renaming over an empty dir is grossly uninteresting. Not to mention that there is no such event on rmdir() nor on unlink(), even if nlink is decremented to non zero. So I would rather if we just removed the fsnotify_link_count(target) call altogether or add the missing calls, but I won't do that now. Thanks, Amir.
> 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... Yes there are LTP tests you can use. I'll use this opportunity to dump the content of my LTP dev branch and recap the WIP (mainly by Matthew) on fanotify LTP tests: * 9eb01c6b4 - (HEAD -> fanotify_dirent, github/fanotify_dirent) syscalls/fanotify102: new test for dirent events * 2218e949c - (github/fanotify_fid, fanotify_fid) syscalls/fanotify14: new test to validate FAN_REPORT_FID interface return values * b7ad5c4fe - syscalls/fanotify13: new test to verify FAN_REPORT_FID functionality * 10abf45c1 - syscalls/fanotify01: add FAN_REPORT_FID test cases * 9185a0aa1 - (github/fanotify_exec, fanotify_exec) syscalls/fanotify10: increase test coverage to support FAN_OPEN_EXEC mask * cd7ab20d5 - syscalls/fanotify12: add new test to support new FAN_OPEN_EXEC event mask * 56ba122c0 - syscalls/fanotify03: add FAN_OPEN_EXEC_PERM tcase support * f596b4a5d - syscalls/fanotify03: included execve() to generate_events() to increase test coverage * f23d990ad - syscalls/fanotify03: defined additional tcase members to support more tcase control * 7846bbbed - syscalls/fanotify03: add test for FAN_MARK_FILESYSTEM permission events fanotify_exec branch has Matthew's tests for FAN_OPEN_EXEC. Following request from LTP maintainer, he will post them around v5.0-rc3 time. fanotify_fid branch has 2 new FAN_REPORT_FID tests by Matthew, which verify the functionality of reporting fid. fanotify_dirent branch has 1 WIP test to verify new events functionality. its temporary name fanotify102 goes after the test inotify02 which verifies similar functionality with inotify. Matthew is going to work this test into shape and add verification that the reported fid in dirent events (and _SELF events) is correct. Thanks, Amir.
On Thu 10-01-19 11:50:30, Amir Goldstein wrote: > On Thu, Jan 10, 2019 at 11:22 AM Jan Kara <jack@suse.cz> wrote: > > > > On Thu 10-01-19 09:49:24, Amir Goldstein wrote: > > > On Wed, Jan 9, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> 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? > > > > > > > > > > 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. > > > > OK, thanks! > > > > > 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). > > > > I see. So how about adding ISDIR flag properly to all events and just mask > > it out in inotify_handle_event() for bug-to-bug compatibility? I think we > > can start sending ISDIR flag properly for fsnotify_link_count() from because > > that is unlikely to surprise anybody given other ATTRIB events can have it. > > Because the fact that ISDIR flag is missing looks like a bug to me. > > > > Agreed. I will work this into the series and post v5. > > TBH, I think that the ATTRIB event just before renaming over an empty > dir is grossly uninteresting. Not to mention that there is no such event > on rmdir() nor on unlink(), even if nlink is decremented to non zero. But you won't get other event telling you that some dir entry got actually overwritten by the rename, will you? Note that the inode need not get deleted if it has other links to it. Actually sending fsnotify_nameremove() would look like a more sensible choice but that ship has already sailed... > So I would rather if we just removed the fsnotify_link_count(target) > call altogether or add the missing calls, but I won't do that now. I would not remove existing calls - possibility of breaking compatibility just because we don't find the calls very useful simply is not worth it. Adding new calls is possible but let's leave that for later once things settle down a bit. Honza
> > TBH, I think that the ATTRIB event just before renaming over an empty > > dir is grossly uninteresting. Not to mention that there is no such event > > on rmdir() nor on unlink(), even if nlink is decremented to non zero. > > But you won't get other event telling you that some dir entry got actually > overwritten by the rename, will you? Note that the inode need not get > deleted if it has other links to it. Actually sending fsnotify_nameremove() > would look like a more sensible choice but that ship has already sailed... > You get a MOVED_TO event on the modified dir. So long as we do not report dirent filenames, that is just as informative as getting ATTRIB event on the modified dir (MOVED_TO has greater chance to imply overwrite than ATTRIB does). The inode of an overwritten empty directory will produce DELETE_SELF event, because empty directory cannot have links to it. I am not saying ATTRIB event for decremented nlink on non dir doesn't make sense. I am saying ATTRIB|ISDIR event in that case doesn't make a whole lot of sense. Anyway I am not changing this now. Thanks, Amir.
Jan, 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. Thanks, Amir. Changes since v3: - Re-organize structs fsnotify_event and fanotify_event to pack the fields better on 64bit arch (NEW patch "move mask out of...") - Re-arrange building event fid in two main patches: "encode fid..." to in-kernel event struct and "copy event fid..." to uapi format - Avoid exporting statfs_by_dentry() and add 2 NEW cleanup patches to use new helper vfs_get_fsid() - Replace dodgy code using pointer bit 0 with proper type field in fanotify_event to indicate if path or fid info was stored - Store up to 12 bytes (on 32bit) or 16 bytes (on 64bit) of embedded file handle in fanotify_event to refrain from small memory allocations - Re-arrange code in copy_event_to_user(), so adding copy of fid to user is less convoluted - Replace convoluted code to verify all marks for a reported event have the same cached fsid with simpler code than uses any cached fsid Changes since v2: - Discard FSNOTIFY_EVENT_DENTRY data type changes - Cache fsid in connector instead of calling vfs_statfs() on every event - Deny setting fid watch on filesystem with no fsid (tmpfs) - Deny setting fid watch on filesystem with non root fsid (btrfs subvol) - Report FAN_ONDIR for all event types with FAN_REPORT_FID [1] https://github.com/amir73il/linux/commits/fanotify_dirent [2] https://github.com/amir73il/inotify-tools/commits/fanotify_dirent [3] https://github.com/amir73il/linux/commits/fanotify_fid-v3 [4] https://github.com/amir73il/linux/commits/fanotify_fid-v4 Amir Goldstein (15): fsnotify: annotate directory entry modification events fsnotify: send all event types to super block marks fsnotify: move mask out of struct fsnotify_event fanotify: rename struct fanotify_{,perm_}event_info fanotify: open code fill_event_metadata() fanotify: encode file identifier for FAN_REPORT_FID fanotify: copy event fid info to user fanotify: enable FAN_REPORT_FID init flag fanotify: cache fsid in fsnotify_mark_connector vfs: add vfs_get_fsid() helper fanotify: use vfs_get_fsid() helper instead of vfs_statfs() fanotify: check FS_ISDIR flag instead of d_is_dir() fanotify: support events with data type FSNOTIFY_EVENT_INODE fanotify: add support for create/attrib/move/delete events fanotify: report FAN_ONDIR to listener with FAN_REPORT_FID fs/notify/fanotify/fanotify.c | 228 +++++++++++++++++---- fs/notify/fanotify/fanotify.h | 109 ++++++++-- fs/notify/fanotify/fanotify_user.c | 284 +++++++++++++++++++-------- fs/notify/fsnotify.c | 15 +- fs/notify/inotify/inotify.h | 1 + fs/notify/inotify/inotify_fsnotify.c | 9 +- fs/notify/inotify/inotify_user.c | 5 +- fs/notify/mark.c | 44 ++++- fs/notify/notification.c | 22 +-- fs/statfs.c | 14 ++ include/linux/fanotify.h | 33 +++- include/linux/fsnotify.h | 45 ++++- include/linux/fsnotify_backend.h | 70 ++++--- include/linux/statfs.h | 3 + include/uapi/linux/fanotify.h | 29 +++ 15 files changed, 701 insertions(+), 210 deletions(-)