11557 Log Spacemap Project

Review Request #2298 — Created Sept. 12, 2019 and submitted

jjelinek
illumos-gate
11557
general

A port from ZoL of the Log Spacemap project, along with some additional relevant fixes. This is a performance improvement which Delphix has been using for a long time. I've included the long description from the ZoL commit message in the bug report.



  • 0
  • 0
  • 22
  • 5
  • 27
Description From Last Updated
seeemef@mac.com
  1. Looks good. I just found some comment nits.

  2. usr/src/uts/common/fs/zfs/metaslab.c (Diff revision 1)
     
     

    typo "it's"

  3. usr/src/uts/common/fs/zfs/metaslab.c (Diff revision 1)
     
     

    typo superfluous full stop

  4. typo "olderst"

  5. 
      
tsoome
  1. I do know the temptation to keep diffs as is, but I really dislike how the quality is going down, we really should not let that happen.

    1. PS: yep, I do know the C99 does allow many things. So does traditional C. The ctyle requirements are there to help with readability, not to try to compere with compilers syntax check:)

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

    add new line after variable block. We do not need to be literally same as ZoL, especially when it hurts readability.

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

    It would be nice to have new line there too.

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

    move this empry line down:)

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

    remove empty line

  6. usr/src/cmd/zdb/zdb.c (Diff revision 1)
     
     

    remove empty line

  7. usr/src/cmd/zdb/zdb.c (Diff revision 1)
     
     

    Here we see why we should not call initializers in variable declaration block...

    1. The ZFS code very commonly uses C99 variable decleration rules, so I'm not going to change that, but I added a line to seperate it from the simple declarations.

  8. usr/src/cmd/zdb/zdb.c (Diff revision 1)
     
     

    same as previous comment. declare variables, then call initializers.

  9. usr/src/cmd/zdb/zdb.c (Diff revision 1)
     
     

    remove empty line

  10. usr/src/uts/common/fs/zfs/metaslab.c (Diff revision 1)
     
     

    switch this empty line with next.

  11. usr/src/uts/common/fs/zfs/metaslab.c (Diff revision 1)
     
     

    red ugliness.

  12. usr/src/uts/common/fs/zfs/range_tree.c (Diff revision 1)
     
     

    move this declaration up and (leave initializer down:)

  13. usr/src/uts/common/fs/zfs/spa.c (Diff revision 1)
     
     

    add new line

  14. usr/src/uts/common/fs/zfs/spa.c (Diff revision 1)
     
     

    add new line

  15. usr/src/uts/common/fs/zfs/spa.c (Diff revision 1)
     
     

    switch this and next line

  16. sigh... like really?:)

    1. Not sure what you mean, but I added a blank line.

  17. there is no reason why we should declare calculated_limit here (versus 5 lines above).

  18. add new line

  19. add new line.

  20. The same story... mix of code and variable declarations. Ugly as hell and really bad to read.

    1. As before, this is not something I am willing to change since it is a common convention throughout the ZFS code.

  21. add new line

  22. remove empty line

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

    and variable declarations in the middle of code..

  24. 
      
tsoome
  1. Ship It!
  2. 
      
jjelinek
seeemef@mac.com
  1. Ship It!
  2. 
      
kkantor
  1. Looks good to me if you think the set_tunable64 usage is okay.

  2. It is okay that this and the next line both set int types to 64-bit values (and lines 70 & 58)?

    1. Good catch! I've updated the test code.

  3. usr/src/uts/common/fs/zfs/metaslab.c (Diff revision 2)
     
     

    Interesting change and comment update!

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

    Do we use int here for size clarity instead of boolean_t? Relatedly, is there a reason these new globals are all int types and not more specific like uint32_t?

    1. It's common to have int for tunables since setting these via /etc/system or with mdb is normally done using an integer write command. We could make these uint_t, but it's probably simplest to stay aligned with ZoL here since these are being used by the code as booleans so the sign is not a factor.

  5. 
      
jjelinek
seeemef@mac.com
  1. Ship It!
  2. 
      
citrus
  1. Ship It!
  2. 
      
kkantor
  1. Looks good!

  2. It looks like this pair of set_tunable64s were missed.

    1. Yes, thanks for catching that and sorry about overlooking it.

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

Status: Closed (submitted)

Loading...