diff mbox series

fanotify10: Calling drop_cache twice to ensure the inode is evicted

Message ID 20240830130003.3245531-1-wozizhi@huawei.com
State Accepted
Headers show
Series fanotify10: Calling drop_cache twice to ensure the inode is evicted | expand

Commit Message

Zizhi Wo Aug. 30, 2024, 1 p.m. UTC
In this test case, some scenarios are designed to verify whether the
FANOTIFY_EVICTABLE flag takes effect: by verifying that information cannot
be obtained from the corresponding inode after drop_cache, as this flag
does not ping the inode.

However, drop_cache is only performed once here, which may result in the
inode not being released in NUMA scenarios. Suppose the inode is located
on NUMA0 and the dentry is located on NUMA1; the first drop_cache can only
ensure that the inode is added to the LRU list, but does not guarantee that
evict() can been called because dispose_list does not yet include this
inode when traversing NUMA0, which causes the testcase execution fail.

For the single-file scenario in this testcase, executing drop_cache twice
is necessary to ensure the inode is evicted, thus allowing the testcase to
pass.

Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
---
 testcases/kernel/syscalls/fanotify/fanotify10.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Petr Vorel Sept. 3, 2024, 2:08 p.m. UTC | #1
Hi all,

> In this test case, some scenarios are designed to verify whether the
> FANOTIFY_EVICTABLE flag takes effect: by verifying that information cannot
> be obtained from the corresponding inode after drop_cache, as this flag
> does not ping the inode.

> However, drop_cache is only performed once here, which may result in the
> inode not being released in NUMA scenarios. Suppose the inode is located
> on NUMA0 and the dentry is located on NUMA1; the first drop_cache can only
> ensure that the inode is added to the LRU list, but does not guarantee that
> evict() can been called because dispose_list does not yet include this
> inode when traversing NUMA0, which causes the testcase execution fail.

I wonder if there can be some detection that inode is evicted.
Or, can it happen that even 2x drop is not enough?

> For the single-file scenario in this testcase, executing drop_cache twice
> is necessary to ensure the inode is evicted, thus allowing the testcase to
> pass.

Acked-by: Petr Vorel <pvorel@suse.cz>

@Amir, Jan, could you please have a look?

Kind regards,
Petr

> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> ---
>  testcases/kernel/syscalls/fanotify/fanotify10.c | 2 ++
>  1 file changed, 2 insertions(+)

> diff --git a/testcases/kernel/syscalls/fanotify/fanotify10.c b/testcases/kernel/syscalls/fanotify/fanotify10.c
> index c6d8ec922..42018de0d 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify10.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify10.c
> @@ -515,6 +515,8 @@ static void drop_caches(void)
>  	if (syncfs(fd_syncfs) < 0)
>  		tst_brk(TBROK | TERRNO, "Unexpected error when syncing filesystem");

> +	/* Need to drop twice to ensure the inode is evicted. */
> +	SAFE_FILE_PRINTF(DROP_CACHES_FILE, "3");
>  	SAFE_FILE_PRINTF(DROP_CACHES_FILE, "3");
>  }
Zizhi Wo Sept. 4, 2024, 7:34 a.m. UTC | #2
在 2024/9/3 22:08, Petr Vorel 写道:
> Hi all,
> 
>> In this test case, some scenarios are designed to verify whether the
>> FANOTIFY_EVICTABLE flag takes effect: by verifying that information cannot
>> be obtained from the corresponding inode after drop_cache, as this flag
>> does not ping the inode.
> 
>> However, drop_cache is only performed once here, which may result in the
>> inode not being released in NUMA scenarios. Suppose the inode is located
>> on NUMA0 and the dentry is located on NUMA1; the first drop_cache can only
>> ensure that the inode is added to the LRU list, but does not guarantee that
>> evict() can been called because dispose_list does not yet include this
>> inode when traversing NUMA0, which causes the testcase execution fail.
> 
> I wonder if there can be some detection that inode is evicted.
> Or, can it happen that even 2x drop is not enough?

In this testcase scenario, there is only a single layer of dentry, and
there are no interdependencies between the dentries. Therefore, the
dependency chain only consists of dentry->inode. So in my opinion, 
performing drop_cache twice ensures that the inode is evicted?

I haven't been able to come up with a good solution for detecting
whether an inode has been evicted. I would appreciate hearing everyone's
thoughts and suggestions on this matter. Thank you for your response.

Thanks,
Zizhi Wo

> 
>> For the single-file scenario in this testcase, executing drop_cache twice
>> is necessary to ensure the inode is evicted, thus allowing the testcase to
>> pass.
> 
> Acked-by: Petr Vorel <pvorel@suse.cz>
> 
> @Amir, Jan, could you please have a look?
> 
> Kind regards,
> Petr
> 
>> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
>> ---
>>   testcases/kernel/syscalls/fanotify/fanotify10.c | 2 ++
>>   1 file changed, 2 insertions(+)
> 
>> diff --git a/testcases/kernel/syscalls/fanotify/fanotify10.c b/testcases/kernel/syscalls/fanotify/fanotify10.c
>> index c6d8ec922..42018de0d 100644
>> --- a/testcases/kernel/syscalls/fanotify/fanotify10.c
>> +++ b/testcases/kernel/syscalls/fanotify/fanotify10.c
>> @@ -515,6 +515,8 @@ static void drop_caches(void)
>>   	if (syncfs(fd_syncfs) < 0)
>>   		tst_brk(TBROK | TERRNO, "Unexpected error when syncing filesystem");
> 
>> +	/* Need to drop twice to ensure the inode is evicted. */
>> +	SAFE_FILE_PRINTF(DROP_CACHES_FILE, "3");
>>   	SAFE_FILE_PRINTF(DROP_CACHES_FILE, "3");
>>   }
Amir Goldstein Sept. 4, 2024, 8:57 a.m. UTC | #3
On Tue, Sep 3, 2024 at 4:08 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi all,
>
> > In this test case, some scenarios are designed to verify whether the
> > FANOTIFY_EVICTABLE flag takes effect: by verifying that information cannot
> > be obtained from the corresponding inode after drop_cache, as this flag
> > does not ping the inode.
>
> > However, drop_cache is only performed once here, which may result in the
> > inode not being released in NUMA scenarios. Suppose the inode is located
> > on NUMA0 and the dentry is located on NUMA1; the first drop_cache can only
> > ensure that the inode is added to the LRU list, but does not guarantee that
> > evict() can been called because dispose_list does not yet include this
> > inode when traversing NUMA0, which causes the testcase execution fail.
>
> I wonder if there can be some detection that inode is evicted.
> Or, can it happen that even 2x drop is not enough?
>
> > For the single-file scenario in this testcase, executing drop_cache twice
> > is necessary to ensure the inode is evicted, thus allowing the testcase to
> > pass.
>
> Acked-by: Petr Vorel <pvorel@suse.cz>
>
> @Amir, Jan, could you please have a look?
>

Same solution was already applied to fanotify23

8ccf5b45c fanotify23: Make evictable marks tests more reliable

So ACK.

BTW, fanotify23 also restricts itself to ext2 for more deterministic
inode eviction behavior, so this could be considered for fanotify10 as well.

For example, AFAIK, xfs has its own internal eviction logic, which can keep
inodes in cache for one more shrinker cycle.

Thanks,
Amir.
Jan Kara Sept. 4, 2024, 9:07 a.m. UTC | #4
On Tue 03-09-24 16:08:07, Petr Vorel wrote:
> Hi all,
> 
> > In this test case, some scenarios are designed to verify whether the
> > FANOTIFY_EVICTABLE flag takes effect: by verifying that information cannot
> > be obtained from the corresponding inode after drop_cache, as this flag
> > does not ping the inode.
> 
> > However, drop_cache is only performed once here, which may result in the
> > inode not being released in NUMA scenarios. Suppose the inode is located
> > on NUMA0 and the dentry is located on NUMA1; the first drop_cache can only
> > ensure that the inode is added to the LRU list, but does not guarantee that
> > evict() can been called because dispose_list does not yet include this
> > inode when traversing NUMA0, which causes the testcase execution fail.
> 
> I wonder if there can be some detection that inode is evicted.
> Or, can it happen that even 2x drop is not enough?
> 
> > For the single-file scenario in this testcase, executing drop_cache twice
> > is necessary to ensure the inode is evicted, thus allowing the testcase to
> > pass.
> 
> Acked-by: Petr Vorel <pvorel@suse.cz>
> 
> @Amir, Jan, could you please have a look?

Yeah, as Amir wrote, I've ended up doing similar thing for fanotify23 so:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> > Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
> > ---
> >  testcases/kernel/syscalls/fanotify/fanotify10.c | 2 ++
> >  1 file changed, 2 insertions(+)
> 
> > diff --git a/testcases/kernel/syscalls/fanotify/fanotify10.c b/testcases/kernel/syscalls/fanotify/fanotify10.c
> > index c6d8ec922..42018de0d 100644
> > --- a/testcases/kernel/syscalls/fanotify/fanotify10.c
> > +++ b/testcases/kernel/syscalls/fanotify/fanotify10.c
> > @@ -515,6 +515,8 @@ static void drop_caches(void)
> >  	if (syncfs(fd_syncfs) < 0)
> >  		tst_brk(TBROK | TERRNO, "Unexpected error when syncing filesystem");
> 
> > +	/* Need to drop twice to ensure the inode is evicted. */
> > +	SAFE_FILE_PRINTF(DROP_CACHES_FILE, "3");
> >  	SAFE_FILE_PRINTF(DROP_CACHES_FILE, "3");
> >  }
Petr Vorel Sept. 4, 2024, 9:47 a.m. UTC | #5
Hi all,

> On Tue 03-09-24 16:08:07, Petr Vorel wrote:
> > Hi all,

> > > In this test case, some scenarios are designed to verify whether the
> > > FANOTIFY_EVICTABLE flag takes effect: by verifying that information cannot
> > > be obtained from the corresponding inode after drop_cache, as this flag
> > > does not ping the inode.

> > > However, drop_cache is only performed once here, which may result in the
> > > inode not being released in NUMA scenarios. Suppose the inode is located
> > > on NUMA0 and the dentry is located on NUMA1; the first drop_cache can only
> > > ensure that the inode is added to the LRU list, but does not guarantee that
> > > evict() can been called because dispose_list does not yet include this
> > > inode when traversing NUMA0, which causes the testcase execution fail.

> > I wonder if there can be some detection that inode is evicted.
> > Or, can it happen that even 2x drop is not enough?

> > > For the single-file scenario in this testcase, executing drop_cache twice
> > > is necessary to ensure the inode is evicted, thus allowing the testcase to
> > > pass.

> > Acked-by: Petr Vorel <pvorel@suse.cz>

> > @Amir, Jan, could you please have a look?

> Yeah, as Amir wrote, I've ended up doing similar thing for fanotify23 so:

My bad memory (8ccf5b45cc), thanks for reminder and your review.
I checked only these two fanotify tests are dropping caches.

Merged.

Kind regards,
Petr
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/fanotify/fanotify10.c b/testcases/kernel/syscalls/fanotify/fanotify10.c
index c6d8ec922..42018de0d 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify10.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify10.c
@@ -515,6 +515,8 @@  static void drop_caches(void)
 	if (syncfs(fd_syncfs) < 0)
 		tst_brk(TBROK | TERRNO, "Unexpected error when syncing filesystem");
 
+	/* Need to drop twice to ensure the inode is evicted. */
+	SAFE_FILE_PRINTF(DROP_CACHES_FILE, "3");
 	SAFE_FILE_PRINTF(DROP_CACHES_FILE, "3");
 }