From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753130AbcD1QMt (ORCPT ); Thu, 28 Apr 2016 12:12:49 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:53838 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752424AbcD1QMr (ORCPT ); Thu, 28 Apr 2016 12:12:47 -0400 MIME-Version: 1.0 In-Reply-To: References: <1461805789-3632-1-git-send-email-ming.lei@canonical.com> <1461805789-3632-3-git-send-email-ming.lei@canonical.com> Date: Fri, 29 Apr 2016 00:12:44 +0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3 3/3] block: avoid to call .bi_end_io() recursively From: Ming Lei To: Mikulas Patocka Cc: Jens Axboe , Linux Kernel Mailing List , linux-block@vger.kernel.org, Christoph Hellwig , Btrfs BTRFS , Shaun Tancheff , Alan Cox , Neil Brown , Liu Bo , Jens Axboe Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Apr 28, 2016 at 11:58 PM, Mikulas Patocka wrote: > > > On Thu, 28 Apr 2016, Ming Lei wrote: > >> Hi Mikulas, >> >> On Thu, Apr 28, 2016 at 11:29 PM, Mikulas Patocka wrote: >> > >> > >> > On Thu, 28 Apr 2016, Ming Lei wrote: >> > >> >> There were reports about heavy stack use by recursive calling >> >> .bi_end_io()([1][2][3]). For example, more than 16K stack is >> >> consumed in a single bio complete path[3], and in [2] stack >> >> overflow can be triggered if 20 nested dm-crypt is used. >> >> >> >> Also patches[1] [2] [3] were posted for addressing the issue, >> >> but never be merged. And the idea in these patches is basically >> >> similar, all serializes the recursive calling of .bi_end_io() by >> >> percpu list. >> >> >> >> This patch still takes the same idea, but uses bio_list to >> >> implement it, which turns out more simple and the code becomes >> >> more readable meantime. >> >> >> >> One corner case which wasn't covered before is that >> >> bi_endio() may be scheduled to run in process context(such >> >> as btrfs), and this patch just bypasses the optimizing for >> >> that case because one new context should have enough stack space, >> >> and this approach isn't capable of optimizing it too because >> >> there isn't easy way to get a per-task linked list head. >> > >> > Hi >> > >> > You could use preempt_disable() and then you could use per-cpu list even >> > in the process context. >> >> Image why the .bi_end_io() is scheduled to process context, and the only >> workable/simple way I thought of is to use per-task list because it may sleep. > > The bi_end_io callback should not sleep, even if it is called from the > process context. If it shouldn't sleep, why is it scheduled to run in process context by paying extra context switch cost? And you can find that btrfs_subio_endio_read() does sleep for checksum stuff. Thanks, Ming > >> Given new context should have enough stack and only btrfs has this kind of >> usage as far as I see, so don't think that is worth of the optimization. >> >> Thanks, >> Ming > > Mikulas > -- > To unsubscribe from this list: send the line "unsubscribe linux-block" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html