Remove projectquota space upgrade and usage update of ZFS_INVALID_PROJID

[jsai20]

Upgrading to feature@project_quota, currently trigger upgrade task around project quota usage upgrade, which marks each inode dirty via, dmu_objset_id_quota_upgrade()->dmu_objset_id_quota_upgrade_cb()-> dmu_objset_space_upgrade().
Project quota space upgrade task marks each dnode dirty, expecting that when the dnode_sync() is done, dnode’s project usage would be accounted. But as there is no change in projid, so effectively, project quota upgrade task doesn't change anything.

When actual projid is set on an object via zfs project -p <projid> -s <file/dir>, then object usage gets account-ed under the projid set. But usage for projid=0 (ZFS_DEFAULT_PROJID) underflows and becomes negative. Because quota update task moves usage from projid "0" to new projid set.

Solution:
dmu_objset_space_upgrade() for projectquota doesn't change the usage accounting, so skip it. This effectively avoids dirtying large number of dnodes, which is un-necessary.

When object doesn't have projid set, means its ZFS_INVALID_PROJID, so change zpl_get_file_info() to consider projid as ZFS_INVALID_PROJID instead of ZFS_DEFAULT_PROJID=0. And skip updating usage for ZFS_INVALID_PROJID in do_*quota_update(). This effectively avoid the underflow of usage accounting on ZFS_DEFAULT_PROJID.

Motivation and Context

Description

Problem:
Upgrading to feature@project_quota, currently trigger upgrade task around project quota usage upgrade, which marks each inode dirty via, dmu_objset_id_quota_upgrade()->dmu_objset_id_quota_upgrade_cb()-> dmu_objset_space_upgrade().
Project quota space upgrade task marks each dnode dirty, expecting that when the dnode_sync() is done, dnode’s project usage would be accounted. But as there is no change in projid, so effectively, project quota upgrade task doesn't change anything.

When actual projid is set on an object via zfs project -p <projid> -s <file/dir>, then object usage gets account-ed under the projid set. But usage for projid=0 (ZFS_DEFAULT_PROJID) underflows and becomes negative. Because quota update task moves usage from projid "0" to new projid set.

Solution:
dmu_objset_space_upgrade() for projectquota doesn't change the usage accounting, so skip it. This effectively avoids dirtying large number of dnodes, which is un-necessary.

When object doesn't have projid set, means its ZFS_INVALID_PROJID, so change zpl_get_file_info() to consider projid as ZFS_INVALID_PROJID instead of ZFS_DEFAULT_PROJID=0. And skip updating usage for ZFS_INVALID_PROJID in do_*quota_update(). This effectively avoid the underflow of usage accounting on ZFS_DEFAULT_PROJID.

How Has This Been Tested?

Without fix:

$sudo zpool create -o feature@project_quota=disabled  mytestpool disk.1
$ sudo zfs create -o xattr=sa mytestpool/fs1
$ cd /mytestpool/fs1/
$ sudo chmod 0777 ../fs1
$ dd if=/dev/urandom of=file1 bs=64K count=2
$ cd -
$ sudo zpool upgrade mytestpool

This system supports ZFS pool feature flags.

Enabled the following features on 'mytestpool':
  project_quota
$
$ sudo zfs umount mytestpool/fs1
$ sudo zfs mount mytestpool/fs1
$ cd /mytestpool/fs1
$
$ sudo zfs projectspace mytestpool/fs1
$ zfs project -p 111 -s file1
$ sudo zfs projectspace mytestpool/fs1
TYPE     NAME   USED  QUOTA  OBJUSED  OBJQUOTA
Project  0     16.0E   none    16.0E      none
Project  111    128K   none        1      none
$
$ dd if=/dev/urandom of=file2 bs=64K count=2
$ sudo zfs projectspace mytestpool/fs1
TYPE     NAME   USED  QUOTA  OBJUSED  OBJQUOTA
Project  0     16.0E   none    16.0E      none
Project  111    128K   none        1      none
$
$ zfs project -p 111 -s file2
$ sudo zfs projectspace mytestpool/fs1
TYPE     NAME   USED  QUOTA  OBJUSED  OBJQUOTA
Project  0     16.0E   none    16.0E      none
Project  111    257K   none        2      none
$

With Fix:

$ sudo zpool create -o feature@project_quota=disabled  mytestpool disk.1
$ sudo zfs create -o xattr=sa mytestpool/fs1
$ cd /mytestpool/fs1/
$ sudo chmod 0777 ../fs1
$ dd if=/dev/urandom of=file1 bs=64K count=2

$ cd -
$ sudo zpool upgrade mytestpool
This system supports ZFS pool feature flags.

Enabled the following features on 'mytestpool':
  project_quota

$ sudo zfs umount mytestpool/fs1
$ sudo zfs mount mytestpool/fs1

$ cd /mytestpool/fs1
$ sudo zfs projectspace mytestpool/fs1
$ zfs project -p 111 -s file1
$ sudo zfs projectspace mytestpool/fs1
TYPE     NAME  USED  QUOTA  OBJUSED  OBJQUOTA
Project  111   128K   none        1      none
$
$ dd if=/dev/urandom of=file2 bs=64K count=2
$ sudo zfs projectspace mytestpool/fs1
TYPE     NAME  USED  QUOTA  OBJUSED  OBJQUOTA
Project  0     128K   none        1      none
Project  111   128K   none        1      none
$ zfs project -p 111 -s file2
$ sudo zfs projectspace mytestpool/fs1
TYPE     NAME  USED  QUOTA  OBJUSED  OBJQUOTA
Project  111   257K   none        2      none
$

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[amotin]

I am not really familiar with project quotas, so while from one side proposed solution kind of makes sense, from another I wonder what is the expected behavior for the case of project not being set. Should there really be a default project, and so ZFS should properly initialize it as part of the upgrade, which as I understand it does not do now, or as you have made it -- there should be no default project. Considering that on my pool where I never used projects I see reasonable space accounting for project 0, this change may be a POLA violation for people who might already use it.

[jsai20]

I am not really familiar with project quotas, so while from one side proposed solution kind of makes sense, from another I wonder what is the expected behavior for the case of project not being set. Should there really be a default project, and so ZFS should properly initialize it as part of the upgrade, which as I understand it does not do now, or as you have made it -- there should be no default project. Considering that on my pool where I never used projects I see reasonable space accounting for project 0, this change may be a POLA violation for people who might already use it.

After project quota feature upgrade, new objects created by default would have project id 0 set on them and they gets accounted in project 0, so change as such not eliminating project 0 accounting.
Old objects created before upgrade, don't have project id set on them. I consider their project id as ZFS_INVALID_PROJID (-1). Current code of zpl_get_file_info() considers object without project id as under project 0 only, which is incorrect. So, change includes a fix to consider project id as ZFS_INVALID_PROJID, if object doesn't have project id set yet. And don't do ondisk update/accounting for ZFS_INVALID_PROJID. This effectively avoids underflow in usage accounting for ZFS_DEFAULT_PROJID, when new/actual projid is set on object.

About upgrading old objects, you are correct about broken dmu_objset_space_upgrade(). Currently, upgrade task doesn't move any accounting, so it sort of no-op. This change is skipping dmu_objset_space_upgrade() for project quota, considering that its any way no-op. So, as such, this patch doesn't change the end impacts with respect to upgrade and it avoids dirtying lot of inodes un-necessary.
But yes, Ideally, dmu_objset_space_upgrade() should move old objects accounting to project 0 (ZFS_DEFAULT_PROJID). This means that it needs to call sa_add_projid() on each object. As dmu_objset_space_upgrade() is a dmu layer function, but sa_add_projid() is SA layer function, which would need a znode and sa handle, so I am not really sure, if its really feasible to do sa_add_projid(sa_handle, ZFS_DEFAULT_PROJID) on each object of dataset inside dmu_objset_space_upgrade().

[amotin]

@jsai20 I don't like the concept that objects created before upgrade are counted as ZFS_INVALID_PROJID, while after upgrade as project 0. It makes project 0 accounting useless, if it was ever intended otherwise. I haven't looked on the code in question and can't say out of my head how to better solve the migration, but if we go from assumption that project 0 should be valid, then upgrade fixing should be the way to go.

[jsai20]

@jsai20 I don't like the concept that objects created before upgrade are counted as ZFS_INVALID_PROJID, while after upgrade as project 0. It makes project 0 accounting useless, if it was ever intended otherwise. I haven't looked on the code in question and can't say out of my head how to better solve the migration, but if we go from assumption that project 0 should be valid, then upgrade fixing should be the way to go.

ok @amotin . Let me also check bit more on correctly upgrading old objects to project 0.

[amotin]

Let me also check bit more on correctly upgrading old objects to project 0.

Purely theoretical, I guess it might be not updating the objects, but just accounting them. But I guess it might be impossible since the operation is not done atomically and we need to track what is already done.

[jsai20]

Let me also check bit more on correctly upgrading old objects to project 0.

Purely theoretical, I guess it might be not updating the objects, but just accounting them. But I guess it might be impossible since the operation is not done atomically and we need to track what is already done.

I double checked it. Upgrade neither updates the object with project 0, nor updates the usage accounting for project 0.

For correctly updating accounting the old objects in project 0 (ZFS_DEFAULT_PROJID), it need to re-layout SA's on object and for that it need to set project id 0 via sa_add_projid() interface. I explored doing it, but its not feasible to use this SA layer function sa_add_projid() in dmu_objset_space_upgrade(). It needs SA handle. To get the SA handle, need to do zfs_zget() on object, which needs zfsvfs. zfsvfs is not initialised, when dmu_objset_space_update() gets called during mount.

As such I don't see any real benefits of usage accounting for project 0, neither for objects created before upgrade nor for objects created after grade. Its just un-necessary work of dirtying inodes in upgrade without any real benefits .
Just to keep it simple, we can just skip updating usage accounting for project 0 always (both for objects created before upgrade and after upgrade).

I would update a patch which doesn't account usage for project 0.

[amotin]

@jsai20 It does not build on FreeBSD:

/home/zfs/zfs/module/zfs/dmu_objset.c:2530:26: error: no member named 'comm' in 'struct thread'
   2530 |                     __func__, current->comm, current->pid, name);
        |                               ~~~~~~~  ^
  /home/zfs/zfs/module/zfs/dmu_objset.c:2530:41: error: no member named 'pid' in 'struct thread'
   2530 |                     __func__, current->comm, current->pid, name);
        |                                              ~~~~~~~  ^
  /home/zfs/zfs/module/zfs/dmu_objset.c:2535:26: error: no member named 'comm' in 'struct thread'
   2535 |                     __func__, current->comm, current->pid, name);
        |                               ~~~~~~~  ^
  /home/zfs/zfs/module/zfs/dmu_objset.c:2535:41: error: no member named 'pid' in 'struct thread'
   2535 |                     __func__, current->comm, current->pid, name);
        |                                              ~~~~~~~  ^

It looks like excessively verbose debug that should be removed.

[jsai20]

@jsai20 It does not build on FreeBSD:

/home/zfs/zfs/module/zfs/dmu_objset.c:2530:26: error: no member named 'comm' in 'struct thread'
   2530 |                     __func__, current->comm, current->pid, name);
        |                               ~~~~~~~  ^
  /home/zfs/zfs/module/zfs/dmu_objset.c:2530:41: error: no member named 'pid' in 'struct thread'
   2530 |                     __func__, current->comm, current->pid, name);
        |                                              ~~~~~~~  ^
  /home/zfs/zfs/module/zfs/dmu_objset.c:2535:26: error: no member named 'comm' in 'struct thread'
   2535 |                     __func__, current->comm, current->pid, name);
        |                               ~~~~~~~  ^
  /home/zfs/zfs/module/zfs/dmu_objset.c:2535:41: error: no member named 'pid' in 'struct thread'
   2535 |                     __func__, current->comm, current->pid, name);
        |                                              ~~~~~~~  ^

It looks like excessively verbose debug that should be removed.

Updated new patch Replacing "current->comm" with getcomm() and "current->pid()" with getpid().

[amotin]

Updated new patch Replacing "current->comm" with getcomm() and "current->pid()" with getpid().

The building seems going better now, but why do you think these messages are useful enough to be seen on a console of every system? IMO its a mess.

[jsai20]

Updated new patch Replacing "current->comm" with getcomm() and "current->pid()" with getpid().

The building seems going better now, but why do you think these messages are useful enough to be seen on a console of every system? IMO its a mess.

Ok. I would remove them.