[developer] Re: [openzfs/openzfs] 7149 move libzpool's taskq library into libcmdutils (#141)
I think it makes sense to decouple this from #451 and change the name to something like "libzfs should use taskq in libfakekernel". -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/141#issuecomment-338808451 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T012acd019d09e76d-Mbf1bc0780ae3d51e264eb411 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7149 move libzpool's taskq library into libcmdutils (#141)
Having libzpool use libfakekernel seems like the right answer to me. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/141#issuecomment-338625995 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T012acd019d09e76d-M3cc968cae5e611ee8f6a51cb Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7149 move libzpool's taskq library into libcmdutils (#141)
Thanks @gwr, that all makes sense to me. Without making any promises, I'll try and pick this up again in a couple weeks, after the developer summit. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/141#issuecomment-338334153 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T012acd019d09e76d-Mc346016eea6fd00f06ee1929 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7149 move libzpool's taskq library into libcmdutils (#141)
Thinking about this some more, it _might_ be worthwhile considering the idea of having libzpool just use libfakekernel, but I'm not sure we made that sufficiently generic either. If not, perhaps extract what can be shared (taskq) and promote it to a library of it's own, I guess. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/141#issuecomment-338326217 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T012acd019d09e76d-M02759efa88be29165e25ea83 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7149 move libzpool's taskq library into libcmdutils (#141)
The libzpool taskq was there first, but was not sufficiently generic for "fksmbd". (That was our "fake/debug" SMB server in a user process.) We tried to make the taskq code in libfakekernel a little more generic, so it _might_ make sense to work from that, but that's a decision for whoever works on this. As for interfaces, it would be helpful if this could be usable in both consumers (libzpool and libfksmbsrv) with relatively modest changes, i.e. perhaps a special header file with #define(s) sufficient to let us build the SMB server code without (much) change. I know that's a lot to ask, which is part of why this hasn't happened already. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/141#issuecomment-338325324 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T012acd019d09e76d-Mcae9dfefb66968c70f94da38 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7149 move libzpool's taskq library into libcmdutils (#141)
Thanks for your time and replies on this, @gwr :) > Maybe a separate PR for the new lib might be easier... Like this PR? IIRC, this only includes changes related to exposing the taskq code for userspace, so maybe @andy-js is right, and I should just reopen this PR so we can hash out the details of this new library isolated from the ZFS changes. If I do that, am I understanding correctly, that the next steps would be to: 1. Use a new library for this new taskq code, rather than putting it into `libcmdutils`? Naming is hard, so do folks have a recommendation? `libutaskq`? 2. Remove the `system_taskq` interfaces 3. Re-evaluate the "exit on out-of-memory" behavior. From Rob's comments, it sounds like (and I tend to agree) we really should be returning `ENOMEM` (or w/e is appropriate) to the consumer, rather than terminating when hitting an out of memory condition. 4. What about the naming and CTF issue? I don't think I fully understand the concern there. If folks think it's best to start with this PR by itself, I can pick this up after the dev summit next week, and try to get it moving forward again. Since it's a dependency of #451, I think it's worth spending some time to get this moving forward. > I'd like to see how it goes replacing the one in libfakekernel with it. Honestly, I didn't even realize there was already a taskq implementation in `libfakekernel`. Now I'm curious what the differences are between what's being done here, and what already exists in `libfakekernel`. I presume, we'd want to merge the two implementations, right? (assuming what's already in `libfakekernel` isn't sufficient for #451). I'll ask around internally in Delphix and try to get a better understanding of why we didn't just use `libfakekernel`'s taskq to begin with. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/141#issuecomment-338269143 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T012acd019d09e76d-M359b5e2f284fbe54a5e84750 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7149 move libzpool's taskq library into libcmdutils (#141)
Is there some place (i.e. a git branch) where I can pick up the new taskq library? I'd like to see how it goes replacing the one in libfakekernel with it. Maybe a separate PR for the new lib might be easier... -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/141#issuecomment-338261325 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T012acd019d09e76d-Ma9fe67b6b76d543451c1 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7149 move libzpool's taskq library into libcmdutils (#141)
Should I go ahead and move forward with the work in #451? Part of the reason I haven't tried to RTI that, are the issues brought up here. #451 pulls a lot of this patch into it, and I was hesitant to upstream that, until I had the time to field any concerns that would have been raised during the RTI. Can folks comment on that PR, about if the approach taken there is something that would be appropriate to land in illumos (or not)? It would be helpful for me, to have some clear and actionable things to address in that PR, so I can move it forward. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/141#issuecomment-338253423 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T012acd019d09e76d-Mfc573f9e80cc7eba642e97f8 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7149 move libzpool's taskq library into libcmdutils (#141)
presumably, yes -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/141#issuecomment-338207035 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T012acd019d09e76d-M65073e161a65f3f681081377 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7149 move libzpool's taskq library into libcmdutils (#141)
Well, then libfakekernel's taskq could be dropped as well once this is done? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/141#issuecomment-338203180 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T012acd019d09e76d-M96a1e9acc8f8699cedbdf30b Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7149 move libzpool's taskq library into libcmdutils (#141)
Well, the rest of what's in libfakekernel is probably not as "generic" as we might like a libtaskq or libutaskq to be, so I'd probably go with a separate library. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/141#issuecomment-338196031 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T012acd019d09e76d-M804da9f8d6075d4d197578aa Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7149 move libzpool's taskq library into libcmdutils (#141)
> Maybe a new library would be better? libfakekernel? ;-) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/141#issuecomment-338085360 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T012acd019d09e76d-Mf4b2ed9391e106b4990f2573 Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7149 move libzpool's taskq library into libcmdutils (#141)
That sounds like a good idea. libcmdutils is becoming a bucket for everything. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/141#issuecomment-338036480 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T012acd019d09e76d-M55e62b27be637b2d62fd3a8a Powered by Topicbox: https://topicbox.com
[developer] Re: [openzfs/openzfs] 7149 move libzpool's taskq library into libcmdutils (#141)
I think it's time to reopen this. The problem was addressed in #359 by giving the user space implementation a different name (utaskq). Apart from a minor omission the changes look good. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/openzfs/openzfs/pull/141#issuecomment-337964828 -- openzfs-developer Archives: https://openzfs.topicbox.com/groups/developer/discussions/T012acd019d09e76d-M90d914e09660ec7f349da544 Powered by Topicbox: https://topicbox.com
