Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

From: NeilBrown <neilb@suse.com>
To: Mikulas Patocka <mpatocka@redhat.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
	LKML <linux-kernel@vger.kernel.org>,
	Michal Hocko <mhocko@kernel.org>,
	linux-mm@kvack.org, dm-devel@redhat.com,
	Mel Gorman <mgorman@suse.de>,
	David Rientjes <rientjes@google.com>,
	Ondrej Kozina <okozina@redhat.com>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks
Date: Thu, 28 Jul 2016 07:36:12 +1000	[thread overview]
Message-ID: <87bn1i4vcj.fsf@notabene.neil.brown.name> (raw)
In-Reply-To: <alpine.LRH.2.02.1607270948040.1779@file01.intranet.prod.int.rdu2.redhat.com>

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

On Thu, Jul 28 2016, Mikulas Patocka wrote:

> On Wed, 27 Jul 2016, NeilBrown wrote:
>
>> On Tue, Jul 26 2016, Mikulas Patocka wrote:
>> 
>> > On Sat, 23 Jul 2016, NeilBrown wrote:
>> >
>> >> "dirtying ... from the reclaim context" ??? What does that mean?
>> >> According to
>> >>   Commit: 26eecbf3543b ("[PATCH] vm: pageout throttling")
>> >> From the history tree, the purpose of throttle_vm_writeout() is to
>> >> limit the amount of memory that is concurrently under I/O.
>> >> That seems strange to me because I thought it was the responsibility of
>> >> each backing device to impose a limit - a maximum queue size of some
>> >> sort.
>> >
>> > Device mapper doesn't impose any limit for in-flight bios.
>> 
>> I would suggest that it probably should. At least it should
>> "set_wb_congested()" when the number of in-flight bios reaches some
>> arbitrary threshold.
>
> If we set the device mapper device as congested, it can again trigger that 
> mempool alloc throttling bug.
>
> I.e. suppose that we swap to a dm-crypt device. The dm-crypt device 
> becomes clogged and sets its state as congested. The underlying block 
> device is not congested.
>
> The mempool_alloc function in the dm-crypt workqueue sets the 
> PF_LESS_THROTTLE flag, and tries to allocate memory, but according to 
> Michal's patches, processes with PF_LESS_THROTTLE may still get throttled.
>
> So if we set the dm-crypt device as congested, it can incorrectly throttle 
> the dm-crypt workqueue that does allocations of temporary pages and 
> encryption.
>
> I think that approach with PF_LESS_THROTTLE in mempool_alloc is incorrect 
> and that mempool allocations should never be throttled.

I very much agree with that last statement!  It may be that to get to
that point we will need all backing devices to signal congestion
correctly.

>
>> > I've made some patches that limit in-flight bios for device mapper in
>> > the past, but there were not integrated into upstream.
>> 
>> I second the motion to resurrect these.
>
> I uploaded those patches here:
>
> http://people.redhat.com/~mpatocka/patches/kernel/dm-limit-outstanding-bios/

Thanks!  I'll have a look.

NeilBrown

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