mm,thp: add read-only THP support for (non-shmem) FS
Subsystems (2)
Hello Song Liu,
The patch 89e1c65c0db7: "mm,thp: add read-only THP support for (non-shmem) FS" from Aug 7, 2019, leads to the following static checker warning:
mm/khugepaged.c:1532 collapse_file() error: double unlock 'irq:'
mm/khugepaged.c
1398 if (xa_is_value(page) || !PageUptodate(page)) {
1399 xas_unlock_irq(&xas);
^^^^^^^^^^^^^^^^^^^^We enable IRQs.
1400 /* swap in or instantiate fallocated page */
1401 if (shmem_getpage(mapping->host, index, &page,
1402 SGP_NOHUGE)) {
1403 result = SCAN_FAIL;
1404 goto xa_unlocked;
1405 }
1406 } else if (trylock_page(page)) {
1407 get_page(page);
1408 xas_unlock_irq(&xas);
1409 } else {
1410 result = SCAN_PAGE_LOCK;
1411 goto xa_locked;
1412 }
1413 } else { /* !is_shmem */
1414 if (!page || xa_is_value(page)) {
1415 xas_unlock_irq(&xas);
1416 page_cache_sync_readahead(mapping, &file->f_ra,
1417 file, index,
1418 PAGE_SIZE);
1419 /* drain pagevecs to help isolate_lru_page() */
1420 lru_add_drain();
1421 page = find_lock_page(mapping, index);
1422 if (unlikely(page == NULL)) {
1423 result = SCAN_FAIL;
1424 goto xa_unlocked;
1425 }
1426 } else if (!PageUptodate(page)) {
1427 xas_unlock_irq(&xas);
1428 wait_on_page_locked(page);
1429 if (!trylock_page(page)) {
1430 result = SCAN_PAGE_LOCK;
1431 goto xa_unlocked;
1432 }
1433 get_page(page);
1434 } else if (PageDirty(page)) {
1435 result = SCAN_FAIL;
1436 goto xa_locked;
1437 } else if (trylock_page(page)) {
1438 get_page(page);
1439 xas_unlock_irq(&xas);
1440 } else {
1441 result = SCAN_PAGE_LOCK;
1442 goto xa_locked;
1443 }
1444 }
1445
1446 /*
1447 * The page must be locked, so we can drop the i_pages lock
1448 * without racing with truncate.
1449 */
1450 VM_BUG_ON_PAGE(!PageLocked(page), page);
1451 VM_BUG_ON_PAGE(!PageUptodate(page), page);
1452
1453 /*
1454 * If file was truncated then extended, or hole-punched, before
1455 * we locked the first page, then a THP might be there already.
1456 */
1457 if (PageTransCompound(page)) {
1458 result = SCAN_PAGE_COMPOUND;
1459 goto out_unlock;
1460 }
1461
1462 if (page_mapping(page) != mapping) {
1463 result = SCAN_TRUNCATED;
1464 goto out_unlock;
1465 }
1466
1467 if (isolate_lru_page(page)) {
1468 result = SCAN_DEL_PAGE_LRU;
1469 goto out_unlock;
1470 }
1471
1472 if (page_has_private(page) &&
1473 !try_to_release_page(page, GFP_KERNEL)) {
1474 result = SCAN_PAGE_HAS_PRIVATE;
1475 break;The patch adds this break statement but IRQs are enabled at this point.
1476 }
1477
1478 if (page_mapped(page))
1479 unmap_mapping_pages(mapping, index, 1, false);
1480
1481 xas_lock_irq(&xas);
1482 xas_set(&xas, index);
1483
1484 VM_BUG_ON_PAGE(page != xas_load(&xas), page);
1485 VM_BUG_ON_PAGE(page_mapped(page), page);
1486
1487 /*
1488 * The page is expected to have page_count() == 3:
1489 * - we hold a pin on it;
1490 * - one reference from page cache;
1491 * - one from isolate_lru_page;
1492 */
1493 if (!page_ref_freeze(page, 3)) {
1494 result = SCAN_PAGE_COUNT;
1495 xas_unlock_irq(&xas);
1496 putback_lru_page(page);
1497 goto out_unlock;
1498 }
1499
1500 /*
1501 * Add the page to the list to be able to undo the collapse if
1502 * something go wrong.
1503 */
1504 list_add_tail(&page->lru, &pagelist);
1505
1506 /* Finally, replace with the new page. */
1507 xas_store(&xas, new_page);
1508 continue;
1509 out_unlock:
1510 unlock_page(page);
1511 put_page(page);
1512 goto xa_unlocked;
1513 }
1514
1515 if (is_shmem)
1516 __inc_node_page_state(new_page, NR_SHMEM_THPS);
1517 else {
1518 __inc_node_page_state(new_page, NR_FILE_THPS);
1519 filemap_nr_thps_inc(mapping);
1520 }
1521
1522 if (nr_none) {
1523 struct zone *zone = page_zone(new_page);
1524
1525 __mod_node_page_state(zone->zone_pgdat, NR_FILE_PAGES, nr_none);
1526 if (is_shmem)
1527 __mod_node_page_state(zone->zone_pgdat,
1528 NR_SHMEM, nr_none);
1529 }
1530
1531 xa_locked:
1532 xas_unlock_irq(&xas);
^^^^^^^^^^^^^^^^^^^^Double unlock.
1533 xa_unlocked:
1534
1535 if (result == SCAN_SUCCEED) {regards,
dan carpenter
+ Andrew
Quoted 11 lines · Dan Carpenter
Hello Song Liu,
The patch
89e1c65c0db7: "mm,thp: add read-only THP support for (non-shmem) FS" from Aug 7, 2019, leads to the following static checker warning:mm/khugepaged.c:1532 collapse_file() error: double unlock 'irq:'
[...]
Thanks Dan! The following patch fixes this issue.
Hi Andrew, could you please add this fix to the mm tree?
Thanks,
Song
================= 8< ======================
From 30fd3683bb62cb81bb144e945edb768aff7ca445 Mon Sep 17 00:00:00 2001
From: Song Liu <[email protected]>
Date: Fri, 9 Aug 2019 07:15:17 -0700
Subject: [PATCH] khugepaged: fix double unlock in collapse_file()
In collapse_file, when try_to_release_page() fails, we need to goto out_unlock, because we already called xas_unlock_irq().
--- mm/khugepaged.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/khugepaged.c +1 -1
@@ -1472,7 +1472,7 @@ static void collapse_file(struct mm_struct *mm,if (page_has_private (page) && !try_to_release_page (page,GFP_KERNEL )) { result =SCAN_PAGE_HAS_PRIVATE ; -break ; + goto out_unlock; }if (page_mapped (page))
--
2.17.1