lkml.org 
[lkml]   [2016]   [Apr]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v3 3/3] block: avoid to call .bi_end_io() recursively
    From
    On Thu, Apr 28, 2016 at 11:58 PM, Mikulas Patocka <mpatocka@redhat.com> wrote:
    >
    >
    > On Thu, 28 Apr 2016, Ming Lei wrote:
    >
    >> Hi Mikulas,
    >>
    >> On Thu, Apr 28, 2016 at 11:29 PM, Mikulas Patocka <mpatocka@redhat.com> 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

    \
     
     \ /
      Last update: 2016-04-28 18:41    [W:9.370 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site