diff mbox series

xfs: show the proper user quota options

Message ID 1606124332-22100-1-git-send-email-kaixuxia@tencent.com (mailing list archive)
State Accepted
Headers show
Series xfs: show the proper user quota options | expand

Commit Message

kaixuxia Nov. 23, 2020, 9:38 a.m. UTC
From: Kaixu Xia <kaixuxia@tencent.com>

The quota option 'usrquota' should be shown if both the XFS_UQUOTA_ACCT
and XFS_UQUOTA_ENFD flags are set. The option 'uqnoenforce' should be
shown when only the XFS_UQUOTA_ACCT flag is set. The current code logic
seems wrong, Fix it and show proper options.

Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
 fs/xfs/xfs_super.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Eric Sandeen Nov. 23, 2020, 6:36 p.m. UTC | #1
On 11/23/20 3:38 AM, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> The quota option 'usrquota' should be shown if both the XFS_UQUOTA_ACCT
> and XFS_UQUOTA_ENFD flags are set. The option 'uqnoenforce' should be
> shown when only the XFS_UQUOTA_ACCT flag is set. The current code logic
> seems wrong, Fix it and show proper options.

I'm failing to see the difference in the logic here.  Under the current
code, what combination of flags yields the wrong string, and what does
this patch change in that respect?

Thanks,
-Eric

> 
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> ---
>  fs/xfs/xfs_super.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index e3e229e52512..5ebd6cdc44a7 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -199,10 +199,12 @@ xfs_fs_show_options(
>  		seq_printf(m, ",swidth=%d",
>  				(int)XFS_FSB_TO_BB(mp, mp->m_swidth));
>  
> -	if (mp->m_qflags & (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD))
> -		seq_puts(m, ",usrquota");
> -	else if (mp->m_qflags & XFS_UQUOTA_ACCT)
> -		seq_puts(m, ",uqnoenforce");
> +	if (mp->m_qflags & XFS_UQUOTA_ACCT) {
> +		if (mp->m_qflags & XFS_UQUOTA_ENFD)
> +			seq_puts(m, ",usrquota");
> +		else
> +			seq_puts(m, ",uqnoenforce");
> +	}
>  
>  	if (mp->m_qflags & XFS_PQUOTA_ACCT) {
>  		if (mp->m_qflags & XFS_PQUOTA_ENFD)
>
Darrick J. Wong Nov. 24, 2020, 12:30 a.m. UTC | #2
On Mon, Nov 23, 2020 at 05:38:52PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> The quota option 'usrquota' should be shown if both the XFS_UQUOTA_ACCT
> and XFS_UQUOTA_ENFD flags are set. The option 'uqnoenforce' should be
> shown when only the XFS_UQUOTA_ACCT flag is set. The current code logic
> seems wrong, Fix it and show proper options.

This needs a regression test case to make sure that quota mount options
passed in ==> quota options in /proc/mounts, wouldn't you say? ;)

--D

> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> ---
>  fs/xfs/xfs_super.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index e3e229e52512..5ebd6cdc44a7 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -199,10 +199,12 @@ xfs_fs_show_options(
>  		seq_printf(m, ",swidth=%d",
>  				(int)XFS_FSB_TO_BB(mp, mp->m_swidth));
>  
> -	if (mp->m_qflags & (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD))
> -		seq_puts(m, ",usrquota");
> -	else if (mp->m_qflags & XFS_UQUOTA_ACCT)
> -		seq_puts(m, ",uqnoenforce");
> +	if (mp->m_qflags & XFS_UQUOTA_ACCT) {
> +		if (mp->m_qflags & XFS_UQUOTA_ENFD)
> +			seq_puts(m, ",usrquota");
> +		else
> +			seq_puts(m, ",uqnoenforce");
> +	}
>  
>  	if (mp->m_qflags & XFS_PQUOTA_ACCT) {
>  		if (mp->m_qflags & XFS_PQUOTA_ENFD)
> -- 
> 2.20.0
>
Eric Sandeen Nov. 24, 2020, 1:26 a.m. UTC | #3
On 11/23/20 12:36 PM, Eric Sandeen wrote:
> On 11/23/20 3:38 AM, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> The quota option 'usrquota' should be shown if both the XFS_UQUOTA_ACCT
>> and XFS_UQUOTA_ENFD flags are set. The option 'uqnoenforce' should be
>> shown when only the XFS_UQUOTA_ACCT flag is set. The current code logic
>> seems wrong, Fix it and show proper options.
> 
> I'm failing to see the difference in the logic here.  Under the current
> code, what combination of flags yields the wrong string, and what does
> this patch change in that respect?

<djwong pointed out what I missed>

But I guess that just emphasizes the need for a test :)

Thanks,
-Eric
kaixuxia Nov. 24, 2020, 8:37 a.m. UTC | #4
On 2020/11/24 8:30, Darrick J. Wong wrote:
> On Mon, Nov 23, 2020 at 05:38:52PM +0800, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> The quota option 'usrquota' should be shown if both the XFS_UQUOTA_ACCT
>> and XFS_UQUOTA_ENFD flags are set. The option 'uqnoenforce' should be
>> shown when only the XFS_UQUOTA_ACCT flag is set. The current code logic
>> seems wrong, Fix it and show proper options.
> 
> This needs a regression test case to make sure that quota mount options
> passed in ==> quota options in /proc/mounts, wouldn't you say? ;)

Hi Darrick,

The simple test case as follows:

Before the patch:
 # mount -o uqnoenforce /dev/vdc1 /data1
 # cat /proc/mounts | grep xfs
/dev/vdc1 /data1 xfs rw,relatime,attr2,inode64,logbufs=8,logbsize=32k,usrquota 0 0

After the patch:
 # mount -o uqnoenforce /dev/vdc1 /data1
 # cat /proc/mounts | grep xfs
/dev/vdc1 /data1 xfs rw,relatime,attr2,inode64,logbufs=8,logbsize=32k,uqnoenforce 0 0

I'm not sure if a xfstest case is needed:)

Thanks,
Kaixu

> 
> --D
> 
>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
>> ---
>>  fs/xfs/xfs_super.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index e3e229e52512..5ebd6cdc44a7 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -199,10 +199,12 @@ xfs_fs_show_options(
>>  		seq_printf(m, ",swidth=%d",
>>  				(int)XFS_FSB_TO_BB(mp, mp->m_swidth));
>>  
>> -	if (mp->m_qflags & (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD))
>> -		seq_puts(m, ",usrquota");
>> -	else if (mp->m_qflags & XFS_UQUOTA_ACCT)
>> -		seq_puts(m, ",uqnoenforce");
>> +	if (mp->m_qflags & XFS_UQUOTA_ACCT) {
>> +		if (mp->m_qflags & XFS_UQUOTA_ENFD)
>> +			seq_puts(m, ",usrquota");
>> +		else
>> +			seq_puts(m, ",uqnoenforce");
>> +	}
>>  
>>  	if (mp->m_qflags & XFS_PQUOTA_ACCT) {
>>  		if (mp->m_qflags & XFS_PQUOTA_ENFD)
>> -- 
>> 2.20.0
>>
Darrick J. Wong Nov. 24, 2020, 4:50 p.m. UTC | #5
On Tue, Nov 24, 2020 at 04:37:07PM +0800, kaixuxia wrote:
> 
> 
> On 2020/11/24 8:30, Darrick J. Wong wrote:
> > On Mon, Nov 23, 2020 at 05:38:52PM +0800, xiakaixu1987@gmail.com wrote:
> >> From: Kaixu Xia <kaixuxia@tencent.com>
> >>
> >> The quota option 'usrquota' should be shown if both the XFS_UQUOTA_ACCT
> >> and XFS_UQUOTA_ENFD flags are set. The option 'uqnoenforce' should be
> >> shown when only the XFS_UQUOTA_ACCT flag is set. The current code logic
> >> seems wrong, Fix it and show proper options.
> > 
> > This needs a regression test case to make sure that quota mount options
> > passed in ==> quota options in /proc/mounts, wouldn't you say? ;)
> 
> Hi Darrick,
> 
> The simple test case as follows:
> 
> Before the patch:
>  # mount -o uqnoenforce /dev/vdc1 /data1
>  # cat /proc/mounts | grep xfs
> /dev/vdc1 /data1 xfs rw,relatime,attr2,inode64,logbufs=8,logbsize=32k,usrquota 0 0
> 
> After the patch:
>  # mount -o uqnoenforce /dev/vdc1 /data1
>  # cat /proc/mounts | grep xfs
> /dev/vdc1 /data1 xfs rw,relatime,attr2,inode64,logbufs=8,logbsize=32k,uqnoenforce 0 0
> 
> I'm not sure if a xfstest case is needed:)

It's been broken for a decade and nobody noticed.

YES IT IS.

--D

> 
> Thanks,
> Kaixu
> 
> > 
> > --D
> > 
> >> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> >> ---
> >>  fs/xfs/xfs_super.c | 10 ++++++----
> >>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> >> index e3e229e52512..5ebd6cdc44a7 100644
> >> --- a/fs/xfs/xfs_super.c
> >> +++ b/fs/xfs/xfs_super.c
> >> @@ -199,10 +199,12 @@ xfs_fs_show_options(
> >>  		seq_printf(m, ",swidth=%d",
> >>  				(int)XFS_FSB_TO_BB(mp, mp->m_swidth));
> >>  
> >> -	if (mp->m_qflags & (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD))
> >> -		seq_puts(m, ",usrquota");
> >> -	else if (mp->m_qflags & XFS_UQUOTA_ACCT)
> >> -		seq_puts(m, ",uqnoenforce");
> >> +	if (mp->m_qflags & XFS_UQUOTA_ACCT) {
> >> +		if (mp->m_qflags & XFS_UQUOTA_ENFD)
> >> +			seq_puts(m, ",usrquota");
> >> +		else
> >> +			seq_puts(m, ",uqnoenforce");
> >> +	}
> >>  
> >>  	if (mp->m_qflags & XFS_PQUOTA_ACCT) {
> >>  		if (mp->m_qflags & XFS_PQUOTA_ENFD)
> >> -- 
> >> 2.20.0
> >>
> 
> -- 
> kaixuxia
Darrick J. Wong Dec. 6, 2020, 11:12 p.m. UTC | #6
On Mon, Nov 23, 2020 at 05:38:52PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> The quota option 'usrquota' should be shown if both the XFS_UQUOTA_ACCT
> and XFS_UQUOTA_ENFD flags are set. The option 'uqnoenforce' should be
> shown when only the XFS_UQUOTA_ACCT flag is set. The current code logic
> seems wrong, Fix it and show proper options.
> 
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>

FWIW this causes a regression in xfs/513 since mount option uqnoenforce
no longer causes 'usrquota' to be emitted in /proc/mounts.  Do you have
a patch to fix fstests?

--D

> ---
>  fs/xfs/xfs_super.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index e3e229e52512..5ebd6cdc44a7 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -199,10 +199,12 @@ xfs_fs_show_options(
>  		seq_printf(m, ",swidth=%d",
>  				(int)XFS_FSB_TO_BB(mp, mp->m_swidth));
>  
> -	if (mp->m_qflags & (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD))
> -		seq_puts(m, ",usrquota");
> -	else if (mp->m_qflags & XFS_UQUOTA_ACCT)
> -		seq_puts(m, ",uqnoenforce");
> +	if (mp->m_qflags & XFS_UQUOTA_ACCT) {
> +		if (mp->m_qflags & XFS_UQUOTA_ENFD)
> +			seq_puts(m, ",usrquota");
> +		else
> +			seq_puts(m, ",uqnoenforce");
> +	}
>  
>  	if (mp->m_qflags & XFS_PQUOTA_ACCT) {
>  		if (mp->m_qflags & XFS_PQUOTA_ENFD)
> -- 
> 2.20.0
>
kaixuxia Dec. 7, 2020, 3:13 a.m. UTC | #7
On 2020/12/7 7:12, Darrick J. Wong wrote:
> On Mon, Nov 23, 2020 at 05:38:52PM +0800, xiakaixu1987@gmail.com wrote:
>> From: Kaixu Xia <kaixuxia@tencent.com>
>>
>> The quota option 'usrquota' should be shown if both the XFS_UQUOTA_ACCT
>> and XFS_UQUOTA_ENFD flags are set. The option 'uqnoenforce' should be
>> shown when only the XFS_UQUOTA_ACCT flag is set. The current code logic
>> seems wrong, Fix it and show proper options.
>>
>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> 
> FWIW this causes a regression in xfs/513 since mount option uqnoenforce
> no longer causes 'usrquota' to be emitted in /proc/mounts.  Do you have
> a patch to fix fstests?

Yeah, I'll send the patches to fix the regression in xfs/513 and add the
xfstest case for this bug ASAP:)

Thanks,
Kaixu
> 
> --D
> 
>> ---
>>  fs/xfs/xfs_super.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index e3e229e52512..5ebd6cdc44a7 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -199,10 +199,12 @@ xfs_fs_show_options(
>>  		seq_printf(m, ",swidth=%d",
>>  				(int)XFS_FSB_TO_BB(mp, mp->m_swidth));
>>  
>> -	if (mp->m_qflags & (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD))
>> -		seq_puts(m, ",usrquota");
>> -	else if (mp->m_qflags & XFS_UQUOTA_ACCT)
>> -		seq_puts(m, ",uqnoenforce");
>> +	if (mp->m_qflags & XFS_UQUOTA_ACCT) {
>> +		if (mp->m_qflags & XFS_UQUOTA_ENFD)
>> +			seq_puts(m, ",usrquota");
>> +		else
>> +			seq_puts(m, ",uqnoenforce");
>> +	}
>>  
>>  	if (mp->m_qflags & XFS_PQUOTA_ACCT) {
>>  		if (mp->m_qflags & XFS_PQUOTA_ENFD)
>> -- 
>> 2.20.0
>>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index e3e229e52512..5ebd6cdc44a7 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -199,10 +199,12 @@  xfs_fs_show_options(
 		seq_printf(m, ",swidth=%d",
 				(int)XFS_FSB_TO_BB(mp, mp->m_swidth));
 
-	if (mp->m_qflags & (XFS_UQUOTA_ACCT|XFS_UQUOTA_ENFD))
-		seq_puts(m, ",usrquota");
-	else if (mp->m_qflags & XFS_UQUOTA_ACCT)
-		seq_puts(m, ",uqnoenforce");
+	if (mp->m_qflags & XFS_UQUOTA_ACCT) {
+		if (mp->m_qflags & XFS_UQUOTA_ENFD)
+			seq_puts(m, ",usrquota");
+		else
+			seq_puts(m, ",uqnoenforce");
+	}
 
 	if (mp->m_qflags & XFS_PQUOTA_ACCT) {
 		if (mp->m_qflags & XFS_PQUOTA_ENFD)