[PATCH] ubifs: sequence each shrink task

[Zhihao Cheng]

Hi,
> From: duguowei <duguowei at xiaomi.com>
> 
> Because of 'list_move_tail', if two or more tasks are shrinking, there
> will be different results for them.
> 
> For example:
> After the first round, if shrink_run_no of entry equals run_no of task,
> task will break directly at the beginning of next round; if they are
> not equal, task will continue to shrink until encounter one entry
> which has the same number.
> 
> It is difficult to confirm the real results of all tasks, so add a lock
> to only allow one task to shrink at the same time.
> 
Which situation we need to confirm the exact results for shrinker tasks? 
ubifs_shrink_scan() provides a rough reclaiming method to make sure that 
ubifs can iterate all filesystem entries, it looks that skipping one or 
two entries occasionally not occurs any problems.

BTW, could you please provide a reproducer that can trigger 
'c->shrinker_run_no == run_no' to skip fs entry. 'shrinker_run_no' is an 
'unsigned int' type value, it looks that the skipping condition is 
seldomly statisfied even with many ubifs filesystem entries.
> Signed-off-by: duguowei <duguowei at xiaomi.com>
> ---
>   fs/ubifs/shrinker.c | 22 +++++++++++++++-------
>   1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ubifs/shrinker.c b/fs/ubifs/shrinker.c
> index d00a6f20ac7b..900644ef432d 100644
> --- a/fs/ubifs/shrinker.c
> +++ b/fs/ubifs/shrinker.c
> @@ -42,6 +42,9 @@ static unsigned int shrinker_run_no;
>   /* Protects 'ubifs_infos' list */
>   DEFINE_SPINLOCK(ubifs_infos_lock);
>   
> +/* Sequence each shrink task*/
> +DEFINE_SPINLOCK(ubifs_shrink_lock);
> +
>   /* Global clean znode counter (for all mounted UBIFS instances) */
>   atomic_long_t ubifs_clean_zn_cnt;
>   
> @@ -145,13 +148,12 @@ static int shrink_tnc_trees(int nr, int age, int *contention)
>   {
>   	struct ubifs_info *c;
>   	struct list_head *p;
> -	unsigned int run_no;
>   	int freed = 0;
>   
>   	spin_lock(&ubifs_infos_lock);
> -	do {
> -		run_no = ++shrinker_run_no;
> -	} while (run_no == 0);
> +	shrinker_run_no++;
> +	if (shrinker_run_no == 0)
> +		shrinker_run_no = 1;
>   	/* Iterate over all mounted UBIFS file-systems and try to shrink them */
>   	p = ubifs_infos.next;
>   	while (p != &ubifs_infos) {
> @@ -160,7 +162,7 @@ static int shrink_tnc_trees(int nr, int age, int *contention)
>   		 * We move the ones we do to the end of the list, so we stop
>   		 * when we see one we have already done.
>   		 */
> -		if (c->shrinker_run_no == run_no)
> +		if (c->shrinker_run_no == shrinker_run_no)
>   			break;
>   		if (!mutex_trylock(&c->umount_mutex)) {
>   			/* Some un-mount is in progress, try next FS */
> @@ -183,7 +185,7 @@ static int shrink_tnc_trees(int nr, int age, int *contention)
>   		 * OK, now we have TNC locked, the file-system cannot go away -
>   		 * it is safe to reap the cache.
>   		 */
> -		c->shrinker_run_no = run_no;
> +		c->shrinker_run_no = shrinker_run_no;
>   		freed += shrink_tnc(c, nr, age, contention);
>   		mutex_unlock(&c->tnc_mutex);
>   		spin_lock(&ubifs_infos_lock);
> @@ -283,7 +285,9 @@ unsigned long ubifs_shrink_scan(struct shrinker *shrink,
>   	int contention = 0;
>   	unsigned long freed;
>   	long clean_zn_cnt = atomic_long_read(&ubifs_clean_zn_cnt);
> +	int ret = 0;
>   
> +	spin_lock(&ubifs_shrink_lock);
>   	if (!clean_zn_cnt) {
>   		/*
>   		 * No clean znodes, nothing to reap. All we can do in this case
> @@ -293,7 +297,9 @@ unsigned long ubifs_shrink_scan(struct shrinker *shrink,
>   		 * later.
>   		 */
>   		dbg_tnc("no clean znodes, kick a thread");
> -		return kick_a_thread();
> +		ret = kick_a_thread();
> +		spin_unlock(&ubifs_shrink_lock);
> +		return ret;
>   	}
>   
>   	freed = shrink_tnc_trees(nr, OLD_ZNODE_AGE, &contention);
> @@ -310,10 +316,12 @@ unsigned long ubifs_shrink_scan(struct shrinker *shrink,
>   
>   	if (!freed && contention) {
>   		dbg_tnc("freed nothing, but contention");
> +		spin_unlock(&ubifs_shrink_lock);
>   		return SHRINK_STOP;
>   	}
>   
>   out:
>   	dbg_tnc("%lu znodes were freed, requested %lu", freed, nr);
> +	spin_unlock(&ubifs_shrink_lock);
>   	return freed;
>   }
>