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 5.2 compat: Define wait_on_page_writeback() if GPL-only exported #8794

Merged
merged 1 commit into from
May 25, 2019

Conversation

kusumi
Copy link
Member

@kusumi kusumi commented May 22, 2019

Motivation and Context

Fixes #8775.

Description

wait_on_page_writeback() was made GPL only in torvalds/linux@19343b5bdd.
Define one if it's GPL only.

How Has This Been Tested?

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:

@codecov
Copy link

codecov bot commented May 22, 2019

Codecov Report

Merging #8794 into master will increase coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8794      +/-   ##
==========================================
+ Coverage   78.72%   78.74%   +0.01%     
==========================================
  Files         382      382              
  Lines      117809   117810       +1     
==========================================
+ Hits        92747    92771      +24     
+ Misses      25062    25039      -23
Flag Coverage Δ
#kernel 79.29% <0%> (-0.05%) ⬇️
#user 67.28% <ø> (-0.22%) ⬇️

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 e55db32...9f83bf6. Read the comment docs.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label May 23, 2019
@@ -75,4 +77,18 @@

#endif /* ZFS_GLOBAL_ZONE_PAGE_STATE */

/* Linux 5.2 made wait_on_page_writeback() GPL only */
#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 2, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

We're going to need to add an configure check for this, see the ZFS_AC_KERNEL_BIO_SET_DEV_GPL_ONLY check in config/kernel-bio_set_dev.m4 for an example.

Although, what might be simpler in this particular case is to update zfs_putpage to directly call wait_on_page_bit() since it's the only caller. Then we don't need any compatibility code.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're going to need to add an configure check for this, see the ZFS_AC_KERNEL_BIO_SET_DEV_GPL_ONLY check in config/kernel-bio_set_dev.m4 for an example.

Added.

Although, what might be simpler in this particular case is to update zfs_putpage to directly call wait_on_page_bit() since it's the only caller. Then we don't need any compatibility code.

It now only has wait_on_page_writeback() in module/zfs/zfs_vnops.c for GPL export case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, that's not quite what I was suggesting, but after re-reading my comment I see why it was confusing. We could absolutely go with your updated patch. However, since there's only this one caller I was thinking we could do away with the configure check entirely and do something much simpler, like this:

@@ -4526,8 +4526,10 @@ zfs_putpage(struct inode *ip, struct page *pp, struct wri
                unlock_page(pp);
                rangelock_exit(lr);
 
-               if (wbc->sync_mode != WB_SYNC_NONE)
-                       wait_on_page_writeback(pp);
+               if (wbc->sync_mode != WB_SYNC_NONE) {
+                       if (PageWriteback(pp))
+                               wait_on_page_bit(pp, PG_writeback);
+               }
 
                ZFS_EXIT(zfsvfs);
                return (0);

Copy link
Member Author

@kusumi kusumi May 24, 2019

Choose a reason for hiding this comment

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

Oh.
I could change to do like that, but it would start to be more complex if there are >1 consumers of it.
Well, maybe not, it's just two lines of code in the end...

Copy link
Contributor

Choose a reason for hiding this comment

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

This code actually hasn't changed in quite a while, and offhand I can't think of any pending work where we'd need to add another caller. So why don't we keep it simple for now and avoid the wrapper entirely. As you said, it's only two lines after all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@@ -1,6 +1,8 @@
#ifndef _ZFS_PAGE_COMPAT_H
#define _ZFS_PAGE_COMPAT_H

#include <linux/version.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be dropped when refreshed to include the configure check

Copy link
Member Author

Choose a reason for hiding this comment

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

Dropped accordingly.

@behlendorf behlendorf requested a review from tonyhutter May 23, 2019 21:21
@behlendorf behlendorf added the Type: Building Indicates an issue related to building binaries label May 23, 2019
@kusumi kusumi changed the title Linux 5.2 compat: Add zfs_wait_on_page_writeback() Linux 5.2 compat: Define wait_on_page_writeback() if GPL-only exported May 24, 2019
wait_on_page_writeback() was made GPL only in torvalds/linux@19343b5bdd.

Directly call wait_on_page_bit() without using wait_on_page_writeback()
interface, given zfs_putpage() is the only caller for now.

Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@osnexus.com>
@behlendorf behlendorf requested a review from loli10K May 25, 2019 00:16
@behlendorf behlendorf merged commit bfd5a70 into openzfs:master May 25, 2019
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 25, 2019
behlendorf pushed a commit that referenced this pull request Jun 7, 2019
wait_on_page_writeback() was made GPL only in torvalds/linux@19343b5bdd.

Directly call wait_on_page_bit() without using wait_on_page_writeback()
interface, given zfs_putpage() is the only caller for now.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: loli10K <ezomori.nozomu@gmail.com>
Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@osnexus.com>
Closes #8794
allanjude pushed a commit to allanjude/zfs that referenced this pull request Jun 7, 2019
wait_on_page_writeback() was made GPL only in torvalds/linux@19343b5bdd.

Directly call wait_on_page_bit() without using wait_on_page_writeback()
interface, given zfs_putpage() is the only caller for now.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: loli10K <ezomori.nozomu@gmail.com>
Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@osnexus.com>
Closes openzfs#8794
allanjude pushed a commit to allanjude/zfs that referenced this pull request Jun 15, 2019
wait_on_page_writeback() was made GPL only in torvalds/linux@19343b5bdd.

Directly call wait_on_page_bit() without using wait_on_page_writeback()
interface, given zfs_putpage() is the only caller for now.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: loli10K <ezomori.nozomu@gmail.com>
Signed-off-by: Tomohiro Kusumi <kusumi.tomohiro@osnexus.com>
Closes openzfs#8794
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) Type: Building Indicates an issue related to building binaries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wait_on_page_writeback() is now EXPORT_SYMBOL_GPL()'d
3 participants