[LTP] [PATCH] syscalls/fanotify10: Make evictable marks test more reliable

[LTP] [PATCH] syscalls/fanotify10: Make evictable marks test more reliable

[Jan Kara]

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.

Reported-by: Jan Stancek <jstancek@redhat.com>
Reported-by: Dominique Leuenberger <dimstar@opensuse.org>
Acked-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 testcases/kernel/syscalls/fanotify/fanotify10.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

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)
-- 
2.35.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/fanotify10: Make evictable marks test more reliable

[Petr Vorel]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/fanotify10: Make evictable marks test more reliable

[Jan Kara]

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/fanotify10: Make evictable marks test more reliable

[Amir Goldstein]

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.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/fanotify10: Make evictable marks test more reliable

[Petr Vorel]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/fanotify10: Make evictable marks test more reliable

[Amir Goldstein]

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.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/fanotify10: Make evictable marks test more reliable

[Petr Vorel]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp