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:     Andrew Morton <akpm@linux-foundation.org>
CC:     open list <linux-kernel@vger.kernel.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "matthew.wilcox@oracle.com" <matthew.wilcox@oracle.com>,
        Kernel Team <Kernel-team@fb.com>,
        "william.kucharski@oracle.com" <william.kucharski@oracle.com>,
        "kirill.shutemov@linux.intel.com" <kirill.shutemov@linux.intel.com>,
        "Johannes Weiner" <hannes@cmpxchg.org>,
        Hugh Dickins <hughd@google.com>
Subject: Re: [PATCH v3] mm,thp: recheck each page before collapsing file THP
Thread-Topic: [PATCH v3] mm,thp: recheck each page before collapsing file THP
Thread-Index: AQHVhd5xLYpzi8cJoUynxajagY1vhadhKiEAgABE9QCAHELcAIAADSqA
Date:   Wed, 6 Nov 2019 05:45:41 +0000
Message-ID: <1C277765-F16E-4818-855C-FB0D1EC23772@fb.com>
References: <20191018180345.4188310-1-songliubraving@fb.com>
 <20191018181712.91dd9e9f9941642300e1b8d9@linux-foundation.org>
 <9DC29F5B-1DF5-408F-BEDF-FD1FBAAB1361@fb.com>
 <20191105205834.aaebbbfead54637d17a84775@linux-foundation.org>
In-Reply-To: <20191105205834.aaebbbfead54637d17a84775@linux-foundation.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: <32F903B0D4D2EA4FB32FFA392B373E1D@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: b1b0dbb0-bf0d-4f0d-caa7-08d7627c8f34
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Nov 2019 05:45:41.1584
 (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: H+J+oNbMT/J+UBLQisKYGUAbM3oeaxhB00GogppC0DAxgjB1cRf0OSEG623+sM8KBTgx65vbs+2IFXXDFjbkpw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR15MB1280
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.95,18.0.572
 definitions=2019-11-06_01:2019-11-05,2019-11-06 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 mlxscore=0 bulkscore=0
 phishscore=0 suspectscore=0 malwarescore=0 adultscore=0 impostorscore=0
 lowpriorityscore=0 spamscore=0 clxscore=1015 mlxlogscore=999
 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-1908290000 definitions=main-1911060059
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 Nov 5, 2019, at 8:58 PM, Andrew Morton <akpm@linux-foundation.org> wro=
te:
>=20
> On Sat, 19 Oct 2019 05:24:00 +0000 Song Liu <songliubraving@fb.com> wrote=
:
>=20
>>> We don't have a ref on that page.  After we've released the xarray lock
>>> we have no business playing with *page at all, correct?
>>=20
>> Yeah, this piece is not just redundant, but also buggy. I am also=20
>> including some information about it.=20
>>=20
>> Updated commit log:
>>=20
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D 8< =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>=20
>> In collapse_file(), for !is_shmem case, current check cannot guarantee=20
>> the locked page is up-to-date. Specifically, xas_unlock_irq() should not
>> be called before lock_page() and get_page(); and it is necessary to=20
>> recheck PageUptodate() after locking the page.=20
>>=20
>> With this bug and CONFIG_READ_ONLY_THP_FOR_FS=3Dy, madvise(HUGE)'ed .tex=
t=20
>> may contain corrupted data. This is because khugepaged mistakenly=20
>> collapses some not up-to-date sub pages into a huge page, and assumes th=
e=20
>> huge page is up-to-date. This will NOT corrupt data in the disk, because=
=20
>> the page is read-only and never written back. Fix this by properly=20
>> checking PageUptodate() after locking the page. This check replaces=20
>> "VM_BUG_ON_PAGE(!PageUptodate(page), page);".=20
>>=20
>> Also, move PageDirty() check after locking the page. Current khugepaged=
=20
>> should not try to collapse dirty file THP, because it is limited to=20
>> read-only .text. Add a warning with the PageDirty() check as it should=20
>> not happen. This warning is added after page_mapping() check, because=20
>> if the page is truncated, it might be dirty.
>=20
> I've lost the plot on this patch.  I have the v3 patch plus these fixes:
>=20
> http://lkml.kernel.org/r/20191028221414.3685035-1-songliubraving@fb.com
> http://lkml.kernel.org/r/20191022191006.411277-1-songliubraving@fb.com
> http://lkml.kernel.org/r/20191030200736.3455046-1-songliubraving@fb.com
>=20
> and there's a v4 which I can't correlate with the above.  And there has
> been discussion about deferring some of the filemap_flush() changes
> until later.
>=20
> So I think it's best if we just start again.  Can you please prepare
> and send out a v5 (which might be a 2-patch series)?

Sounds good. Sending v5 soon.=20

Thanks,
Song