Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC PATCH v3 5/8] jbd2,ext4: add a shrinker to release
 checkpointed buffers
To:     Dave Chinner <david@fromorbit.com>
CC:     <linux-ext4@vger.kernel.org>, <tytso@mit.edu>,
        <adilger.kernel@dilger.ca>, <jack@suse.cz>, <yukuai3@huawei.com>
References: <20210527135641.420514-1-yi.zhang@huawei.com>
 <20210527135641.420514-6-yi.zhang@huawei.com>
 <20210609030039.GB2419729@dread.disaster.area>
From:   Zhang Yi <yi.zhang@huawei.com>
Message-ID: <6df860b8-0e65-72d3-1276-d7f585258a87@huawei.com>
Date:   Thu, 10 Jun 2021 10:38:53 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.3.1
MIME-Version: 1.0
In-Reply-To: <20210609030039.GB2419729@dread.disaster.area>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi, Dave

On 2021/6/9 11:00, Dave Chinner wrote:
> On Thu, May 27, 2021 at 09:56:38PM +0800, Zhang Yi wrote:
>> +/**
>> + * jbd2_journal_shrink_scan()
>> + *
>> + * Scan the checkpointed buffer on the checkpoint list and release the
>> + * journal_head.
>> + */
>> +unsigned long jbd2_journal_shrink_scan(struct shrinker *shrink,
>> +				       struct shrink_control *sc)
>> +{
>> +	journal_t *journal = container_of(shrink, journal_t, j_shrinker);
>> +	unsigned long nr_to_scan = sc->nr_to_scan;
>> +	unsigned long nr_shrunk;
>> +	unsigned long count;
>> +
>> +	count = percpu_counter_sum_positive(&journal->j_jh_shrink_count);
>> +	trace_jbd2_shrink_scan_enter(journal, sc->nr_to_scan, count);
>> +
>> +	nr_shrunk = jbd2_journal_shrink_checkpoint_list(journal, &nr_to_scan);
>> +
>> +	count = percpu_counter_sum_positive(&journal->j_jh_shrink_count);
>> +	trace_jbd2_shrink_scan_exit(journal, nr_to_scan, nr_shrunk, count);
>> +
>> +	return nr_shrunk;
>> +}
>> +
>> +/**
>> + * jbd2_journal_shrink_scan()
>> + *
>> + * Count the number of checkpoint buffers on the checkpoint list.
>> + */
>> +unsigned long jbd2_journal_shrink_count(struct shrinker *shrink,
>> +					struct shrink_control *sc)
>> +{
>> +	journal_t *journal = container_of(shrink, journal_t, j_shrinker);
>> +	unsigned long count;
>> +
>> +	count = percpu_counter_sum_positive(&journal->j_jh_shrink_count);
>> +	trace_jbd2_shrink_count(journal, sc->nr_to_scan, count);
>> +
>> +	return count;
>> +}
> 
> NACK.
> 
> You should not be putting an expensive percpu counter sum in a
> shrinker object count routine. These gets called a *lot* under
> memory pressure, and summing over hundreds/thousands of CPUs on
> every direct reclaim instance over every mounted filesystem
> instance that uses jbd2 is really, really going to hurt system
> performance under memory pressure.
> 
> And, quite frankly, summing twice in the scan routine just to trace
> the result with no other purpose is unnecessary and excessive CPU
> overhead for a shrinker.
> 
> Just use percpu_counter_read() in all cases here - it is more than
> accurate enough for the purposes of both tracing and memory reclaim.

OK, I missed this point, thanks for the comments, I will use
percpu_counter_read_positive() in the next iteration.

Thanks,
Yi.