Closed #536.
--
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/536#event-1534882276
--
openzfs: openzfs-developer
Permalink:
https://openzfs.topic
This is superseded by #596
--
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/536#issuecomment-375198185
--
openzfs: openzfs-developer
Permalink:
I can live with this.
Rather than having 3 userspace taskq implementations (the one in libzpool,
libfakekernel and libutaskq) we now have 2 (one in libfakekernel and one in
libzfs). That's an improvement.
--
You are receiving this because you are subscribed to this thread.
Reply to this email
Thanks @gwr.
I was planning to have libzpool continue to use the taskq implementation in
libfakekernel, and only have libzfs use this new implementation (since we don't
want libzfs to use the libfakekernel one). Does that sound OK to you?
I appreciate the input. I'll try and get a cleaned up an
gwr commented on this pull request.
I didn't look in detail, but the approach looks reasonable.
Is utaskq.c also used in libzpool? Or was you plan to use it both places? I
don't see changes for that.
--
You are receiving this because you are subscribed to this thread.
Reply to this email di
@prakashsurya pushed 1 commit.
f418fd7 Remove unused reference to "system_utaskq"
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/openzfs/openzfs/pull/536/files/7197fc21f851f2d59194659e386513d53de0a8b4..f418fd7d977d20d4fea45bec5059c537
In my latest commit, I've pulled in the taskq implementation that we've been
using in delphix-os (via libcmdutils), directly into libzfs (with slight
modifications).
It might be possible to pull this out into a completely separate (and new)
library, but I'm not sure that's actually useful, sinc
@prakashsurya pushed 1 commit.
7197fc2 Add "utaskq" implementation to libzfs
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/openzfs/openzfs/pull/536/files/a22cfea20bc08dafea04c748b071bce5a250085e..7197fc21f851f2d59194659e386513d53de0a
Going further than Robert here, after looking at how this ended up, and some
later work on other (unpublished) consumers of libfakekernel, I would not
advocate _not_ trying to have libfakekernel share a libutaskq library. The
libfakekernel one really needs to present a "kernel-like" thread mode
I will investigate the work needed to break the taskq implementation out of
libfakekernel, and into a new libutaskq library. I'll report back when I have
something reasonable to show. Thanks for all the input! I appreciate it.
--
You are receiving this because you are subscribed to this thread.
On 2/16/18 9:17 , Prakash Surya wrote:
> @rmustacc OK. So, if I'm understanding you correctly, you'd like us to take
> the taskq implementation from libfakekernel, move it into a new userspace
> library (e.g. libutaskq), and then make libfakekernel and libzfs consume this
> new library?
>
I gu
@prakashsurya You may count me as a reviewer if you wish to proceed down the
current path, though I am having some seconds thoughts based on Robert's
comments. We could break this up in a subsequent change I guess. I'd be okay
with that.
--
You are receiving this because you are subscribed t
On 2/16/18 9:17 , Prakash Surya wrote:
> @rmustacc OK. So, if I'm understanding you correctly, you'd like us to take
> the taskq implementation from libfakekernel, move it into a new userspace
> library (e.g. libutaskq), and then make libfakekernel and libzfs consume this
> new library?
>
I th
@rmustacc OK. So, if I'm understanding you correctly, you'd like us to take the
taskq implementation from libfakekernel, move it into a new userspace library
(e.g. libutaskq), and then make libfakekernel and libzfs consume this new
library?
--
You are receiving this because you are subscribed
@prakashsurya I do think libzfs in general being a consumer of a fake kernel
library is the wrong thing. I mean, wouldn't you say the same thing if it was
linked against libzpool? It feels like we're going to all these contortions to
emulate a kernel environment which we don't do for other code
rmustacc commented on this pull request.
> @@ -88,6 +89,9 @@
#include
#defineMAXISALEN 257 /* based on sysinfo(2) man page */
+static int mount_tq_nthr = 512;/* taskq threads for multi-threaded
mounting */
Well, it's pretty easy to go tune down the max lwps you h
rmustacc commented on this pull request.
> + */
+static void
+zfs_dispatch_mount(libzfs_handle_t *hdl, zfs_handle_t **handles,
+size_t num_handles, int idx, zfs_iter_f func, void *data, taskq_t *tq)
+{
+ mnt_param_t *mnt_param = zfs_alloc(hdl, sizeof (mnt_param_t));
+
+ mnt_param
@andy-js do you have any more questions/comments on this? can I count you as a
reviewer?
--
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/536#issuecomment-366291880
andy-js commented on this pull request.
> @@ -88,6 +89,9 @@
#include
#defineMAXISALEN 257 /* based on sysinfo(2) man page */
+static int mount_tq_nthr = 512;/* taskq threads for multi-threaded
mounting */
I suppose this could be made a tuneable through /etc/defau
andy-js commented on this pull request.
> -#ifdef __cplusplus
-extern "C" {
-#endif
-
-#include
-#include
-#include
-#include
-#include
-#include
-
-#ifdef __cplusplus
-}
-#endif
-
-#endif /* _SYS_ZFS_CONTEXT_H */
I'm glad to see this file go. Too bad we can't also drop the one in
@rmustacc any more questions/comments on this?
--
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/536#issuecomment-366075230
--
openzfs-developer
Ar
ahrens commented on this pull request.
> + */
+static void
+zfs_dispatch_mount(libzfs_handle_t *hdl, zfs_handle_t **handles,
+size_t num_handles, int idx, zfs_iter_f func, void *data, taskq_t *tq)
+{
+ mnt_param_t *mnt_param = zfs_alloc(hdl, sizeof (mnt_param_t));
+
+ mnt_param->
prakashsurya commented on this pull request.
> @@ -88,6 +89,9 @@
#include
#defineMAXISALEN 257 /* based on sysinfo(2) man page */
+static int mount_tq_nthr = 512;/* taskq threads for multi-threaded
mounting */
I talked to Seb about this today, and it's like I was
@prakashsurya pushed 1 commit.
181dfd2 Use LOCK_ERRORCHECK plus mutex_enter/mutex_exit
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/openzfs/openzfs/pull/536/files/b3f4bdf804d187b4d17d6721d3b47c2e53685e07..181dfd22949ac3867ec32f0a039
prakashsurya commented on this pull request.
> + */
+static void
+zfs_dispatch_mount(libzfs_handle_t *hdl, zfs_handle_t **handles,
+size_t num_handles, int idx, zfs_iter_f func, void *data, taskq_t *tq)
+{
+ mnt_param_t *mnt_param = zfs_alloc(hdl, sizeof (mnt_param_t));
+
+ mnt_p
prakashsurya commented on this pull request.
> @@ -88,6 +89,9 @@
#include
#defineMAXISALEN 257 /* based on sysinfo(2) man page */
+static int mount_tq_nthr = 512;/* taskq threads for multi-threaded
mounting */
@sebroy can you comment here? I don't see anything in
prakashsurya commented on this pull request.
> @@ -416,6 +416,7 @@ enum {
#defineVSW_XID 0x40/* file system supports extended ids */
#defineVSW_CANLOFI 0x80/* file system supports lofi mounts */
#defineVSW_ZMOUNT 0x100 /* file system always a
@rmustacc Thanks for the reply. Let me know after you give this version a look.
If you think `libzfs` being a consumer of `libfakekernel`'s taskq
implementation, then let's discuss the best way to break that out so we can do
this properly.
--
You are receiving this because you are subscribed t
about libfakekernel dependencies.
to me - it is not good code design.
we have dependency to libfakekernel everywere we need just taskq.
why not use shared sources for taskq in places where we need it?
or i have missed something?
we no need duplicate code with taskq - we can use the same sources i
rmustacc commented on this pull request.
> @@ -416,6 +416,7 @@ enum {
#defineVSW_XID 0x40/* file system supports extended ids */
#defineVSW_CANLOFI 0x80/* file system supports lofi mounts */
#defineVSW_ZMOUNT 0x100 /* file system always allow
@prakashsurya Thanks for splitting out the change regarding the file system
part, that makes things clearer.
Honestly though, no, I still feel like this is the wrong approach in terms of
leveraging the fake kernel library. But the fake kernel and libzpool are
similar to me in so far as their go
@andy-js @rmustacc is this good to RTI? it's passing the build, zloop, and
zfstest.
--
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/536#issuecomment-364567269
-
On Fri, Feb 9, 2018 at 7:33 AM, Schweiss, Chip wrote:
> I've been following this review for a while. Sorry, if this is not the
> correct place for this comment.
>
> Personally, I've been doing similar outside zpool import by doing a 'zpool
> import -N' then parallel executing 'zfs mount {folder
I've been following this review for a while. Sorry, if this is not the
correct place for this comment.
Personally, I've been doing similar outside zpool import by doing a 'zpool
import -N' then parallel executing 'zfs mount {folder}' followed by
parallel executing 'zfs share {folder}'. My zpool
@prakashsurya pushed 1 commit.
c8a3e65 Fix cstyle; use tab for indentation, not spaces
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/openzfs/openzfs/pull/536/files/3a83672ecf670c528f05209997189ad8c22ce836..c8a3e654b0940d3ba0914b397f2
@prakashsurya pushed 2 commits.
20d3e56 Missed a mutex_{enter,exit} pair, causing a crash
3a83672 Fix lint; add guards for _TASKQUSER in lgrp_user.h
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/openzfs/openzfs/pull/536/files/7b561c
@andy-js @rmustacc let me know if my latest change is what y'all were thinking.
I haven't tested this yet, beyond verifying it builds.
--
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/5
@prakashsurya pushed 1 commit.
7b561cc Iterate based Rob's and Andrew's feedback
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/openzfs/openzfs/pull/536/files/b9e80a4220e45881599deb6ee5309e1cd9cae1b6..7b561cc953f07bae0a3486d53d9a50e9e
Thanks. I'll look into that, and push a new revision when I have something
working.
--
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/536#issuecomment-363920916
-
I think @rmustacc is exactly right. We should not be building the zfs command
(or libzfs) in fakekernel context. This is a highly specialised environment
that really only makes sense for things like libzpool which include kernel code.
I suggest you have a look at this earlier webrev of mine wh
> Unrelated to the above confusion, do you have an analysis somewhere about why
> we're marking specific file systems as requiring to be added to or removed
> from the mount in progress tables? It seems like this is probably an
> important part of this change. Can we get some comments as to why
Thanks for giving this a look @rmustacc.
> Doesn't it strike others as odd when you have a userland program use a
> kmutex_t, but then also referring to the userland bits like a normal
> USYNC_THREAD, which is definitely a userland mutex thing.
Thanks for catching this; this is wrong. Rather t
When trying to review this, I found the use of the fake kernel here really
confusing. Why are we using it for a normal user land program? Doesn't it
strike others as odd when you have a userland program use a `kmutex_t`, but
then also referring to the userland bits like a normal USYNC_THREAD, wh
@andy-js I'd appreciate your review on this, if you have the time.
I've rebased this onto the latest master changes, and the `libfakekernel`
changes you made in #494. I ran into some build issues where some files would
`#include ` and this would conflict with symbols/macros from
`libfakekernel`
44 matches
Mail list logo