Re: [PATCH v8 02/14] mm: shmem: shmem_getattr(): set blksize to highest supported THP order

[David Hildenbrand (Arm)]

On 9/18/26 03:45, Luiz Capitulino wrote:
> Today, shmem_getattr() sets stat->blksize to PMD size whenever
> shmem_huge_global_enabled() returns non-zero. While this works fine
> for the normal THP-enabled case as explained by Baolin in [1], this
> has two problems:
> 
>  1. Theoretically, when shmem is configured for within_size, this
>     could set blksize to PMD size even though the allocation may
>     be a smaller mTHP order
> 
>  2. A future commit will allow shmem THP support to be enabled
>     even when the CPU doesn't support PMD-sized pages. We should
>     not allow blksize to be set to PMD size in this case
> 
> In order to fix #1 and prepare for #2, this commit sets blksize
> to the size of the highest supported order returned by
> shmem_huge_global_enabled().
> 
>   [1] https://lore.kernel.org/linux-mm/6591a74c-7ef9-4614-9ae9-cb2fbed86ebf@xxxxxxxxxxxxxxxxx/
> 
> Suggested-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> Acked-by: Zi Yan <ziy@xxxxxxxxxx>
> Signed-off-by: Luiz Capitulino <luizcap@xxxxxxxxxx>
> ---
>  mm/shmem.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index b572c60f2af8..776dff8a848e 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1506,6 +1506,7 @@ static int shmem_getattr(struct mnt_idmap *idmap,
>  {
>  	struct inode *inode = path->dentry->d_inode;
>  	struct shmem_inode_info *info = SHMEM_I(inode);
> +	unsigned int orders;
>  
>  	/* Fast-path hint; recalc under info->lock corrects any stale read. */
>  	if (data_race(info->alloced - info->swapped != inode->i_mapping->nrpages))
> @@ -1522,8 +1523,9 @@ static int shmem_getattr(struct mnt_idmap *idmap,
>  			STATX_ATTR_NODUMP);
>  	generic_fillattr(idmap, request_mask, inode, stat);
>  
> -	if (shmem_huge_global_enabled(inode, 0, 0, false, NULL, 0))
> -		stat->blksize = HPAGE_PMD_SIZE;
> +	orders = shmem_huge_global_enabled(inode, 0, 0, false, NULL, 0);
> +	if (orders)
> +		stat->blksize = PAGE_SIZE << highest_order(orders);
>  
>  	if (request_mask & STATX_BTIME) {
>  		stat->result_mask |= STATX_BTIME;

Heh, looking into the history of this I stumble over

commit 89fdcd262fd40712da65e922558872a12b203332
Author: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
Date:   Thu Jun 7 17:06:59 2018 -0700

    mm: shmem: make stat.st_blksize return huge page size if THP is on

    Since tmpfs THP was supported in 4.8, hugetlbfs is not the only
    filesystem with huge page support anymore.  tmpfs can use huge page via
    THP when mounting by "huge=" mount option.

Which is a good read, especially regarding Hugh's comments:

"
    : Sorry, I have no enthusiasm for this patch; but do I feel strongly
    : enough to override you and everyone else to NAK it?  No, I don't feel
    : that strongly, maybe st_blksize isn't worth arguing over.
    :
    : We did look at struct stat when designing huge tmpfs, to see if there
    : were any fields that should be adjusted for it; but concluded none.
    : Yes, it would sometimes be nice to have a quickly accessible indicator
    : for when tmpfs has been mounted huge (scanning /proc/mounts for options
    : can be tiresome, agreed); but since tmpfs tries to supply huge (or not)
    : pages transparently, no difference seemed right.
"

(I agree with Hugh, also I find it odd to indicate for THP something > PAGE_SIZE)

Anyhow, that ship has sailed.

Effectively, I think "stat->blksize" is not that relevant. But indeed, today it
gives us the highest *possible* size we could see on that system.

shmem_huge_global_enabled() does not seem to depend on any runtime toggles that
could change, only on inode properties.

So your change looks good to me.

Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>

-- 
Cheers,

David