[PATCH v3 0/3] block: avoid to call .bi_end_io() recursively

[PATCH v3 0/3] block: avoid to call .bi_end_io() recursively

[Ming Lei]

Hi,

The 1st patch handles bio error in dio_end_io() which is only
used by btrfs.

The 2nd patch uses bio_endio() to call .bi_end_io() in dio_end_io().

The 3rd patch avoids to call .bi_end_io recursively in complete path.

xfstests(-g auto) is run over ext4, xfs and btrfs with this patchset
and no regression is reported.

With this patchset, lots of stack space can be saved in bio complete
path. Even there was stack overflow report in bio complete path, see
details in 3/3 commit log.

V3:
	- no behaviour change
	- reorganize the function of unwind_bio_endio() and make it
	more clean

V2:
	- introduce patch 1 & 2
	- deal with running bio_endio() from process context, and
	makes generic/323 & generic/224 of xfstests happy on btrfs

V1:
	- change to RFC
	- fix when unwind_bio_endio() is called recursively
	- run xfstest again: no regression found on ext4,
	but generic/323 and generic/224 cause kernel oops


Ming Lei (3):
  fs: direct-io: handle error in dio_end_io()
  fs: direct-io: call .bi_end_io via bio_endio()
  bflock: avoid to call .bi_end_io() recursively

 block/bio.c    | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/direct-io.c |  8 +++-----
 2 files changed, 56 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH v3 1/3] fs: direct-io: handle error in dio_end_io()

[Ming Lei]

If error is passed to dio_end_io(), it should have been
dealt with. Unfortunately current code just ignores that
silently.

Only btrfs uses dio_end_io().

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 fs/direct-io.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 4720377..a8dd60a 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -352,6 +352,9 @@ void dio_end_io(struct bio *bio, int error)
 {
 	struct dio *dio = bio->bi_private;
 
+	if (!bio->bi_error)
+		bio->bi_error = error;
+
 	if (dio->is_async)
 		dio_bio_end_aio(bio);
 	else
-- 
1.9.1

[PATCH v3 2/3] fs: direct-io: call .bi_end_io via bio_endio()

[Ming Lei]

bio_endio() is the graceful way to complete one bio.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 fs/direct-io.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index a8dd60a..0a35e51 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -350,15 +350,10 @@ static void dio_bio_end_io(struct bio *bio)
  */
 void dio_end_io(struct bio *bio, int error)
 {
-	struct dio *dio = bio->bi_private;
-
 	if (!bio->bi_error)
 		bio->bi_error = error;
 
-	if (dio->is_async)
-		dio_bio_end_aio(bio);
-	else
-		dio_bio_end_io(bio);
+	bio_endio(bio);
 }
 EXPORT_SYMBOL_GPL(dio_end_io);
 
-- 
1.9.1

Re: [PATCH v3 2/3] fs: direct-io: call .bi_end_io via bio_endio()

[Christoph Hellwig]

On Thu, Apr 28, 2016 at 09:09:48AM +0800, Ming Lei wrote:
> bio_endio() is the graceful way to complete one bio.



> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  fs/direct-io.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index a8dd60a..0a35e51 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -350,15 +350,10 @@ static void dio_bio_end_io(struct bio *bio)
>   */
>  void dio_end_io(struct bio *bio, int error)
>  {
> -	struct dio *dio = bio->bi_private;
> -
>  	if (!bio->bi_error)
>  		bio->bi_error = error;
>  
> -	if (dio->is_async)
> -		dio_bio_end_aio(bio);
> -	else
> -		dio_bio_end_io(bio);
> +	bio_endio(bio);
>  }
>  EXPORT_SYMBOL_GPL(dio_end_io);

dio_end_io is only used by btrfs.  So instead of this and the previous
patch you should just open code the error assignment and call to
bio_endio in btrfs.

Re: [PATCH v3 2/3] fs: direct-io: call .bi_end_io via bio_endio()

[Ming Lei]

On Mon, May 2, 2016 at 10:50 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, Apr 28, 2016 at 09:09:48AM +0800, Ming Lei wrote:
>> bio_endio() is the graceful way to complete one bio.
>
>
>
>>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  fs/direct-io.c | 7 +------
>>  1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/fs/direct-io.c b/fs/direct-io.c
>> index a8dd60a..0a35e51 100644
>> --- a/fs/direct-io.c
>> +++ b/fs/direct-io.c
>> @@ -350,15 +350,10 @@ static void dio_bio_end_io(struct bio *bio)
>>   */
>>  void dio_end_io(struct bio *bio, int error)
>>  {
>> -     struct dio *dio = bio->bi_private;
>> -
>>       if (!bio->bi_error)
>>               bio->bi_error = error;
>>
>> -     if (dio->is_async)
>> -             dio_bio_end_aio(bio);
>> -     else
>> -             dio_bio_end_io(bio);
>> +     bio_endio(bio);
>>  }
>>  EXPORT_SYMBOL_GPL(dio_end_io);
>
> dio_end_io is only used by btrfs.  So instead of this and the previous
> patch you should just open code the error assignment and call to
> bio_endio in btrfs.

OK.

Thanks,

[PATCH v3 3/3] block: avoid to call .bi_end_io() recursively

[Ming Lei]

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.

xfstests(-g auto) is run with this patch and no regression is
found on ext4, xfs and btrfs.

[1] http://marc.info/?t=121428502000004&r=1&w=2
[2] http://marc.info/?l=dm-devel&m=139595190620008&w=2
[3] http://marc.info/?t=145974644100001&r=1&w=2

Cc: Shaun Tancheff <shaun.tancheff@seagate.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Neil Brown <neilb@suse.de>
Cc: Liu Bo <bo.li.liu@oracle.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 block/bio.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 807d25e..e20a83c 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -68,6 +68,8 @@ static DEFINE_MUTEX(bio_slab_lock);
 static struct bio_slab *bio_slabs;
 static unsigned int bio_slab_nr, bio_slab_max;
 
+static DEFINE_PER_CPU(struct bio_list *, bio_end_list) = { NULL };
+
 static struct kmem_cache *bio_find_or_create_slab(unsigned int extra_size)
 {
 	unsigned int sz = sizeof(struct bio) + extra_size;
@@ -1737,6 +1739,56 @@ static inline bool bio_remaining_done(struct bio *bio)
 	return false;
 }
 
+static void __bio_endio(struct bio *bio)
+{
+	if (bio->bi_end_io)
+		bio->bi_end_io(bio);
+}
+
+/* disable local irq when manipulating the percpu bio_list */
+static void unwind_bio_endio(struct bio *bio)
+{
+	struct bio_list *bl;
+	unsigned long flags;
+
+	/*
+	 * We can't optimize if bi_endio() is scheduled to run from
+	 * process context because there isn't easy way to get a
+	 * per-task bio list head or allocate a per-task variable.
+	 */
+	if (!in_interrupt()) {
+		/*
+		 * It has to be a top calling when it is run from
+		 * process context.
+		 */
+		WARN_ON(this_cpu_read(bio_end_list));
+		__bio_endio(bio);
+		return;
+	}
+
+	local_irq_save(flags);
+	bl = __this_cpu_read(bio_end_list);
+	if (bl) {
+		bio_list_add(bl, bio);
+	} else {
+		struct bio_list bl_in_stack;
+
+		bl = &bl_in_stack;
+		bio_list_init(bl);
+
+		__this_cpu_write(bio_end_list, bl);
+		while (bio) {
+			local_irq_restore(flags);
+			__bio_endio(bio);
+			local_irq_save(flags);
+
+			bio = bio_list_pop(bl);
+		}
+		__this_cpu_write(bio_end_list, NULL);
+	}
+	local_irq_restore(flags);
+}
+
 /**
  * bio_endio - end I/O on a bio
  * @bio:	bio
@@ -1765,8 +1817,7 @@ again:
 		goto again;
 	}
 
-	if (bio->bi_end_io)
-		bio->bi_end_io(bio);
+	unwind_bio_endio(bio);
 }
 EXPORT_SYMBOL(bio_endio);
 
-- 
1.9.1

Re: [PATCH v3 3/3] block: avoid to call .bi_end_io() recursively

[Mikulas Patocka]

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.

Mikulas

> xfstests(-g auto) is run with this patch and no regression is
> found on ext4, xfs and btrfs.
> 
> [1] http://marc.info/?t=121428502000004&r=1&w=2
> [2] http://marc.info/?l=dm-devel&m=139595190620008&w=2
> [3] http://marc.info/?t=145974644100001&r=1&w=2
> 
> Cc: Shaun Tancheff <shaun.tancheff@seagate.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Mikulas Patocka <mpatocka@redhat.com>
> Cc: Alan Cox <alan@linux.intel.com>
> Cc: Neil Brown <neilb@suse.de>
> Cc: Liu Bo <bo.li.liu@oracle.com>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  block/bio.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/block/bio.c b/block/bio.c
> index 807d25e..e20a83c 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -68,6 +68,8 @@ static DEFINE_MUTEX(bio_slab_lock);
>  static struct bio_slab *bio_slabs;
>  static unsigned int bio_slab_nr, bio_slab_max;
>  
> +static DEFINE_PER_CPU(struct bio_list *, bio_end_list) = { NULL };
> +
>  static struct kmem_cache *bio_find_or_create_slab(unsigned int extra_size)
>  {
>  	unsigned int sz = sizeof(struct bio) + extra_size;
> @@ -1737,6 +1739,56 @@ static inline bool bio_remaining_done(struct bio *bio)
>  	return false;
>  }
>  
> +static void __bio_endio(struct bio *bio)
> +{
> +	if (bio->bi_end_io)
> +		bio->bi_end_io(bio);
> +}
> +
> +/* disable local irq when manipulating the percpu bio_list */
> +static void unwind_bio_endio(struct bio *bio)
> +{
> +	struct bio_list *bl;
> +	unsigned long flags;
> +
> +	/*
> +	 * We can't optimize if bi_endio() is scheduled to run from
> +	 * process context because there isn't easy way to get a
> +	 * per-task bio list head or allocate a per-task variable.
> +	 */
> +	if (!in_interrupt()) {
> +		/*
> +		 * It has to be a top calling when it is run from
> +		 * process context.
> +		 */
> +		WARN_ON(this_cpu_read(bio_end_list));
> +		__bio_endio(bio);
> +		return;
> +	}
> +
> +	local_irq_save(flags);
> +	bl = __this_cpu_read(bio_end_list);
> +	if (bl) {
> +		bio_list_add(bl, bio);
> +	} else {
> +		struct bio_list bl_in_stack;
> +
> +		bl = &bl_in_stack;
> +		bio_list_init(bl);
> +
> +		__this_cpu_write(bio_end_list, bl);
> +		while (bio) {
> +			local_irq_restore(flags);
> +			__bio_endio(bio);
> +			local_irq_save(flags);
> +
> +			bio = bio_list_pop(bl);
> +		}
> +		__this_cpu_write(bio_end_list, NULL);
> +	}
> +	local_irq_restore(flags);
> +}
> +
>  /**
>   * bio_endio - end I/O on a bio
>   * @bio:	bio
> @@ -1765,8 +1817,7 @@ again:
>  		goto again;
>  	}
>  
> -	if (bio->bi_end_io)
> -		bio->bi_end_io(bio);
> +	unwind_bio_endio(bio);
>  }
>  EXPORT_SYMBOL(bio_endio);
>  
> -- 
> 1.9.1
> 

Re: [PATCH v3 3/3] block: avoid to call .bi_end_io() recursively

[Ming Lei]

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.

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
>
>> xfstests(-g auto) is run with this patch and no regression is
>> found on ext4, xfs and btrfs.
>>
>> [1] http://marc.info/?t=121428502000004&r=1&w=2
>> [2] http://marc.info/?l=dm-devel&m=139595190620008&w=2
>> [3] http://marc.info/?t=145974644100001&r=1&w=2
>>
>> Cc: Shaun Tancheff <shaun.tancheff@seagate.com>
>> Cc: Christoph Hellwig <hch@infradead.org>
>> Cc: Mikulas Patocka <mpatocka@redhat.com>
>> Cc: Alan Cox <alan@linux.intel.com>
>> Cc: Neil Brown <neilb@suse.de>
>> Cc: Liu Bo <bo.li.liu@oracle.com>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  block/bio.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 53 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/bio.c b/block/bio.c
>> index 807d25e..e20a83c 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -68,6 +68,8 @@ static DEFINE_MUTEX(bio_slab_lock);
>>  static struct bio_slab *bio_slabs;
>>  static unsigned int bio_slab_nr, bio_slab_max;
>>
>> +static DEFINE_PER_CPU(struct bio_list *, bio_end_list) = { NULL };
>> +
>>  static struct kmem_cache *bio_find_or_create_slab(unsigned int extra_size)
>>  {
>>       unsigned int sz = sizeof(struct bio) + extra_size;
>> @@ -1737,6 +1739,56 @@ static inline bool bio_remaining_done(struct bio *bio)
>>       return false;
>>  }
>>
>> +static void __bio_endio(struct bio *bio)
>> +{
>> +     if (bio->bi_end_io)
>> +             bio->bi_end_io(bio);
>> +}
>> +
>> +/* disable local irq when manipulating the percpu bio_list */
>> +static void unwind_bio_endio(struct bio *bio)
>> +{
>> +     struct bio_list *bl;
>> +     unsigned long flags;
>> +
>> +     /*
>> +      * We can't optimize if bi_endio() is scheduled to run from
>> +      * process context because there isn't easy way to get a
>> +      * per-task bio list head or allocate a per-task variable.
>> +      */
>> +     if (!in_interrupt()) {
>> +             /*
>> +              * It has to be a top calling when it is run from
>> +              * process context.
>> +              */
>> +             WARN_ON(this_cpu_read(bio_end_list));
>> +             __bio_endio(bio);
>> +             return;
>> +     }
>> +
>> +     local_irq_save(flags);
>> +     bl = __this_cpu_read(bio_end_list);
>> +     if (bl) {
>> +             bio_list_add(bl, bio);
>> +     } else {
>> +             struct bio_list bl_in_stack;
>> +
>> +             bl = &bl_in_stack;
>> +             bio_list_init(bl);
>> +
>> +             __this_cpu_write(bio_end_list, bl);
>> +             while (bio) {
>> +                     local_irq_restore(flags);
>> +                     __bio_endio(bio);
>> +                     local_irq_save(flags);
>> +
>> +                     bio = bio_list_pop(bl);
>> +             }
>> +             __this_cpu_write(bio_end_list, NULL);
>> +     }
>> +     local_irq_restore(flags);
>> +}
>> +
>>  /**
>>   * bio_endio - end I/O on a bio
>>   * @bio:     bio
>> @@ -1765,8 +1817,7 @@ again:
>>               goto again;
>>       }
>>
>> -     if (bio->bi_end_io)
>> -             bio->bi_end_io(bio);
>> +     unwind_bio_endio(bio);
>>  }
>>  EXPORT_SYMBOL(bio_endio);
>>
>> --
>> 1.9.1
>>
> --
> 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

Re: [PATCH v3 3/3] block: avoid to call .bi_end_io() recursively

[Mikulas Patocka]

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.

> 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

Re: [PATCH v3 3/3] block: avoid to call .bi_end_io() recursively

[Ming Lei]

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

Re: [PATCH v3 3/3] block: avoid to call .bi_end_io() recursively

[Mikulas Patocka]

On Fri, 29 Apr 2016, Ming Lei wrote:

> 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?

Some device mapper (and other) drivers use a worker thread to process 
bios. So the bio may be finished from the worker thread. It would be 
advantageous to prevent stack overflow even in this case.

> And you can find that btrfs_subio_endio_read() does sleep for checksum stuff.

I'm not an expert on btrfs. What happens if it is called from an 
interrupt? Do you have an actual stracktrace when this function is called 
from bio_endio and when it sleeps?

> Thanks,
> Ming

Mikulas

Re: [PATCH v3 3/3] block: avoid to call .bi_end_io() recursively

[Ming Lei]

On Fri, Apr 29, 2016 at 12:59 AM, Mikulas Patocka <mpatocka@redhat.com> wrote:
>
>
> On Fri, 29 Apr 2016, Ming Lei wrote:
>
>> 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?
>
> Some device mapper (and other) drivers use a worker thread to process
> bios. So the bio may be finished from the worker thread. It would be
> advantageous to prevent stack overflow even in this case.

If the .bi_end_io wouldn't sleep, it can be put back into interrupt context
for the sake of performance when this patch is merged. The cost of context
switch in high IOPS case isn't cheap.

It isn't easy to avoid the recursive calling in process context except you
can add something 'task_struct' or introduce 'alloca()' in kernel. Or do you
have better ideas?

>
>> And you can find that btrfs_subio_endio_read() does sleep for checksum stuff.
>
> I'm not an expert on btrfs. What happens if it is called from an
> interrupt? Do you have an actual stracktrace when this function is called

What do you expect if sleepable function is called in softirq or
hardirq handler? :-)

> from bio_endio and when it sleeps?

The problem is observed in xfstests generic/323 by this patch v1. Sometimes the
test hangs, and sometimes kernel oops is triggered. and the issue is
fixed by introducing 'if (!in_interrupt())' block for handling running
.bi_end_io() from
process context.

If the block of 'if (!in_interrupt())' is removed and
preempt_disable()/preempt_enable() is added around bio->bi_end_io(),
the following kernel warning can be seen easily:

[   51.086303] BUG: sleeping function called from invalid context at
mm/slab.h:388
[   51.087442] in_atomic(): 1, irqs_disabled(): 0, pid: 633, name: kworker/u8:4
[   51.088575] CPU: 3 PID: 633 Comm: kworker/u8:4 Not tainted 4.6.0-rc3+ #2017
[   51.088578] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
BIOS rel-1.9.0-0-g01a84be-prebuilt.qemu-project.org 04/01/2014
[   51.088637] Workqueue: btrfs-endio btrfs_endio_helper [btrfs]
[   51.088640]  0000000000000000 ffff88007bbebc00 ffffffff813d92d3
ffff88007ba6ce00
[   51.088643]  0000000000000184 ffff88007bbebc18 ffffffff810a38bb
ffffffff81a35310
[   51.088645]  ffff88007bbebc40 ffffffff810a3949 0000000002400040
0000000002400040
[   51.088648] Call Trace:
[   51.088651]  [<ffffffff813d92d3>] dump_stack+0x63/0x90
[   51.088655]  [<ffffffff810a38bb>] ___might_sleep+0xdb/0x120
[   51.088657]  [<ffffffff810a3949>] __might_sleep+0x49/0x80
[   51.088659]  [<ffffffff811e98e7>] kmem_cache_alloc+0x1a7/0x210
[   51.088670]  [<ffffffffc0102741>] ? alloc_extent_state+0x21/0xe0 [btrfs]
[   51.088680]  [<ffffffffc0102741>] alloc_extent_state+0x21/0xe0 [btrfs]
[   51.088689]  [<ffffffffc01046ce>] __clear_extent_bit+0x2ae/0x3d0 [btrfs]
[   51.088698]  [<ffffffffc0104e6a>] clear_extent_bit+0x2a/0x30 [btrfs]
[   51.088708]  [<ffffffffc00e96d0>] btrfs_endio_direct_read+0x70/0xf0 [btrfs]
[   51.088711]  [<ffffffff8139f1e7>] bio_endio+0xf7/0x140
[   51.088718]  [<ffffffffc00dbb9c>] end_workqueue_fn+0x3c/0x40 [btrfs]
[   51.088728]  [<ffffffffc01184d7>] normal_work_helper+0xc7/0x310 [btrfs]
[   51.088737]  [<ffffffffc01187f2>] btrfs_endio_helper+0x12/0x20 [btrfs]
[   51.088740]  [<ffffffff81097b77>] process_one_work+0x157/0x420
[   51.088742]  [<ffffffff810985ab>] worker_thread+0x12b/0x4d0
[   51.088744]  [<ffffffff817171a8>] ? __schedule+0x368/0x950
[   51.088746]  [<ffffffff81098480>] ? rescuer_thread+0x380/0x380
[   51.088748]  [<ffffffff8109de84>] kthread+0xd4/0xf0
[   51.088750]  [<ffffffff8171b7a2>] ret_from_fork+0x22/0x40
[   51.088752]  [<ffffffff8109ddb0>] ? kthread_park+0x60/0x60


Not mention wait_for_completion() in both __btrfs_correct_data_nocsum()
and __btrfs_subio_endio_read(), which are called by btrfs_subio_endio_read()
in btrfs_endio_direct_read().


Thanks,
Ming

>
>> 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

Re: [PATCH v3 3/3] block: avoid to call .bi_end_io() recursively

[Mikulas Patocka]

On Fri, 29 Apr 2016, Ming Lei wrote:

> On Fri, Apr 29, 2016 at 12:59 AM, Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> >
> > On Fri, 29 Apr 2016, Ming Lei wrote:
> >
> >> 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?
> >
> > Some device mapper (and other) drivers use a worker thread to process
> > bios. So the bio may be finished from the worker thread. It would be
> > advantageous to prevent stack overflow even in this case.
> 
> If the .bi_end_io wouldn't sleep, it can be put back into interrupt context
> for the sake of performance when this patch is merged. The cost of context
> switch in high IOPS case isn't cheap.

If some block device driver in a process context finds out that it needs 
to terminate a bio, it calls bio_endio in a process context. Why would it 
need to trigger an interrupt just to call bio_endio? (and how could it 
trigger an interrupt if that driver perhaps doesn't use interrupts at 
all?) I don't know what are you trying to suggest.

> It isn't easy to avoid the recursive calling in process context except you
> can add something 'task_struct' or introduce 'alloca()' in kernel. Or do you
> have better ideas?

preempt_disable around the bi_endio callback should be sufficient.

> >
> >> And you can find that btrfs_subio_endio_read() does sleep for checksum stuff.
> >
> > I'm not an expert on btrfs. What happens if it is called from an
> > interrupt? Do you have an actual stracktrace when this function is called
> 
> What do you expect if sleepable function is called in softirq or
> hardirq handler? :-)
> 
> > from bio_endio and when it sleeps?
> 
> The problem is observed in xfstests generic/323 by this patch v1. Sometimes the
> test hangs, and sometimes kernel oops is triggered. and the issue is
> fixed by introducing 'if (!in_interrupt())' block for handling running
> .bi_end_io() from
> process context.
> 
> If the block of 'if (!in_interrupt())' is removed and
> preempt_disable()/preempt_enable() is added around bio->bi_end_io(),
> the following kernel warning can be seen easily:
> 
> [   51.086303] BUG: sleeping function called from invalid context at
> mm/slab.h:388
> [   51.087442] in_atomic(): 1, irqs_disabled(): 0, pid: 633, name: kworker/u8:4
> [   51.088575] CPU: 3 PID: 633 Comm: kworker/u8:4 Not tainted 4.6.0-rc3+ #2017
> [   51.088578] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
> BIOS rel-1.9.0-0-g01a84be-prebuilt.qemu-project.org 04/01/2014
> [   51.088637] Workqueue: btrfs-endio btrfs_endio_helper [btrfs]
> [   51.088640]  0000000000000000 ffff88007bbebc00 ffffffff813d92d3
> ffff88007ba6ce00
> [   51.088643]  0000000000000184 ffff88007bbebc18 ffffffff810a38bb
> ffffffff81a35310
> [   51.088645]  ffff88007bbebc40 ffffffff810a3949 0000000002400040
> 0000000002400040
> [   51.088648] Call Trace:
> [   51.088651]  [<ffffffff813d92d3>] dump_stack+0x63/0x90
> [   51.088655]  [<ffffffff810a38bb>] ___might_sleep+0xdb/0x120
> [   51.088657]  [<ffffffff810a3949>] __might_sleep+0x49/0x80
> [   51.088659]  [<ffffffff811e98e7>] kmem_cache_alloc+0x1a7/0x210
> [   51.088670]  [<ffffffffc0102741>] ? alloc_extent_state+0x21/0xe0 [btrfs]
> [   51.088680]  [<ffffffffc0102741>] alloc_extent_state+0x21/0xe0 [btrfs]
> [   51.088689]  [<ffffffffc01046ce>] __clear_extent_bit+0x2ae/0x3d0 [btrfs]
> [   51.088698]  [<ffffffffc0104e6a>] clear_extent_bit+0x2a/0x30 [btrfs]
> [   51.088708]  [<ffffffffc00e96d0>] btrfs_endio_direct_read+0x70/0xf0 [btrfs]
> [   51.088711]  [<ffffffff8139f1e7>] bio_endio+0xf7/0x140
> [   51.088718]  [<ffffffffc00dbb9c>] end_workqueue_fn+0x3c/0x40 [btrfs]
> [   51.088728]  [<ffffffffc01184d7>] normal_work_helper+0xc7/0x310 [btrfs]
> [   51.088737]  [<ffffffffc01187f2>] btrfs_endio_helper+0x12/0x20 [btrfs]
> [   51.088740]  [<ffffffff81097b77>] process_one_work+0x157/0x420
> [   51.088742]  [<ffffffff810985ab>] worker_thread+0x12b/0x4d0
> [   51.088744]  [<ffffffff817171a8>] ? __schedule+0x368/0x950
> [   51.088746]  [<ffffffff81098480>] ? rescuer_thread+0x380/0x380
> [   51.088748]  [<ffffffff8109de84>] kthread+0xd4/0xf0
> [   51.088750]  [<ffffffff8171b7a2>] ret_from_fork+0x22/0x40
> [   51.088752]  [<ffffffff8109ddb0>] ? kthread_park+0x60/0x60
> 
> 
> Not mention wait_for_completion() in both __btrfs_correct_data_nocsum()
> and __btrfs_subio_endio_read(), which are called by btrfs_subio_endio_read()
> in btrfs_endio_direct_read().

btrfs is calling bio_endio on bio that it created. So, bio_endio in btrfs 
can be replaced with:
if (bio->bi_end_io == btrfs_endio_direct_read)
	btrfs_endio_direct_read(bio);
else
	bio_endio(bio);

... or just maybe just with this:
bio->bi_end_io(bio);

This could be fixed easily.

Mikulas

> Thanks,
> Ming
> 
> >
> >> 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
> 

Re: [PATCH v3 3/3] block: avoid to call .bi_end_io() recursively

[Ming Lei]

On Fri, Apr 29, 2016 at 4:39 PM, Mikulas Patocka <mpatocka@redhat.com> wrote:
>
>
> On Fri, 29 Apr 2016, Ming Lei wrote:
>
>> On Fri, Apr 29, 2016 at 12:59 AM, Mikulas Patocka <mpatocka@redhat.com> wrote:
>> >
>> >
>> > On Fri, 29 Apr 2016, Ming Lei wrote:
>> >
>> >> 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?
>> >
>> > Some device mapper (and other) drivers use a worker thread to process
>> > bios. So the bio may be finished from the worker thread. It would be
>> > advantageous to prevent stack overflow even in this case.
>>
>> If the .bi_end_io wouldn't sleep, it can be put back into interrupt context
>> for the sake of performance when this patch is merged. The cost of context
>> switch in high IOPS case isn't cheap.
>
> If some block device driver in a process context finds out that it needs
> to terminate a bio, it calls bio_endio in a process context. Why would it
> need to trigger an interrupt just to call bio_endio? (and how could it
> trigger an interrupt if that driver perhaps doesn't use interrupts at
> all?) I don't know what are you trying to suggest.

That should be the failure path, and this patch just aims at the normal bio
complete path which is run at 99.999% time. And irq handler often
borrows current process's stack and cause huge stack uses.

>
>> It isn't easy to avoid the recursive calling in process context except you
>> can add something 'task_struct' or introduce 'alloca()' in kernel. Or do you
>> have better ideas?
>
> preempt_disable around the bi_endio callback should be sufficient.

How can a sleepable function be run in preempt disabled context?

Looks you still don't believe the bi_endio scheduled to process context
can sleep, do you?

>
>> >
>> >> And you can find that btrfs_subio_endio_read() does sleep for checksum stuff.
>> >
>> > I'm not an expert on btrfs. What happens if it is called from an
>> > interrupt? Do you have an actual stracktrace when this function is called
>>
>> What do you expect if sleepable function is called in softirq or
>> hardirq handler? :-)
>>
>> > from bio_endio and when it sleeps?
>>
>> The problem is observed in xfstests generic/323 by this patch v1. Sometimes the
>> test hangs, and sometimes kernel oops is triggered. and the issue is
>> fixed by introducing 'if (!in_interrupt())' block for handling running
>> .bi_end_io() from
>> process context.
>>
>> If the block of 'if (!in_interrupt())' is removed and
>> preempt_disable()/preempt_enable() is added around bio->bi_end_io(),
>> the following kernel warning can be seen easily:
>>
>> [   51.086303] BUG: sleeping function called from invalid context at
>> mm/slab.h:388
>> [   51.087442] in_atomic(): 1, irqs_disabled(): 0, pid: 633, name: kworker/u8:4
>> [   51.088575] CPU: 3 PID: 633 Comm: kworker/u8:4 Not tainted 4.6.0-rc3+ #2017
>> [   51.088578] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
>> BIOS rel-1.9.0-0-g01a84be-prebuilt.qemu-project.org 04/01/2014
>> [   51.088637] Workqueue: btrfs-endio btrfs_endio_helper [btrfs]
>> [   51.088640]  0000000000000000 ffff88007bbebc00 ffffffff813d92d3
>> ffff88007ba6ce00
>> [   51.088643]  0000000000000184 ffff88007bbebc18 ffffffff810a38bb
>> ffffffff81a35310
>> [   51.088645]  ffff88007bbebc40 ffffffff810a3949 0000000002400040
>> 0000000002400040
>> [   51.088648] Call Trace:
>> [   51.088651]  [<ffffffff813d92d3>] dump_stack+0x63/0x90
>> [   51.088655]  [<ffffffff810a38bb>] ___might_sleep+0xdb/0x120
>> [   51.088657]  [<ffffffff810a3949>] __might_sleep+0x49/0x80
>> [   51.088659]  [<ffffffff811e98e7>] kmem_cache_alloc+0x1a7/0x210
>> [   51.088670]  [<ffffffffc0102741>] ? alloc_extent_state+0x21/0xe0 [btrfs]
>> [   51.088680]  [<ffffffffc0102741>] alloc_extent_state+0x21/0xe0 [btrfs]
>> [   51.088689]  [<ffffffffc01046ce>] __clear_extent_bit+0x2ae/0x3d0 [btrfs]
>> [   51.088698]  [<ffffffffc0104e6a>] clear_extent_bit+0x2a/0x30 [btrfs]
>> [   51.088708]  [<ffffffffc00e96d0>] btrfs_endio_direct_read+0x70/0xf0 [btrfs]
>> [   51.088711]  [<ffffffff8139f1e7>] bio_endio+0xf7/0x140
>> [   51.088718]  [<ffffffffc00dbb9c>] end_workqueue_fn+0x3c/0x40 [btrfs]
>> [   51.088728]  [<ffffffffc01184d7>] normal_work_helper+0xc7/0x310 [btrfs]
>> [   51.088737]  [<ffffffffc01187f2>] btrfs_endio_helper+0x12/0x20 [btrfs]
>> [   51.088740]  [<ffffffff81097b77>] process_one_work+0x157/0x420
>> [   51.088742]  [<ffffffff810985ab>] worker_thread+0x12b/0x4d0
>> [   51.088744]  [<ffffffff817171a8>] ? __schedule+0x368/0x950
>> [   51.088746]  [<ffffffff81098480>] ? rescuer_thread+0x380/0x380
>> [   51.088748]  [<ffffffff8109de84>] kthread+0xd4/0xf0
>> [   51.088750]  [<ffffffff8171b7a2>] ret_from_fork+0x22/0x40
>> [   51.088752]  [<ffffffff8109ddb0>] ? kthread_park+0x60/0x60
>>
>>
>> Not mention wait_for_completion() in both __btrfs_correct_data_nocsum()
>> and __btrfs_subio_endio_read(), which are called by btrfs_subio_endio_read()
>> in btrfs_endio_direct_read().
>
> btrfs is calling bio_endio on bio that it created. So, bio_endio in btrfs
> can be replaced with:
> if (bio->bi_end_io == btrfs_endio_direct_read)
>         btrfs_endio_direct_read(bio);
> else
>         bio_endio(bio);
>
> ... or just maybe just with this:
> bio->bi_end_io(bio);

It is really ugly and should be avoided most of times.

>
> This could be fixed easily.

Suppose it might be done in this way, and there may be other .bi_end_io
which can sleep too, you need to make sure there isn't any such usage
first before running it with preempt disabled.

Anyway looks you worry about the seldom-run failure path, which isn't
addressed by this patch. And you can optimize that in future and that isn't
contradictory with this patch.

Thanks,
Ming

>
> Mikulas
>
>> Thanks,
>> Ming
>>
>> >
>> >> 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
>>
> --
> 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

Re: [PATCH v3 3/3] block: avoid to call .bi_end_io() recursively

[Mikulas Patocka]

On Fri, 29 Apr 2016, Ming Lei wrote:

> > If some block device driver in a process context finds out that it needs
> > to terminate a bio, it calls bio_endio in a process context. Why would it
> > need to trigger an interrupt just to call bio_endio? (and how could it
> > trigger an interrupt if that driver perhaps doesn't use interrupts at
> > all?) I don't know what are you trying to suggest.
> 
> That should be the failure path, and this patch just aims at the normal bio
> complete path which is run at 99.999% time. And irq handler often
> borrows current process's stack and cause huge stack uses.

For some block devices it is common to call bi_endio from process context, 
for example dm-crypt or dm-verity do it for all read bios. loop driver 
calls bi_endio from process context on every bio it processes.

> >> It isn't easy to avoid the recursive calling in process context except you
> >> can add something 'task_struct' or introduce 'alloca()' in kernel. Or do you
> >> have better ideas?
> >
> > preempt_disable around the bi_endio callback should be sufficient.
> 
> How can a sleepable function be run in preempt disabled context?
>
> Looks you still don't believe the bi_endio scheduled to process context
> can sleep, do you?

I don't believe bi_endio function can sleep. bi_endio function doesn't 
know if it will be called from process or interrupt context - so it can't 
sleep. If bi_endio function slept, bug would happen if it were called from 
interrupt context.

btrfs is a special case where btrfs calls bio_endio on bios that it 
created on it own (note that this trick only works when the same driver 
creates a bio and terminates the bio with bio_endio - if some other driver 
terminated the bio, it wouldn't work because the other driver could 
terminate the bio from interrupt context).

That could be simply fixed by calling bio->bi_end_io or 
btrfs_endio_direct_read directly or making a new function btrfs_endio.

> >> Not mention wait_for_completion() in both __btrfs_correct_data_nocsum()
> >> and __btrfs_subio_endio_read(), which are called by btrfs_subio_endio_read()
> >> in btrfs_endio_direct_read().
> >
> > btrfs is calling bio_endio on bio that it created. So, bio_endio in btrfs
> > can be replaced with:
> > if (bio->bi_end_io == btrfs_endio_direct_read)
> >         btrfs_endio_direct_read(bio);
> > else
> >         bio_endio(bio);
> >
> > ... or just maybe just with this:
> > bio->bi_end_io(bio);
> 
> It is really ugly and should be avoided most of times.
> 
> >
> > This could be fixed easily.
> 
> Suppose it might be done in this way, and there may be other .bi_end_io
> which can sleep too, you need to make sure there isn't any such usage
> first before running it with preempt disabled.
> 
> Anyway looks you worry about the seldom-run failure path, which isn't
> addressed by this patch. And you can optimize that in future and that isn't
> contradictory with this patch.

Worrying about seldom-run failrure paths is nothing wrong. These paths are 
not any less important than the common code paths.

Mikulas

> Thanks,
> Ming

Re: [PATCH v3 3/3] block: avoid to call .bi_end_io() recursively

[Jens Axboe]

On 04/27/2016 07:09 PM, 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.
>
> xfstests(-g auto) is run with this patch and no regression is
> found on ext4, xfs and btrfs.

I don't like this at all, it's going about fixing this the completely 
wrong way. For the fast path, single device, we don't have an issue. Yet 
this patch slows down that part unnecessarily.

Fix this for the fat dm/md stacks, and don't add unwarranted fat in the 
normal fast path. The dm/md stacks won't notice a bit of extra overhead, 
whereas the core path is pretty well optimized.

-- 
Jens Axboe

Re: [PATCH v3 0/3] block: avoid to call .bi_end_io() recursively

[Christoph Hellwig]

On Thu, Apr 28, 2016 at 09:09:00AM +0800, Ming Lei wrote:
> The 3rd patch avoids to call .bi_end_io recursively in complete path.

Care to explain what the reason for doing that is?

If we have excessively long bi_end_io chains we should probably look
into fixing them instead.