| Message ID | 20200502162744.9589-3-amir73il@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | fanotify ltp tests for v5.7-rc1 | expand |
On Sat 02-05-20 19:27:42, Amir Goldstein wrote: > Test reporting events with fid also with recusrive inode marks: > - Test events "on self" (FAN_DELETE_SELF) on file and dir > - Test events "on child" (FAN_MODIFY) on file > > With recursive inode marks, verify that the FAN_MODIFY event reported > to parent "on child" is merged with the FAN_MODIFY event reported to > child. > > The new test case is a regression test for commit f367a62a7cad: > > fanotify: merge duplicate events on parent and child The test looks OK but do we want a test for this? I mean: A test like this seems to imply we promise to merge identical events. Although that is a good general guideline, I consider it rather an optimization that may or may not happen but userspace should not rely on it. Thoughts? Honza > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > Reviewed-by: Petr Vorel <pvorel@suse.cz> > Reviewed-by: Cyril Hrubis <chrubis@suse.cz> > Reviewed-by: Matthew Bobrowski <mbobrowski@mbobrowski.org> > --- > .../kernel/syscalls/fanotify/fanotify15.c | 81 +++++++++++++++++-- > 1 file changed, 73 insertions(+), 8 deletions(-) > > diff --git a/testcases/kernel/syscalls/fanotify/fanotify15.c b/testcases/kernel/syscalls/fanotify/fanotify15.c > index 2e860edb2..a9ed2ec81 100644 > --- a/testcases/kernel/syscalls/fanotify/fanotify15.c > +++ b/testcases/kernel/syscalls/fanotify/fanotify15.c > @@ -9,6 +9,10 @@ > * Test file that has been purposely designed to verify > * FAN_REPORT_FID functionality while using newly defined dirent > * events. > + * > + * Test case #1 is a regression test for commit f367a62a7cad: > + * > + * fanotify: merge duplicate events on parent and child > */ > #define _GNU_SOURCE > #include "config.h" > @@ -53,28 +57,49 @@ static int fanotify_fd; > static char events_buf[EVENT_BUF_LEN]; > static struct event_t event_set[EVENT_MAX]; > > -static void do_test(void) > +static struct test_case_t { > + const char *tname; > + struct fanotify_mark_type mark; > + unsigned long mask; > +} test_cases[] = { > + { > + "FAN_REPORT_FID on filesystem including FAN_DELETE_SELF", > + INIT_FANOTIFY_MARK_TYPE(FILESYSTEM), > + FAN_DELETE_SELF, > + }, > + { > + "FAN_REPORT_FID on directory with FAN_EVENT_ON_CHILD", > + INIT_FANOTIFY_MARK_TYPE(INODE), > + FAN_EVENT_ON_CHILD, > + }, > +}; > + > +static void do_test(unsigned int number) > { > int i, fd, len, count = 0; > > struct file_handle *event_file_handle; > struct fanotify_event_metadata *metadata; > struct fanotify_event_info_fid *event_fid; > + struct test_case_t *tc = &test_cases[number]; > + struct fanotify_mark_type *mark = &tc->mark; > + > + tst_res(TINFO, "Test #%d: %s", number, tc->tname); > > - if (fanotify_mark(fanotify_fd, FAN_MARK_ADD | FAN_MARK_FILESYSTEM, > + if (fanotify_mark(fanotify_fd, FAN_MARK_ADD | mark->flag, tc->mask | > FAN_CREATE | FAN_DELETE | FAN_MOVE | > - FAN_MODIFY | FAN_DELETE_SELF | FAN_ONDIR, > + FAN_MODIFY | FAN_ONDIR, > AT_FDCWD, TEST_DIR) == -1) { > if (errno == ENODEV) > tst_brk(TCONF, > "FAN_REPORT_FID not supported on %s " > "filesystem", tst_device->fs_type); > tst_brk(TBROK | TERRNO, > - "fanotify_mark(%d, FAN_MARK_ADD | FAN_MARK_FILESYSTEM, " > + "fanotify_mark(%d, FAN_MARK_ADD | %s, " > "FAN_CREATE | FAN_DELETE | FAN_MOVE | " > - "FAN_MODIFY | FAN_DELETE_SELF | FAN_ONDIR, " > + "FAN_MODIFY | FAN_ONDIR | 0x%lx, " > "AT_FDCWD, %s) failed", > - fanotify_fd, TEST_DIR); > + fanotify_fd, mark->name, tc->mask, TEST_DIR); > } > > /* All dirent events on testdir are merged */ > @@ -87,8 +112,21 @@ static void do_test(void) > fd = SAFE_CREAT(FILE1, 0644); > SAFE_CLOSE(fd); > > + /* Recursive watch file for events "on self" */ > + if (mark->flag == FAN_MARK_INODE && > + fanotify_mark(fanotify_fd, FAN_MARK_ADD | mark->flag, > + FAN_MODIFY | FAN_DELETE_SELF, > + AT_FDCWD, FILE1) == -1) { > + tst_brk(TBROK | TERRNO, > + "fanotify_mark(%d, FAN_MARK_ADD | %s, " > + "FAN_DELETE_SELF, AT_FDCWD, %s) failed", > + fanotify_fd, mark->name, FILE1); > + } > + > /* > * Event on child file is not merged with dirent events. > + * FAN_MODIFY event reported on file mark should be merged with the > + * FAN_MODIFY event reported on parent directory watch. > */ > event_set[count].mask = FAN_MODIFY; > event_set[count].handle.handle_bytes = MAX_HANDLE_SZ; > @@ -128,6 +166,17 @@ static void do_test(void) > > SAFE_MKDIR(DIR1, 0755); > > + /* Recursive watch subdir for events "on self" */ > + if (mark->flag == FAN_MARK_INODE && > + fanotify_mark(fanotify_fd, FAN_MARK_ADD | mark->flag, > + FAN_DELETE_SELF | FAN_ONDIR, > + AT_FDCWD, DIR1) == -1) { > + tst_brk(TBROK | TERRNO, > + "fanotify_mark(%d, FAN_MARK_ADD | %s," > + "FAN_DELETE_SELF | FAN_ONDIR, AT_FDCWD, %s) failed", > + fanotify_fd, mark->name, DIR1); > + } > + > SAFE_RENAME(DIR1, DIR2); > > event_set[count].mask = FAN_ONDIR | FAN_DELETE_SELF; > @@ -141,6 +190,17 @@ static void do_test(void) > /* Read dir events from the event queue */ > len += SAFE_READ(0, fanotify_fd, events_buf + len, EVENT_BUF_LEN - len); > > + /* > + * Cleanup the mark > + */ > + if (fanotify_mark(fanotify_fd, FAN_MARK_FLUSH | mark->flag, 0, > + AT_FDCWD, TEST_DIR) < 0) { > + tst_brk(TBROK | TERRNO, > + "fanotify_mark (%d, FAN_MARK_FLUSH | %s, 0," > + "AT_FDCWD, '"TEST_DIR"') failed", > + fanotify_fd, mark->name); > + } > + > /* Process each event in buffer */ > for (i = 0, metadata = (struct fanotify_event_metadata *) events_buf; > FAN_EVENT_OK(metadata, len); i++) { > @@ -259,9 +319,14 @@ static struct tst_test test = { > .mount_device = 1, > .mntpoint = MOUNT_POINT, > .all_filesystems = 1, > - .test_all = do_test, > + .test = do_test, > + .tcnt = ARRAY_SIZE(test_cases), > .setup = do_setup, > - .cleanup = do_cleanup > + .cleanup = do_cleanup, > + .tags = (const struct tst_tag[]) { > + {"linux-git", "f367a62a7cad"}, > + {} > + } > }; > > #else > -- > 2.17.1 >
On Mon, May 4, 2020 at 11:07 AM Jan Kara <jack@suse.cz> wrote: > > On Sat 02-05-20 19:27:42, Amir Goldstein wrote: > > Test reporting events with fid also with recusrive inode marks: > > - Test events "on self" (FAN_DELETE_SELF) on file and dir > > - Test events "on child" (FAN_MODIFY) on file > > > > With recursive inode marks, verify that the FAN_MODIFY event reported > > to parent "on child" is merged with the FAN_MODIFY event reported to > > child. > > > > The new test case is a regression test for commit f367a62a7cad: > > > > fanotify: merge duplicate events on parent and child > > The test looks OK but do we want a test for this? I mean: A test like this > seems to imply we promise to merge identical events. Although that is a > good general guideline, I consider it rather an optimization that may or > may not happen but userspace should not rely on it. Thoughts? The thing is, those are not really two identical events. This is in fact the same event (fsnotify_change() hook was called once). The fact that listener process may have an inode watch, parent directory watch and a filesystem watch should not affect the number of read events. Now it's true that internally, fsnotify_dentry() emits two event flavors to parent and to victim. For inotify it even made some sense, because listener would read two different event flavors with two different formats. With fanotify (either reporting fd or fid) receiving two events makes very little sense. I agree that the fix (merging those events) is best effort and we cannot commit to merging the events, but this isolated regression test does check the best effort fix reliably and this is the reason I think it should stay. Upcoming FAN_REPORT_NAME is about to change the picture a bit towards the inotify behavior - victim watch gets event without name, parent watch gets event with name, filesystem watch gets both event flavors... that is, if you will agree to this behavior, but we shall continue this discussion on the fanotiify_name patches.... Thanks, Amir.
On Mon 04-05-20 11:51:27, Amir Goldstein wrote: > On Mon, May 4, 2020 at 11:07 AM Jan Kara <jack@suse.cz> wrote: > > > > On Sat 02-05-20 19:27:42, Amir Goldstein wrote: > > > Test reporting events with fid also with recusrive inode marks: > > > - Test events "on self" (FAN_DELETE_SELF) on file and dir > > > - Test events "on child" (FAN_MODIFY) on file > > > > > > With recursive inode marks, verify that the FAN_MODIFY event reported > > > to parent "on child" is merged with the FAN_MODIFY event reported to > > > child. > > > > > > The new test case is a regression test for commit f367a62a7cad: > > > > > > fanotify: merge duplicate events on parent and child > > > > The test looks OK but do we want a test for this? I mean: A test like this > > seems to imply we promise to merge identical events. Although that is a > > good general guideline, I consider it rather an optimization that may or > > may not happen but userspace should not rely on it. Thoughts? > > The thing is, those are not really two identical events. > This is in fact the same event (fsnotify_change() hook was called once). > The fact that listener process may have an inode watch, parent directory > watch and a filesystem watch should not affect the number of read events. Yeah, I agree that in this case we should be merging the event if sanely possible (which is why I've merged that patch). > Now it's true that internally, fsnotify_dentry() emits two event flavors to > parent and to victim. For inotify it even made some sense, because listener > would read two different event flavors with two different formats. > With fanotify (either reporting fd or fid) receiving two events makes very > little sense. > > I agree that the fix (merging those events) is best effort and we cannot > commit to merging the events, but this isolated regression test does > check the best effort fix reliably and this is the reason I think it > should stay. OK, I'm not too concerned about this test. But still the functionality is more in the area of "nice to have" than "must have" so in future we may break this if the implementation would get too hairy. But I guess we can remove the test in that case. > Upcoming FAN_REPORT_NAME is about to change the picture a bit > towards the inotify behavior - victim watch gets event without name, > parent watch gets event with name, filesystem watch gets both event > flavors... that is, if you will agree to this behavior, but we shall continue > this discussion on the fanotiify_name patches.... Yes. Honza
Hi Jan, ... > > > > The new test case is a regression test for commit f367a62a7cad: > > > > fanotify: merge duplicate events on parent and child > > > The test looks OK but do we want a test for this? I mean: A test like this > > > seems to imply we promise to merge identical events. Although that is a > > > good general guideline, I consider it rather an optimization that may or > > > may not happen but userspace should not rely on it. Thoughts? > > The thing is, those are not really two identical events. > > This is in fact the same event (fsnotify_change() hook was called once). > > The fact that listener process may have an inode watch, parent directory > > watch and a filesystem watch should not affect the number of read events. > Yeah, I agree that in this case we should be merging the event if sanely > possible (which is why I've merged that patch). > > Now it's true that internally, fsnotify_dentry() emits two event flavors to > > parent and to victim. For inotify it even made some sense, because listener > > would read two different event flavors with two different formats. > > With fanotify (either reporting fd or fid) receiving two events makes very > > little sense. > > I agree that the fix (merging those events) is best effort and we cannot > > commit to merging the events, but this isolated regression test does > > check the best effort fix reliably and this is the reason I think it > > should stay. > OK, I'm not too concerned about this test. But still the functionality is > more in the area of "nice to have" than "must have" so in future we may > break this if the implementation would get too hairy. But I guess we can > remove the test in that case. Yes, nothing is set in stone. > > Upcoming FAN_REPORT_NAME is about to change the picture a bit > > towards the inotify behavior - victim watch gets event without name, > > parent watch gets event with name, filesystem watch gets both event > > flavors... that is, if you will agree to this behavior, but we shall continue > > this discussion on the fanotiify_name patches.... > Yes. Can I add your ack tag to the whole patchset? Or do you still consider whether any of them should be merged? Kind regards, Petr > Honza
On Mon 04-05-20 20:49:36, Petr Vorel wrote: > Hi Jan, > > ... > > > > > The new test case is a regression test for commit f367a62a7cad: > > > > > > fanotify: merge duplicate events on parent and child > > > > > The test looks OK but do we want a test for this? I mean: A test like this > > > > seems to imply we promise to merge identical events. Although that is a > > > > good general guideline, I consider it rather an optimization that may or > > > > may not happen but userspace should not rely on it. Thoughts? > > > > The thing is, those are not really two identical events. > > > This is in fact the same event (fsnotify_change() hook was called once). > > > The fact that listener process may have an inode watch, parent directory > > > watch and a filesystem watch should not affect the number of read events. > > > Yeah, I agree that in this case we should be merging the event if sanely > > possible (which is why I've merged that patch). > > > > Now it's true that internally, fsnotify_dentry() emits two event flavors to > > > parent and to victim. For inotify it even made some sense, because listener > > > would read two different event flavors with two different formats. > > > With fanotify (either reporting fd or fid) receiving two events makes very > > > little sense. > > > > I agree that the fix (merging those events) is best effort and we cannot > > > commit to merging the events, but this isolated regression test does > > > check the best effort fix reliably and this is the reason I think it > > > should stay. > > > OK, I'm not too concerned about this test. But still the functionality is > > more in the area of "nice to have" than "must have" so in future we may > > break this if the implementation would get too hairy. But I guess we can > > remove the test in that case. > Yes, nothing is set in stone. > > > > Upcoming FAN_REPORT_NAME is about to change the picture a bit > > > towards the inotify behavior - victim watch gets event without name, > > > parent watch gets event with name, filesystem watch gets both event > > > flavors... that is, if you will agree to this behavior, but we shall continue > > > this discussion on the fanotiify_name patches.... > > > Yes. > Can I add your ack tag to the whole patchset? > Or do you still consider whether any of them should be merged? Feel free to add my Ack-by tag: Acked-by: Jan Kara <jack@suse.cz> I didn't check all the details but overall the patches look good to me. Honza
Hi all, > > Can I add your ack tag to the whole patchset? > > Or do you still consider whether any of them should be merged? > Feel free to add my Ack-by tag: > Acked-by: Jan Kara <jack@suse.cz> > I didn't check all the details but overall the patches look good to me. Merged with that few small fixes we discussed. Thanks for your work! > Honza Kind regards, Petr
diff --git a/testcases/kernel/syscalls/fanotify/fanotify15.c b/testcases/kernel/syscalls/fanotify/fanotify15.c index 2e860edb2..a9ed2ec81 100644 --- a/testcases/kernel/syscalls/fanotify/fanotify15.c +++ b/testcases/kernel/syscalls/fanotify/fanotify15.c @@ -9,6 +9,10 @@ * Test file that has been purposely designed to verify * FAN_REPORT_FID functionality while using newly defined dirent * events. + * + * Test case #1 is a regression test for commit f367a62a7cad: + * + * fanotify: merge duplicate events on parent and child */ #define _GNU_SOURCE #include "config.h" @@ -53,28 +57,49 @@ static int fanotify_fd; static char events_buf[EVENT_BUF_LEN]; static struct event_t event_set[EVENT_MAX]; -static void do_test(void) +static struct test_case_t { + const char *tname; + struct fanotify_mark_type mark; + unsigned long mask; +} test_cases[] = { + { + "FAN_REPORT_FID on filesystem including FAN_DELETE_SELF", + INIT_FANOTIFY_MARK_TYPE(FILESYSTEM), + FAN_DELETE_SELF, + }, + { + "FAN_REPORT_FID on directory with FAN_EVENT_ON_CHILD", + INIT_FANOTIFY_MARK_TYPE(INODE), + FAN_EVENT_ON_CHILD, + }, +}; + +static void do_test(unsigned int number) { int i, fd, len, count = 0; struct file_handle *event_file_handle; struct fanotify_event_metadata *metadata; struct fanotify_event_info_fid *event_fid; + struct test_case_t *tc = &test_cases[number]; + struct fanotify_mark_type *mark = &tc->mark; + + tst_res(TINFO, "Test #%d: %s", number, tc->tname); - if (fanotify_mark(fanotify_fd, FAN_MARK_ADD | FAN_MARK_FILESYSTEM, + if (fanotify_mark(fanotify_fd, FAN_MARK_ADD | mark->flag, tc->mask | FAN_CREATE | FAN_DELETE | FAN_MOVE | - FAN_MODIFY | FAN_DELETE_SELF | FAN_ONDIR, + FAN_MODIFY | FAN_ONDIR, AT_FDCWD, TEST_DIR) == -1) { if (errno == ENODEV) tst_brk(TCONF, "FAN_REPORT_FID not supported on %s " "filesystem", tst_device->fs_type); tst_brk(TBROK | TERRNO, - "fanotify_mark(%d, FAN_MARK_ADD | FAN_MARK_FILESYSTEM, " + "fanotify_mark(%d, FAN_MARK_ADD | %s, " "FAN_CREATE | FAN_DELETE | FAN_MOVE | " - "FAN_MODIFY | FAN_DELETE_SELF | FAN_ONDIR, " + "FAN_MODIFY | FAN_ONDIR | 0x%lx, " "AT_FDCWD, %s) failed", - fanotify_fd, TEST_DIR); + fanotify_fd, mark->name, tc->mask, TEST_DIR); } /* All dirent events on testdir are merged */ @@ -87,8 +112,21 @@ static void do_test(void) fd = SAFE_CREAT(FILE1, 0644); SAFE_CLOSE(fd); + /* Recursive watch file for events "on self" */ + if (mark->flag == FAN_MARK_INODE && + fanotify_mark(fanotify_fd, FAN_MARK_ADD | mark->flag, + FAN_MODIFY | FAN_DELETE_SELF, + AT_FDCWD, FILE1) == -1) { + tst_brk(TBROK | TERRNO, + "fanotify_mark(%d, FAN_MARK_ADD | %s, " + "FAN_DELETE_SELF, AT_FDCWD, %s) failed", + fanotify_fd, mark->name, FILE1); + } + /* * Event on child file is not merged with dirent events. + * FAN_MODIFY event reported on file mark should be merged with the + * FAN_MODIFY event reported on parent directory watch. */ event_set[count].mask = FAN_MODIFY; event_set[count].handle.handle_bytes = MAX_HANDLE_SZ; @@ -128,6 +166,17 @@ static void do_test(void) SAFE_MKDIR(DIR1, 0755); + /* Recursive watch subdir for events "on self" */ + if (mark->flag == FAN_MARK_INODE && + fanotify_mark(fanotify_fd, FAN_MARK_ADD | mark->flag, + FAN_DELETE_SELF | FAN_ONDIR, + AT_FDCWD, DIR1) == -1) { + tst_brk(TBROK | TERRNO, + "fanotify_mark(%d, FAN_MARK_ADD | %s," + "FAN_DELETE_SELF | FAN_ONDIR, AT_FDCWD, %s) failed", + fanotify_fd, mark->name, DIR1); + } + SAFE_RENAME(DIR1, DIR2); event_set[count].mask = FAN_ONDIR | FAN_DELETE_SELF; @@ -141,6 +190,17 @@ static void do_test(void) /* Read dir events from the event queue */ len += SAFE_READ(0, fanotify_fd, events_buf + len, EVENT_BUF_LEN - len); + /* + * Cleanup the mark + */ + if (fanotify_mark(fanotify_fd, FAN_MARK_FLUSH | mark->flag, 0, + AT_FDCWD, TEST_DIR) < 0) { + tst_brk(TBROK | TERRNO, + "fanotify_mark (%d, FAN_MARK_FLUSH | %s, 0," + "AT_FDCWD, '"TEST_DIR"') failed", + fanotify_fd, mark->name); + } + /* Process each event in buffer */ for (i = 0, metadata = (struct fanotify_event_metadata *) events_buf; FAN_EVENT_OK(metadata, len); i++) { @@ -259,9 +319,14 @@ static struct tst_test test = { .mount_device = 1, .mntpoint = MOUNT_POINT, .all_filesystems = 1, - .test_all = do_test, + .test = do_test, + .tcnt = ARRAY_SIZE(test_cases), .setup = do_setup, - .cleanup = do_cleanup + .cleanup = do_cleanup, + .tags = (const struct tst_tag[]) { + {"linux-git", "f367a62a7cad"}, + {} + } }; #else
Test reporting events with fid also with recusrive inode marks: - Test events "on self" (FAN_DELETE_SELF) on file and dir - Test events "on child" (FAN_MODIFY) on file With recursive inode marks, verify that the FAN_MODIFY event reported to parent "on child" is merged with the FAN_MODIFY event reported to child. The new test case is a regression test for commit f367a62a7cad: fanotify: merge duplicate events on parent and child Signed-off-by: Amir Goldstein <amir73il@gmail.com> Reviewed-by: Petr Vorel <pvorel@suse.cz> Reviewed-by: Cyril Hrubis <chrubis@suse.cz> Reviewed-by: Matthew Bobrowski <mbobrowski@mbobrowski.org> --- .../kernel/syscalls/fanotify/fanotify15.c | 81 +++++++++++++++++-- 1 file changed, 73 insertions(+), 8 deletions(-)