Commit r345200 (new ARC reclamation threads) looks suspicious to me - second potential problem

[Alexander Motin]

On 20.05.2019 12:42, Mark Johnston wrote:
> On Mon, May 20, 2019 at 07:05:07PM +0300, Lev Serebryakov wrote:
>>
>>  I'm looking at last commit to
>> 'sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c' (r345200) and
>> have another question.
>>
>>  Here are such code:
>>
>> 4960 	        /*
>> 4961 	         * Kick off asynchronous kmem_reap()'s of all our caches.
>> 4962 	         */
>> 4963 	        arc_kmem_reap_soon();
>> 4964 	
>> 4965 	        /*
>> 4966 	         * Wait at least arc_kmem_cache_reap_retry_ms between
>> 4967 	         * arc_kmem_reap_soon() calls. Without this check it is
>> possible to
>> 4968 	         * end up in a situation where we spend lots of time reaping
>> 4969 	         * caches, while we're near arc_c_min.  Waiting here also
>> gives the
>> 4970 	         * subsequent free memory check a chance of finding that the
>> 4971 	         * asynchronous reap has already freed enough memory, and
>> we don't
>> 4972 	         * need to call arc_reduce_target_size().
>> 4973 	         */
>> 4974 	        delay((hz * arc_kmem_cache_reap_retry_ms + 999) / 1000);
>> 4975 	
>>
>>  But looks like `arc_kmem_reap_soon()` is synchronous on FreeBSD! So,
>> this `delay()` looks very wrong. Am I right?

Why is it wrong?

>>   Looks like it should be `#ifdef illumos`.
> 
> See also r338142, which I believe was reverted by the update.

My r345200 indeed reverted that value, but I don't see a problem there.
When OS need more RAM, pagedaemon will drain UMA caches by itself.  I
don't see a point in re-draining UMA caches in a tight loop without
delay.  If caches are not sufficient to sustain one second of workload,
then usefulness of such caches is not very clear and shrinking ARC to
free some space may be a right move.  Also making ZFS drain its caches
more active then anything else in a system looks unfair to me.

-- 
Alexander Motin