Re: [PATCH v3 RESEND] mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED for shmem

[Charan Teja Kalla]

Thanks again Mark for the review comments!!

On 1/10/2022 6:06 PM, Mark Hemment wrote:
> On Thu, 6 Jan 2022 at 17:06, Charan Teja Reddy
> <quic_charante@xxxxxxxxxxx> wrote:
>>
>> From: Charan Teja Reddy <charante@xxxxxxxxxxxxxx>
>>
>> Currently fadvise(2) is supported only for the files that doesn't
>> associated with noop_backing_dev_info thus for the files, like shmem,
>> fadvise results into NOP. But then there is file_operations->fadvise()
>> that lets the file systems to implement their own fadvise
>> implementation. Use this support to implement some of the POSIX_FADV_XXX
>> functionality for shmem files.
> ...
>> +static void shmem_isolate_pages_range(struct address_space *mapping, loff_t start,
>> +                               loff_t end, struct list_head *list)
>> +{
>> +       XA_STATE(xas, &mapping->i_pages, start);
>> +       struct page *page;
>> +
>> +       rcu_read_lock();
>> +       xas_for_each(&xas, page, end) {
>> +               if (xas_retry(&xas, page))
>> +                       continue;
>> +               if (xa_is_value(page))
>> +                       continue;
>> +               if (!get_page_unless_zero(page))
>> +                       continue;
>> +               if (isolate_lru_page(page))
>> +                       continue;
> 
> Need to unwind the get_page on failure to isolate.

Will be done.

> 
> Should PageUnevicitable() pages (SHM_LOCK) be skipped?
> (That is, does SHM_LOCK override DONTNEED?)


Should be skipped. Will be done.

> 
> ...
>> +static int shmem_fadvise_dontneed(struct address_space *mapping, loff_t start,
>> +                               loff_t end)
>> +{
>> +       int ret;
>> +       struct page *page;
>> +       LIST_HEAD(list);
>> +       struct writeback_control wbc = {
>> +               .sync_mode = WB_SYNC_NONE,
>> +               .nr_to_write = LONG_MAX,
>> +               .range_start = 0,
>> +               .range_end = LLONG_MAX,
>> +               .for_reclaim = 1,
>> +       };
>> +
>> +       if (!shmem_mapping(mapping))
>> +               return -EINVAL;
>> +
>> +       if (!total_swap_pages)
>> +               return 0;
>> +
>> +       lru_add_drain();
>> +       shmem_isolate_pages_range(mapping, start, end, &list);
>> +
>> +       while (!list_empty(&list)) {
>> +               page = lru_to_page(&list);
>> +               list_del(&page->lru);
>> +               if (page_mapped(page))
>> +                       goto keep;
>> +               if (!trylock_page(page))
>> +                       goto keep;
>> +               if (unlikely(PageTransHuge(page))) {
>> +                       if (split_huge_page_to_list(page, &list))
>> +                               goto keep;
>> +               }
> 
> I don't know the shmem code and the lifecycle of a shm-page, so
> genuine questions;
> When the try-lock succeeds, should there be a test for PageWriteback()
> (page skipped if true)?  Also, does page->mapping need to be tested
> for NULL to prevent races with deletion from the page-cache?

I failed to envisage it. I should have considered both these conditions
here. BTW, I am just thinking about why we shouldn't use
reclaim_pages(page_list) function here with an extra set_page_dirty() on
a page that is isolated? It just call the shrink_page_list() where all
these conditions are properly handled. What is your opinion here?

> 
> ...
>> +
>> +               clear_page_dirty_for_io(page);
>> +               SetPageReclaim(page);
>> +               ret = shmem_writepage(page, &wbc);
>> +               if (ret || PageWriteback(page)) {
>> +                       if (ret)
>> +                               unlock_page(page);
>> +                       goto keep;
>> +               }
>> +
>> +               if (!PageWriteback(page))
>> +                       ClearPageReclaim(page);
>> +
>> +               /*
>> +                * shmem_writepage() place the page in the swapcache.
>> +                * Delete the page from the swapcache and release the
>> +                * page.
>> +                */
>> +               __mod_node_page_state(page_pgdat(page),
>> +                               NR_ISOLATED_ANON + page_is_file_lru(page), compound_nr(page));
>> +               lock_page(page);
>> +               delete_from_swap_cache(page);
>> +               unlock_page(page);
>> +               put_page(page);
>> +               continue;
>> +keep:
>> +               putback_lru_page(page);
>> +               __mod_node_page_state(page_pgdat(page),
>> +                               NR_ISOLATED_ANON + page_is_file_lru(page), compound_nr(page));
>> +       }
> 
> The putback_lru_page() drops the last reference hold this code has on
> 'page'.  Is it safe to use 'page' after dropping this reference?

True. Will correct it in the next revision.

> 
> Cheers,
> Mark
>