-
Notifications
You must be signed in to change notification settings - Fork 24
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
Fix intermittent EINVAL in ZFSPool.sync #194
base: master
Are you sure you want to change the base?
Conversation
Sometimes ZFSPool.sync doesn't pass the pool_name argument to lzc_sync correctly. lzc_sync sees an empty string. Replacing the self.name property access with the self.handle instance variable access seems to fix the problem, though I don't understand why. Fixes truenas#193
cc @alek-p |
@@ -2892,7 +2892,7 @@ cdef class ZFSPool(object): | |||
IF HAVE_LZC_SYNC: | |||
def sync(self, force=False): | |||
cdef int ret | |||
cdef const char *c_name = self.name | |||
cdef const char *c_name = libzfs.zpool_get_name(self.handle) |
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.
I don't follow, it's doing the same thing in name
property ?
Do you have a reproduction case which I could perhaps try locally ?
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.
Yes, it's doing the exact same thing as in the name
property. But no, I don't have a very portable reproduction case.
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.
Here's a short script that frequently fails. Though if I change the pool name to the much smaller and less busy zroot
, then it passes.
import libzfs
zfs = libzfs.ZFS()
ds = zfs.get_dataset("atl-rb22b-ss.b.24")
pool = ds.pool
pool.sync()
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.
@asomers can you please create a ticket at https://ixsystems.atlassian.net and link this script there and we can investigate as to why this could be failing potentially. Do mention your scale version please as well. Thanks!
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.
What is a "scale version"?
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.
BTW I'm not a TrueNAS user. I'm using a custom Python application on FreeBSD stable/13.
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.
@asomers interesting. I am not able to reproduce using the same script ( i am executing the same code path in a loop ). I wonder if it's a cython/python version issue ?
Could you share some more details wrt cython/python version you have please ? Thanks!
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.
Python 3.9.13
Cython version 0.29.30
Sometimes ZFSPool.sync doesn't pass the pool_name argument to lzc_sync
correctly. lzc_sync sees an empty string. Replacing the self.name
property access with the self.handle instance variable access seems to
fix the problem, though I don't understand why.
Fixes #193