On 10/16/26 7:18 PM, Gregory Price wrote:
> On Fri, Oct 16, 2026 at 05:48:11PM +0530, Sarthak Sharma wrote:
>>> +/*
>>> + * The owner of a file may page out its clean page-cache pages. Run as an
>>> + * unprivileged owner, so that capabilities cannot stand in for ownership.
>>> + */
>>> +TEST_F(madvise, pageout_evicts_owned_file_page)
>>> +{
>>> + char *mapping, *control;
>>> +
>>> + if (!geteuid() && !drop_privileges())
>>> + SKIP(return, "could not drop privileges");
>>> + pin_to_current_cpu(_metadata);
>>
>> Is there a reason why we are pinning to the current CPU here and not in
>> the next two tests?
>
> Oversight on my part. I added it where i got a failure, but didn't
> carry it forward to the other tests.
>
>>> + ASSERT_EQ(mapping[0], 0);
>>> + ASSERT_EQ(mapping[page_size], 0);
>>> + mapping[0] = 1;
>>
>> If both pages are on CPU 1's per CPU LRU batch here and then hte task
>> migrates to CPU 2 here
>>
>>> +
>>> + ASSERT_EQ(madvise(mapping, 2 * page_size, MADV_PAGEOUT), 0);
>>
>> Then this can cause lru_add_drain() on CPU 2, and therefore both pages
>> do not reach the main LRU list. Thus they won't be reclaimed and the
>> test can skip here since the CoW page wasn't swapped.
>
> I think there are other corner cases here as well. So yes i'll set up
> thei pinning - probably worth it to just pin the entire suite of tests.
Yup, that would be better.
>
> My original set had a bunch of this tested more explicitly, but this is
> really implementation details that are not worth the additional test
> complexity. I think we can just rid ourselves of that and just pin
> every test and make sure it does what we expect in that case.
>
>>
>>> + ASSERT_TRUE(pagemap_is_populated(self->pagemap_fd,
>>> + mapping + page_size));
>>> + swapped = pagemap_is_swapped(self->pagemap_fd, mapping);
>>> + ASSERT_EQ(mapping[0], 1);
>>> + ASSERT_EQ(mapping[page_size], 0);
>>> + if (!swapped)
>>> + SKIP(return, "MADV_PAGEOUT did not swap the COW page");
>>
>> Also, here we skip if we couldn't swap out the CoW page. Won't it make
>> more sense to check with pageout_until_swapped() first if pages can be
>> skipped, and then fail here if the CoW page wasn't skipped after all the
>> retries, similar to how some other tests implement it?
>>
>
> Ah yeah good catch. I'll adjust like this:
>
> /* Ensure pageout-swap actually works first with a control page */
> control[0] = 1;
> if (!pageout_until_swapped(_metadata, control, page_size,
> self->pagemap_fd))
> SKIP(return, "MADV_PAGEOUT did not swap the control page");
>
> /* Run the actual test */
> ASSERT_EQ(mapping[0], 0);
> ASSERT_EQ(mapping[page_size], 0);
> mapping[0] = 1;
>
> ASSERT_TRUE(pageout_until_swapped(_metadata, mapping, 2 * page_size,
> self->pagemap_fd));
> ASSERT_TRUE(pagemap_is_populated(self->pagemap_fd,
> mapping + page_size));
> ASSERT_EQ(mapping[0], 1);
> ASSERT_EQ(mapping[page_size], 0);
Yes, that looks good. Thanks!
