On Mon, Jun 04 2018, Jens Axboe wrote:
> On 6/4/18 9:51 AM, Linus Torvalds wrote:
>> On Sun, Jun 3, 2018 at 5:42 PM Jens Axboe <axboe@xxxxxxxxx> wrote:
>>>
>>> drivers/md/md.c | 61 +--
>>> drivers/md/md.h | 4 +-
>>
>> So I've pulled this, but I get a new warning:
>>
>> drivers/md/md.c: In function ‘mddev_put’:
>> drivers/md/md.c:543:1: warning: the frame size of 2112 bytes is
>> larger than 2048 bytes [-Wframe-larger-than=]
>>
>> which seems to be due to commit afeee514ce7f ("md: convert to
>> bioset_init()/mempool_init()").
>>
>> As of that commit, mddev_put() now allocates *two* "struct bio_set"
>> structures on the stack, and those really aren't small. Yes, part of
>> it is that I do my test-builds with all that debug stuff, but those
>> structures have several embedded mempool_t members, and they really
>> are pretty sizable.
>>
>> And I _really_ do not think it's acceptable to have that kind of stack
>> usage in there. I'm not sure you can trigger mddev_put() in the IO
>> paths, but even without that I don't think it's acceptable.
>>
>> Also, the code simply looks like complete and utter garbage. It does
>>
>> bs = mddev->bio_set;
>> sync_bs = mddev->sync_set;
>>
>> to _copy_ those structures, and then does bioset_exit() on them. WTF?
>>
>> Why the hell doesn't it just do biset_exit() on the originals instead,
>> before freeing the mddev?
>>
>> I've pulled this, but this needs to be fixed. That is just broken
>> garbage right now. No way in hell is it acceptable to waste 2kB of
>> stack space on something idiotic like this.
>
> Works is already underway to get this fixed up. I agree that the
> excessive stack usage isn't great, but the code in that function
> is pretty miserable, as you say. That's not new with the patch,
> the conversion just carried that forward.
>
> CC'ing Neil to get his input on how best to clean that up, I'd
> be much more comfortable with that logic improved rather than
> just catering to the stack usage issue. Also include Arnd, as
> he had a test patch for it.
Cc:ing Shaohua as well, as he is the current maintainer, and authored
Commit: 7184ef8bab0c ("MD: fix sleep in atomic")
which seems most relevant.
The justification given in that patch is that bioset_free() will take a
mutex.
This must have been in destory_workqueue() when destroying
bs->rescue_workqueue.
md biosets don't have a work queue (BIOSET_NEED_RESCUER isn't set),
so these calls will no longer take a mutex. So this commit
can now be reverted. I think that will clean up this code suitably.
I really should get back to BIOSET_NEED_RESCUER and see if I can discard
it completely.
Kent - the only remaining user is bcache, and the main reason I haven't
removed it is that I cannot follow the code (or at least, I couldn't
last time I tried). Are you able to see if it is still needed?
Thanks,
NeilBrown
>
> --
> Jens Axboe
Attachment:
signature.asc
Description: PGP signature
