On Thu, Feb 10, 2022 at 02:03:23PM -0500, Brian Foster wrote: > On Wed, Feb 02, 2022 at 01:22:40PM +1100, Dave Chinner wrote: > > On Mon, Jan 24, 2022 at 11:57:12AM -0500, Brian Foster wrote: > > > On Wed, Jan 19, 2022 at 04:36:36PM -0800, Darrick J. Wong wrote: > > > > > Of course if you wanted to recycle inactive inodes or do something else > > > > > entirely, then it's probably not worth going down this path.. > > > > > > > > I'm a bit partial to /trying/ to recycle inactive inodes because (a) > > > > it's less tangling with -fsdevel for you and (b) inode scans in the > > > > online repair patchset got a little weird once xfs_iget lost the ability > > > > to recycle _NEEDS_INACTIVE inodes... > > > > > > > > OTOH I tried to figure out how to deal with the lockless list that those > > > > inodes are put on, and I couldn't figure out how to get them off the > > > > list safely, so that might be a dead end. If you have any ideas I'm all > > > > ears. :) > > > > > > > > > > So one of the things that I've been kind of unclear on about the current > > > deferred inactivation implementation is the primary goal of the percpu > > > optimization. I obviously can see the value of the percpu list in > > > general, but how much processing needs to occur in percpu context to > > > achieve the primary goal? > > > > > > For example, I can see how a single or small multi threaded sustained > > > file removal might be batched efficiently, but what happens if said task > > > happens to bounce around many cpus? > > > > In that case we have a scheduler problem, not a per-cpu > > infrastructure issue. > > > > Last I tested on my box, a single threaded rm -rf had executed on > something like 24 of the 80 cpus available after about ~1m of runtime. Not surprising - the scheduler will select a sibling cores that share caches when the previous CPU the task was running on is busy running CPU affine tasks (i.e. the per-cpu inactive kworker thread). But how many CPUs it bounces the workload around over a long period is not important. What is important is the cache residency between the inodes when they are queued and when they are then inactivated. That's measured in microseconds to single digit milliseconds (i.e. within a single scheduling tick), and this is the metric that matters the most. It doesn't matter if the first batch is unlinked on CPU 1 and then inactived on CPU 1 while the scheduler moves the unlink task to CPU 2 where it queues the next batch to be inactivated on CPU 2, and so on. What matters is the data set in each batch remains on the same CPU for inactivation processing. The tracing I've done indicates taht the majority of the time that the scehduler bounces the tasks between two or three sibling CPUs repeatedly. This occurs when the inactivation is keeping up with the unlink queueing side. When we have lots of extents to remove in inactivation, the amount of inactivation work is much greater than the unlink() work, and so we see inactivation batch processing taking longer and the CPU spread of the unlink task gets larger because there are more CPUs running CPU-affine tasks doing background inactivation. IOWs, having the number of CPUs the unlink task is scheduled on grow over the long term is not at all unexpected - this is exactly what we'd expect to see when we move towards async background processing of complex operations... > Of course the inactivation work for an inode occurs on the cpu that the > rm task was running on when the inode was destroyed, but I don't think > there's any guarantee that kworker task will be the next task selected > on the cpu if the system is loaded with other runnable tasks (and then > whatever task is selected doesn't pollute the cache). The scheduler will select the next CPU based on phsyical machine topology - core latencies, shared caches, numa distances, whether the target CPU has affinity bound tasks already queued, etc. > For example, I > also noticed rm-<pidX> -> rm-<pidY> context switching on concurrent > remove tests. That is obviously not a caching issue in this case because > both tasks are still doing remove work, but behavior is less > deterministic of the target task happens to be something unrelated. Of > course, that doesn't mean the approach might not otherwise be effective > for the majority of common workloads.. As long as the context switch rate does not substantially increase, having tasks migrate between sibling cores every so often isn't a huge problem. > > That per-ag based back end processing is exactly what Darrick's > > original proposals did: > > > > https://lore.kernel.org/linux-xfs/162758431072.332903.17159226037941080971.stgit@magnolia/ > > > > It used radix tree walks run in background kworker threads to batch > > all the inode inactivation in a given AG via radix tree walks to > > find them. > > > > It performed poorly at scale because it destroyed the CPU affinity > > between the unlink() operation and the inactivation operation of the > > unlinked inode when the last reference to the inode is dropped and > > the inode evicted from task syscall exit context. REgardless of > > whether there is a per-cpu front end or not, the per-ag based > > processing will destroy the CPU affinity of the data set being > > processed because we cannot guarantee that the per-ag objects are > > all local to the CPU that is processing the per-ag objects.... > > > > Ok. The role/significance of cpu caching was not as apparent to me when > I had last replied to this thread. The discussion around the rcu inode > lifecycle issue helped clear some of that up. > > That said, why not conditionally tag and divert to a background worker > when the inodegc is disabled? That could allow NEEDS_INACTIVE inodes to > be claimed/recycled from other contexts in scenarios like when the fs is > frozen, since they won't be stuck in inaccessible and inactive percpu > queues, but otherwise preserves current behavior in normal runtime > conditions. Darrick mentioned online repair wanting to do something > similar earlier, but it's not clear to me if scrub could or would want > to disable the percpu inodegc workers in favor of a temporary/background > mode while repair is running. I'm just guessing that performance is > probably small enough of a concern in that situation that it wouldn't be > a mitigating factor. Hm? WE probably could do this, but I'm not sure the complexity is justified by the rarity of the problem it is trying to avoid. Freezes are not long term, nor are they particularly common for performance sensitive workloads. Hence I'm just not this corner case is important enough to justify doing the work given that we've had similar freeze-will-delay-some-stuff-indefinitely behaviour for a long time... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
