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

Linux compat 4.16: blk_queue_flag_{set,clear} #7431

Merged
merged 1 commit into from
Apr 13, 2018

Conversation

behlendorf
Copy link
Contributor

Description

The HAVE_BLK_QUEUE_WRITE_CACHE_GPL_ONLY case was overlooked in
the original 10f88c5 commit because blk_queue_write_cache()
was available for the in-kernel builds.

Update the blk_queue_flag_{set,clear} wrappers to call the locked
versions to avoid confusion. This is safe for all existing callers.

The blk_queue_set_write_cache() function has been updated to use
these wrappers. This means setting/clearing both QUEUE_FLAG_WC
and QUEUE_FLAG_FUA is no longer atomic but this only done early
in zvol_alloc() prior to any requests so there is no issue.

Motivation and Context

Issue #7428

How Has This Been Tested?

Local build on Fedora Rawhide. Pending full ZTS from buildbot.

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.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

The HAVE_BLK_QUEUE_WRITE_CACHE_GPL_ONLY case was overlooked in
the original 10f88c5 commit because blk_queue_write_cache()
was available for the in-kernel builds.

Update the blk_queue_flag_{set,clear} wrappers to call the locked
versions to avoid confusion.  This is safe for all existing callers.

The blk_queue_set_write_cache() function has been updated to use
these wrappers.  This means setting/clearing both QUEUE_FLAG_WC
and QUEUE_FLAG_FUA is no longer atomic but this only done early
in zvol_alloc() prior to any requests so there is no issue.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#7428
@behlendorf behlendorf requested a review from dinatale2 April 12, 2018 20:24
@behlendorf behlendorf merged commit d6bb221 into openzfs:master Apr 13, 2018
@codecov
Copy link

codecov bot commented Apr 13, 2018

Codecov Report

Merging #7431 into master will decrease coverage by 0.07%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7431      +/-   ##
==========================================
- Coverage    76.2%   76.12%   -0.08%     
==========================================
  Files         330      330              
  Lines      104294   104292       -2     
==========================================
- Hits        79474    79392      -82     
- Misses      24820    24900      +80
Flag Coverage Δ
#kernel 76.05% <66.66%> (+0.11%) ⬆️
#user 65.33% <ø> (-0.07%) ⬇️

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 7fab636...65e0657. Read the comment docs.

@nivedita76
Copy link
Contributor

The change to use queue_flag_set instead of queue_flag_set_unlocked causes a warning with lockdep turned on, when zvol_create_minor_impl calls it.

:(

@tonyhutter
Copy link
Contributor

@nivedita76 can you post the warning, and tell us which kernel and distro you are using?

@nivedita76
Copy link
Contributor

[   19.505179] WARNING: CPU: 79 PID: 1244 at ./include/linux/blkdev.h:720 zvol_create_minor_impl+0x958/0xa00
[   19.506064] Modules linked in:
[   19.507018] CPU: 79 PID: 1244 Comm: z_zvol Not tainted 4.16.2-stable-rani-zfs+ #16
[   19.508046] Hardware name: ASUSTeK COMPUTER INC. Z10PE-D8 WS/Z10PE-D8 WS, BIOS 3407 03/10/2017
[   19.508937] RIP: 0010:zvol_create_minor_impl+0x958/0xa00
[   19.509834] RSP: 0000:ffffa4cf0c62fc60 EFLAGS: 00010246
[   19.510751] RAX: 0000000000000000 RBX: ffff8e2c74ccd080 RCX: 0000000000000001
[   19.511585] RDX: ffff8e2c605da1f0 RSI: 00000000ffffffff RDI: ffff8e2c605da4a0
[   19.512424] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
[   19.513250] R10: ffffa4cf0c62fbd0 R11: 0000000000000000 R12: ffff8e2c61956000
[   19.514063] R13: 0000000000000000 R14: ffff8e2c61586500 R15: 3d9c1d0af06cba80
[   19.514870] FS:  0000000000000000(0000) GS:ffff8e2c7e400000(0000) knlGS:0000000000000000
[   19.515678] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   19.516471] CR2: 0000000000000000 CR3: 000000189e012001 CR4: 00000000003606e0
[   19.517256] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   19.518023] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   19.518770] Call Trace:
[   19.519497]  zvol_task_cb+0x2bc/0x680
[   19.520254]  ? taskq_thread+0x2e5/0x600
[   19.520987]  taskq_thread+0x2fa/0x600
[   19.521687]  ? wake_up_q+0x80/0x80
[   19.522387]  ? taskq_thread_should_stop+0x70/0x70
[   19.523083]  kthread+0x121/0x140
[   19.523770]  ? kthread_create_worker_on_cpu+0x70/0x70
[   19.524467]  ret_from_fork+0x35/0x40
[   19.525157] Code: a0 fd ff ff 0f 0b e9 99 fd ff ff 48 8d 78 18 be ff ff ff ff 48 89 54 24 08 e8 65 cb c8 ff 48 8b 54 24 08 85 c0 0f 85 69 fa ff ff <0f> 0b e9 62 fa ff ff 3d 00 00 00 01 bb 00 00 00 01 be 01 00 00 

4.16.2, gentoo

@behlendorf
Copy link
Contributor Author

@nivedita76 is any additional lock dep information logged? The warning posted appears to be a false positive.

tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Apr 16, 2018
The HAVE_BLK_QUEUE_WRITE_CACHE_GPL_ONLY case was overlooked in
the original 10f88c5 commit because blk_queue_write_cache()
was available for the in-kernel builds.

Update the blk_queue_flag_{set,clear} wrappers to call the locked
versions to avoid confusion.  This is safe for all existing callers.

The blk_queue_set_write_cache() function has been updated to use
these wrappers.  This means setting/clearing both QUEUE_FLAG_WC
and QUEUE_FLAG_FUA is no longer atomic but this only done early
in zvol_alloc() prior to any requests so there is no issue.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Reviewed-by: Kash Pande <kash@tripleback.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#7428 
Closes openzfs#7431
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Apr 16, 2018
The HAVE_BLK_QUEUE_WRITE_CACHE_GPL_ONLY case was overlooked in
the original 10f88c5 commit because blk_queue_write_cache()
was available for the in-kernel builds.

Update the blk_queue_flag_{set,clear} wrappers to call the locked
versions to avoid confusion.  This is safe for all existing callers.

The blk_queue_set_write_cache() function has been updated to use
these wrappers.  This means setting/clearing both QUEUE_FLAG_WC
and QUEUE_FLAG_FUA is no longer atomic but this only done early
in zvol_alloc() prior to any requests so there is no issue.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Reviewed-by: Kash Pande <kash@tripleback.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#7428
Closes openzfs#7431

Requires-spl: openzfs#701
@nivedita76
Copy link
Contributor

No that’s it. It’s not a false positive as such no? The queue is not locked and that non-unlocked function asserts that the queue is locked when it’s called.

tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request May 3, 2018
The HAVE_BLK_QUEUE_WRITE_CACHE_GPL_ONLY case was overlooked in
the original 10f88c5 commit because blk_queue_write_cache()
was available for the in-kernel builds.

Update the blk_queue_flag_{set,clear} wrappers to call the locked
versions to avoid confusion.  This is safe for all existing callers.

The blk_queue_set_write_cache() function has been updated to use
these wrappers.  This means setting/clearing both QUEUE_FLAG_WC
and QUEUE_FLAG_FUA is no longer atomic but this only done early
in zvol_alloc() prior to any requests so there is no issue.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Reviewed-by: Kash Pande <kash@tripleback.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#7428
Closes openzfs#7431
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request May 4, 2018
The HAVE_BLK_QUEUE_WRITE_CACHE_GPL_ONLY case was overlooked in
the original 10f88c5 commit because blk_queue_write_cache()
was available for the in-kernel builds.

Update the blk_queue_flag_{set,clear} wrappers to call the locked
versions to avoid confusion.  This is safe for all existing callers.

The blk_queue_set_write_cache() function has been updated to use
these wrappers.  This means setting/clearing both QUEUE_FLAG_WC
and QUEUE_FLAG_FUA is no longer atomic but this only done early
in zvol_alloc() prior to any requests so there is no issue.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Reviewed-by: Kash Pande <kash@tripleback.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#7428
Closes openzfs#7431
tonyhutter pushed a commit that referenced this pull request May 10, 2018
The HAVE_BLK_QUEUE_WRITE_CACHE_GPL_ONLY case was overlooked in
the original 10f88c5 commit because blk_queue_write_cache()
was available for the in-kernel builds.

Update the blk_queue_flag_{set,clear} wrappers to call the locked
versions to avoid confusion.  This is safe for all existing callers.

The blk_queue_set_write_cache() function has been updated to use
these wrappers.  This means setting/clearing both QUEUE_FLAG_WC
and QUEUE_FLAG_FUA is no longer atomic but this only done early
in zvol_alloc() prior to any requests so there is no issue.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Reviewed-by: Kash Pande <kash@tripleback.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes #7428
Closes #7431
teknoman117 pushed a commit to teknoman117/zfs that referenced this pull request Jun 7, 2018
The HAVE_BLK_QUEUE_WRITE_CACHE_GPL_ONLY case was overlooked in
the original 10f88c5 commit because blk_queue_write_cache()
was available for the in-kernel builds.

Update the blk_queue_flag_{set,clear} wrappers to call the locked
versions to avoid confusion.  This is safe for all existing callers.

The blk_queue_set_write_cache() function has been updated to use
these wrappers.  This means setting/clearing both QUEUE_FLAG_WC
and QUEUE_FLAG_FUA is no longer atomic but this only done early
in zvol_alloc() prior to any requests so there is no issue.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Reviewed-by: Kash Pande <kash@tripleback.net>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#7428 
Closes openzfs#7431
@behlendorf behlendorf deleted the issue-7428 branch April 19, 2021 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants