Re: [PATCH] dm: Avoid sleeping while holding the dm_bufio lock

[Doug Anderson]

Hi,

On Wed, Dec 14, 2016 at 4:53 PM, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> Hi,
>
> On Wed, Nov 23, 2016 at 12:57 PM, Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote:
>> Hi
>>
>> The GFP_NOIO allocation frees clean cached pages. The GFP_NOWAIT
>> allocation doesn't. Your patch would incorrectly reuse buffers in a
>> situation when the memory is filled with clean cached pages.
>>
>> Here I'm proposing an alternate patch that first tries GFP_NOWAIT
>> allocation, then drops the lock and tries GFP_NOIO allocation.
>>
>> Note that the root cause why you are seeing this stacktrace is, that your
>> block device is congested - i.e. there are too many requests in the
>> device's queue - and note that fixing this wait won't fix the root cause
>> (congested device).
>>
>> The congestion limits are set in blk_queue_congestion_threshold to 7/8 to
>> 13/16 size of the nr_requests value.
>>
>> If you don't want your device to report the congested status, you can
>> increase /sys/block/<device>/queue/nr_requests - you should test if your
>> chromebook is faster of slower with this setting increased. But note that
>> this setting won't increase the IO-per-second of the device.
>>
>> Mikulas
>>
>>
>> On Thu, 17 Nov 2016, Douglas Anderson wrote:
>>
>>> We've seen in-field reports showing _lots_ (18 in one case, 41 in
>>> another) of tasks all sitting there blocked on:
>>>
>>>   mutex_lock+0x4c/0x68
>>>   dm_bufio_shrink_count+0x38/0x78
>>>   shrink_slab.part.54.constprop.65+0x100/0x464
>>>   shrink_zone+0xa8/0x198
>>>
>>> In the two cases analyzed, we see one task that looks like this:
>>>
>>>   Workqueue: kverityd verity_prefetch_io
>>>
>>>   __switch_to+0x9c/0xa8
>>>   __schedule+0x440/0x6d8
>>>   schedule+0x94/0xb4
>>>   schedule_timeout+0x204/0x27c
>>>   schedule_timeout_uninterruptible+0x44/0x50
>>>   wait_iff_congested+0x9c/0x1f0
>>>   shrink_inactive_list+0x3a0/0x4cc
>>>   shrink_lruvec+0x418/0x5cc
>>>   shrink_zone+0x88/0x198
>>>   try_to_free_pages+0x51c/0x588
>>>   __alloc_pages_nodemask+0x648/0xa88
>>>   __get_free_pages+0x34/0x7c
>>>   alloc_buffer+0xa4/0x144
>>>   __bufio_new+0x84/0x278
>>>   dm_bufio_prefetch+0x9c/0x154
>>>   verity_prefetch_io+0xe8/0x10c
>>>   process_one_work+0x240/0x424
>>>   worker_thread+0x2fc/0x424
>>>   kthread+0x10c/0x114
>>>
>>> ...and that looks to be the one holding the mutex.
>>>
>>> The problem has been reproduced on fairly easily:
>>> 0. Be running Chrome OS w/ verity enabled on the root filesystem
>>> 1. Pick test patch: http://crosreview.com/412360
>>> 2. Install launchBalloons.sh and balloon.arm from
>>>      http://crbug.com/468342
>>>    ...that's just a memory stress test app.
>>> 3. On a 4GB rk3399 machine, run
>>>      nice ./launchBalloons.sh 4 900 100000
>>>    ...that tries to eat 4 * 900 MB of memory and keep accessing.
>>> 4. Login to the Chrome web browser and restore many tabs
>>>
>>> With that, I've seen printouts like:
>>>   DOUG: long bufio 90758 ms
>>> ...and stack trace always show's we're in dm_bufio_prefetch().
>>>
>>> The problem is that we try to allocate memory with GFP_NOIO while
>>> we're holding the dm_bufio lock.  Instead we should be using
>>> GFP_NOWAIT.  Using GFP_NOIO can cause us to sleep while holding the
>>> lock and that causes the above problems.
>>>
>>> The current behavior explained by David Rientjes:
>>>
>>>   It will still try reclaim initially because __GFP_WAIT (or
>>>   __GFP_KSWAPD_RECLAIM) is set by GFP_NOIO.  This is the cause of
>>>   contention on dm_bufio_lock() that the thread holds.  You want to
>>>   pass GFP_NOWAIT instead of GFP_NOIO to alloc_buffer() when holding a
>>>   mutex that can be contended by a concurrent slab shrinker (if
>>>   count_objects didn't use a trylock, this pattern would trivially
>>>   deadlock).
>>>
>>> Suggested-by: David Rientjes <rientjes@xxxxxxxxxx>
>>> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
>>> ---
>>> Note that this change was developed and tested against the Chrome OS
>>> 4.4 kernel tree, not mainline.  Due to slight differences in verity
>>> between mainline and Chrome OS it became too difficult to reproduce my
>>> testing setup on mainline.  This patch still seems correct and
>>> relevant to upstream, so I'm posting it.  If this is not acceptible to
>>> you then please ignore this patch.
>>>
>>> Also note that when I tested the Chrome OS 3.14 kernel tree I couldn't
>>> reproduce the long delays described in the patch.  Presumably
>>> something changed in either the kernel config or the memory management
>>> code between the two kernel versions that made this crop up.  In a
>>> similar vein, it is possible that problems described in this patch are
>>> no longer reproducible upstream.  However, the arguments made in this
>>> patch (that we don't want to block while holding the mutex) still
>>> apply so I think the patch may still have merit.
>>>
>>>  drivers/md/dm-bufio.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
>>> index b3ba142e59a4..3c767399cc59 100644
>>> --- a/drivers/md/dm-bufio.c
>>> +++ b/drivers/md/dm-bufio.c
>>> @@ -827,7 +827,8 @@ static struct dm_buffer *__alloc_buffer_wait_no_callback(struct dm_bufio_client
>>>        * dm-bufio is resistant to allocation failures (it just keeps
>>>        * one buffer reserved in cases all the allocations fail).
>>>        * So set flags to not try too hard:
>>> -      *      GFP_NOIO: don't recurse into the I/O layer
>>> +      *      GFP_NOWAIT: don't wait; if we need to sleep we'll release our
>>> +      *                  mutex and wait ourselves.
>>>        *      __GFP_NORETRY: don't retry and rather return failure
>>>        *      __GFP_NOMEMALLOC: don't use emergency reserves
>>>        *      __GFP_NOWARN: don't print a warning in case of failure
>>> @@ -837,7 +838,8 @@ static struct dm_buffer *__alloc_buffer_wait_no_callback(struct dm_bufio_client
>>>        */
>>>       while (1) {
>>>               if (dm_bufio_cache_size_latch != 1) {
>>> -                     b = alloc_buffer(c, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
>>> +                     b = alloc_buffer(c, GFP_NOWAIT | __GFP_NORETRY |
>>> +                                      __GFP_NOMEMALLOC | __GFP_NOWARN);
>>>                       if (b)
>>>                               return b;
>>>               }
>>> --
>>> 2.8.0.rc3.226.g39d4020
>>>
>>> --
>>> dm-devel mailing list
>>> dm-devel@xxxxxxxxxx
>>> https://www.redhat.com/mailman/listinfo/dm-devel
>>>
>>
>> From: Mikulas Patocka <mpatocka@xxxxxxxxxx>
>>
>> Subject: dm-bufio: drop the lock when doing GFP_NOIO alloaction
>>
>> Drop the lock when doing GFP_NOIO alloaction beacuse the allocation can
>> take some time.
>>
>> Note that we won't do GFP_NOIO allocation when we loop for the second
>> time, because the lock shouldn't be dropped between __wait_for_free_buffer
>> and __get_unclaimed_buffer.
>>
>> Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx>
>>
>> ---
>>  drivers/md/dm-bufio.c |   13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> Index: linux-2.6/drivers/md/dm-bufio.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/md/dm-bufio.c
>> +++ linux-2.6/drivers/md/dm-bufio.c
>> @@ -822,11 +822,13 @@ enum new_flag {
>>  static struct dm_buffer *__alloc_buffer_wait_no_callback(struct dm_bufio_client *c, enum new_flag nf)
>>  {
>>         struct dm_buffer *b;
>> +       bool tried_noio_alloc = false;
>>
>>         /*
>>          * dm-bufio is resistant to allocation failures (it just keeps
>>          * one buffer reserved in cases all the allocations fail).
>>          * So set flags to not try too hard:
>> +        *      GFP_NOWAIT: don't sleep and don't release cache
>>          *      GFP_NOIO: don't recurse into the I/O layer
>>          *      __GFP_NORETRY: don't retry and rather return failure
>>          *      __GFP_NOMEMALLOC: don't use emergency reserves
>> @@ -837,7 +839,7 @@ static struct dm_buffer *__alloc_buffer_
>>          */
>>         while (1) {
>>                 if (dm_bufio_cache_size_latch != 1) {
>> -                       b = alloc_buffer(c, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
>> +                       b = alloc_buffer(c, GFP_NOWAIT | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
>>                         if (b)
>>                                 return b;
>>                 }
>> @@ -845,6 +847,15 @@ static struct dm_buffer *__alloc_buffer_
>>                 if (nf == NF_PREFETCH)
>>                         return NULL;
>>
>> +               if (dm_bufio_cache_size_latch != 1 && !tried_noio_alloc) {
>> +                       dm_bufio_unlock(c);
>> +                       b = alloc_buffer(c, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
>> +                       dm_bufio_lock(c);
>> +                       if (b)
>> +                               return b;
>> +                       tried_noio_alloc = true;
>> +               }
>> +
>>                 if (!list_empty(&c->reserved_buffers)) {
>>                         b = list_entry(c->reserved_buffers.next,
>>                                        struct dm_buffer, lru_list);
>
> I saw a git pull go by today from Mike Snitzer with my version of the
> patch in it.  I think this is fine because I think my version of the
> patch works all right, but I think Mikulas's version of the patch (see
> above) is even better.
>
> Since the "git pull" was to Linus and I believe that my version of the
> patch is functional (even if it's not optimal), maybe the right thing
> to do is to send a new patch with Mikulas's changes atop mine.
> Mikulas: does that sound good to you?  Do you want to send it?

Dang, or I could read the full list of patches in the pull and realize
you guys already did that.  Sigh.  Sorry for the noise.

-Doug