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

From: Charan Teja Kalla <quic_charante@quicinc.com>
To: Frank van der Linden <fvdl@google.com>
Cc: <akpm@linux-foundation.org>, <hughd@google.com>,
	<willy@infradead.org>, <markhemm@googlemail.com>,
	<rientjes@google.com>, <surenb@google.com>, <shakeelb@google.com>,
	<quic_pkondeti@quicinc.com>, <linux-mm@kvack.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V7 0/2] mm: shmem: support POSIX_FADV_[WILL|DONT]NEED for shmem files
Date: Sat, 15 Apr 2023 00:40:17 +0530	[thread overview]
Message-ID: <0853b4b0-770f-f742-95bc-eb74a1859138@quicinc.com> (raw)
In-Reply-To: <CAPTztWadceJtPUrSab1Tj2WV=uAhLo+CrxqyeSQ8rLn0FtM_zA@mail.gmail.com>

Thanks Frank!!

On 4/14/2023 11:14 PM, Frank van der Linden wrote:
>> I didn't see this patch before, so I looked a bit at the history. At
>> some point, in v3, dealing with mapped pages for DONTNEED was left
>> out, they are now skipped. Unfortunately, that makes this patch no
>> longer usable for a case that we have: restoring the (approximate)
>> swap state of a tmpfs file. This involves walking a potentially large
>> number of regions, and explicitly pushing them out to swap. This can
>> be used to e.g. restore the state VM memory that is backed by a tmpfs
>> file, avoiding memory usage by cold VM pages after resume.
>>

This is an interesting use case and I feel this really supports this
patchset. IIUC, supporting the reclaim of mapped file pages through
fadvise() helps this usecase where you can avoid traversing the large
number of vma regions as you can directly issue the fadvise() on the
shmem file 'fd' and it takes care. Am I correct?

> Hmm, actually, looking at it a bit more, there are several issues
> here. One is that with fadvise, you can't be sure if you are the only
> one dealing with the page in a mapped way(with madvise, if mapcount ==
> 1, that mean's it's just you, but you don't know that for fadvise, so
> that makes correctly dealing with mapped pages harder).
> 
Sorry, Why not for fadvise()? I can still attempt only if the page is
mapped and its mapcount == 1, but then we already have madvise() for
such pages and why not we simply use it.

> Also, the madvise loop issue can probably also be dealt with via io_uring.
> 
> So, I think we can deal with the use case I mentioned in a different
> way, that is mostly unrelated to this patchset, so basically:
> disregard my previous email.
Sure! If this patch looks good for you, Reviewed/Ack tags can help me..

Thanks,
Charan