11479 zfs project support

Review Request #2213 — Created Aug. 8, 2019 and submitted

jjelinek
illumos-gate
11479
general

Port from ZoL of:
9c5167d19 Project Quota on ZFS
1de321e62 Add support for user/group dnode accounting & quota
along with some secondary commits.

This code was more Linux-specific than most of ZFS. There are a lot of changes in the new tests to make them run on illumos. There was also Linux-speciic code in the kernel changes which I had to port to illumos.



  • 0
  • 0
  • 27
  • 8
  • 35
Description From Last Updated
seeemef@mac.com
  1. LGTM with just some comment or string fixes found by me

  2. usr/src/cmd/zfs/zfs_main.c (Diff revision 1)
     
     

    " \n" is odd but I guess ZoL uses that? (Here and below a few.)

    1. Left as-is to match ZoL behavior

  3. usr/src/cmd/zfs/zfs_main.c (Diff revision 1)
     
     

    Typo "subdirectorie". Is "specifying" better here?

  4. usr/src/cmd/zfs/zfs_main.c (Diff revision 1)
     
     

    typo "descendants"

    1. I think this spelling of "descendants" is correct and is also the proper usage here and in all of the other places you identified. If you can point me to a reference which indicates otherwise, I'll revisit this.

  5. usr/src/cmd/zfs/zfs_project.c (Diff revision 1)
     
     

    I guess we can't touch a copyright or attribution but "Intle"?

    1. I agree this looks like a typo, but since it is the copyright, I won't change it.

  6. usr/src/man/man1m/zfs.1m (Diff revision 1)
     
     

    Is "set or changed" better here?

  7. usr/src/man/man1m/zfs.1m (Diff revision 1)
     
     

    "So need neither" is odd

    1. This is odd, but is the original wording from ZoL and I'm not sure what to use instead.

  8. usr/src/man/man1m/zfs.1m (Diff revision 1)
     
     

    typo "childrens"

  9. usr/src/man/man1m/zfs.1m (Diff revision 1)
     
     

    If "directories" then "files"?

  10. usr/src/man/man1m/zfs.1m (Diff revision 1)
     
     

    If "directories" then "files"?

  11. usr/src/man/man1m/zfs.1m (Diff revision 1)
     
     

    typo "childrens"

  12. usr/src/man/man1m/zfs.1m (Diff revision 1)
     
     

    If "directories'" then "files'"?

  13. usr/src/man/man1m/zfs.1m (Diff revision 1)
     
     

    If "directories" then "files"?

  14. usr/src/man/man1m/zfs.1m (Diff revision 1)
     
     
  15. usr/src/man/man1m/zfs.1m (Diff revision 1)
     
     
  16. usr/src/man/man1m/zfs.1m (Diff revision 1)
     
     

    typo "descendants"

    1. I changed the wording to reflect what I think is meant here.

  17. "overwrites" better (here and below)?

    1. I think this is ok since "fs" is read as "filesystem".

  18. typo "descendants" (here and below)

  19. usr/src/uts/common/fs/zfs/sa.c (Diff revision 1)
     
     

    Better as "existing" or "extant"?

  20. usr/src/uts/common/fs/zfs/zfs_vfsops.c (Diff revision 1)
     
     

    I don't follow why the handling here and for GROUPOBJQUOTA doesn't set to zero on ENOENT

    1. This is a good catch. I did some research on the ZoL code and this was fixed in ZoL via another commit which is recorded as a fix already in OpenZFS. Since we have that code already in illumos (but without the relevant project-related pieces), I just took the two relevant fixes from that commit and incorporated them here.

  21. usr/src/uts/common/fs/zfs/zfs_vnops.c (Diff revision 1)
     
     

    This paragraph doesn't read very clearly.

    1. I tried to reword it a little to make it clearer.

  22. usr/src/uts/common/fs/zfs/zfs_vnops.c (Diff revision 1)
     
     

    "existing" or "extant" is better?

  23. usr/src/uts/common/fs/zfs/zfs_vnops.c (Diff revision 1)
     
     

    "it means"?

  24. usr/src/uts/common/fs/zfs/zfs_vnops.c (Diff revision 1)
     
     

    typo "descendant"

  25. usr/src/uts/common/fs/zfs/zfs_vnops.c (Diff revision 1)
     
     

    "it means"?

  26. usr/src/uts/common/fs/zfs/zfs_vnops.c (Diff revision 1)
     
     

    typo "descendant"

  27. 
      
jjelinek
seeemef@mac.com
  1. LGTM

  2. 
      
kkantor
  1. Just a few nit-picky things. I'm not sure if it's worth fixing any of them.

    Also, I would prefer that we keep our test .ksh files with 644 permissions rather than the ZoL 755 permissions.

    1. I've updated the permissions.

  2. usr/src/cmd/zfs/zfs_project.c (Diff revision 2)
     
     

    Do we need to add a (void) here to prevent smatch from flagging this?

  3. Are these commented out because we illumos doesn't have these programs to modify xattrs?

    1. Yes, these are Linux programs. illumos uses an entirely different CLI for managing xattrs. See the 'runat' command and the fsattr(5) man page. I left these in so we could compare to the intended ZoL behavior, but since these tests were so Linux-specific, I figured we would have to just maintain our own versions.

  4. usr/src/uts/common/fs/zfs/dmu_objset.c (Diff revision 2)
     
     

    This variable appears to be unused.

  5. usr/src/uts/common/fs/zfs/dmu_objset.c (Diff revision 2)
     
     

    This is another ignored return value. I'm not sure how much we care about stuff like that.

  6. 
      
jjelinek
kkantor
  1. Ship It!
  2. 
      
jjelinek
Review request changed

Status: Closed (submitted)

Loading...