linux-ext4 - Re: [RFC PATCH v3 5/8] jbd2,ext4: add a shrinker to release checkpointed buffers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <6df860b8-0e65-72d3-1276-d7f585258a87@huawei.com>
Date:   Thu, 10 Jun 2021 10:38:53 +0800
From:   Zhang Yi <yi.zhang@...wei.com>
To:     Dave Chinner <david@...morbit.com>
CC:     <linux-ext4@...r.kernel.org>, <tytso@....edu>,
        <adilger.kernel@...ger.ca>, <jack@...e.cz>, <yukuai3@...wei.com>
Subject: Re: [RFC PATCH v3 5/8] jbd2,ext4: add a shrinker to release
 checkpointed buffers

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.