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

Virtuozzo kernel compat: handle kernel DIRECT_IO via ITERs while VFS_RW_ITERATE are unsupported #11410

Closed
finist0 opened this issue Dec 29, 2020 · 7 comments · Fixed by #11411
Labels
Status: Triage Needed New issue which needs to be triaged Type: Defect Incorrect behavior (e.g. crash, hang)

Comments

@finist0
Copy link
Contributor

finist0 commented Dec 29, 2020

System information

Type Version/Name
Distribution Name Virtuozzo
Distribution Version 7.5 (Update 15)
Linux Kernel 3.10.0-1127.18.2.vz7.163.46
Architecture x86_64
ZFS Version zfs-0.8.5 and zfs-2.0.0
SPL Version spl-0.7.13

Describe the problem you're observing

We, Virtuozzo, got a report that zfs module fails to compile for Virtuozzo kernel 3.10.0-1127.18.2.vz7.163.46
http://repo.virtuozzo.com/vz/releases/7.5.0-586/x86_64/os/Packages/v/vzkernel-3.10.0-1127.18.2.vz7.163.46.x86_64.rpm

while it compiled fine for previous Virtuozzo kernel 3.10.0-1127.8.2.vz7.151.14
http://repo.virtuozzo.com/vz/releases/7.5.0-586/x86_64/os/Packages/v/vzkernel-3.10.0-1127.8.2.vz7.151.14.x86_64.rpm

https://bugs.openvz.org/browse/OVZ-7243

Describe how to reproduce the problem

Get an installation of Virtuozzo 7.5 (optionally just install vzkernel and vzkernel-devel packages for version 3.10.0-1127.18.2.vz7.163.46 from http://repo.virtuozzo.com/vz/releases/7.5.0-586/x86_64/os/Packages/v/ in a, say, CentOS 7 environment) and try to compile zfs module.
You will get the following compilation error:

zfs-kmod-2.0.0/_kmod_build_3.10.0-1127.18.2.vz7.163.46/../zfs-2.0.0/module/zfs/../os/linux/zfs/zpl_file.c:502:2:
error: #error "Unknown direct IO interface"
zfs-kmod-2.0.0/_kmod_build_3.10.0-1127.18.2.vz7.163.46/../zfs-2.0.0/module/zfs/../os/linux/zfs/zpl_file.c:1026:15:
error: 'zpl_direct_IO' undeclared here (not in a function)
  .direct_IO = zpl_direct_IO,

Whats is the reason?

Virtuozzo kernel does not have vfs rw iterators (struct file_operations :: .read_iter()/.write_iter()) compatible with mainstream version, so zfs correctly does not detect HAVE_VFS_RW_ITERATE define.

Previously (for example in vzkernel vz7.151.14) struct address_space_operations :: .direct_IO() worked via IOVECs and zfs correctly detected HAVE_VFS_DIRECT_IO_IOVEC and compilation worked fine, .direct_IO() definition via iovec was used.

zpl_file.c:

#if defined(HAVE_VFS_RW_ITERATE)

#if defined(HAVE_VFS_DIRECT_IO_ITER)
lalala
#elif defined(HAVE_VFS_DIRECT_IO_ITER_OFFSET)
lalala
#elif defined(HAVE_VFS_DIRECT_IO_ITER_RW_OFFSET)
lalala            // we should be here in vz7.163.x
#else
#error "Unknown direct IO interface"
#endif

#else

#if defined(HAVE_VFS_DIRECT_IO_IOVEC)
lalala            // we are here in vz7.151.14
#else
#error "Unknown direct IO interface"
#endif
#endif /* HAVE_VFS_RW_ITERATE */

But recently we have changed struct address_space_operations :: .direct_IO() to use iterators and now we came to a situation that zfs configurator

  • does not detect HAVE_VFS_RW_ITERATE (correctly)
  • does detect HAVE_VFS_DIRECT_IO_ITER_RW_OFFSET (correctly)
  • now .direct_IO() definition is found.

i suggest to add a .direct_IO() definition for the case HAVE_VFS_DIRECT_IO_ITER_RW_OFFSET + no HAVE_VFS_RW_ITERATE.

Will send the pull request soon.

Note: even with this patch in zfs, it won't compile for kernel 3.10.0-1127.18.2.vz7.163.46, another kernel patch is to be applied first (we will apply it soon): https://lists.openvz.org/pipermail/devel/2020-December/076222.html
If you want to test the compilation right now, until we make an official kernel with this patch, you can take a test kernel at
http://fe.virtuozzo.com/ceacebe1809ed2631696da3ea06c3853/

@finist0 finist0 added Status: Triage Needed New issue which needs to be triaged Type: Defect Incorrect behavior (e.g. crash, hang) labels Dec 29, 2020
finist0 added a commit to finist0/zfs that referenced this issue Dec 29, 2020
…+ no HAVE_VFS_RW_ITERATE case

Virtuozzo 7 kernels starting 3.10.0-1127.18.2.vz7.163.46
have the following configuration:

  * no HAVE_VFS_RW_ITERATE
  * HAVE_VFS_DIRECT_IO_ITER_RW_OFFSET

=> let's add implementation of zpl_direct_IO() via zpl_aio_{read,write}()
in this case.

https://bugs.openvz.org/browse/OVZ-7243

Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
Closes openzfs#11410
finist0 added a commit to finist0/zfs that referenced this issue Dec 29, 2020
…+ no HAVE_VFS_RW_ITERATE case

Virtuozzo 7 kernels starting 3.10.0-1127.18.2.vz7.163.46
have the following configuration:

  * no HAVE_VFS_RW_ITERATE
  * HAVE_VFS_DIRECT_IO_ITER_RW_OFFSET

=> let's add implementation of zpl_direct_IO() via
zpl_aio_{read,write}() in this case.

https://bugs.openvz.org/browse/OVZ-7243

Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
Closes openzfs#11410
Issue openzfs#11410
@sempervictus
Copy link
Contributor

Nice to see we're not the "strangest kernel" consumer (grsec, heavily patched) using ZFS. Are you guys "really" on 3.10 or is it like RHEL's 2.6 with bits and pieces backported all the way from the 5.x series?

@finist0
Copy link
Contributor Author

finist0 commented Dec 29, 2020

Are you guys "really" on 3.10 or is it like RHEL's 2.6 with bits and pieces backported all the way from the 5.x series?

We base on RHEL7 kernel (so already inherit a LOT of backports from mainstream, you know) + heavily patch the kernel ourselves + do perform more backports ourselves as well. :)

@sempervictus
Copy link
Contributor

Hat's off to you guys for that herculean effort, either you're racking up points to get into digital heaven or doing penance for several prior lifetimes of writing public code in VB/C#/malbolge.
I looked over the PR, looks rational, and surprisingly simple. Any idea how that path stacks up performance-wise against the higher-level interfaces available to other kernels? May be a performance boost for them too.

@finist0
Copy link
Contributor Author

finist0 commented Dec 29, 2020

... either you're racking up points to get into digital heaven or doing penance for several prior lifetimes of writing public code in VB/C#/malbolge.

Ha-ha, we are just making donations to have a chance of getting more life counts in the future. :)

Any idea how that path stacks up performance-wise against the higher-level interfaces available to other kernels? May be a performance boost for them too.

Well, we previously had it working via iovecs, so no any performance drop is expected.
i've spent couple of days trying to rewrite our ITERs to be compatible with mainstream version, and the kernel worked fine, but zfs reported errors on a simple file read. Most probably i did not mimic ms iters good enough.
And i think reworking the code deeper is dangerous - heaven knows how long we can spend later for stabilizing the code all over the kernel, so choosing between possible performance gains and stability i chose the latter.

i think it's more efficient to spend more resources for new VZ version development (which will have kernel with ITERs out of the box).

@finist0
Copy link
Contributor Author

finist0 commented Dec 29, 2020

error: commit subject over 72 characters
make: *** [commitcheck] Error 1

Should i fix it? i thought the rule about string length does not apply to the subject line.

@sempervictus
Copy link
Contributor

Agreed - fast and unstable isn't good for anyone.
We maintain our own fork of Arch (i guess you could call it that) here at Semper Victus, used as a hardened core hardware OS atop which we run actual workloads in containers/VMs/etc as well as desktop/laptop systems for engineers. Since we're a grsec customer, and not all of our clients/partners are, we have to maintain two trees - a grsec tree, and a linux hardened tree (upstreaming a bunch of that to VyOS currently). Right now we're evaluating 5.9 so we have 4 trees (suffice to say 5.9 ain't primetime-ready yet for our absurd out-of-tree additions). Going off of official releases instead of a distro (we used to maintain an ubuntu flavor of our own) actually makes an in-house kernel easier to maintain over time and the backports get a LOT easier (to be fair our own backports are handled by the grsec team since they do this 10000 better than us, we only have to deal with the hardened branch for those). If you guys are looking for a new base kernel, the 5.4 series is actually surprisingly stable and will be maintained for years to come. Anything newer has a lot of churn (mostly for the better IMO, but its API-breaking as hell for code not as well thought out as ZFS), so probably makes sense to hold off till 5.12+ until they calm down a bit upstream. Hopefully the tailspin IBM just put on RHEL and derivatives ends up dethroning that unholy mess as the defacto linux distro, and elevates the simpler Arch/Alpine type designs to the forefront for actual resource management and dispatch at the low-level of OS constructs.

Re commit style - yeah, should be changed, all lines need to fit into the limit IIRC. Actually forced me to make all my commits in all projects correctly, good habit to get into.

finist0 added a commit to finist0/zfs that referenced this issue Dec 29, 2020
Virtuozzo 7 kernels starting 3.10.0-1127.18.2.vz7.163.46
have the following configuration:

  * no HAVE_VFS_RW_ITERATE
  * HAVE_VFS_DIRECT_IO_ITER_RW_OFFSET

=> let's add implementation of zpl_direct_IO() via
zpl_aio_{read,write}() in this case.

https://bugs.openvz.org/browse/OVZ-7243

Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
Closes openzfs#11410
Issue openzfs#11410
@finist0
Copy link
Contributor Author

finist0 commented Dec 29, 2020

If you guys are looking for a new base kernel, the 5.4 series is actually surprisingly stable and will be maintained for years to come.

Thank you for the hint, but i doubt we ever chose to base on a plain mainstream kernel - sitting upon some distro kernel has huge benefits, most important of them:

  • the hardware support is tested (and maintained!) by distro vendor
  • security fixes, inherited from base kernel is also a plus (despite the fact most of important for our usecases security issues we fix faster than RHEL nowadays - nevertheless we do not have to handle all of them always ourselves - that's really saves efforts)

behlendorf pushed a commit that referenced this issue Dec 30, 2020
Virtuozzo 7 kernels starting 3.10.0-1127.18.2.vz7.163.46
have the following configuration:

  * no HAVE_VFS_RW_ITERATE
  * HAVE_VFS_DIRECT_IO_ITER_RW_OFFSET

=> let's add implementation of zpl_direct_IO() via
zpl_aio_{read,write}() in this case.

https://bugs.openvz.org/browse/OVZ-7243

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
Closes #11410 
Closes #11411
behlendorf pushed a commit that referenced this issue Jan 5, 2021
Virtuozzo 7 kernels starting 3.10.0-1127.18.2.vz7.163.46
have the following configuration:

  * no HAVE_VFS_RW_ITERATE
  * HAVE_VFS_DIRECT_IO_ITER_RW_OFFSET

=> let's add implementation of zpl_direct_IO() via
zpl_aio_{read,write}() in this case.

https://bugs.openvz.org/browse/OVZ-7243

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
Closes #11410 
Closes #11411
jsai20 pushed a commit to jsai20/zfs that referenced this issue Mar 30, 2021
Virtuozzo 7 kernels starting 3.10.0-1127.18.2.vz7.163.46
have the following configuration:

  * no HAVE_VFS_RW_ITERATE
  * HAVE_VFS_DIRECT_IO_ITER_RW_OFFSET

=> let's add implementation of zpl_direct_IO() via
zpl_aio_{read,write}() in this case.

https://bugs.openvz.org/browse/OVZ-7243

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
Closes openzfs#11410 
Closes openzfs#11411
sempervictus pushed a commit to sempervictus/zfs that referenced this issue May 31, 2021
Virtuozzo 7 kernels starting 3.10.0-1127.18.2.vz7.163.46
have the following configuration:

  * no HAVE_VFS_RW_ITERATE
  * HAVE_VFS_DIRECT_IO_ITER_RW_OFFSET

=> let's add implementation of zpl_direct_IO() via
zpl_aio_{read,write}() in this case.

https://bugs.openvz.org/browse/OVZ-7243

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
Closes openzfs#11410 
Closes openzfs#11411
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Triage Needed New issue which needs to be triaged Type: Defect Incorrect behavior (e.g. crash, hang)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants