Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support setting user properties in a channel program #9950

Merged
merged 1 commit into from
Feb 14, 2020

Conversation

jasonbking
Copy link
Contributor

@jasonbking jasonbking commented Feb 5, 2020

This adds support for setting user properties in a
zfs channel program by adding 'zfs.sync.set_prop'
and 'zfs.check.set_prop' to the ZFS LUA API.

Motivation and Context

Work in SmartOS utilities this features, and it seems generally useful enough that the broader ZFS community might also find it useful. This change is limited to only zfs user properties. Setting user properties is all our work required, and that seems a useful delineation point--handling non-user properties in a channel program is a far more complex proposition (though nothing in this work should preclude extending it to support non-user properties in the future).

Description

This just adds the two new channel programs zfs.sync.set_prop and zfs.check.set_prop. They validate the property is a user property (and return EINVAL if the property is not a user property), and then set the property.

How Has This Been Tested?

This has been in use in a SmartOS development branch for several months (aside from paths, the code is identical between OpenZFS and illumos ZFS) without issue. In addition, a new test for the zfs test suite has been added which sets and verifies a user property in a channel program.

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)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux 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.

Contributions-by: Jason King jason.king@joyent.com
Signed-off-by: Sara Hartse sara.hartse@delphix.com
Signed-off-by: Jason King jason.king@joyent.com

@jasonbking jasonbking requested a review from behlendorf February 5, 2020 21:13
@codecov
Copy link

codecov bot commented Feb 6, 2020

Codecov Report

Merging #9950 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9950      +/-   ##
==========================================
+ Coverage   79.18%   79.21%   +0.02%     
==========================================
  Files         385      386       +1     
  Lines      122382   122424      +42     
==========================================
+ Hits        96904    96973      +69     
+ Misses      25478    25451      -27     
Flag Coverage Δ
#kernel 79.37% <100.00%> (-0.02%) ⬇️
#user 66.54% <0.00%> (-0.16%) ⬇️
Impacted Files Coverage Δ
module/os/linux/spl/spl-kmem-cache.c 75.45% <0.00%> (-9.46%) ⬇️
module/zfs/bpobj.c 86.86% <0.00%> (-4.29%) ⬇️
module/lua/lmem.c 83.33% <0.00%> (-4.17%) ⬇️
lib/libzpool/kernel.c 64.09% <0.00%> (-2.96%) ⬇️
module/zfs/dsl_synctask.c 92.40% <0.00%> (-2.54%) ⬇️
lib/libzfs/libzfs_changelist.c 84.37% <0.00%> (-1.96%) ⬇️
module/os/linux/zfs/vdev_file.c 80.37% <0.00%> (-1.87%) ⬇️
module/zfs/vdev_queue.c 94.49% <0.00%> (-1.84%) ⬇️
module/zfs/vdev_indirect_births.c 89.53% <0.00%> (-1.17%) ⬇️
cmd/ztest/ztest.c 82.70% <0.00%> (-0.81%) ⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 610eec4...4d24afc. Read the comment docs.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Feb 6, 2020
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This looks good just a couple tiny nits.

@behlendorf behlendorf requested a review from shartse February 7, 2020 17:10
Comment on lines 37 to 38
static int
zcp_set_user_prop(lua_State *state, dsl_pool_t *dp, const char *dsname,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is essentially a helper for the _sync() func, I'm not sure it makes sense for this to return a value, since it "can't fail" (except via longjmp)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it to void

This adds support for setting user properties in a
zfs channel program by adding 'zfs.sync.set_prop'
and 'zfs.check.set_prop' to the ZFS LUA API.

Contributions-by: Jason King <jason.king@joyent.com>
Signed-off-by: Sara Hartse <sara.hartse@delphix.com>
Signed-off-by: Jason King <jason.king@joyent.com>
@jasonbking
Copy link
Contributor Author

Should be all squashed and rebased to current now... I think I've addressed all the comments (at least so far)..

@behlendorf behlendorf requested a review from shartse February 13, 2020 19:24
Copy link
Contributor

@shartse shartse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Feb 14, 2020
@behlendorf behlendorf merged commit 13b5a4d into openzfs:master Feb 14, 2020
@jasonbking jasonbking deleted the zcp_set_prop branch February 14, 2020 22:28
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
This adds support for setting user properties in a
zfs channel program by adding 'zfs.sync.set_prop'
and 'zfs.check.set_prop' to the ZFS LUA API.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Matt Ahrens <matt@delphix.com>
Co-authored-by: Sara Hartse <sara.hartse@delphix.com>
Contributions-by: Jason King <jason.king@joyent.com>
Signed-off-by: Sara Hartse <sara.hartse@delphix.com>
Signed-off-by: Jason King <jason.king@joyent.com>
Closes openzfs#9950
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants