[PATCH] erofs(shrinker): return SHRINK_EMPTY if no objects to free

[PATCH] erofs(shrinker): return SHRINK_EMPTY if no objects to free

[Chen Linxuan]

Comments in file include/linux/shrinker.h says that
`count_objects` of `struct shrinker` should return SHRINK_EMPTY
when there are no objects to free.

> If there are no objects to free, it should return SHRINK_EMPTY,
> while 0 is returned in cases of the number of freeable items cannot
> be determined or shrinker should skip this cache for this time
> (e.g., their number is below shrinkable limit).

Signed-off-by: Chen Linxuan <chenlinxuan@uniontech.com>
---
 fs/erofs/zutil.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/erofs/zutil.c b/fs/erofs/zutil.c
index 0dd65cefce33..682863fa9e1c 100644
--- a/fs/erofs/zutil.c
+++ b/fs/erofs/zutil.c
@@ -243,7 +243,9 @@ void erofs_shrinker_unregister(struct super_block *sb)
 static unsigned long erofs_shrink_count(struct shrinker *shrink,
 					struct shrink_control *sc)
 {
-	return atomic_long_read(&erofs_global_shrink_cnt);
+	unsigned long count = atomic_long_read(&erofs_global_shrink_cnt);
+
+	return count ? count : SHRINK_EMPTY;
 }
 
 static unsigned long erofs_shrink_scan(struct shrinker *shrink,
-- 
2.43.0

[PATCH] erofs(shrinker): return SHRINK_EMPTY if no objects to free

[Chen Linxuan]

Comments in file include/linux/shrinker.h says that
`count_objects` of `struct shrinker` should return SHRINK_EMPTY
when there are no objects to free.

> If there are no objects to free, it should return SHRINK_EMPTY,
> while 0 is returned in cases of the number of freeable items cannot
> be determined or shrinker should skip this cache for this time
> (e.g., their number is below shrinkable limit).

Signed-off-by: Chen Linxuan <chenlinxuan@uniontech.com>
---
 fs/erofs/zutil.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/erofs/zutil.c b/fs/erofs/zutil.c
index 0dd65cefce33..682863fa9e1c 100644
--- a/fs/erofs/zutil.c
+++ b/fs/erofs/zutil.c
@@ -243,7 +243,9 @@ void erofs_shrinker_unregister(struct super_block *sb)
 static unsigned long erofs_shrink_count(struct shrinker *shrink,
 					struct shrink_control *sc)
 {
-	return atomic_long_read(&erofs_global_shrink_cnt);
+	unsigned long count = atomic_long_read(&erofs_global_shrink_cnt);
+
+	return count ? count : SHRINK_EMPTY;
 }
 
 static unsigned long erofs_shrink_scan(struct shrinker *shrink,
-- 
2.43.0

Re: [PATCH] erofs(shrinker): return SHRINK_EMPTY if no objects to free

[Gao Xiang]

Hi Linxuan,

On 2025/1/16 15:20, Chen Linxuan wrote:
> Comments in file include/linux/shrinker.h says that
> `count_objects` of `struct shrinker` should return SHRINK_EMPTY
> when there are no objects to free.
> 
>> If there are no objects to free, it should return SHRINK_EMPTY,
>> while 0 is returned in cases of the number of freeable items cannot
>> be determined or shrinker should skip this cache for this time
>> (e.g., their number is below shrinkable limit).

Thanks for the patch!

Yeah, it seems that is the case.  Yet it'd better to document
what the impact if 0 is returned here if you know more..

> 
> Signed-off-by: Chen Linxuan <chenlinxuan@uniontech.com>
> ---
>   fs/erofs/zutil.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/erofs/zutil.c b/fs/erofs/zutil.c
> index 0dd65cefce33..682863fa9e1c 100644
> --- a/fs/erofs/zutil.c
> +++ b/fs/erofs/zutil.c
> @@ -243,7 +243,9 @@ void erofs_shrinker_unregister(struct super_block *sb)
>   static unsigned long erofs_shrink_count(struct shrinker *shrink,
>   					struct shrink_control *sc)
>   {
> -	return atomic_long_read(&erofs_global_shrink_cnt);
> +	unsigned long count = atomic_long_read(&erofs_global_shrink_cnt);
> +
> +	return count ? count : SHRINK_EMPTY;

I guess you could just use

	return atomic_long_read(&erofs_global_shrink_cnt) ?: SHRINK_EMPTY;

Thanks,
Gao Xiang

>   }
>   
>   static unsigned long erofs_shrink_scan(struct shrinker *shrink,

Re: [PATCH] erofs(shrinker): return SHRINK_EMPTY if no objects to free

[Chen Linxuan]

On Thu, 2025-01-16 at 15:51 +0800, Gao Xiang wrote:
> Hi Linxuan,
> 
> On 2025/1/16 15:20, Chen Linxuan wrote:
> > Comments in file include/linux/shrinker.h says that
> > `count_objects` of `struct shrinker` should return SHRINK_EMPTY
> > when there are no objects to free.
> > 
> > > If there are no objects to free, it should return SHRINK_EMPTY,
> > > while 0 is returned in cases of the number of freeable items cannot
> > > be determined or shrinker should skip this cache for this time
> > > (e.g., their number is below shrinkable limit).
> 
> Thanks for the patch!
> 
> Yeah, it seems that is the case.  Yet it'd better to document
> what the impact if 0 is returned here if you know more..

Sorry, I have no idea about that.

> 
> > 
> > Signed-off-by: Chen Linxuan <chenlinxuan@uniontech.com>
> > ---
> >   fs/erofs/zutil.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/erofs/zutil.c b/fs/erofs/zutil.c
> > index 0dd65cefce33..682863fa9e1c 100644
> > --- a/fs/erofs/zutil.c
> > +++ b/fs/erofs/zutil.c
> > @@ -243,7 +243,9 @@ void erofs_shrinker_unregister(struct super_block *sb)
> >   static unsigned long erofs_shrink_count(struct shrinker *shrink,
> >   					struct shrink_control *sc)
> >   {
> > -	return atomic_long_read(&erofs_global_shrink_cnt);
> > +	unsigned long count = atomic_long_read(&erofs_global_shrink_cnt);
> > +
> > +	return count ? count : SHRINK_EMPTY;
> 
> I guess you could just use
> 
> 	return atomic_long_read(&erofs_global_shrink_cnt) ?: SHRINK_EMPTY;
> 
> Thanks,
> Gao Xiang
> 
> >   }
> >   
> >   static unsigned long erofs_shrink_scan(struct shrinker *shrink,
> 
> 
> 

Re: [PATCH] erofs(shrinker): return SHRINK_EMPTY if no objects to free

[Gao Xiang]

On 2025/1/16 16:24, Chen Linxuan wrote:
> On Thu, 2025-01-16 at 15:51 +0800, Gao Xiang wrote:
>> Hi Linxuan,
>>
>> On 2025/1/16 15:20, Chen Linxuan wrote:
>>> Comments in file include/linux/shrinker.h says that
>>> `count_objects` of `struct shrinker` should return SHRINK_EMPTY
>>> when there are no objects to free.
>>>
>>>> If there are no objects to free, it should return SHRINK_EMPTY,
>>>> while 0 is returned in cases of the number of freeable items cannot
>>>> be determined or shrinker should skip this cache for this time
>>>> (e.g., their number is below shrinkable limit).
>>
>> Thanks for the patch!
>>
>> Yeah, it seems that is the case.  Yet it'd better to document
>> what the impact if 0 is returned here if you know more..
> 
> Sorry, I have no idea about that.

I guess it has no difference if the shrinker is not memcg-aware,
see:
https://lore.kernel.org/r/153063070859.1818.11870882950920963480.stgit@localhost.localdomain

But I'm fine to use SHRINK_EMPTY since it's clearly documented.

So could you resend a patch to address my suggestion?

Thanks,
Gao Xiang

Re: [PATCH] erofs(shrinker): return SHRINK_EMPTY if no objects to free

[Chen Linxuan]

On Thu, 2025-01-16 at 16:45 +0800, Gao Xiang wrote:
> 
> On 2025/1/16 16:24, Chen Linxuan wrote:
> > On Thu, 2025-01-16 at 15:51 +0800, Gao Xiang wrote:
> > > Hi Linxuan,
> > > 
> > > On 2025/1/16 15:20, Chen Linxuan wrote:
> > > > Comments in file include/linux/shrinker.h says that
> > > > `count_objects` of `struct shrinker` should return SHRINK_EMPTY
> > > > when there are no objects to free.
> > > > 
> > > > > If there are no objects to free, it should return SHRINK_EMPTY,
> > > > > while 0 is returned in cases of the number of freeable items cannot
> > > > > be determined or shrinker should skip this cache for this time
> > > > > (e.g., their number is below shrinkable limit).
> > > 
> > > Thanks for the patch!
> > > 
> > > Yeah, it seems that is the case.  Yet it'd better to document
> > > what the impact if 0 is returned here if you know more..
> > 
> > Sorry, I have no idea about that.
> 
> I guess it has no difference if the shrinker is not memcg-aware,
> see:
> https://lore.kernel.org/r/153063070859.1818.11870882950920963480.stgit@localhost.localdomain
> 
> But I'm fine to use SHRINK_EMPTY since it's clearly documented.
> 
> So could you resend a patch to address my suggestion?

v2 patch has been sent.

> 
> Thanks,
> Gao Xiang
> 
>