Re: [GIT PULL] Block changes for 4.18-rc

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 3067 bytes --]

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@kernel.dk> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]