Re: [PATCH v10 15/41] KVM: guest_memfd: Handle lru_add fbatch refcounts during conversion safety check

[Binbin Wu]

On 8/8/2026 5:52 AM, Ackerley Tng via B4 Relay wrote:
> From: Ackerley Tng <ackerleytng@xxxxxxxxxx>
> 
> A guest_memfd folio is safe for conversion if guest_memfd holds the last
> references on it. Any other references on the folio may indicate another
> user, and guest_memfd cannot convert it to private if there may be an
> existing host user.
> 
> A folio will have extra refcounts if it is present in a per-CPU lru_add
> fbatch. guest_memfd does not actually participate in LRU, but
> freshly-allocated folios are still added to the lru_add fbatch for batch
> LRU statistics processing.
> 
> This one known "usage" of the folio is handled by draining the lru_add
> fbatch. After draining, if the refcount is still elevated, then there's
> truly some other user of this page, and the page is not safe for
> conversion.
> 
> If the page may be dma pinned, DMA is obviously using it and hence not safe
> for conversions. If the page is still mapped after guest_memfd tried to
> unmap it earlier in the conversion process, it is also obviously not safe
> for conversion. Exit early to avoid unnecessary draining in these 2 cases.
> 
> Provide a drain status to only drain once ever while processing a batch of
> folios.
> 
> Acked-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>
> Suggested-by: David Hildenbrand <david@xxxxxxxxxx>
> Signed-off-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>

With the updated version of change log,

Reviewed-by: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>

One nit below.

[...]

> +static bool __folio_safe_for_conversion(struct folio *folio,
> +					enum lru_cache_drained *drained)
> +{
> +	const int filemap_get_folios_refcount = 1;
> +

Nit:
After adding the helper, the use of filemap_get_folios_refcount seems
less obvious. It introduces unnecessary tight coupling with the caller,
though it has only one caller currently.

Is it better to pass the value as extra_refs, similar to what
lru_cache_drain_for_folio() does? 


> +	if (folio_maybe_dma_pinned(folio) || folio_mapped(folio))
> +		return false;
> +
> +	lru_cache_drain_for_folio(folio, filemap_get_folios_refcount,
> +				  drained);
> +
> +	return folio_ref_count(folio) ==
> +	       folio_nr_pages(folio) + filemap_get_folios_refcount;
> +}
> +
>  static bool kvm_gmem_is_safe_for_conversion(struct inode *inode, pgoff_t start,
>  					    size_t nr_pages, pgoff_t *err_index)
>  {
> +	enum lru_cache_drained drained = LRU_CACHE_NOT_DRAINED;
>  	struct address_space *mapping = inode->i_mapping;
> -	const int filemap_get_folios_refcount = 1;
>  	pgoff_t last = start + nr_pages - 1;
>  	struct folio_batch fbatch;
>  	bool safe = true;
> @@ -560,9 +576,8 @@ static bool kvm_gmem_is_safe_for_conversion(struct inode *inode, pgoff_t start,
>  		for (i = 0; i < folio_batch_count(&fbatch); ++i) {
>  			struct folio *folio = fbatch.folios[i];
>  
> -			if (folio_ref_count(folio) !=
> -			    folio_nr_pages(folio) + filemap_get_folios_refcount) {
> -				safe = false;
> +			safe = __folio_safe_for_conversion(folio, &drained);
> +			if (!safe) {
>  				*err_index = max(start, folio->index);
>  				break;
>  			}
>