Re: [PATCH 3/3] raid5: allow r5l_io_unit allocations to fail

[NeilBrown <neilb@suse.com>]

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

On Fri, Dec 18 2015, Christoph Hellwig wrote:

> On Fri, Dec 18, 2015 at 12:51:07PM +1100, NeilBrown wrote:
>> > if the reclaim thread doesn't have anything to reclaim,
>> > r5l_run_no_space_stripes isn't called. we might miss the retry.
>> 
>> so something like this:
>
> that looks fine to me.  I'll give a spin on my QA setup.
>
>> > I'm a little worrying about the GFP_ATOMIC allocation. In the first try,
>> > GFP_NOWAIT is better. And on the other hand, why sleep is bad here? We
>> > could use GFP_NOIO | __GFP_NORETRY, there is no deadlock risk.
>> >
>> > In the retry, GFP_NOIO looks better. No deadlock too, since it's not
>> > called from raid5d (maybe we shouldn't call from reclaim thread if using
>> > GFP_NOIO, a workqueue is better). Otherwise we could keep retring but do
>> > nothing.
>> 
>> I did wonder a little bit about that.
>> GFP_ATOMIC is (__GFP_HIGH)
>> GFP_NOIO | __GFP_NORETRY is  (__GFP_WAIT | __GFP_NORETRY)
>> 
>> It isn't clear that we need 'HIGH', and WAIT with NORETRY should be OK.
>> It allows __alloc_pages_direct_reclaim, but only once and never waits
>> for other IO.
>
> In general we go for HIGH on non-sleeping allocation to avoid having
> the stalled.  This is especially important in the I/O path.
>
> WAIT means we will reclaim and wait for it, which looks a little dangerous
> to me.  In general I'd prefer not to use obscure gfp flag combination
> unless there is a real need, and it's clearly documented.

I don't think "will reclaim and wait for it" is accurate.

Certainly page_alloc will only try direct reclaim when WAIT is set, but
I don't think it actually waits for anything particular.

There are various waits in the direct reclaim path such as
throttle_vm_writeout(), but they seem to be throttling delays rather
than "wait for something particular" delays.
Also direct reclaim (e.g. in shrinkers) are allows to sleep (as long as
they don't violate NOFS or NOIO), so waiting can definitely happen for
non-throttling reasons.

I *think* (not 100% sure) that __GFP_WAIT means that the alloc code is
allowed to schedule().  Without that flag it mustn't.

So it makes sense to use WAIT when it is appropriate for a delay to be
introduce to throttle allocations.  I don't think that is the case in
raid5d.  raid5d is too low level - throttling it will not directly
affect higher level allocations.

So while my reasoning is a bit different, I agree that we don't really
want WAIT in an allocation in raid5d.

.... oh.  I just noticed that __GFP_WAIT was renamed to __GFP_RECLAIM
last month.  And there is a new __GFP_DIRECT_RECLAIM.  I think I'm going
to have to learn how this stuff works again.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]