Re: [PATCH 1/2] generic: per-type quota timers set/get test

From: Eric Sandeen <sandeen@sandeen.net>
To: Zorro Lang <zlang@redhat.com>, fstests@vger.kernel.org
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 1/2] generic: per-type quota timers set/get test
Date: Tue, 18 Feb 2020 16:02:41 -0600	[thread overview]
Message-ID: <9b442ccc-a32f-2199-c682-16a403caf9f2@sandeen.net> (raw)
In-Reply-To: <af69203a-3e6f-31a3-d083-acd1f6654fbb@sandeen.net>

On 2/18/20 3:44 PM, Eric Sandeen wrote:
> 
> 
> On 2/18/20 3:41 PM, Eric Sandeen wrote:
>> On 2/16/20 12:16 PM, Zorro Lang wrote:
>>> Set different grace time, make sure each of quota (user, group and
>>> project) timers can be set (by setquota) and get (by repquota)
>>> correctly.
>>>
>>> Signed-off-by: Zorro Lang <zlang@redhat.com>
>>> ---
>>>
>>> Hi,
>>>
>>> This case test passed on ext4, but on XFS (xfs-linux for-next branch with
>>> Eric's patchset: [PATCH 0/4] xfs: enable per-type quota timers and warn limits)
>>> I got below different output:
>>
>> *sigh* I wish we didn't have so many different quota tools & interfaces.
>>
>> Behold:
>>
>> # export MIN=60
>> # setquota -t -g $((30 * MIN)) $((40 * MIN)) /mnt/scratch
>>
>>
>> # ./repquota -g /mnt/scratch
>> *** Report for group quotas on device /dev/pmem0p2
>> Block grace time: 00:00; Inode grace time: 00:00
>> ...
>>
>>
>> # xfs_quota -x -c "state -g"  /mnt/scratch
>> Group quota state on /mnt/scratch (/dev/pmem0p2)
>>   Accounting: ON
>>   Enforcement: ON
>>   Inode: #132 (1 blocks, 1 extents)
>> Blocks grace time: [0 days 00:30:00]
>> Inodes grace time: [0 days 00:40:00]
>> Realtime Blocks grace time: [--------]
>>
>> seems like this may actually be a bug in the quota/repquota code, trying to dig through
>> that now.
>>
>> repquota actually calls & gets group quota grace, but perhaps it gets overwritten with the subsequent call for the (empty) user quota:
>>
>> # strace -v -e quotactl ./repquota -g /mnt/scratch 2>&1 | grep "STAT|"
>> quotactl(Q_XGETQSTAT|USRQUOTA, "/dev/pmem0p2", 0, {version=1, flags=XFS_QUOTA_UDQ_ACCT|XFS_QUOTA_UDQ_ENFD|XFS_QUOTA_GDQ_ACCT|XFS_QUOTA_GDQ_ENFD, incoredqs=60, u_ino=131, u_nblks=1, u_nextents=1, g_ino=132, g_nblks=1, g_nextents=1, btimelimit=0, itimelimit=0, rtbtimelimit=0, bwarnlimit=0, iwarnlimit=0}) = 0
>> quotactl(Q_XGETQSTAT|GRPQUOTA, "/dev/pmem0p2", 0, {version=1, flags=XFS_QUOTA_UDQ_ACCT|XFS_QUOTA_UDQ_ENFD|XFS_QUOTA_GDQ_ACCT|XFS_QUOTA_GDQ_ENFD, incoredqs=60, u_ino=131, u_nblks=1, u_nextents=1, g_ino=132, g_nblks=1, g_nextents=1, btimelimit=1800, itimelimit=2400, rtbtimelimit=0, bwarnlimit=0, iwarnlimit=0}) = 0
>> quotactl(Q_XGETQSTAT|PRJQUOTA, "/dev/pmem0p2", 0, {version=1, flags=XFS_QUOTA_UDQ_ACCT|XFS_QUOTA_UDQ_ENFD|XFS_QUOTA_GDQ_ACCT|XFS_QUOTA_GDQ_ENFD, incoredqs=60, u_ino=131, u_nblks=1, u_nextents=1, g_ino=132, g_nblks=1, g_nextents=1, btimelimit=0, itimelimit=0, rtbtimelimit=0, bwarnlimit=0, iwarnlimit=0}) = 0
>> quotactl(Q_XGETQSTAT|USRQUOTA, "/dev/pmem0p2", 0, {version=1, flags=XFS_QUOTA_UDQ_ACCT|XFS_QUOTA_UDQ_ENFD|XFS_QUOTA_GDQ_ACCT|XFS_QUOTA_GDQ_ENFD, incoredqs=60, u_ino=131, u_nblks=1, u_nextents=1, g_ino=132, g_nblks=1, g_nextents=1, btimelimit=0, itimelimit=0, rtbtimelimit=0, bwarnlimit=0, iwarnlimit=0}) = 0
>>
>> but why doesn't this happen for ext4 ...
> 
> ... because on ext4, it makes different quotactl calls... of course :( :
> 
> quotactl(Q_GETINFO|GRPQUOTA, "/dev/pmem0p2", 0, {bgrace=1800, igrace=2400, flags=0, valid=IIF_BGRACE|IIF_IGRACE|IIF_FLAGS}) = 0

Ok, repquota only ever inits the quota info for this purpose with type 0 (i.e. usrquota)

I guess this fixes it:

diff --git a/quotaio_xfs.c b/quotaio_xfs.c
index 56daf89..b22c7b4 100644
--- a/quotaio_xfs.c
+++ b/quotaio_xfs.c
@@ -81,7 +81,7 @@ static int xfs_init_io(struct quota_handle *h)
        struct xfs_mem_dqinfo info;
        int qcmd;
 
-       qcmd = QCMD(Q_XFS_GETQSTAT, 0);
+       qcmd = QCMD(Q_XFS_GETQSTAT, h->qh_type);
        memset(&info, 0, sizeof(struct xfs_mem_dqinfo));
        if (quotactl(qcmd, h->qh_quotadev, 0, (void *)&info) < 0)
                return -1;

and I need to see what's going on here, but probably this too:

diff --git a/quotaon_xfs.c b/quotaon_xfs.c
index d557a75..d137240 100644
--- a/quotaon_xfs.c
+++ b/quotaon_xfs.c
@@ -32,7 +32,7 @@ static int xfs_state_check(int qcmd, int type, int flags, const char *dev, int r
        if (flags & STATEFLAG_ALL)
                return 0;       /* noop */
 
-       if (quotactl(QCMD(Q_XFS_GETQSTAT, 0), dev, 0, (void *)&info) < 0) {
+       if (quotactl(QCMD(Q_XFS_GETQSTAT, type), dev, 0, (void *)&info) < 0) {
                errstr(_("quotactl() on %s: %s\n"), dev, strerror(errno));
                return -1;
        }

