commit 7e934cf5ace1dceeb804f7493fa28bb697ed3c52 upstream.
xas_for_each_marked() is using entry == NULL as a termination condition
of the iteration. When xas_for_each_marked() is used protected only by
RCU, this can however race with xas_store(xas, NULL) in the following
way:
TASK1 TASK2
page_cache_delete() find_get_pages_range_tag()
xas_for_each_marked()
xas_find_marked()
off = xas_find_chunk()
xas_store(&xas, NULL)
xas_init_marks(&xas);
...
rcu_assign_pointer(*slot, NULL);
entry = xa_entry(off);
And thus xas_for_each_marked() terminates prematurely possibly leading
to missed entries in the iteration (translating to missing writeback of
some pages or a similar problem).
If we find a NULL entry that has been marked, skip it (unless we're trying
to allocate an entry).
Reported-by: Jan Kara <jack@suse.cz>
CC: stable@vger.kernel.org
Fixes: ef8e5717db01 ("page cache: Convert delete_batch to XArray")
Change-Id: I99b5db5bfdc34e9fa4bfc0a963d5e93dde33fa38
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Since a28334862993 ("page cache: Finish XArray conversion"), on most
major Linux distributions, the page cache doesn't correctly transition
when the hot data set is changing, and leaves the new pages thrashing
indefinitely instead of kicking out the cold ones.
On a freshly booted, freshly ssh'd into virtual machine with 1G RAM
running stock Arch Linux:
[root@ham ~]# ./reclaimtest.sh
+ dd of=workingset-a bs=1M count=0 seek=600
+ cat workingset-a
+ cat workingset-a
+ cat workingset-a
+ cat workingset-a
+ cat workingset-a
+ cat workingset-a
+ cat workingset-a
+ cat workingset-a
+ ./mincore workingset-a
153600/153600 workingset-a
+ dd of=workingset-b bs=1M count=0 seek=600
+ cat workingset-b
+ cat workingset-b
+ cat workingset-b
+ cat workingset-b
+ ./mincore workingset-a workingset-b
104029/153600 workingset-a
120086/153600 workingset-b
+ cat workingset-b
+ cat workingset-b
+ cat workingset-b
+ cat workingset-b
+ ./mincore workingset-a workingset-b
104029/153600 workingset-a
120268/153600 workingset-b
workingset-b is a 600M file on a 1G host that is otherwise entirely
idle. No matter how often it's being accessed, it won't get cached.
While investigating, I noticed that the non-resident information gets
aggressively reclaimed - /proc/vmstat::workingset_nodereclaim. This is
a problem because a workingset transition like this relies on the
non-resident information tracked in the page cache tree of evicted
file ranges: when the cache faults are refaults of recently evicted
cache, we challenge the existing active set, and that allows a new
workingset to establish itself.
Tracing the shrinker that maintains this memory revealed that all page
cache tree nodes were allocated to the root cgroup. This is a problem,
because 1) the shrinker sizes the amount of non-resident information
it keeps to the size of the cgroup's other memory and 2) on most major
Linux distributions, only kernel threads live in the root cgroup and
everything else gets put into services or session groups:
[root@ham ~]# cat /proc/self/cgroup
0::/user.slice/user-0.slice/session-c1.scope
As a result, we basically maintain no non-resident information for the
workloads running on the system, thus breaking the caching algorithm.
Looking through the code, I found the culprit in the above-mentioned
patch: when switching from the radix tree to xarray, it dropped the
__GFP_ACCOUNT flag from the tree node allocations - the flag that
makes sure the allocated memory gets charged to and tracked by the
cgroup of the calling process - in this case, the one doing the fault.
To fix this, allow xarray users to specify per-tree flag that makes
xarray allocate nodes using __GFP_ACCOUNT. Then restore the page cache
tree annotation to request such cgroup tracking for the cache nodes.
With this patch applied, the page cache correctly converges on new
workingsets again after just a few iterations:
[root@ham ~]# ./reclaimtest.sh
+ dd of=workingset-a bs=1M count=0 seek=600
+ cat workingset-a
+ cat workingset-a
+ cat workingset-a
+ cat workingset-a
+ cat workingset-a
+ cat workingset-a
+ cat workingset-a
+ cat workingset-a
+ ./mincore workingset-a
153600/153600 workingset-a
+ dd of=workingset-b bs=1M count=0 seek=600
+ cat workingset-b
+ ./mincore workingset-a workingset-b
124607/153600 workingset-a
87876/153600 workingset-b
+ cat workingset-b
+ ./mincore workingset-a workingset-b
81313/153600 workingset-a
133321/153600 workingset-b
+ cat workingset-b
+ ./mincore workingset-a workingset-b
63036/153600 workingset-a
153600/153600 workingset-b
Cc: stable@vger.kernel.org # 4.20+
Change-Id: I6a27c715c83ce74f336a7af98f84d1561d266c95
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Reviewed-by: Shakeel Butt <shakeelb@google.com>
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Jason feels this is clearer, and it saves a function and an exported
symbol.
Suggested-by: Jason Gunthorpe <jgg@ziepe.ca>
Change-Id: I487b88ee59e5122648c2df56a5e052a85bed5fef
Signed-off-by: Matthew Wilcox <willy@infradead.org>
xa_cmpxchg() was a little too magic in turning ZERO entries into NULL,
and would leave the entry set to the ZERO entry instead of releasing
it for future use. After careful review of existing users of
xa_cmpxchg(), change the semantics so that it does not translate either
incoming argument from NULL into ZERO entries.
Add several tests to the test-suite to make sure this problem doesn't
come back.
Reported-by: Jason Gunthorpe <jgg@ziepe.ca>
Change-Id: I846221b090e9e1ea678c17f9363f6f0ad2fd2f91
Signed-off-by: Matthew Wilcox <willy@infradead.org>
This differs slightly from the IDR equivalent in five ways.
1. It can allocate up to UINT_MAX instead of being limited to INT_MAX,
like xa_alloc(). Also like xa_alloc(), it will write to the 'id'
pointer before placing the entry in the XArray.
2. The 'next' cursor is allocated separately from the XArray instead
of being part of the IDR. This saves memory for all the users which
do not use the cyclic allocation API and suits some users better.
3. It returns -EBUSY instead of -ENOSPC.
4. It will attempt to wrap back to the minimum value on memory allocation
failure as well as on an -EBUSY error, assuming that a user would
rather allocate a small ID than suffer an ID allocation failure.
5. It reports whether it has wrapped, which is important to some users.
Change-Id: Ic44147430ef41524901111c3743a73de68592ea4
Signed-off-by: Matthew Wilcox <willy@infradead.org>
It was too easy to forget to initialise the start index. Add an
xa_limit data structure which can be used to pass min & max, and
define a couple of special values for common cases. Also add some
more tests cribbed from the IDR test suite. Change the return value
from -ENOSPC to -EBUSY to match xa_insert().
Change-Id: I9f624fabe03a59da190560aac0dbb20e73b12ff1
Signed-off-by: Matthew Wilcox <willy@infradead.org>
A lot of places want to allocate IDs starting at 1 instead of 0.
While the xa_alloc() API supports this, it's not very efficient if lots
of IDs are allocated, due to having to walk down to the bottom of the
tree to see if ID 1 is available, then all the way over to the next
non-allocated ID. This method marks ID 0 as being occupied which wastes
one slot in the XArray, but preserves xa_empty() as working.
Change-Id: I75bb9323676903892b448e145eb4d6f4e7c16353
Signed-off-by: Matthew Wilcox <willy@infradead.org>
Specifying a starting ID greater than the maximum ID isn't something
attempted very often, but it should fail. It was succeeding due to
xas_find_marked() returning the wrong error state, so add tests for
both xa_alloc() and xas_find_marked().
Fixes: b803b42823d0 ("xarray: Add XArray iterators")
Change-Id: I87b125bf66950d6decd5c952a38f5390ecd2bd84
Signed-off-by: Matthew Wilcox <willy@infradead.org>
Userspace translates EEXIST to "File exists" which isn't a very good
error message for the problem. "Device or resource busy" is a better
indication of what went wrong.
Change-Id: I634e32f94b1a82d7c44e90a4d33f0d2de13e3a39
Signed-off-by: Matthew Wilcox <willy@infradead.org>
xa_erase does not allocate memory and doesn't have a gfp parameter.
Update the descriptions of all four variants to be more useful.
Change-Id: I0d8445c16b3d42150b0e4d131f97571aef0a9bb4
Signed-off-by: Matthew Wilcox <willy@infradead.org>
xa_insert() should treat reserved entries as occupied, not as available.
Also, it should treat requests to insert a NULL pointer as a request
to reserve the slot. Add xa_insert_bh() and xa_insert_irq() for
completeness.
Change-Id: I865ef4fa5cf997ad154abd0776664d9ee5d09aa7
Signed-off-by: Matthew Wilcox <willy@infradead.org>
On m68k, statically allocated pointers may only be two-byte aligned.
This clashes with the XArray's method for tagging internal pointers.
Permit storing these pointers in single slots (ie not in multislots).
Change-Id: I6d6944b57a5ea94c16240f2c91ab6da10482e3a7
Signed-off-by: Matthew Wilcox <willy@infradead.org>
The following sequence of calls would result in an infinite loop in
xa_find_after():
xa_store(xa, 0, x, GFP_KERNEL);
index = 0;
xa_for_each(xa, entry, index, ULONG_MAX, XA_PRESENT) { }
xa_find_after() was confusing the situation where we found no entry in
the tree with finding a multiorder entry, so it would look for the
successor entry forever. Just check for this case explicitly. Includes
a few new checks in the test suite to be sure this doesn't reappear.
Change-Id: I6e0b127663cd3f2d5c385f6d7507132ae2082f71
Signed-off-by: Matthew Wilcox <willy@infradead.org>
A regular xa_init_flags() put all dynamically-initialised XArrays into
the same locking class. That leads to lockdep believing that taking
one XArray lock while holding another is a deadlock. It's possible to
work around some of these situations with separate locking classes for
irq/bh/regular XArrays, and SINGLE_DEPTH_NESTING, but that's ugly, and
it doesn't work for all situations (where we have completely unrelated
XArrays).
Change-Id: I3aaba0a1c411b7941aa5bd9e92cd939ac66aef5d
Signed-off-by: Matthew Wilcox <willy@infradead.org>
Make xa_erase() take the spinlock and then call __xa_erase(), but make
it out of line since it's such a common function.
Change-Id: I878a84e3cb0833e7b84807986cef1bb1ad5bb5fe
Signed-off-by: Matthew Wilcox <willy@infradead.org>
xa_cmpxchg() was one of the largest functions in the xarray
implementation. By turning it into a wrapper and having the callers
take the lock (like several other functions), we save 160 bytes on a
tinyconfig build and reduce the duplication in xarray.c.
Change-Id: I31c97a82dae4eaab5ddaada6901092a94d392cb0
Signed-off-by: Matthew Wilcox <willy@infradead.org>
The xa_reserve() function was a little unusual in that it attempted to
be callable for all kinds of locking scenarios. Make it look like the
other APIs with __xa_reserve, xa_reserve_bh and xa_reserve_irq variants.
Change-Id: I292670b8f2610bd87f51cd201aa5616e718f7bfd
Signed-off-by: Matthew Wilcox <willy@infradead.org>
This version of xa_store_range() really only supports load and store.
Our only user only needs basic load and store functionality, so there's
no need to do the extra work to support marking and overlapping stores
correctly yet.
Change-Id: I240de2bbfd3113758f810b152dfebff110a2c35f
Signed-off-by: Matthew Wilcox <willy@infradead.org>
Add the optional ability to track which entries in an XArray are free
and provide xa_alloc() to replace most of the functionality of the IDR.
Change-Id: I1e51411a11f6007bb5c290a452872567aa7a5c02
Signed-off-by: Matthew Wilcox <willy@infradead.org>
This function reserves a slot in the XArray for users which need
to acquire multiple locks before storing their entry in the tree and
so cannot use a plain xa_store().
Change-Id: Ic8073c1f9fde56d5e239ebed66fe3c92b4ed26f6
Signed-off-by: Matthew Wilcox <willy@infradead.org>
This hopefully temporary function is useful for users who have not yet
been converted to multi-index entries.
Change-Id: I2edc893d5e723c21eaf9efbf88deba8831af6055
Signed-off-by: Matthew Wilcox <willy@infradead.org>
This iterator iterates over each entry that is stored in the index or
indices specified by the xa_state. This is intended for use for a
conditional store of a multiindex entry, or to allow entries which are
about to be removed from the xarray to be disposed of properly.
Change-Id: I711639d3ce472e596503a985e551999b4ffa0d52
Signed-off-by: Matthew Wilcox <willy@infradead.org>
The xas_next and xas_prev functions move the xas index by one position,
and adjust the rest of the iterator state to match it. This is more
efficient than calling xas_set() as it keeps the iterator at the leaves
of the tree instead of walking the iterator from the root each time.
Change-Id: I2db0ff13439561c45debd089a68dfe8637e581b5
Signed-off-by: Matthew Wilcox <willy@infradead.org>
This function frees all the internal memory allocated to the xarray
and reinitialises it to be empty.
Change-Id: I3981880850e53bee90ae8472c1b3d6f7fcbe2ba8
Signed-off-by: Matthew Wilcox <willy@infradead.org>
The xa_extract function combines the functionality of
radix_tree_gang_lookup() and radix_tree_gang_lookup_tagged().
It extracts entries matching the specified filter into a normal array.
Change-Id: I87805acc6e0f33b8a640dfd019f3cebe50bf0d2f
Signed-off-by: Matthew Wilcox <willy@infradead.org>
The xa_for_each iterator allows the user to efficiently walk a range
of the array, executing the loop body once for each entry in that
range that matches the filter. This commit also includes xa_find()
and xa_find_after() which are helper functions for xa_for_each() but
may also be useful in their own right.
In the xas family of functions, we have xas_for_each(), xas_find(),
xas_next_entry(), xas_for_each_tagged(), xas_find_tagged(),
xas_next_tagged() and xas_pause().
Change-Id: I8a8bf73a071c637a8834096f37ec58bb9e947bb2
Signed-off-by: Matthew Wilcox <willy@infradead.org>
Like cmpxchg(), xa_cmpxchg will only store to the index if the current
entry matches the old entry. It returns the current entry, which is
usually more useful than the errno returned by radix_tree_insert().
For the users who really only want the errno, the xa_insert() wrapper
provides a more convenient calling convention.
Change-Id: Ic1b6b7fa94a4f7c5c3fbd95872fcd8e16f8327d4
Signed-off-by: Matthew Wilcox <willy@infradead.org>
xa_store() differs from radix_tree_insert() in that it will overwrite an
existing element in the array rather than returning an error. This is
the behaviour which most users want, and those that want more complex
behaviour generally want to use the xas family of routines anyway.
For memory allocation, xa_store() will first attempt to request memory
from the slab allocator; if memory is not immediately available, it will
drop the xa_lock and allocate memory, keeping a pointer in the xa_state.
It does not use the per-CPU cache, although those will continue to exist
until all radix tree users are converted to the xarray.
This patch also includes xa_erase() and __xa_erase() for a streamlined
way to store NULL. Since there is no need to allocate memory in order
to store a NULL in the XArray, we do not need to trouble the user with
deciding what memory allocation flags to use.
Change-Id: Ice8910b2746c054c47944c79f7ba115b2cebcea2
Signed-off-by: Matthew Wilcox <willy@infradead.org>
XArray marks are like the radix tree tags, only slightly more strongly
typed. They are renamed in order to distinguish them from tagged
pointers. This commit adds the basic get/set/clear operations.
Change-Id: Ibd405ae12bbd0de3609e52a555d485d8e2bec5d9
Signed-off-by: Matthew Wilcox <willy@infradead.org>
The xa_load function brings with it a lot of infrastructure; xa_empty(),
xa_is_err(), and large chunks of the XArray advanced API that are used
to implement xa_load.
As the test-suite demonstrates, it is possible to use the XArray functions
on a radix tree. The radix tree functions depend on the GFP flags being
stored in the root of the tree, so it's not possible to use the radix
tree functions on an XArray.
Change-Id: I63189fb05efb31166da1da4e2df6ed417f6600fa
Signed-off-by: Matthew Wilcox <willy@infradead.org>
This is a direct replacement for struct radix_tree_root. Some of the
struct members have changed name; convert those, and use a #define so
that radix_tree users continue to work without change.
Change-Id: I5a2ed4f4b39af3c0801ba1131c063dd796174af5
Signed-off-by: Matthew Wilcox <willy@infradead.org>
Reviewed-by: Josef Bacik <jbacik@fb.com>
