zfs icon indicating copy to clipboard operation
zfs copied to clipboard

Fix sa_add_projid to lookup and update SA_ZPL_DXATTR (avoid DXATTR loss)

Open jsai20 opened this issue 1 year ago • 1 comments

sa_add_projid() gets called via zfs_setattr() for setting project id on old file/dir, which were created before upgrading to project quota feature. This function does lookup for all possible SA and update them all together along with project ID at needed fixed offset. But its missing lookup and update of SA_ZPL_DXATTR, effectively it losses SA_ZPL_DXATTR.

Signed-off-by: Jitendra Patidar [email protected] Closes #16287

Motivation and Context

Description

How Has This Been Tested?

Types of changes

  • [x] 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:

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

jsai20 avatar Jun 21 '24 16:06 jsai20

@jsai20 I'm new to this particular bit of code and an trying to understand what is being fixed. Is it that when you update old, pre-project-quota files with sa_add_projid(), that the project ID is not getting set in the SA xattrs, and this PR fixes it?

tonyhutter avatar Jul 13 '24 00:07 tonyhutter

@jsai20 I'm new to this particular bit of code and an trying to understand what is being fixed. Is it that when you update old, pre-project-quota files with sa_add_projid(), that the project ID is not getting set in the SA xattrs, and this PR fixes it?

@tonyhutter Following header comment on sa_add_projid(), explains why sa_add_projid() needed. /*

  • For the existed object that is upgraded from old system, its ondisk layout
  • has no slot for the project ID attribute. But quota accounting logic needs
  • to access related slots by offset directly. So we need to adjust these old
  • objects' layout to make the project ID to some unified and fixed offset. */ #define SA_PROJID_OFFSET 128

So, sa_add_projid() function basically expected to do lookup for all possible existing SA on object, reorder them to fix offset for project id. And finally update all SA's together.

sa_add_projid() currently doesn't lookup and update of SA_ZPL_DXATTR, effectively it losses existing SA_ZPL_DXATTR on object.

jsai20 avatar Jul 15 '24 04:07 jsai20

Ok, so it basically allows you to set project ID on a upgraded pool with xattr=sa.

tonyhutter avatar Jul 15 '24 23:07 tonyhutter

Looking for one more reviewer on this

tonyhutter avatar Jul 23 '24 23:07 tonyhutter

I don't know anything about DXATTR, but the logic seems right when compared to the way it's used elsewhere.

A couple of structure nits, but I won't hold it up because of them. Good work, thanks!

Thanks @robn. I just updated a new patch addressing your comments.

jsai20 avatar Jul 24 '24 05:07 jsai20

LGTM!

robn avatar Jul 24 '24 05:07 robn