On Fri, Dec 18 2015, Christoph Hellwig wrote:

On Fri, Dec 18, 2015 at 12:51:07PM +1100, NeilBrown wrote:

quoted

quoted

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.

quoted

quoted

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