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.

Sign up for GitHub

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

Jump to bottom

zvol kqfilter #13773

Closed
wants to merge 2 commits into from
Closed

zvol kqfilter #13773

wants to merge 2 commits into from

Conversation

rob-wing
Copy link
Contributor

Motivation and Context

Allow a user to be notified when a zvol with volmode=dev has been resized.

Description

Add kqfilter support to the zvol cdev. This change only applies to zvol with volmode=dev.

How Has This Been Tested?

  1. Create a zvol with volmode=dev
  2. Monitor the NOTE_ATTRIB event on the zvol file descriptor with a kevent program.
  3. Resize the zvol.
  4. Verified that the NOTE_ATTRIB event is fired.

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:

@rob-wing rob-wing changed the title Zvol kq zvol kqfilter Aug 14, 2022
@amotin amotin requested a review from a user August 15, 2022 16:53
@amotin
Copy link
Member

amotin commented Aug 15, 2022

Some previous reviews went here: https://reviews.freebsd.org/D33401 .

ghost
ghost approved these changes Aug 15, 2022
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

amotin
amotin reviewed Aug 15, 2022
View reviewed changes
module/os/freebsd/zfs/zvol_os.c Outdated
@@ -1305,6 +1361,8 @@ zvol_os_free(zvol_state_t *zv)

if (dev != NULL) {
ASSERT3P(dev->si_drv2, ==, NULL);
knlist_clear(&zsd->zsd_selinfo.si_note, 0);
knlist_destroy(&zsd->zsd_selinfo.si_note);
destroy_dev(dev);
Copy link
Member

Choose a reason for hiding this comment

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

I suspect that knlist_clear()/knlist_destroy() should go after destroy_dev(). At least that is what I see in few other places in kernel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I looked through the code a bit to understand why that is. One of the other patterns I noticed were variations of:

destroy_dev()
seldrain()
knlist_clear()
knlist_destroy()

I didn't come up with a confident conclusion - my best guess is that devices with d_poll() handlers should be destroyed before draining selinfo. Since the knote list is attached to selinfo, it would make sense to clear/destroy the knlist after the device is destroyed.

I've updated this chunk of the pull request to be consistent with rest of the kernel.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) Status: Revision Needed Changes are required for the PR to be accepted and removed Status: Accepted Ready to integrate (reviewed, tested) labels Aug 23, 2022
@behlendorf
Copy link
Contributor

Before we can merge this there are a few style warnings which need to be resolved, https://github.com/openzfs/zfs/runs/7963871000?check_suite_focus=true

@rob-wing
FreeBSD: add knlist_init_sx() for exclusive locks
7ffd8ff 
This will be used to implement kqfilter support for zvol cdevs.

Signed-off-by: Rob Wing <rew@FreeBSD.org>
@rob-wing
FreeBSD: add kqfilter support for zvol cdev
cb6044a 
The only event hooked up is NOTE_ATTRIB, which is triggered when the
device is resized.

Signed-off-by: Rob Wing <rew@FreeBSD.org>
@rob-wing
Copy link
Contributor Author

The style issues reported by make checkstyle have been addressed with the previous update.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Revision Needed Changes are required for the PR to be accepted labels Sep 2, 2022
@behlendorf behlendorf closed this in 9d08874 Sep 6, 2022
behlendorf pushed a commit that referenced this pull request Sep 6, 2022
@rob-wing @behlendorf
FreeBSD: add kqfilter support for zvol cdev
983096a 
The only event hooked up is NOTE_ATTRIB, which is triggered when the
device is resized.

Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Rob Wing <rew@FreeBSD.org>
Closes #13773
beren12 pushed a commit to beren12/zfs that referenced this pull request Sep 19, 2022
@rob-wing @beren12
FreeBSD: add knlist_init_sx() for exclusive locks
cdf3ef8 
This will be used to implement kqfilter support for zvol cdevs.

Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Rob Wing <rew@FreeBSD.org>
Closes openzfs#13773
beren12 pushed a commit to beren12/zfs that referenced this pull request Sep 19, 2022
@rob-wing @beren12
FreeBSD: add kqfilter support for zvol cdev
ee740ad 
The only event hooked up is NOTE_ATTRIB, which is triggered when the
device is resized.

Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Rob Wing <rew@FreeBSD.org>
Closes openzfs#13773
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amotin amotin amotin approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rob-wing @amotin @behlendorf