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

[Slawa Olhovchenkov]

On Mon, May 20, 2019 at 10:20:45AM -0600, Ian Lepore wrote:

> On Mon, 2019-05-20 at 19:05 +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?
> > 
> >   Looks like it should be `#ifdef illumos`.
> > 
> 
> One of the things arc_kmem_reap_soon() does is call
> dnlc_reduce_cache(), and that sets a variable and does a condition
> variable broadcast, presumably causing other threads to wake up and do
> some work.  So, presumably the delay (which appears to really be a call
> to pause(9) on freebsd) allows time for that async work to happen
> before calling arc_available_memory().

This call perform before any kmem reap and only conditional by
arc_meta_used>=arc_meta_limit. In any way kmem reap is very long
operation, longest any work in arc_dnlc_evicts_thread()
(vnlru_free()).