Skip to content

Code cleanup after sharing zfs_write between platforms #11176

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

Closed
wants to merge 6 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 6, 2020

Motivation and Context

Housekeeping

Description

  • Factor uid, gid, and projid out of loop in zfs_write
  • Remove redundant oid parameter to update_pages
    The oid comes from the znode we are already passing.
  • Prune unused alias for dmu_assign_arcbuf_by_dbuf
  • Move uio_prefaultpages definition to uio.h
  • Const some unchanging variables in zfs_write
    Show that these values will not be changing later.

How Has This Been Tested?

ZTS

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:

Ryan Moeller added 5 commits November 6, 2020 17:49
The oid comes from the znode we are already passing.

Signed-off-by: Ryan Moeller <[email protected]>
Show that these values will not be changing later.

Signed-off-by: Ryan Moeller <[email protected]>
@ghost ghost added the Status: Code Review Needed Ready for review and testing label Nov 6, 2020
@ghost ghost requested review from behlendorf and amotin November 9, 2020 18:43
const uint64_t uid = KUID_TO_SUID(ZTOUID(zp));
const uint64_t gid = KGID_TO_SGID(ZTOGID(zp));
const uint64_t projid = zp->z_projid;

Copy link
Member

Choose a reason for hiding this comment

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

This function is so big, that I doubt there will be many free registers to store those values across the loop iterations.

Generally I think this per-block loop in ZFS is over-aggressive, causing more than necessary separate log records, which is a downside now that zil code is able to properly split the large writes between multiple blocks. So I would prefer to see here less cycle iterations rather than optimizations to make it behave.

@ghost ghost force-pushed the zfs_write branch from 34e7989 to 66b5f5d Compare November 9, 2020 23:12
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Nov 10, 2020
behlendorf pushed a commit that referenced this pull request Nov 10, 2020
The oid comes from the znode we are already passing.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes #11176
behlendorf pushed a commit that referenced this pull request Nov 10, 2020
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes #11176
behlendorf pushed a commit that referenced this pull request Nov 10, 2020
Show that these values will not be changing later.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes #11176
behlendorf pushed a commit that referenced this pull request Nov 10, 2020
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes #11176
@behlendorf
Copy link
Contributor

Merged, with the exception of the "Prune unused alias for dmu_assign_arcbuf_by_dbuf" commit. We actually have one external consumer, Lustre, which uses this alias. For now, I'd like to keep it to preserve compatibility.

@ghost ghost deleted the zfs_write branch November 10, 2020 19:16
behlendorf pushed a commit that referenced this pull request Dec 23, 2020
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes #11176
behlendorf pushed a commit that referenced this pull request Dec 23, 2020
The oid comes from the znode we are already passing.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes #11176
behlendorf pushed a commit that referenced this pull request Dec 23, 2020
Show that these values will not be changing later.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes #11176
behlendorf pushed a commit that referenced this pull request Dec 23, 2020
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes #11176
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11176
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
The oid comes from the znode we are already passing.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11176
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11176
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
Show that these values will not be changing later.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11176
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11176
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11176
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
The oid comes from the znode we are already passing.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11176
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11176
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Show that these values will not be changing later.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11176
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11176
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 6, 2021
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11176
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 13, 2021
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11176
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 13, 2021
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Matt Macy <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ryan Moeller <[email protected]>
Closes openzfs#11176
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.

3 participants