Re: [PATCH RFC] xfs: allow adjusting individual quota grace times

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Sat, May 09, 2020 at 02:47:02PM -0500, Eric Sandeen wrote:
> vfs/ext3/4 quota allows the administrator to push out the grace time
> for soft quota with the setquota -T command:
> 
> setquota -T [ -u | -g ] [ -F quotaformat ] name block-grace inode-grace -a | filesystem...
> 
>        -T, --edit-times
>               Alter times for individual user/group when softlimit is enforced.
>               Times block-grace and inode-grace are specified in seconds or can
>               be string 'unset'.
> 
> Essentially, if you do "setquota -T -u username 1d 1d" and "username" is
> over their soft quotas and into their grace time, it will extend the
> grace time expiry to 1d from now.
> 
> xfs can't do this, today.  The patch below is a first cut at allowing us
> to do this, and userspace updates are needed as well (I have those in a
> patch stack.)
> 
> I'm not looking so much for patch review right now, though, what I'm
> wondering is if this is a change we can make from the ABI perspective?
> 
> Because today, if you try to pass in a UID other than 0 (i.e. the
> default grace period) it just gets ignored by the kernel, not rejected.
> 
> So there's no real way to know that the grace period adjustment failed
> on an older kernel.  We could consider that a bug and fix it, or
> consider it a change in behavior that we can't just make without
> at least some form of versioning.  Thoughts?
> 
> Anyway, the patch below moves the disk quota grace period adjustment out
> from "if id == 0" and allows the change for any ID; it only sets the
> default grace value in the "id == 0" case.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index f48561b7e947..e58ee98f938c 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -555,32 +555,41 @@ xfs_qm_scall_setqlim(
>  		ddq->d_rtbwarns = cpu_to_be16(newlim->d_rt_spc_warns);
>  
>  	if (id == 0) {
> -		/*
> -		 * Timelimits for the super user set the relative time
> -		 * the other users can be over quota for this file system.
> -		 * If it is zero a default is used.  Ditto for the default
> -		 * soft and hard limit values (already done, above), and
> -		 * for warnings.
> -		 */
> -		if (newlim->d_fieldmask & QC_SPC_TIMER) {
> -			defq->btimelimit = newlim->d_spc_timer;
> -			ddq->d_btimer = cpu_to_be32(newlim->d_spc_timer);
> -		}
> -		if (newlim->d_fieldmask & QC_INO_TIMER) {
> -			defq->itimelimit = newlim->d_ino_timer;
> -			ddq->d_itimer = cpu_to_be32(newlim->d_ino_timer);
> -		}
> -		if (newlim->d_fieldmask & QC_RT_SPC_TIMER) {
> -			defq->rtbtimelimit = newlim->d_rt_spc_timer;
> -			ddq->d_rtbtimer = cpu_to_be32(newlim->d_rt_spc_timer);
> -		}
>  		if (newlim->d_fieldmask & QC_SPC_WARNS)
>  			defq->bwarnlimit = newlim->d_spc_warns;
>  		if (newlim->d_fieldmask & QC_INO_WARNS)
>  			defq->iwarnlimit = newlim->d_ino_warns;
>  		if (newlim->d_fieldmask & QC_RT_SPC_WARNS)
>  			defq->rtbwarnlimit = newlim->d_rt_spc_warns;
> -	} else {
> +	}
> +
> +	/*
> +	 * Timelimits for the super user set the relative time the other users
> +	 * can be over quota for this file system. If it is zero a default is
> +	 * used.  Ditto for the default soft and hard limit values (already
> +	 * done, above), and for warnings.
> +	 *
> +	 * For other IDs, userspace can bump out the grace period if over
> +	 * the soft limit.
> +	 */
> +	if (newlim->d_fieldmask & QC_SPC_TIMER) {
> +		if (!id)
> +			defq->btimelimit = newlim->d_spc_timer;
> +		ddq->d_btimer = cpu_to_be32(newlim->d_spc_timer);
> +	}
> +	if (newlim->d_fieldmask & QC_INO_TIMER) {
> +		printk("setting inode timer to %d\n", newlim->d_ino_timer);

Stray printk here.

> +		if (!id)
> +			defq->itimelimit = newlim->d_ino_timer;
> +		ddq->d_itimer = cpu_to_be32(newlim->d_ino_timer);
> +	}
> +	if (newlim->d_fieldmask & QC_RT_SPC_TIMER) {
> +		if (!id)
> +			defq->rtbtimelimit = newlim->d_rt_spc_timer;
> +		ddq->d_rtbtimer = cpu_to_be32(newlim->d_rt_spc_timer);
> +	}

Otherwise I guess this looks reasonable.  It might help to patchbomb all
the kernel/userspace/fstests changes together so I don't have to go
scurrying around to find the pieces(?)

--D

> +
> +	if (id != 0) {
>  		/*
>  		 * If the user is now over quota, start the timelimit.
>  		 * The user will not be 'warned'.
> 
>