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

Remove list_size struct member from list implementation #15812

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

MigeljanImeri
Copy link
Contributor

Motivation and Context

In a previous pull request I opened (#15478), I added an additional counter to track the number of pending sync IO operations as it wasn't properly being tracked before. This adds extra space requirements to the vdev_queue struct and to counteract this increase it was suggested to remove the list_size struct member as it was only used in a few asserts in the code.

Description

I removed the list_size struct member in the list implementations in two separate commits, in order to keep the kernel list and the user-space list changes separate, in case this has to be reverted in the future.

How Has This Been Tested?

I ran zloop.sh and no apparent crashes were seen.

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)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Copy link
Contributor

@bwatkinson bwatkinson left a comment

Choose a reason for hiding this comment

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

Make sure to fix the checksytle error.

I do wonder if it is worth going into each call sight for list_create() and removing the passed second size parameter? That might be overkill though. I will leave it up to others to decide if that should also be done.

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

Thanks. Just one comment, but I do not insist.

lib/libspl/list.c Outdated Show resolved Hide resolved
Removed the list_size struct member as it was only used in a single
assertion, as mentioned in PR openzfs#15478.

Signed-off-by: MigeljanImeri <imerimigel@gmail.com>
Removed the list_size struct member as it was only used in a single
assertion, as mentioned in PR openzfs#15478.

Signed-off-by: MigeljanImeri <imerimigel@gmail.com>
@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Jan 26, 2024
@behlendorf behlendorf merged commit 78e8c1f into openzfs:master Jan 26, 2024
23 of 25 checks passed
@behlendorf
Copy link
Contributor

When merging I intentionally squashed these two commits. This is such a small change I don't think we need to leave the kernel and user portions split just in case we end up wanting to revert part of it.

lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
Removed the list_size struct member as it was only used in a single
assertion, as mentioned in PR openzfs#15478.

Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: MigeljanImeri <imerimigel@gmail.com>
Closes openzfs#15812
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
Removed the list_size struct member as it was only used in a single
assertion, as mentioned in PR openzfs#15478.

Reviewed-by: Brian Atkinson <batkinson@lanl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: MigeljanImeri <imerimigel@gmail.com>
Closes openzfs#15812
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