| Message ID | 20220825140306.7150-1-jack@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | syscalls/fanotify10: Make evictable marks test more reliable | expand |
Hi Jan, all, > In some setups evictable marks tests are failing because the inode with > evictable mark does not get evicted. Make sure we sync the filesystem > before we try to drop caches to increase likelyhood the inode will get > evicted. Merged with minor changes to keep checkpatch.pl happy. Thanks for fixing this and Cc me on previous discussion. Given on previous discussion the behavior on ext2 vs. xfs: would it make sense to transform the test to use .all_filesystems = 1 ? Kind regards, Petr
On Fri 26-08-22 15:12:14, Petr Vorel wrote: > Hi Jan, all, > > > In some setups evictable marks tests are failing because the inode with > > evictable mark does not get evicted. Make sure we sync the filesystem > > before we try to drop caches to increase likelyhood the inode will get > > evicted. > > Merged with minor changes to keep checkpatch.pl happy. Thanks! > Given on previous discussion the behavior on ext2 vs. xfs: > would it make sense to transform the test to use .all_filesystems = 1 ? Well, I don't think it would improve test coverage in any interesting way. This test tests stuff in fsnotify layer & VFS. The differences in filesystem inode reclaim are not target of this test - we are just trying to check that fsnotify does not block inode reclaim by holding inode references and for that any filesystem works. Or did you mean something else? Honza
On Fri, Aug 26, 2022 at 5:19 PM Jan Kara <jack@suse.cz> wrote: > > On Fri 26-08-22 15:12:14, Petr Vorel wrote: > > Hi Jan, all, > > > > > In some setups evictable marks tests are failing because the inode with > > > evictable mark does not get evicted. Make sure we sync the filesystem > > > before we try to drop caches to increase likelyhood the inode will get > > > evicted. > > > > Merged with minor changes to keep checkpatch.pl happy. > > Thanks! > > > Given on previous discussion the behavior on ext2 vs. xfs: > > would it make sense to transform the test to use .all_filesystems = 1 ? On the contrary. We want the inode reclaim to be as predictable as possible. That is why I suggested to force the test to use ext2 because xfs has some specialized inode reclaim > > Well, I don't think it would improve test coverage in any interesting way. > This test tests stuff in fsnotify layer & VFS. The differences in > filesystem inode reclaim are not target of this test - we are just trying > to check that fsnotify does not block inode reclaim by holding inode > references and for that any filesystem works. Or did you mean something > else? > Agree. I see no reason to change that. Thanks, Amir.
> On Fri, Aug 26, 2022 at 5:19 PM Jan Kara <jack@suse.cz> wrote: > > On Fri 26-08-22 15:12:14, Petr Vorel wrote: > > > Hi Jan, all, > > > > In some setups evictable marks tests are failing because the inode with > > > > evictable mark does not get evicted. Make sure we sync the filesystem > > > > before we try to drop caches to increase likelyhood the inode will get > > > > evicted. > > > Merged with minor changes to keep checkpatch.pl happy. > > Thanks! > > > Given on previous discussion the behavior on ext2 vs. xfs: > > > would it make sense to transform the test to use .all_filesystems = 1 ? > On the contrary. > We want the inode reclaim to be as predictable as possible. > That is why I suggested to force the test to use ext2 > because xfs has some specialized inode reclaim Ah right! Could you please send a patch with .dev_fs_type = "ext2" That should be enough I guess. > > Well, I don't think it would improve test coverage in any interesting way. > > This test tests stuff in fsnotify layer & VFS. The differences in > > filesystem inode reclaim are not target of this test - we are just trying > > to check that fsnotify does not block inode reclaim by holding inode > > references and for that any filesystem works. Or did you mean something > > else? > Agree. I see no reason to change that. Thank you both for info! Kind regards, Petr > Thanks, > Amir.
On Sat, Aug 27, 2022 at 1:09 AM Petr Vorel <pvorel@suse.cz> wrote: > > > On Fri, Aug 26, 2022 at 5:19 PM Jan Kara <jack@suse.cz> wrote: > > > > On Fri 26-08-22 15:12:14, Petr Vorel wrote: > > > > Hi Jan, all, > > > > > > In some setups evictable marks tests are failing because the inode with > > > > > evictable mark does not get evicted. Make sure we sync the filesystem > > > > > before we try to drop caches to increase likelyhood the inode will get > > > > > evicted. > > > > > Merged with minor changes to keep checkpatch.pl happy. > > > > Thanks! > > > > > Given on previous discussion the behavior on ext2 vs. xfs: > > > > would it make sense to transform the test to use .all_filesystems = 1 ? > > > On the contrary. > > We want the inode reclaim to be as predictable as possible. > > That is why I suggested to force the test to use ext2 > > because xfs has some specialized inode reclaim > > Ah right! Could you please send a patch with .dev_fs_type = "ext2" > That should be enough I guess. > Petr, I don't think there is a need for that. I was only pointing out the different fs can have different inode reclaim behaviors. I had not encountered issues when testing fanotify10 on xfs myself. Jan's patch should be enough and then the test can run on the user's choice of filesystem, which is always better than unneeded restrictions. If we see issues on xfs we can reconsider .dev_fs_type = "ext2". Thanks, Amir.
> On Sat, Aug 27, 2022 at 1:09 AM Petr Vorel <pvorel@suse.cz> wrote: > > > On Fri, Aug 26, 2022 at 5:19 PM Jan Kara <jack@suse.cz> wrote: > > > > On Fri 26-08-22 15:12:14, Petr Vorel wrote: > > > > > Hi Jan, all, > > > > > > In some setups evictable marks tests are failing because the inode with > > > > > > evictable mark does not get evicted. Make sure we sync the filesystem > > > > > > before we try to drop caches to increase likelyhood the inode will get > > > > > > evicted. > > > > > Merged with minor changes to keep checkpatch.pl happy. > > > > Thanks! > > > > > Given on previous discussion the behavior on ext2 vs. xfs: > > > > > would it make sense to transform the test to use .all_filesystems = 1 ? > > > On the contrary. > > > We want the inode reclaim to be as predictable as possible. > > > That is why I suggested to force the test to use ext2 > > > because xfs has some specialized inode reclaim > > Ah right! Could you please send a patch with .dev_fs_type = "ext2" > > That should be enough I guess. > Petr, I don't think there is a need for that. > I was only pointing out the different fs can have different inode > reclaim behaviors. > I had not encountered issues when testing fanotify10 on xfs myself. > Jan's patch should be enough and then the test can run > on the user's choice of filesystem, which is always better than > unneeded restrictions. > If we see issues on xfs we can reconsider .dev_fs_type = "ext2". Amir, thanks for explanation. Makes sense. Kind regards, Petr > Thanks, > Amir.
diff --git a/testcases/kernel/syscalls/fanotify/fanotify10.c b/testcases/kernel/syscalls/fanotify/fanotify10.c index 19e43d2c2762..54636ce2ddd4 100644 --- a/testcases/kernel/syscalls/fanotify/fanotify10.c +++ b/testcases/kernel/syscalls/fanotify/fanotify10.c @@ -342,6 +342,15 @@ static void show_fanotify_marks(int fd) } } +static void drop_caches(char *path) +{ + int fd = SAFE_OPEN(path, O_RDONLY); + if (syncfs(fd) < 0) + tst_brk(TBROK | TERRNO, "Unexpected error when syncing filesystem"); + SAFE_CLOSE(fd); + SAFE_FILE_PRINTF(DROP_CACHES_FILE, "3"); +} + static int create_fanotify_groups(unsigned int n) { struct tcase *tc = &tcases[n]; @@ -402,7 +411,7 @@ add_mark: * drop_caches should evict inode from cache and remove evictable marks */ if (evictable_ignored) { - SAFE_FILE_PRINTF(DROP_CACHES_FILE, "3"); + drop_caches(tc->mark_path); for (p = 0; p < num_classes; p++) { for (i = 0; i < GROUPS_PER_PRIO; i++) { if (fd_notify[p][i] > 0)