svn commit: r286223 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs

[Konstantin Belousov]

On Mon, Aug 03, 2015 at 03:52:21AM -0700, Peter Wemm wrote:
> On Monday, August 03, 2015 11:31:58 AM Steven Hartland wrote:
> > On 03/08/2015 10:47, Slawa Olhovchenkov wrote:
> > > On Mon, Aug 03, 2015 at 09:34:10AM +0000, Steven Hartland wrote:
> > >> Author: smh
> > >> Date: Mon Aug  3 09:34:09 2015
> > >> New Revision: 286223
> > >> URL: https://svnweb.freebsd.org/changeset/base/286223
> > >> 
> > >> Log:
> > >>    Fix KSTACK_PAGES check in ZFS module
> > >>    
> > >>    The check introduced by r285946 failed to add the dependency on
> > >>    opt_kstack_pages.h which meant the default value for the platform
> > >>    instead
> > >>    of the customised options KSTACK_PAGES=X was being tested.
> > >>    
> > >>    Also wrap in #ifdef __FreeBSD__ for portability.
> > > 
> > > /usr/src/sys/kern/kern_proc.c:int kstack_pages = KSTACK_PAGES;
> > > 
> > > May be check variable kstack_pages is best way?
> > > Eliminate dependency on foreign opt_XXXX.
> > 
> > I did think of that but as other modules such as dtrace, which is also
> > cddl code, already have this dependency I went with this.
> > 
> > I'm easy though, if there's a concusses that kstack_pages or possibly
> > curthread->td_kstack_pages, which would take into account the
> > possibility of varied thread stack sizes, then I can make that change.
> > 
> > What do others think?
> 
> The whole thing has missing the point.
> 
> Changing the default for the entire kernel just because the zfs compat 
> wrappers can't be bothered requesting a suitable value is.. unfortunate.. 
> particularly when it is in freebsd-provided code, not upstream zfs code.
> 
> Fix the kproc_kthread_add() calls in do_thread_create() and zvol_geom_run() 
> instead.  Enforce a lower bound there for zfs threads instead of making the 
> entire rest of the kernel use more memory.
> 
> eg: I'm thinking along these lines:
> Index: cddl/compat/opensolaris/sys/proc.h
> ====================================================
> --- cddl/compat/opensolaris/sys/proc.h	(revision 286224)
> +++ cddl/compat/opensolaris/sys/proc.h	(working copy)
> @@ -77,6 +77,8 @@
>  	ASSERT(state == TS_RUN);
>  	ASSERT(pp == &p0);
>  
> +	if (stksize < 16384)
> +		stksize = 16384;	/* Enforce lower bound on ZFS threads */
>  	error = kproc_kthread_add(proc, arg, &zfsproc, &td, RFSTOPPED,
>  	    stksize / PAGE_SIZE, "zfskern", "solthread %p", proc);
>  	if (error == 0) {
> 
> 
> Beware, some platforms have large pages (eg: ia64 in -stable has 8k, 16k or 
> 32k pages, from memory).  Specifying an arbitrary number of pages in code 
> that's supposed to be portable isn't a good idea.

This would not help. Issue is the size of the thread0 stack, which
overflows in this case.

I looked at the possibility of making default kernel stack size
configurable by a loader tunable, and the issue is that thread0 gets its
stack set up too early (locore for i386, hammer_time() for amd64).  I.e.,
it is possible to make the setting effective for all threads after thread0,
but not for the one which causes the issue.

I do not want to modify ABI between loader and kernel to pass the parameter.