[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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:

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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:

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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:

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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:

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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)); + +

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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)); + +

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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:

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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)); + +

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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

Re: [developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

Re: [developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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:

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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:

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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:

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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:

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

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,

[developer] Re: [openzfs/openzfs] 8115 parallel zfs mount (v2) (#536)

@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