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

[Steven Hartland]

On 04/08/2015 21:24, Slawa Olhovchenkov wrote:
> On Tue, Aug 04, 2015 at 09:06:55PM +0100, Steven Hartland wrote:
>
>>
>> On 04/08/2015 17:14, Slawa Olhovchenkov wrote:
>>> On Tue, Aug 04, 2015 at 09:30:30AM +0100, Steven Hartland wrote:
>>>
>>>> On 03/08/2015 21:48, Warner Losh wrote:
>>>>>> On Aug 3, 2015, at 1:44 PM, Slawa Olhovchenkov <slw at zxy.spb.ru> wrote:
>>>>>>
>>>>>> On Tue, Aug 04, 2015 at 03:35:50AM +0800, Julian Elischer wrote:
>>>>>>
>>>>>>> On 8/3/15 8:03 PM, Konstantin Belousov wrote:
>>>>>>>> On Mon, Aug 03, 2015 at 12:50:19PM +0100, Steven Hartland wrote:
>>>>>>>>> For this change I don't want to get into fixing the thread0 stack size,
>>>>>>>>> which can be done later, just
>>>>>>>>> to provide a reasonable warning to the user that smaller values could
>>>>>>>>> cause a panic.
>>>>>>>> Hmm, is it limited to the thread0 only ?  I.e., would only increasing
>>>>>>>> the initial thread stack size be enough to boot the kernel ?  The zfs
>>>>>>>> threads do request larger stack size, I know this.
>>>>>>>>
>>>>>>>> Can somebody test the following patch in the i386 configuration which
>>>>>>>> does not boot ?
>>>>>>> I think this is a reasonable thing to do. Thread0 (and proc0) are special.
>>>>>>> I don't see why giving it a specially sized stack would be a problem.
>>>>>> This is always do for ARM.
>>>>>> May be need increase stack size for Thread0 on ARM too?
>>>>> Seems reasonable. There should be a MI way of doing this, but all the code and defines are buried in MD files, so each architecture needs some love to make this a reality.
>>>>>
>>>>> Warner
>>>> In the mean time are people happier with
>>>> https://reviews.freebsd.org/D3279 or should I just leave it using the
>>>> #define until someone has time to work on a full solution?
>>> Checking by #ifdef you check only parametr at time of building zfs.ko,
>>> checking variable you check actual value.
>>> May be check thread stack best if only for current tread.
>> Not sure I follow you as its not a #ifdef check its straight if in the
>> new version i.e.
>> if (kstack_pages < ZFS_MIN_KSTACK_PAGES) {
> This check checked how actual kernel compile vs how compile zfs.ko.
> Remeber that kenel may be compiled independed from modules?
Correct that's the requirement as its thread0 that appears to be the key.
>
>> Just in case you didn't notice kib committed a fix for i386 thread0 in
>> r286288 so this may not be needed at all any more which is good news :)
> If I understund kib fix (and you about ZFS stack requirements) you
> need check curthread->td_kstack_pages (for case old, unfixed kerenel,
> depended from KSTACK_PAGES in Thread0).
>
> I.e. for all cases:
>
> - unfixed kernel with KSTACK_PAGES < 4
> - unfixed kernel with KSTACK_PAGES >= 4
> - fixed kernel with KSTACK_PAGES < 4
> - fixed kernel with KSTACK_PAGES >= 4
> - compiling zfs.ko separately from kernel with different KSTACK_PAGES
> - using zfs.ko with old kernel
>
> checking curthread->td_kstack_pages is right way (or, may be
> curthread->td_kstack_pages*PAGE_SIZE -- I am not cleanly understund
> what need to check -- size in bytes or size in pages).
> Checking KSTACK_PAGES by ifdef can produce vrong result.
Its not clear to me if curthread will be thread0 however in unfixed 
thread0 stack size would have been kstack_pages so at the time this was 
still correct IMO.

Just to be clear this is not checked by an ifdef, that's just OS guard 
which is correct.

Latest revision which now explicitly checks thread0 stack size is now 
available:
https://reviews.freebsd.org/D3279

     Regards
     Steve