Re: [PATCH V7 2/2] mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED for shmem

[Hugh Dickins <hughd@google.com>]

On Mon, 24 Apr 2023, Charan Teja Kalla wrote:
> On 4/21/2023 5:37 AM, Hugh Dickins wrote:
> > This is where I ran out of time.  I'm afraid all the focus on
> > fadvise_calc_endbyte() has distracted you from looking at the DONTNEED
> > in mm/fadvise.c: where there are detailed comments on why and how it
> > then narrows the DONTNEED range.  And aside from needing to duplicate
> > that here for shmem (or put it into another or combined helper), it
> > implies to me that shmem_isolate_pages_range() needs to do a similar
> > narrowing, when it finds that the range overlaps part of a large folio.
> > 
> Sure, will include those range calculations for shmem pages too.

Oh, I forgot this issue, you would have liked me to look at V8 by now,
to see whether I agree with your resolution there.  Sorry, no, I've
not been able to divert my concentration to it yet.

And it's quite likely that I shall disagree, because I've a history of
disagreeing even with myself on such range widening/narrowing issues -
reconciling conflicting precedents is difficult :(

> 
> > Something that has crossed my mind as a worry, but I've not had time
> > to look further into (maybe it's no concern at all) is the question
> > of this syscall temporarily isolating a very large number of folios,
> > whether they need to be (or perhaps already are) counted in
> > NR_ISOLATED_ANON, whether too many isolated needs to be limited.
> 
> They are _not_ counted as ISOLATED_ANON now as this operation is for a
> small duration. I do see there exists too_many_isolated() checks in
> direct reclaim/compaction logic where it is necessary to stop the
> multiple processes in the direct reclaim from isolating too many pages.
> 
> I am not able to envisage such problem here, where usually single
> process doing the fadvise operation on a file. Even If the file is
> opened by multiple processes and do fadvise, the operation is limited
> only to the pages of this file and doesn't impact the system.
> 
> Please let me know if I'm missing something where I should be counting
> these as NR_ISOLATED.

Please grep for NR_ISOLATED, to see where and how they get manipulated
already, and follow the existing examples.  The case that sticks in my
mind is in mm/mempolicy.c, where the migrate_pages() syscall can build
up a gigantic quantity of transiently isolated pages: your syscall can
do the same, so should account for itself in the same way.

I'm not claiming that mm/vmscan.c's too_many_isolated(), and the way it
gets used by shrink_inactive_list(), is perfect: not at all.  But please
follow existing convention.

Sorry, that's all for now.
Hugh