Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Song Liu <songliubraving@fb.com>
To:     Johannes Weiner <hannes@cmpxchg.org>
CC:     Linux-MM <linux-mm@kvack.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Matthew Wilcox <matthew.wilcox@oracle.com>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Kernel Team <Kernel-team@fb.com>,
        "William Kucharski" <william.kucharski@oracle.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "hdanton@sina.com" <hdanton@sina.com>
Subject: Re: [PATCH v9 5/6] mm,thp: add read-only THP support for (non-shmem)
 FS
Thread-Topic: [PATCH v9 5/6] mm,thp: add read-only THP support for (non-shmem)
 FS
Thread-Index: AQHVKurnm0pZpZevHki8fTgvnJjHiqbESjWAgBMt6IA=
Date:   Mon, 22 Jul 2019 23:41:25 +0000
Message-ID: <D2D86FEC-6AAA-4051-A58C-27F305F310A8@fb.com>
References: <20190625001246.685563-1-songliubraving@fb.com>
 <20190625001246.685563-6-songliubraving@fb.com>
 <20190710184811.GF11197@cmpxchg.org>
In-Reply-To: <20190710184811.GF11197@cmpxchg.org>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C2D0730775384D489CA840F0BE9ADA62@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: fc390eae-4e6a-472e-f34c-08d70efe1c86
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Jul 2019 23:41:25.5489
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: songliubraving@fb.com
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR15MB1198
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-07-22_16:,,
 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 priorityscore=1501
 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0
 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0
 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.0.1-1810050000 definitions=main-1907220250
X-FB-Internal: deliver
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Jul 10, 2019, at 11:48 AM, Johannes Weiner <hannes@cmpxchg.org> wrote:
>=20
> On Mon, Jun 24, 2019 at 05:12:45PM -0700, Song Liu wrote:
>> This patch is (hopefully) the first step to enable THP for non-shmem
>> filesystems.
>>=20
>> This patch enables an application to put part of its text sections to TH=
P
>> via madvise, for example:
>>=20
>>    madvise((void *)0x600000, 0x200000, MADV_HUGEPAGE);
>>=20
>> We tried to reuse the logic for THP on tmpfs.
>>=20
>> Currently, write is not supported for non-shmem THP. khugepaged will onl=
y
>> process vma with VM_DENYWRITE. sys_mmap() ignores VM_DENYWRITE requests
>> (see ksys_mmap_pgoff). The only way to create vma with VM_DENYWRITE is
>> execve(). This requirement limits non-shmem THP to text sections.
>>=20
>> The next patch will handle writes, which would only happen when the all
>> the vmas with VM_DENYWRITE are unmapped.
>>=20
>> An EXPERIMENTAL config, READ_ONLY_THP_FOR_FS, is added to gate this
>> feature.
>>=20
>> Acked-by: Rik van Riel <riel@surriel.com>
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>=20
> This is really cool, and less invasive than I anticipated. Nice work.
>=20
> I only have one concern and one question:
>=20
>> @@ -1392,6 +1401,29 @@ static void collapse_file(struct mm_struct *mm,
>> 				result =3D SCAN_FAIL;
>> 				goto xa_unlocked;
>> 			}
>> +		} else if (!page || xa_is_value(page)) {
>> +			xas_unlock_irq(&xas);
>> +			page_cache_sync_readahead(mapping, &file->f_ra, file,
>> +						  index, PAGE_SIZE);
>> +			/* drain pagevecs to help isolate_lru_page() */
>> +			lru_add_drain();
>> +			page =3D find_lock_page(mapping, index);
>> +			if (unlikely(page =3D=3D NULL)) {
>> +				result =3D SCAN_FAIL;
>> +				goto xa_unlocked;
>> +			}
>> +		} else if (!PageUptodate(page)) {
>> +			VM_BUG_ON(is_shmem);
>> +			xas_unlock_irq(&xas);
>> +			wait_on_page_locked(page);
>> +			if (!trylock_page(page)) {
>> +				result =3D SCAN_PAGE_LOCK;
>> +				goto xa_unlocked;
>> +			}
>> +			get_page(page);
>> +		} else if (!is_shmem && PageDirty(page)) {
>> +			result =3D SCAN_FAIL;
>> +			goto xa_locked;
>> 		} else if (trylock_page(page)) {
>> 			get_page(page);
>> 			xas_unlock_irq(&xas);
>=20
> The many else ifs here check fairly complex page state and are hard to
> follow and verify mentally. In fact, it's a bit easier now in the
> patch when you see how it *used* to work with just shmem, but the end
> result is fragile from a maintenance POV.
>=20
> The shmem and file cases have little in common - basically only the
> trylock_page(). Can you please make one big 'if (is_shmem) {} {}'
> structure instead that keeps those two scenarios separate?

Good point! Will fix in next version.=20

>=20
>> @@ -1426,6 +1458,12 @@ static void collapse_file(struct mm_struct *mm,
>> 			goto out_unlock;
>> 		}
>>=20
>> +		if (page_has_private(page) &&
>> +		    !try_to_release_page(page, GFP_KERNEL)) {
>> +			result =3D SCAN_PAGE_HAS_PRIVATE;
>> +			break;
>> +		}
>> +
>> 		if (page_mapped(page))
>> 			unmap_mapping_pages(mapping, index, 1, false);
>=20
>> @@ -1607,6 +1658,17 @@ static void khugepaged_scan_file(struct mm_struct=
 *mm,
>> 			break;
>> 		}
>>=20
>> +		if (page_has_private(page) && trylock_page(page)) {
>> +			int ret;
>> +
>> +			ret =3D try_to_release_page(page, GFP_KERNEL);
>> +			unlock_page(page);
>> +			if (!ret) {
>> +				result =3D SCAN_PAGE_HAS_PRIVATE;
>> +				break;
>> +			}
>> +		}
>> +
>> 		if (page_count(page) !=3D 1 + page_mapcount(page)) {
>> 			result =3D SCAN_PAGE_COUNT;
>> 			break;
>=20
> There is already a try_to_release() inside the page lock section in
> collapse_file(). I'm assuming you added this one because private data
> affects the refcount. But it seems a bit overkill just for that; we
> could also still fail the check, in which case we'd have dropped the
> buffers in vain. Can you fix the check instead?
>=20
> There is an is_page_cache_freeable() function in vmscan.c that handles
> private fs references:
>=20
> static inline int is_page_cache_freeable(struct page *page)
> {
> 	/*
> 	 * A freeable page cache page is referenced only by the caller
> 	 * that isolated the page, the page cache and optional buffer
> 	 * heads at page->private.
> 	 */
> 	int page_cache_pins =3D PageTransHuge(page) && PageSwapCache(page) ?
> 		HPAGE_PMD_NR : 1;
> 	return page_count(page) - page_has_private(page) =3D=3D 1 + page_cache_p=
ins;
> }
>=20
> Wouldn't this work here as well?

Good point! Let me try fix this.=20

Thanks,
Song