Re: [PATCH v3 3/4] cachestat: implement cachestat syscall

[Nhat Pham]

On Mon, Dec 12, 2022 at 7:48 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Thu, Dec 08, 2022 at 12:28:07PM -0800, Nhat Pham wrote:
> > +             struct address_space *mapping = f.file->f_mapping;
> > +             pgoff_t first_index = off >> PAGE_SHIFT;
> > +             pgoff_t last_index =
> > +                     len == 0 ? ULONG_MAX : (off + len - 1) >> PAGE_SHIFT;
> > +             XA_STATE(xas, &mapping->i_pages, first_index);
> > +             struct folio *folio;
> > +
> > +             rcu_read_lock();
> > +             xas_for_each(&xas, folio, last_index) {
> > +                     if (xas_retry(&xas, folio) || !folio)
> > +                             continue;
>
> !folio can't be true.  xas_for_each() terminates when folio is NULL.

Good to know - I'll remove this in the next version.

>
> > +                     if (xa_is_value(folio)) {
> > +                             /* page is evicted */
> > +                             void *shadow = (void *)folio;
> > +                             bool workingset; /* not used */
> > +
> > +                             cs.nr_evicted += 1;
> > +
> > +#ifdef CONFIG_SWAP /* implies CONFIG_MMU */
> > +                             if (shmem_mapping(mapping)) {
> > +                                     /* shmem file - in swap cache */
> > +                                     swp_entry_t swp = radix_to_swp_entry(folio);
> > +
> > +                                     shadow = get_shadow_from_swap_cache(swp);
> > +                             }
> > +#endif
> > +                             if (workingset_test_recent(shadow, true, &workingset))
> > +                                     cs.nr_recently_evicted += 1;
> > +
> > +                             continue;
> > +                     }
> > +
> > +                     /* page is in cache */
> > +                     cs.nr_cache += 1;
> > +
> > +                     if (folio_test_dirty(folio))
> > +                             cs.nr_dirty += 1;
> > +
> > +                     if (folio_test_writeback(folio))
> > +                             cs.nr_writeback += 1;
>
> A folio may represent more than one page.  That's the point of folios.
> So there should be something in here which does
>
>         unsigned long nr = folio_nr_pages();
>
> and then all these '1' should be 'nr'.

Yikes, that's a bug that I missed! Thanks for pointing it out.

> Except that you may need to adjust nr if 'first_index' > folio->index, or
> 'last_index' < folio->index + nr.

Sounds good!

>
> You should test this with XFS, AFS or erofs to be sure you're getting
> results that look right.
>