Re: The question of Q_XQUOTARM ioctl

Re: The question of Q_XQUOTARM ioctl

[Darrick J. Wong]

On Wed, Oct 20, 2021 at 03:56:10AM +0000, xuyang2018.jy@fujitsu.com wrote:
> Hi Darrick
> 
> Sorry for bothering you again.

No problem.  Sorry I lost this email for 2+ months. :(

> After Christoph Hellwig kernel patch("xfs: remove support for disabling
> quota accounting on a mounted file system"), we can't disable quota
> account feature on a mounted file system.
> 
> It causes Q_XQUOTARM ioctl doesn't work well because this ioctl needs
> quota accouting feature is off and it also needs super block has quota
> feature[1].
> 
> For quotactl man-pages about Q_XQUOTARM ioctl, it said "Free the disk
> space taken by disk quotas". I guess it free u/g/p inode.

Yes, that's what it's supposed to do.

> If we do normal mount with uquota feature and umount, then we should
> have free the inode(also changes in disk).
> 
> I don't know the right intention for Q_XQUOTARM now. Can you give me
> some advise? Or, we should remove Q_XQUOTARM ioctl and
> xfs_qm_scall_trunc_qfile code.

I think xfs_qm_scall_trunc_qfiles probably should be doing:

	if (xfs_has_quota(mp) || flags == 0 ||
	    (flags & ~XFS_QMOPT_QUOTALL)) {
		xfs_debug(...);
		return -EINVAL;
	}

Note the inversion in the has_quota test.  That would make it so that
you can truncate the quota files if quota is not on.

> If I understand wrong, please tell me.
> 
> ps: Christoph Hellwig kernel patch causes ltp quotactl07 fail, I found
> his patch by this case.
> 
> [1]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/xfs/xfs_qm_syscalls.c#n108

Why doesn't xfs/220 fail on the remove command?

Oh, because we patched it to filter that out, even though that's the
wrong thing to do.  That test really ought to remount with noquota and
then run xfs_quota -c remove $SCRATCH_DEV

--D

> Best Regards
> Yang Xu

Re: The question of Q_XQUOTARM ioctl

[Christoph Hellwig]

On Mon, Jan 03, 2022 at 06:34:56PM -0800, Darrick J. Wong wrote:
> > 
> > I don't know the right intention for Q_XQUOTARM now. Can you give me
> > some advise? Or, we should remove Q_XQUOTARM ioctl and
> > xfs_qm_scall_trunc_qfile code.
> 
> I think xfs_qm_scall_trunc_qfiles probably should be doing:
> 
> 	if (xfs_has_quota(mp) || flags == 0 ||
> 	    (flags & ~XFS_QMOPT_QUOTALL)) {
> 		xfs_debug(...);
> 		return -EINVAL;
> 	}
> 
> Note the inversion in the has_quota test.  That would make it so that
> you can truncate the quota files if quota is not on.

Yes, that sounds reasonable.  Although I'd split the xfs_has_quota
file into a separate check with a separate debug message.

Re: The question of Q_XQUOTARM ioctl

[Darrick J. Wong]

On Mon, Jan 03, 2022 at 11:21:07PM -0800, Christoph Hellwig wrote:
> On Mon, Jan 03, 2022 at 06:34:56PM -0800, Darrick J. Wong wrote:
> > > 
> > > I don't know the right intention for Q_XQUOTARM now. Can you give me
> > > some advise? Or, we should remove Q_XQUOTARM ioctl and
> > > xfs_qm_scall_trunc_qfile code.
> > 
> > I think xfs_qm_scall_trunc_qfiles probably should be doing:
> > 
> > 	if (xfs_has_quota(mp) || flags == 0 ||
> > 	    (flags & ~XFS_QMOPT_QUOTALL)) {
> > 		xfs_debug(...);
> > 		return -EINVAL;
> > 	}
> > 
> > Note the inversion in the has_quota test.  That would make it so that
> > you can truncate the quota files if quota is not on.

NAK, that's wrong.  xfs_has_quota tells us if the superblock feature bit
is set.  The feature bit guards the sb_[ugp]uotino fields, so the above
code causes us to bail out with EINVAL if the filesystem doesn't have
quota inodes at all.  Thus, inverting the check (to make it so that we
only try to truncate if the fields are garbage) is not correct.

> Yes, that sounds reasonable.  Although I'd split the xfs_has_quota
> file into a separate check with a separate debug message.

So I think the fix here is to fix the testcases.  xfs/220 becomes:

# turn off quota accounting...
$XFS_QUOTA_PROG -x -c off $SCRATCH_DEV

# ...but if the kernel doesn't support turning off accounting, remount with
# noquota option to turn it off...
if $XFS_QUOTA_PROG -x -c 'state -u' $SCRATCH_DEV | grep -q 'Accounting: ON'; then
	_scratch_unmount
	_scratch_mount -o noquota
fi

# ...and remove space allocated to the quota files
# (this used to give wrong ENOSYS returns in 2.6.31)
$XFS_QUOTA_PROG -x -c remove $SCRATCH_DEV


--D