[PATCH 0/5] A few cleanup and fixup patches for hugetlbfs

[PATCH 0/5] A few cleanup and fixup patches for hugetlbfs

[Miaohe Lin]

Hi everyone,
This series contains a few cleaup patches to remove unneeded forward
declaration, use helper macro and so on. Also we fix the confusing
hugetlbfs stat. More details can be found in the respective changelogs.
Thanks!

Miaohe Lin (5):
  hugetlbfs: use helper macro SZ_1{K,M}
  hugetlbfs: remove unneeded hugetlbfs_ops forward declaration
  hugetlbfs: remove unneeded header file
  hugetlbfs: cleanup some comments in inode.c
  hugetlbfs: fix confusing hugetlbfs stat

 fs/hugetlbfs/inode.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

-- 
2.23.0

[PATCH 1/5] hugetlbfs: use helper macro SZ_1{K,M}

[Miaohe Lin]

Use helper macro SZ_1K and SZ_1M to do the size conversion. Minor
readability improvement.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 fs/hugetlbfs/inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 20336cb3c040..e998c416b85f 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -1309,7 +1309,7 @@ static int hugetlbfs_parse_param(struct fs_context *fc, struct fs_parameter *par
 		ps = memparse(param->string, &rest);
 		ctx->hstate = size_to_hstate(ps);
 		if (!ctx->hstate) {
-			pr_err("Unsupported page size %lu MB\n", ps >> 20);
+			pr_err("Unsupported page size %lu MB\n", ps / SZ_1M);
 			return -EINVAL;
 		}
 		return 0;
@@ -1555,7 +1555,7 @@ static struct vfsmount *__init mount_one_hugetlbfs(struct hstate *h)
 	}
 	if (IS_ERR(mnt))
 		pr_err("Cannot mount internal hugetlbfs for page size %luK",
-		       huge_page_size(h) >> 10);
+		       huge_page_size(h) / SZ_1K);
 	return mnt;
 }
 
-- 
2.23.0

Re: [PATCH 1/5] hugetlbfs: use helper macro SZ_1{K,M}

[Mike Kravetz]

On 07/21/22 21:16, Miaohe Lin wrote:
> Use helper macro SZ_1K and SZ_1M to do the size conversion. Minor
> readability improvement.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  fs/hugetlbfs/inode.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks,

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

[PATCH 2/5] hugetlbfs: remove unneeded hugetlbfs_ops forward declaration

[Miaohe Lin]

The forward declaration for hugetlbfs_ops is unnecessary. Remove it.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 fs/hugetlbfs/inode.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index e998c416b85f..a10156df5726 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -40,7 +40,6 @@
 #include <linux/uaccess.h>
 #include <linux/sched/mm.h>
 
-static const struct super_operations hugetlbfs_ops;
 static const struct address_space_operations hugetlbfs_aops;
 const struct file_operations hugetlbfs_file_operations;
 static const struct inode_operations hugetlbfs_dir_inode_operations;
-- 
2.23.0

Re: [PATCH 2/5] hugetlbfs: remove unneeded hugetlbfs_ops forward declaration

[Mike Kravetz]

On 07/21/22 21:16, Miaohe Lin wrote:
> The forward declaration for hugetlbfs_ops is unnecessary. Remove it.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  fs/hugetlbfs/inode.c | 1 -
>  1 file changed, 1 deletion(-)

Thanks,

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

[PATCH 3/5] hugetlbfs: remove unneeded header file

[Miaohe Lin]

The header file signal.h is unneeded now. Remove it.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 fs/hugetlbfs/inode.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index a10156df5726..aa7a5b8fc724 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -11,7 +11,6 @@
 
 #include <linux/thread_info.h>
 #include <asm/current.h>
-#include <linux/sched/signal.h>		/* remove ASAP */
 #include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/mount.h>
-- 
2.23.0

Re: [PATCH 3/5] hugetlbfs: remove unneeded header file

[Mike Kravetz]

On 07/21/22 21:16, Miaohe Lin wrote:
> The header file signal.h is unneeded now. Remove it.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  fs/hugetlbfs/inode.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index a10156df5726..aa7a5b8fc724 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -11,7 +11,6 @@
>  
>  #include <linux/thread_info.h>
>  #include <asm/current.h>
> -#include <linux/sched/signal.h>		/* remove ASAP */

I see the original '#include <linux/sched.h>' with this 'remove ASAP' comment
has been there since the initial git repository build.  No idea why it was
originally added, and can find no reason for it to be there today.

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

>  #include <linux/falloc.h>
>  #include <linux/fs.h>
>  #include <linux/mount.h>
> -- 
> 2.23.0
> 

Re: [PATCH 3/5] hugetlbfs: remove unneeded header file

[Miaohe Lin]

On 2022/7/22 7:18, Mike Kravetz wrote:
> On 07/21/22 21:16, Miaohe Lin wrote:
>> The header file signal.h is unneeded now. Remove it.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  fs/hugetlbfs/inode.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>> index a10156df5726..aa7a5b8fc724 100644
>> --- a/fs/hugetlbfs/inode.c
>> +++ b/fs/hugetlbfs/inode.c
>> @@ -11,7 +11,6 @@
>>  
>>  #include <linux/thread_info.h>
>>  #include <asm/current.h>
>> -#include <linux/sched/signal.h>		/* remove ASAP */
> 
> I see the original '#include <linux/sched.h>' with this 'remove ASAP' comment
> has been there since the initial git repository build.  No idea why it was
> originally added, and can find no reason for it to be there today.

Me too. This might be a historical vestige.

> 
> Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

Many thank for your reviewing.

> 

[PATCH 4/5] hugetlbfs: cleanup some comments in inode.c

[Miaohe Lin]

The function generic_file_buffered_read has been renamed to filemap_read
since commit 87fa0f3eb267 ("mm/filemap: rename generic_file_buffered_read
to filemap_read"). Update the corresponding comment. And duplicated taken
in hugetlbfs_fill_super is removed.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 fs/hugetlbfs/inode.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index aa7a5b8fc724..19fc62a9c2fe 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -313,8 +313,8 @@ hugetlbfs_read_actor(struct page *page, unsigned long offset,
 
 /*
  * Support for read() - Find the page attached to f_mapping and copy out the
- * data. Its *very* similar to generic_file_buffered_read(), we can't use that
- * since it has PAGE_SIZE assumptions.
+ * data. Its *very* similar to filemap_read(), we can't use that since it has
+ * PAGE_SIZE assumptions.
  */
 static ssize_t hugetlbfs_read_iter(struct kiocb *iocb, struct iov_iter *to)
 {
@@ -1383,7 +1383,7 @@ hugetlbfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	/*
 	 * Allocate and initialize subpool if maximum or minimum size is
 	 * specified.  Any needed reservations (for minimum size) are taken
-	 * taken when the subpool is created.
+	 * when the subpool is created.
 	 */
 	if (ctx->max_hpages != -1 || ctx->min_hpages != -1) {
 		sbinfo->spool = hugepage_new_subpool(ctx->hstate,
-- 
2.23.0

Re: [PATCH 4/5] hugetlbfs: cleanup some comments in inode.c

[Mike Kravetz]

On 07/21/22 21:16, Miaohe Lin wrote:
> The function generic_file_buffered_read has been renamed to filemap_read
> since commit 87fa0f3eb267 ("mm/filemap: rename generic_file_buffered_read
> to filemap_read"). Update the corresponding comment. And duplicated taken
> in hugetlbfs_fill_super is removed.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  fs/hugetlbfs/inode.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index aa7a5b8fc724..19fc62a9c2fe 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -313,8 +313,8 @@ hugetlbfs_read_actor(struct page *page, unsigned long offset,
>  
>  /*
>   * Support for read() - Find the page attached to f_mapping and copy out the
> - * data. Its *very* similar to generic_file_buffered_read(), we can't use that
> - * since it has PAGE_SIZE assumptions.
> + * data. Its *very* similar to filemap_read(), we can't use that since it has
> + * PAGE_SIZE assumptions.

Since you are changing the comment, I would just say this provides
functionality similar to filemap_read().  filemap_read is now operating
on folios which may remove any PAGE_SIZE assumptions.  One day when
hugetlb is converted to folios it may end up using filemap_read instead
of hugetlbfs_read_actor.

-- 
Mike Kravetz

>   */
>  static ssize_t hugetlbfs_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  {
> @@ -1383,7 +1383,7 @@ hugetlbfs_fill_super(struct super_block *sb, struct fs_context *fc)
>  	/*
>  	 * Allocate and initialize subpool if maximum or minimum size is
>  	 * specified.  Any needed reservations (for minimum size) are taken
> -	 * taken when the subpool is created.
> +	 * when the subpool is created.
>  	 */
>  	if (ctx->max_hpages != -1 || ctx->min_hpages != -1) {
>  		sbinfo->spool = hugepage_new_subpool(ctx->hstate,
> -- 
> 2.23.0
> 

Re: [PATCH 4/5] hugetlbfs: cleanup some comments in inode.c

[Miaohe Lin]

On 2022/7/22 7:23, Mike Kravetz wrote:
> On 07/21/22 21:16, Miaohe Lin wrote:
>> The function generic_file_buffered_read has been renamed to filemap_read
>> since commit 87fa0f3eb267 ("mm/filemap: rename generic_file_buffered_read
>> to filemap_read"). Update the corresponding comment. And duplicated taken
>> in hugetlbfs_fill_super is removed.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  fs/hugetlbfs/inode.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>> index aa7a5b8fc724..19fc62a9c2fe 100644
>> --- a/fs/hugetlbfs/inode.c
>> +++ b/fs/hugetlbfs/inode.c
>> @@ -313,8 +313,8 @@ hugetlbfs_read_actor(struct page *page, unsigned long offset,
>>  
>>  /*
>>   * Support for read() - Find the page attached to f_mapping and copy out the
>> - * data. Its *very* similar to generic_file_buffered_read(), we can't use that
>> - * since it has PAGE_SIZE assumptions.
>> + * data. Its *very* similar to filemap_read(), we can't use that since it has
>> + * PAGE_SIZE assumptions.
> 
> Since you are changing the comment, I would just say this provides
> functionality similar to filemap_read().  filemap_read is now operating
> on folios which may remove any PAGE_SIZE assumptions.  One day when
> hugetlb is converted to folios it may end up using filemap_read instead
> of hugetlbfs_read_actor.

Sounds reasonable.

> 

Do you mean changing the comment of hugetlbfs_read_iter like below ?

/*
 * Support for read() - Find the page attached to f_mapping and copy out the
 * data. This provides functionality similar to filemap_read().
 */
static ssize_t hugetlbfs_read_iter(struct kiocb *iocb, struct iov_iter *to)

Thanks.

Re: [PATCH 4/5] hugetlbfs: cleanup some comments in inode.c

[Mike Kravetz]

On 07/22/22 14:19, Miaohe Lin wrote:
> On 2022/7/22 7:23, Mike Kravetz wrote:
> > On 07/21/22 21:16, Miaohe Lin wrote:
> >> The function generic_file_buffered_read has been renamed to filemap_read
> >> since commit 87fa0f3eb267 ("mm/filemap: rename generic_file_buffered_read
> >> to filemap_read"). Update the corresponding comment. And duplicated taken
> >> in hugetlbfs_fill_super is removed.
> >>
> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> >> ---
> >>  fs/hugetlbfs/inode.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> >> index aa7a5b8fc724..19fc62a9c2fe 100644
> >> --- a/fs/hugetlbfs/inode.c
> >> +++ b/fs/hugetlbfs/inode.c
> >> @@ -313,8 +313,8 @@ hugetlbfs_read_actor(struct page *page, unsigned long offset,
> >>  
> >>  /*
> >>   * Support for read() - Find the page attached to f_mapping and copy out the
> >> - * data. Its *very* similar to generic_file_buffered_read(), we can't use that
> >> - * since it has PAGE_SIZE assumptions.
> >> + * data. Its *very* similar to filemap_read(), we can't use that since it has
> >> + * PAGE_SIZE assumptions.
> > 
> > Since you are changing the comment, I would just say this provides
> > functionality similar to filemap_read().  filemap_read is now operating
> > on folios which may remove any PAGE_SIZE assumptions.  One day when
> > hugetlb is converted to folios it may end up using filemap_read instead
> > of hugetlbfs_read_actor.
> 
> Sounds reasonable.
> 
> > 
> 
> Do you mean changing the comment of hugetlbfs_read_iter like below ?
> 
> /*
>  * Support for read() - Find the page attached to f_mapping and copy out the
>  * data. This provides functionality similar to filemap_read().
>  */
> static ssize_t hugetlbfs_read_iter(struct kiocb *iocb, struct iov_iter *to)
> 

Exactly!

With such a change, you can add:

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

[PATCH 5/5] hugetlbfs: fix confusing hugetlbfs stat

[Miaohe Lin]

When size option is not specified, f_blocks, f_bavail and f_bfree will be
set to -1 instead of 0. Likewise, when nr_inodes is not specified, f_files
and f_ffree will be set to -1 too. Check max_hpages and max_inodes against
-1 first to make sure 0 is reported for max/free/used when no limit is set
as the comment states.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 fs/hugetlbfs/inode.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 19fc62a9c2fe..44da9828e171 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -1083,16 +1083,20 @@ static int hugetlbfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 		/* If no limits set, just report 0 for max/free/used
 		 * blocks, like simple_statfs() */
 		if (sbinfo->spool) {
-			long free_pages;
-
 			spin_lock_irq(&sbinfo->spool->lock);
-			buf->f_blocks = sbinfo->spool->max_hpages;
-			free_pages = sbinfo->spool->max_hpages
-				- sbinfo->spool->used_hpages;
-			buf->f_bavail = buf->f_bfree = free_pages;
+			if (sbinfo->spool->max_hpages != -1) {
+				long free_pages;
+
+				buf->f_blocks = sbinfo->spool->max_hpages;
+				free_pages = sbinfo->spool->max_hpages
+					     - sbinfo->spool->used_hpages;
+				buf->f_bavail = buf->f_bfree = free_pages;
+			}
 			spin_unlock_irq(&sbinfo->spool->lock);
-			buf->f_files = sbinfo->max_inodes;
-			buf->f_ffree = sbinfo->free_inodes;
+			if (sbinfo->max_inodes != -1) {
+				buf->f_files = sbinfo->max_inodes;
+				buf->f_ffree = sbinfo->free_inodes;
+			}
 		}
 		spin_unlock(&sbinfo->stat_lock);
 	}
-- 
2.23.0

Re: [PATCH 5/5] hugetlbfs: fix confusing hugetlbfs stat

[Mike Kravetz]

On 07/21/22 21:16, Miaohe Lin wrote:
> When size option is not specified, f_blocks, f_bavail and f_bfree will be
> set to -1 instead of 0. Likewise, when nr_inodes is not specified, f_files
> and f_ffree will be set to -1 too. Check max_hpages and max_inodes against
> -1 first to make sure 0 is reported for max/free/used when no limit is set
> as the comment states.

Just curious, where are you seeing values reported as -1?  The check
for sbinfo->spool was supposed to handle these cases.  Seems like it
should handle the max_hpages == -1 case.  But, it doesn't look like it
considers the max_inodes == -1 case.

If I create/mount a hugetlb filesystem without specifying size or nr_inodes,
df seems to report zero instead of -1.

Just want to understand the reasoning behind the change.
-- 
Mike Kravetz

> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  fs/hugetlbfs/inode.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 19fc62a9c2fe..44da9828e171 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -1083,16 +1083,20 @@ static int hugetlbfs_statfs(struct dentry *dentry, struct kstatfs *buf)
>  		/* If no limits set, just report 0 for max/free/used
>  		 * blocks, like simple_statfs() */
>  		if (sbinfo->spool) {
> -			long free_pages;
> -
>  			spin_lock_irq(&sbinfo->spool->lock);
> -			buf->f_blocks = sbinfo->spool->max_hpages;
> -			free_pages = sbinfo->spool->max_hpages
> -				- sbinfo->spool->used_hpages;
> -			buf->f_bavail = buf->f_bfree = free_pages;
> +			if (sbinfo->spool->max_hpages != -1) {
> +				long free_pages;
> +
> +				buf->f_blocks = sbinfo->spool->max_hpages;
> +				free_pages = sbinfo->spool->max_hpages
> +					     - sbinfo->spool->used_hpages;
> +				buf->f_bavail = buf->f_bfree = free_pages;
> +			}
>  			spin_unlock_irq(&sbinfo->spool->lock);
> -			buf->f_files = sbinfo->max_inodes;
> -			buf->f_ffree = sbinfo->free_inodes;
> +			if (sbinfo->max_inodes != -1) {
> +				buf->f_files = sbinfo->max_inodes;
> +				buf->f_ffree = sbinfo->free_inodes;
> +			}
>  		}
>  		spin_unlock(&sbinfo->stat_lock);
>  	}
> -- 
> 2.23.0
> 

Re: [PATCH 5/5] hugetlbfs: fix confusing hugetlbfs stat

[Miaohe Lin]

On 2022/7/22 8:28, Mike Kravetz wrote:
> On 07/21/22 21:16, Miaohe Lin wrote:
>> When size option is not specified, f_blocks, f_bavail and f_bfree will be
>> set to -1 instead of 0. Likewise, when nr_inodes is not specified, f_files
>> and f_ffree will be set to -1 too. Check max_hpages and max_inodes against
>> -1 first to make sure 0 is reported for max/free/used when no limit is set
>> as the comment states.
> 
> Just curious, where are you seeing values reported as -1?  The check

From the standard statvfs() function.

> for sbinfo->spool was supposed to handle these cases.  Seems like it

sbinfo->spool could be created when ctx->max_hpages == -1 while
ctx->min_hpages != -1 in hugetlbfs_fill_super.

> should handle the max_hpages == -1 case.  But, it doesn't look like it
> considers the max_inodes == -1 case.
> 
> If I create/mount a hugetlb filesystem without specifying size or nr_inodes,
> df seems to report zero instead of -1.
> 
> Just want to understand the reasoning behind the change.

I wrote a test program:

#include <sys/statvfs.h>
#include <stdio.h>

int main(void)
{
	struct statvfs buf;

	if (statvfs("/root/huge/", &buf) == -1) {
 		printf("statvfs() error\n");
		return -1;
	}
	printf("f_blocks %lld, f_bavail %lld, f_bfree %lld, f_files %lld, f_ffree %lld\n",
		buf.f_blocks, buf.f_bavail, buf.f_bfree, buf.f_files, buf.f_ffree);
	return 0;
}

And test it in my env:
[root@localhost ~]# mount -t hugetlbfs none /root/huge/
[root@localhost ~]# ./stat
f_blocks 0, f_bavail 0, f_bfree 0, f_files 0, f_ffree 0
[root@localhost ~]# umount /root/huge/
[root@localhost ~]# mount -t hugetlbfs -o min_size=32M none /root/huge/
[root@localhost ~]# ./stat
f_blocks -1, f_bavail -1, f_bfree -1, f_files -1, f_ffree -1
[root@localhost ~]# umount /root/huge/
[root@localhost ~]# mount -t hugetlbfs -o min_size=32M,size=64M none /root/huge/
[root@localhost ~]# ./stat
f_blocks 32, f_bavail 32, f_bfree 32, f_files -1, f_ffree -1
[root@localhost ~]# umount /root/huge/
[root@localhost ~]# mount -t hugetlbfs -o min_size=32M,size=64M,nr_inodes=1024 none /root/huge/
[root@localhost ~]# ./stat
f_blocks 32, f_bavail 32, f_bfree 32, f_files 1024, f_ffree 1023
[root@localhost ~]# umount /root/huge/

Or am I miss something?

> 

Thanks.

Re: [PATCH 5/5] hugetlbfs: fix confusing hugetlbfs stat

[Mike Kravetz]

On 07/22/22 14:38, Miaohe Lin wrote:
> On 2022/7/22 8:28, Mike Kravetz wrote:
> > On 07/21/22 21:16, Miaohe Lin wrote:
> >> When size option is not specified, f_blocks, f_bavail and f_bfree will be
> >> set to -1 instead of 0. Likewise, when nr_inodes is not specified, f_files
> >> and f_ffree will be set to -1 too. Check max_hpages and max_inodes against
> >> -1 first to make sure 0 is reported for max/free/used when no limit is set
> >> as the comment states.
> > 
> > Just curious, where are you seeing values reported as -1?  The check
> 
> From the standard statvfs() function.
> 
> > for sbinfo->spool was supposed to handle these cases.  Seems like it
> 
> sbinfo->spool could be created when ctx->max_hpages == -1 while
> ctx->min_hpages != -1 in hugetlbfs_fill_super.
> 
> > should handle the max_hpages == -1 case.  But, it doesn't look like it
> > considers the max_inodes == -1 case.
> > 
> > If I create/mount a hugetlb filesystem without specifying size or nr_inodes,
> > df seems to report zero instead of -1.
> > 
> > Just want to understand the reasoning behind the change.

Thanks for the additional information (and test program)!

From the hugetlbfs documentation:
"If the ``size``, ``min_size`` or ``nr_inodes`` option is not provided on
 command line then no limits are set."

So, having those values set to -1 indicates there is no limit set.

With this change, 0 is reported for the case where there is no limit set as
well as the case where the max value is 0.

There may be some value in reporting -1 as is done today.

To be honest, I am not sure what is the correct behavior here.  Unless
there is a user visible issue/problem, I am hesitant to change.  Other
opinions are welcome.
-- 
Mike Kravetz

> 
> I wrote a test program:
> 
> #include <sys/statvfs.h>
> #include <stdio.h>
> 
> int main(void)
> {
> 	struct statvfs buf;
> 
> 	if (statvfs("/root/huge/", &buf) == -1) {
>  		printf("statvfs() error\n");
> 		return -1;
> 	}
> 	printf("f_blocks %lld, f_bavail %lld, f_bfree %lld, f_files %lld, f_ffree %lld\n",
> 		buf.f_blocks, buf.f_bavail, buf.f_bfree, buf.f_files, buf.f_ffree);
> 	return 0;
> }
> 
> And test it in my env:
> [root@localhost ~]# mount -t hugetlbfs none /root/huge/
> [root@localhost ~]# ./stat
> f_blocks 0, f_bavail 0, f_bfree 0, f_files 0, f_ffree 0
> [root@localhost ~]# umount /root/huge/
> [root@localhost ~]# mount -t hugetlbfs -o min_size=32M none /root/huge/
> [root@localhost ~]# ./stat
> f_blocks -1, f_bavail -1, f_bfree -1, f_files -1, f_ffree -1
> [root@localhost ~]# umount /root/huge/
> [root@localhost ~]# mount -t hugetlbfs -o min_size=32M,size=64M none /root/huge/
> [root@localhost ~]# ./stat
> f_blocks 32, f_bavail 32, f_bfree 32, f_files -1, f_ffree -1
> [root@localhost ~]# umount /root/huge/
> [root@localhost ~]# mount -t hugetlbfs -o min_size=32M,size=64M,nr_inodes=1024 none /root/huge/
> [root@localhost ~]# ./stat
> f_blocks 32, f_bavail 32, f_bfree 32, f_files 1024, f_ffree 1023
> [root@localhost ~]# umount /root/huge/
> 
> Or am I miss something?
> 
> > 
> 
> Thanks.

Re: [PATCH 5/5] hugetlbfs: fix confusing hugetlbfs stat

[Miaohe Lin]

On 2022/7/23 6:55, Mike Kravetz wrote:
> On 07/22/22 14:38, Miaohe Lin wrote:
>> On 2022/7/22 8:28, Mike Kravetz wrote:
>>> On 07/21/22 21:16, Miaohe Lin wrote:
>>>> When size option is not specified, f_blocks, f_bavail and f_bfree will be
>>>> set to -1 instead of 0. Likewise, when nr_inodes is not specified, f_files
>>>> and f_ffree will be set to -1 too. Check max_hpages and max_inodes against
>>>> -1 first to make sure 0 is reported for max/free/used when no limit is set
>>>> as the comment states.
>>>
>>> Just curious, where are you seeing values reported as -1?  The check
>>
>> From the standard statvfs() function.
>>
>>> for sbinfo->spool was supposed to handle these cases.  Seems like it
>>
>> sbinfo->spool could be created when ctx->max_hpages == -1 while
>> ctx->min_hpages != -1 in hugetlbfs_fill_super.
>>
>>> should handle the max_hpages == -1 case.  But, it doesn't look like it
>>> considers the max_inodes == -1 case.
>>>
>>> If I create/mount a hugetlb filesystem without specifying size or nr_inodes,
>>> df seems to report zero instead of -1.
>>>
>>> Just want to understand the reasoning behind the change.
> 
> Thanks for the additional information (and test program)!
> 
>>From the hugetlbfs documentation:
> "If the ``size``, ``min_size`` or ``nr_inodes`` option is not provided on
>  command line then no limits are set."
> 
> So, having those values set to -1 indicates there is no limit set.
> 
> With this change, 0 is reported for the case where there is no limit set as
> well as the case where the max value is 0.

IMHO, 0 should not be a valid max value otherwise there will be no hugetlb pages
to use. It should mean there's no limit. But maybe I'm wrong.

> 
> There may be some value in reporting -1 as is done today.

There still be a inconsistency:

If the ``size`` and ``min_size`` isn't specified, then reported max value is 0.
But if ``min_size`` is specified while ``size`` isn't specified, the reported
max value is -1.

> 
> To be honest, I am not sure what is the correct behavior here.  Unless
> there is a user visible issue/problem, I am hesitant to change.  Other
> opinions are welcome.

Yes, it might be better to keep it as is. Maybe we could change the comment to
reflect what the current behavior is like below?

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 44da9828e171..f03b1a019cc0 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -1080,7 +1080,7 @@ static int hugetlbfs_statfs(struct dentry *dentry, struct kstatfs *buf)
        buf->f_bsize = huge_page_size(h);
        if (sbinfo) {
                spin_lock(&sbinfo->stat_lock);
-               /* If no limits set, just report 0 for max/free/used
+               /* If no limits set, just report 0 or -1 for max/free/used
                 * blocks, like simple_statfs() */
                if (sbinfo->spool) {
                        spin_lock_irq(&sbinfo->spool->lock);

> 

No strong opinion to keep this patch or above change. Many thanks for your comment and reply. :)

Re: [PATCH 5/5] hugetlbfs: fix confusing hugetlbfs stat

[Mike Kravetz]

On 07/23/22 10:56, Miaohe Lin wrote:
> On 2022/7/23 6:55, Mike Kravetz wrote:
> > On 07/22/22 14:38, Miaohe Lin wrote:
> >> On 2022/7/22 8:28, Mike Kravetz wrote:
> >>> On 07/21/22 21:16, Miaohe Lin wrote:
> >>>> When size option is not specified, f_blocks, f_bavail and f_bfree will be
> >>>> set to -1 instead of 0. Likewise, when nr_inodes is not specified, f_files
> >>>> and f_ffree will be set to -1 too. Check max_hpages and max_inodes against
> >>>> -1 first to make sure 0 is reported for max/free/used when no limit is set
> >>>> as the comment states.
> >>>
> >>> Just curious, where are you seeing values reported as -1?  The check
> >>
> >> From the standard statvfs() function.
> >>
> >>> for sbinfo->spool was supposed to handle these cases.  Seems like it
> >>
> >> sbinfo->spool could be created when ctx->max_hpages == -1 while
> >> ctx->min_hpages != -1 in hugetlbfs_fill_super.
> >>
> >>> should handle the max_hpages == -1 case.  But, it doesn't look like it
> >>> considers the max_inodes == -1 case.
> >>>
> >>> If I create/mount a hugetlb filesystem without specifying size or nr_inodes,
> >>> df seems to report zero instead of -1.
> >>>
> >>> Just want to understand the reasoning behind the change.
> > 
> > Thanks for the additional information (and test program)!
> > 
> >>From the hugetlbfs documentation:
> > "If the ``size``, ``min_size`` or ``nr_inodes`` option is not provided on
> >  command line then no limits are set."
> > 
> > So, having those values set to -1 indicates there is no limit set.
> > 
> > With this change, 0 is reported for the case where there is no limit set as
> > well as the case where the max value is 0.
> 
> IMHO, 0 should not be a valid max value otherwise there will be no hugetlb pages
> to use. It should mean there's no limit. But maybe I'm wrong.

I agree that 0 as a max value makes little sense.  However, it is allowed
today and from what I can tell it is file system specific.  So, there is no
defined behavior.

> 
> > 
> > There may be some value in reporting -1 as is done today.
> 
> There still be a inconsistency:
> 
> If the ``size`` and ``min_size`` isn't specified, then reported max value is 0.
> But if ``min_size`` is specified while ``size`` isn't specified, the reported
> max value is -1.
> 

Agree that this is inconsistent and confusing.

In the case where min_size and size is not specified, -1 for size still may
make sense.  min_size specifies how many pages are reserved for use by the
filesystem.  The only required relation between min_size and size is that if
size is specified, then min_size must be smaller.  Otherwise, it makes no
sense to reserve pages (min_size) that can not be used.

> > To be honest, I am not sure what is the correct behavior here.  Unless
> > there is a user visible issue/problem, I am hesitant to change.  Other
> > opinions are welcome.
> 
> Yes, it might be better to keep it as is. Maybe we could change the comment to
> reflect what the current behavior is like below?
> 
> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
> index 44da9828e171..f03b1a019cc0 100644
> --- a/fs/hugetlbfs/inode.c
> +++ b/fs/hugetlbfs/inode.c
> @@ -1080,7 +1080,7 @@ static int hugetlbfs_statfs(struct dentry *dentry, struct kstatfs *buf)
>         buf->f_bsize = huge_page_size(h);
>         if (sbinfo) {
>                 spin_lock(&sbinfo->stat_lock);
> -               /* If no limits set, just report 0 for max/free/used
> +               /* If no limits set, just report 0 or -1 for max/free/used
>                  * blocks, like simple_statfs() */
>                 if (sbinfo->spool) {
>                         spin_lock_irq(&sbinfo->spool->lock);
> 
> > 
> 
> No strong opinion to keep this patch or above change. Many thanks for your comment and reply. :)
> 

I am fine with the comment change.  Thanks for reading through the code and
trying to make sense of it!
-- 
Mike Kravetz

Re: [PATCH 5/5] hugetlbfs: fix confusing hugetlbfs stat

[Miaohe Lin]

On 2022/7/26 7:40, Mike Kravetz wrote:
> On 07/23/22 10:56, Miaohe Lin wrote:
>> On 2022/7/23 6:55, Mike Kravetz wrote:
>>> On 07/22/22 14:38, Miaohe Lin wrote:
>>>> On 2022/7/22 8:28, Mike Kravetz wrote:
>>>>> On 07/21/22 21:16, Miaohe Lin wrote:
>>>>>> When size option is not specified, f_blocks, f_bavail and f_bfree will be
>>>>>> set to -1 instead of 0. Likewise, when nr_inodes is not specified, f_files
>>>>>> and f_ffree will be set to -1 too. Check max_hpages and max_inodes against
>>>>>> -1 first to make sure 0 is reported for max/free/used when no limit is set
>>>>>> as the comment states.
>>>>>
>>>>> Just curious, where are you seeing values reported as -1?  The check
>>>>
>>>> From the standard statvfs() function.
>>>>
>>>>> for sbinfo->spool was supposed to handle these cases.  Seems like it
>>>>
>>>> sbinfo->spool could be created when ctx->max_hpages == -1 while
>>>> ctx->min_hpages != -1 in hugetlbfs_fill_super.
>>>>
>>>>> should handle the max_hpages == -1 case.  But, it doesn't look like it
>>>>> considers the max_inodes == -1 case.
>>>>>
>>>>> If I create/mount a hugetlb filesystem without specifying size or nr_inodes,
>>>>> df seems to report zero instead of -1.
>>>>>
>>>>> Just want to understand the reasoning behind the change.
>>>
>>> Thanks for the additional information (and test program)!
>>>
>>> >From the hugetlbfs documentation:
>>> "If the ``size``, ``min_size`` or ``nr_inodes`` option is not provided on
>>>  command line then no limits are set."
>>>
>>> So, having those values set to -1 indicates there is no limit set.
>>>
>>> With this change, 0 is reported for the case where there is no limit set as
>>> well as the case where the max value is 0.
>>
>> IMHO, 0 should not be a valid max value otherwise there will be no hugetlb pages
>> to use. It should mean there's no limit. But maybe I'm wrong.
> 
> I agree that 0 as a max value makes little sense.  However, it is allowed
> today and from what I can tell it is file system specific.  So, there is no
> defined behavior.

So it might be better to keep the code as is.

> 
>>
>>>
>>> There may be some value in reporting -1 as is done today.
>>
>> There still be a inconsistency:
>>
>> If the ``size`` and ``min_size`` isn't specified, then reported max value is 0.
>> But if ``min_size`` is specified while ``size`` isn't specified, the reported
>> max value is -1.
>>
> 
> Agree that this is inconsistent and confusing.
> 
> In the case where min_size and size is not specified, -1 for size still may
> make sense.  min_size specifies how many pages are reserved for use by the
> filesystem.  The only required relation between min_size and size is that if
> size is specified, then min_size must be smaller.  Otherwise, it makes no
> sense to reserve pages (min_size) that can not be used.
> 
>>> To be honest, I am not sure what is the correct behavior here.  Unless
>>> there is a user visible issue/problem, I am hesitant to change.  Other
>>> opinions are welcome.
>>
>> Yes, it might be better to keep it as is. Maybe we could change the comment to
>> reflect what the current behavior is like below?
>>
>> diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
>> index 44da9828e171..f03b1a019cc0 100644
>> --- a/fs/hugetlbfs/inode.c
>> +++ b/fs/hugetlbfs/inode.c
>> @@ -1080,7 +1080,7 @@ static int hugetlbfs_statfs(struct dentry *dentry, struct kstatfs *buf)
>>         buf->f_bsize = huge_page_size(h);
>>         if (sbinfo) {
>>                 spin_lock(&sbinfo->stat_lock);
>> -               /* If no limits set, just report 0 for max/free/used
>> +               /* If no limits set, just report 0 or -1 for max/free/used
>>                  * blocks, like simple_statfs() */
>>                 if (sbinfo->spool) {
>>                         spin_lock_irq(&sbinfo->spool->lock);
>>
>>>
>>
>> No strong opinion to keep this patch or above change. Many thanks for your comment and reply. :)
>>
> 
> I am fine with the comment change.  Thanks for reading through the code and
> trying to make sense of it!

I will do it in next version. Many thanks for your time.

>