-
Notifications
You must be signed in to change notification settings - Fork 181
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing this and providing a patch. I've added some review comments inline. When you're refreshing this can you please also do the following.
- Rebase on SPL master.
- Open an additional dummy PR against ZFS with the line
Requires-spl: refs/pull/659/head
in the commit message. This way the SPL commit will be run through our automated testing against a variety of kernels. PR Test SPL PR - style fixes zfs#6726 is a good example of this. - Add your signed-off-by to the commit
git commit -s
.
rc = vfs_read(fp, addr, len, &offset); | ||
#else | ||
rc = kernel_read(fp, addr, len, &offset); | ||
#endif /* HAVE_VFS_WRITE */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like kernel_write()
was exported in v3.8 torvalds/linux@7bb307e and kernel_read()
in 3.9 torvalds/linux@3dc20cb. This is arguably when we should have moved over to these new interfaces.
What we need to do to handle this is to add a configure check for both kernel_read
and kernel_write
and a compatibility wrapper for each of these called spl_kernel_read()
and spl_kernel_write()
for consistency. The wrappers should prefer the kernel_*
interfaces if they're determined to be available. And since these functions are only called from spl-vnode.c
they can be added there and declared static.
There are a couple things to be careful of when implementing the wrappers.
- The arguments for the functions do not appear in the same order.
vfs_read/write
functions don't handle theset_fs()
bit but thekernel_read/write
functions do. This logic will need to be pulled in to the wrapper for only thevfs_read/write
case and out ofvn_rdwr
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I won't have the time to work further on this issue. Concerning your comments: I would actually leave it the way it is. All previous ZFS versions compiled against kernels < 4.14 have been using vfs_read/write and were tested against this. Why risk catching a regression with such kernels due to a possibly buggy early implemenation of kernel_read/write? I don't see a benefit, only potential problems when using kernel_read/write with kernels < 4.14.
I signed off the commit rebased and pushed.
Commit torvalds/linux@bd8df82b - Unexport vfs_read and vfs_write Signed-off-by: Roland Fehrenbacher <rf@q-leap.de>
7b4af07
to
53be762
Compare
I have problem with this patch, cannot mount files on older kernels. Drives mount ok. |
Replaced by #667. |
Patch received basic run-time testing applied to the spl-0.7-release branch and built against 4.14-rc3.