Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 2 May 2023 20:05:28 -0700
From:   Mike Kravetz <mike.kravetz@oracle.com>
To:     Ackerley Tng <ackerleytng@google.com>, willy@infradead.org,
        sidhartha.kumar@oracle.com
Cc:     akpm@linux-foundation.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        muchun.song@linux.dev, jhubbard@nvidia.com, vannapurve@google.com,
        erdemaktas@google.com
Subject: Re: [PATCH 2/2] fs: hugetlbfs: Fix logic to skip allocation on hit
 in page cache
Message-ID: <20230503030528.GC3873@monkey>
References: <cover.1683069252.git.ackerleytng@google.com>
 <f15f57def8e69cf288f0646f819b784fe15fabe2.1683069252.git.ackerleytng@google.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f15f57def8e69cf288f0646f819b784fe15fabe2.1683069252.git.ackerleytng@google.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?D8aAHfjeJSl4pSpugx6YW7AcKoMPHtzhEuTFFPYHwNaT2bONcVXn7OlvRZF5?=
 =?us-ascii?Q?35mQ3QERK09S+Bkti32yWkDCm7BgPEN6LmhHt25OGjMF7uk3ZRBDle/lRmoo?=
 =?us-ascii?Q?+FqANDMtgqyBVpJ+S1f4EyJyJE6T1Rvb99WnTVLCDYl4e2qOE1hWoJSUWpHp?=
 =?us-ascii?Q?1pfFJMZ+05QHMWMp/Lrgw4pnjxjGfoHP7CvqF2GT4r3sm/OaWUWVt8+jpoV5?=
 =?us-ascii?Q?M4e5Yn54kh2vZwP5CekuZ6XWSglkQZUKwAa0vyAszejmYEV60Xqt7ypOR+f8?=
 =?us-ascii?Q?9Vh6vfoi8dxSWjLDihk7hMtJqVoa1hiidRJrA08sdvA29Orpv/4QrpTz3yAy?=
 =?us-ascii?Q?C4s903U4XY9u+fWyL92z5WZqMujiBF23Fq9yba23XOOCui59bMA+twrwmURF?=
 =?us-ascii?Q?L/EPvlXdbj4MWUHRUPY8ob1bYd7nWzHCtLb4E2957pnCu/jrZ0gzmnfNNPcN?=
 =?us-ascii?Q?F3tIVZDTH8puqZZvrpEHAqjcreZn6axIXyjDF1RedALQ5u6g6IXy3wN/2Pv9?=
 =?us-ascii?Q?YfBXWNIf0ddNd5vEGCSI6F6ZL4EdDq78ygXrZ/cy6R1mWK7lM+cjmUC6Hx+V?=
 =?us-ascii?Q?q8eqTQ5aupR3TzG3Crsj3VkjBfUcoc1o3+sHte1uSz8nLfA0PjagKFoLMaiY?=
 =?us-ascii?Q?u1wHAuwsBTop07uOD6CUoYrfuQGJn4Gqdwu0mBUlHb6E3XXcUQpzCvFSrCDH?=
 =?us-ascii?Q?M6gIvrczXCJm3p2cb0c9EpBG5QjnBJX/HZQrtRMP5fPQirBL8ijUtSC5jSsX?=
 =?us-ascii?Q?0wowh0Fbbu9XQQhuPoMQxahb25mxeEZ9ofyCYHvoY94GtFG2xz8467NmiM9C?=
 =?us-ascii?Q?Sg7Zbckz0xRyDz92b3aen/xw5OQf+ml7ZgZOE41fCg8m8KdPRu3xKF0UP9kI?=
 =?us-ascii?Q?TDgWBH77ZX1REAsxyegG3PFmQtR5lFoOynTzCHswAnTOSjc0UTTjpRthR6oF?=
 =?us-ascii?Q?tuIt5RvkdZ9YgoVouDaKy1LWkZgXfMbMyY+eT35lTYE41hEF2BYI8p5AKiZC?=
 =?us-ascii?Q?xrquakLGkHeUKiNywOgAEj245Q3xR/M6Ljw+4WaY4F4lD1YTfyjW/Kf6HAXq?=
 =?us-ascii?Q?G0fX1lupHfQahj2awJquppEDrIrulz7KngxgE6BrGR4Erj/a/a5uW6df1j7e?=
 =?us-ascii?Q?6zGcfzVCc6HRABI+4UY/PU+A2dmMaw5v9qhHwjd00Jv4VU3HBaAv4JXrB/wJ?=
 =?us-ascii?Q?zYCvEY6xkTqrTlPBcrTYqSeTFVds0TrSDc03NZmzCkCiR92ISes3Fm5GetdR?=
 =?us-ascii?Q?yz+lIsMnBQ1C64WDHYw0p2OUt7pS+ZJO9VdqzkMF/neBQOCuvtmFqsqu29XM?=
 =?us-ascii?Q?0H9v1Rd+20TFISOIEy6Iim5FSZikk/hCIyGApBNAMPGhEEGU7NR0TQrdxIbH?=
 =?us-ascii?Q?+TZnIZwUTSIzQIMzu94BuivSQhcnOA+E3yktqTo2W51kQ6nmuF9NjHJYCQ6Y?=
 =?us-ascii?Q?8D0BvIzcOa7ijZO5VjyxRVVfVs2iJl5VjLv0jdfJD3HNkRj66OS89BGwMqQ9?=
 =?us-ascii?Q?luBIjnfS6v+/H+FmbvF5G6kcXtrW1XPJvMZjKFMwyOTET+nC0NGYPyoSUK32?=
 =?us-ascii?Q?DtMrWBeSVSFQZbU5DfJy0M01JeRfwvg3VCEDhoL6?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?us-ascii?Q?Gc0KEPoTjtIboIPz2Krj0FuOBv4ztlTKUedzlMwNxDItZKDYZN573wWcbiJ/?=
 =?us-ascii?Q?6Zk5tUBk9oQUavpjczxszm+x9O2lT9UyNH+yVW5lLRhoti9gmfPWg8jmYxAh?=
 =?us-ascii?Q?js9abjtPxaCR06LYI350ZtFg4kofCsjmvJy1NEoMAsvrmK8kI/YQNSHAu3Vo?=
 =?us-ascii?Q?ijrkZglsdw6vfhRd6yaIET2Ein3k3BMSNMXReiMBlqMu/YOfY3zrEUfrkCDO?=
 =?us-ascii?Q?IO/9JYsE2ovVqlo+vTaJ+qDNSg1fXspWgu1fp15kDdTQeeKw07Njk/8J1kld?=
 =?us-ascii?Q?LDLVJaTZsEIuSQUL4XW4in5IWkWjsMEycTxq4jVCwYRkP4cO9yrDHiyLiAtO?=
 =?us-ascii?Q?X7iC3+2c/oUjK+D8T2JVxT1Oyc1DloLiZNiGeQtAvfOOh7NQIsegtGuDYu5i?=
 =?us-ascii?Q?hqQvsdBOLvPt06KTHE1E83XcnE8cX9aP8EVlmer4YYblTcQkcuXtipfr9S3g?=
 =?us-ascii?Q?KtJTQBs+PiFXgrzAJEmLRdB3ot1lBVftmab0mSuET7FDmU38QjlYVeVUv/4V?=
 =?us-ascii?Q?tG+tJqi8YzQHVapNhXSopqUUUdWQNIo+Dsd3v3FpWHt6CAV1w8GufdoUcfww?=
 =?us-ascii?Q?dJuDW26EDHoGekOaXTV4v+xqJmTtjf5d8IKdwdDuSK4UXZ0iPDSSx5bNJjQL?=
 =?us-ascii?Q?JJmgCuWtLaZ4cGlf/vJ6Nz48U6RDDDyPSWp6905xrwotTGRrTUzt40ds4wb9?=
 =?us-ascii?Q?9Zt3SL8sYyIZpkOMzTRyuh/X7VhfQpcsBmHrQyheuYRw5W3wdN5w4PChEXHG?=
 =?us-ascii?Q?bWBZFtevqIgSz5gViag8GPuvT7T5t2B1X5B6h+7udQMuhF5pkn5kf1FFtcXu?=
 =?us-ascii?Q?Sxf1P5Uigz6ofzhZOLN2C5dZPqlmSRKrwPCU/QCXltaNoc43NcqmIqrfq8SN?=
 =?us-ascii?Q?7OUIJqQXf8mrGhfpvonLIU7zlkWMU+8eVYr/faLi4dvhU774RCBUs/3FjQ6i?=
 =?us-ascii?Q?4EQDd7e0FkDaC7OhUWDedQ=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: da92e58c-1949-422c-d4f3-08db4b83417c
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 May 2023 03:05:31.7107
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7KpYN8YvOuBtcmdOBEKXwRTC5SQfW/JTW2L5BNsPrnKNlFolNuT7GQV7ZD9UxRaaEwVA0c/UqJ0SaXRkyVpFmA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR10MB7669
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-05-02_14,2023-04-27_01,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 malwarescore=0 adultscore=0
 suspectscore=0 mlxlogscore=999 phishscore=0 mlxscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2303200000
 definitions=main-2305030024
X-Proofpoint-GUID: QF25Lze5xQ4Mv8yKWSSaPa96Mvt8lnhH
X-Proofpoint-ORIG-GUID: QF25Lze5xQ4Mv8yKWSSaPa96Mvt8lnhH
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 05/02/23 23:56, Ackerley Tng wrote:
> When fallocate() is called twice on the same offset in the file, the
> second fallocate() should succeed.
> 
> page_cache_next_miss() always advances index before returning, so even
> on a page cache hit, the check would set present to false.

Thank you Ackerley for finding this!

When I read the description of page_cache_next_miss(), I assumed

	present = page_cache_next_miss(mapping, index, 1) != index;

would tell us if there was a page at index in the cache.

However, when looking closer at the code it does not check for a page
at index, but rather starts looking at index+1.  Perhaps that is why
it is named next?

Matthew, I think the use of the above statement was your suggestion.
And you know the xarray code better than anyone.  I just want to make
sure page_cache_next_miss is operating as designed/expected.  If so,
then the changes suggested here make sense.

In addition, the same code is in hugetlbfs_pagecache_present and will
have this same issue.
-- 
Mike Kravetz

> 
> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> ---
>  fs/hugetlbfs/inode.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index ecfdfb2529a3..f640cff1bbce 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -821,7 +821,6 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>  		 */
>  		struct folio *folio;
>  		unsigned long addr;
> -		bool present;
>  
>  		cond_resched();
>  
> @@ -845,10 +844,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
>  		mutex_lock(&hugetlb_fault_mutex_table[hash]);
>  
>  		/* See if already present in mapping to avoid alloc/free */
> -		rcu_read_lock();
> -		present = page_cache_next_miss(mapping, index, 1) != index;
> -		rcu_read_unlock();
> -		if (present) {
> +		if (filemap_has_folio(mapping, index)) {
>  			mutex_unlock(&hugetlb_fault_mutex_table[hash]);
>  			hugetlb_drop_vma_policy(&pseudo_vma);
>  			continue;
> -- 
> 2.40.1.495.gc816e09b53d-goog
>