All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/3] block: avoid to call .bi_end_io() recursively
@ 2016-04-27  3:50 Ming Lei
  2016-04-27  3:51   ` Ming Lei
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Ming Lei @ 2016-04-27  3:50 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel
  Cc: linux-block, Christoph Hellwig, linux-btrfs, Ming Lei,
	open list:FILESYSTEMS (VFS and infrastructure)

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.

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    | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/direct-io.c |  8 +++-----
 2 files changed, 58 insertions(+), 7 deletions(-)

-- 
1.9.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/3] fs: direct-io: handle error in dio_end_io()
@ 2016-04-27  3:51   ` Ming Lei
  0 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2016-04-27  3:51 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel
  Cc: linux-block, Christoph Hellwig, linux-btrfs, Ming Lei,
	Alexander Viro, open list:FILESYSTEMS (VFS and infrastructure)

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 1/3] fs: direct-io: handle error in dio_end_io()
@ 2016-04-27  3:51   ` Ming Lei
  0 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2016-04-27  3:51 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel
  Cc: linux-block, Christoph Hellwig, linux-btrfs, Ming Lei,
	Alexander Viro, open list:FILESYSTEMS (VFS and infrastructure)

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/3] fs: direct-io: call .bi_end_io via bio_endio()
@ 2016-04-27  3:51   ` Ming Lei
  0 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2016-04-27  3:51 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel
  Cc: linux-block, Christoph Hellwig, linux-btrfs, Ming Lei,
	Alexander Viro, open list:FILESYSTEMS (VFS and infrastructure)

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/3] fs: direct-io: call .bi_end_io via bio_endio()
@ 2016-04-27  3:51   ` Ming Lei
  0 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2016-04-27  3:51 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel
  Cc: linux-block, Christoph Hellwig, linux-btrfs, Ming Lei,
	Alexander Viro, open list:FILESYSTEMS (VFS and infrastructure)

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 3/3] block: avoid to call .bi_end_io() recursively
@ 2016-04-27  3:51   ` Ming Lei
  0 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2016-04-27  3:51 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel
  Cc: linux-block, Christoph Hellwig, linux-btrfs, Ming Lei,
	Shaun Tancheff, Mikulas Patocka, Alan Cox, Neil Brown, Liu Bo,
	Jens Axboe

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 | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 807d25e..6b4ca7b 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,58 @@ 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) {
+		struct bio_list bl_in_stack;
+
+		bl = &bl_in_stack;
+		bio_list_init(bl);
+		__this_cpu_write(bio_end_list, bl);
+	} else {
+		bio_list_add(bl, bio);
+		goto out;
+	}
+
+	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);
+ out:
+	local_irq_restore(flags);
+}
+
 /**
  * bio_endio - end I/O on a bio
  * @bio:	bio
@@ -1765,8 +1819,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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 3/3] block: avoid to call .bi_end_io() recursively
@ 2016-04-27  3:51   ` Ming Lei
  0 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2016-04-27  3:51 UTC (permalink / raw)
  To: Jens Axboe, linux-kernel
  Cc: linux-block, Christoph Hellwig, linux-btrfs, Ming Lei,
	Shaun Tancheff, Mikulas Patocka, Alan Cox, Neil Brown, Liu Bo,
	Jens Axboe

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 | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 807d25e..6b4ca7b 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,58 @@ 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) {
+		struct bio_list bl_in_stack;
+
+		bl = &bl_in_stack;
+		bio_list_init(bl);
+		__this_cpu_write(bio_end_list, bl);
+	} else {
+		bio_list_add(bl, bio);
+		goto out;
+	}
+
+	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);
+ out:
+	local_irq_restore(flags);
+}
+
 /**
  * bio_endio - end I/O on a bio
  * @bio:	bio
@@ -1765,8 +1819,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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/3] block: avoid to call .bi_end_io() recursively
  2016-04-27  3:51   ` Ming Lei
  (?)
@ 2016-04-27  4:02   ` NeilBrown
  2016-04-27  4:05     ` Ming Lei
  -1 siblings, 1 reply; 9+ messages in thread
From: NeilBrown @ 2016-04-27  4:02 UTC (permalink / raw)
  To: Ming Lei, Jens Axboe, linux-kernel
  Cc: linux-block, Christoph Hellwig, linux-btrfs, Ming Lei,
	Shaun Tancheff, Mikulas Patocka, Alan Cox, Liu Bo, Jens Axboe

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

On Wed, Apr 27 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.
>
> 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 | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/block/bio.c b/block/bio.c
> index 807d25e..6b4ca7b 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,58 @@ 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) {
> +		struct bio_list bl_in_stack;
> +
> +		bl = &bl_in_stack;
> +		bio_list_init(bl);
> +		__this_cpu_write(bio_end_list, bl);

The patch seems to make sense, but this bit bothers me.
You are expecting bl_in_stack to still be usable after this block of
code completes.  While it probably is, I don't think it is a good idea
to depend on it.
If you move the "struct bio_list bl_in_stack" to the top of the function
I would be a lot happier.

Or you could change the code to:

   if (bl) {
       bio_list_add(bl, bio);
   } else {
       struct bio_list bl_in_stack;
       ... use bl_in_stack,
       while loop
       set bio_end_list to NULL
   }

and the code flow would all be must clearer.

Thanks,
NeilBrown

> +	} else {
> +		bio_list_add(bl, bio);
> +		goto out;
> +	}
> +
> +	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);
> + out:
> +	local_irq_restore(flags);
> +}
> +
>  /**
>   * bio_endio - end I/O on a bio
>   * @bio:	bio
> @@ -1765,8 +1819,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-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/3] block: avoid to call .bi_end_io() recursively
  2016-04-27  4:02   ` NeilBrown
@ 2016-04-27  4:05     ` Ming Lei
  0 siblings, 0 replies; 9+ messages in thread
From: Ming Lei @ 2016-04-27  4:05 UTC (permalink / raw)
  To: NeilBrown
  Cc: Jens Axboe, Linux Kernel Mailing List, linux-block,
	Christoph Hellwig, Btrfs BTRFS, Shaun Tancheff, Mikulas Patocka,
	Alan Cox, Liu Bo, Jens Axboe

On Wed, Apr 27, 2016 at 12:02 PM, NeilBrown <neilb@suse.de> wrote:
> On Wed, Apr 27 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.
>>
>> 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 | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 55 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/bio.c b/block/bio.c
>> index 807d25e..6b4ca7b 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,58 @@ 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) {
>> +             struct bio_list bl_in_stack;
>> +
>> +             bl = &bl_in_stack;
>> +             bio_list_init(bl);
>> +             __this_cpu_write(bio_end_list, bl);
>
> The patch seems to make sense, but this bit bothers me.
> You are expecting bl_in_stack to still be usable after this block of
> code completes.  While it probably is, I don't think it is a good idea
> to depend on it.
> If you move the "struct bio_list bl_in_stack" to the top of the function
> I would be a lot happier.
>
> Or you could change the code to:
>
>    if (bl) {
>        bio_list_add(bl, bio);
>    } else {
>        struct bio_list bl_in_stack;
>        ... use bl_in_stack,
>        while loop
>        set bio_end_list to NULL
>    }
>
> and the code flow would all be must clearer.

Yeah, definitely, thanks for the point.

Thanks,

>
> Thanks,
> NeilBrown
>
>> +     } else {
>> +             bio_list_add(bl, bio);
>> +             goto out;
>> +     }
>> +
>> +     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);
>> + out:
>> +     local_irq_restore(flags);
>> +}
>> +
>>  /**
>>   * bio_endio - end I/O on a bio
>>   * @bio:     bio
>> @@ -1765,8 +1819,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-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-04-27  4:06 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-27  3:50 [PATCH v2 0/3] block: avoid to call .bi_end_io() recursively Ming Lei
2016-04-27  3:51 ` [PATCH v2 1/3] fs: direct-io: handle error in dio_end_io() Ming Lei
2016-04-27  3:51   ` Ming Lei
2016-04-27  3:51 ` [PATCH v2 2/3] fs: direct-io: call .bi_end_io via bio_endio() Ming Lei
2016-04-27  3:51   ` Ming Lei
2016-04-27  3:51 ` [PATCH v2 3/3] block: avoid to call .bi_end_io() recursively Ming Lei
2016-04-27  3:51   ` Ming Lei
2016-04-27  4:02   ` NeilBrown
2016-04-27  4:05     ` Ming Lei

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.