loop: Limit the number of requests in the bio list

PATCH Superseded Lukas Czerner 3 participants 7 messages Sep 27, 2012 – Oct 3, 2012 Block layer

Share & tools

Lukas Czerner

author

2012-09-27 17:33 UTC #0 lore raw

PATCH 1/1 loop: Limit the number of requests in the bio list

Currently there is not limitation of number of requests in the loop bio list. This can lead into some nasty situations when the caller spawns tons of bio requests taking huge amount of memory. This is even more obvious with discard where blkdev_issue_discard() will submit all bios for the range and wait for them to finish afterwards. On really big loop devices this can lead to OOM situation as reported by Dave Chinner.

With this patch we will wait in loop_make_request() if the number of bios in the loop bio list would exceed 'nr_requests' number of requests. We'll wake up the process as we process the bios form the list.

Signed-off-by Lukas Czerner <[email protected]>

Reported-by Dave Chinner <[email protected]>

---
 drivers/block/loop.c |   13 +++++++++++++
 include/linux/loop.h |    3 +++
 2 files changed, 16 insertions(+), 0 deletions(-)

drivers/block/loop.c +13 -0

index 3bba655..2af969c 100644
@@ -463,6 +463,7 @@ out:
  */
 static void loop_add_bio(struct loop_device *lo, struct bio *bio)
 {
+	lo->lo_bio_count++;
 	bio_list_add(&lo->lo_bio_list, bio);
 }

@@ -471,6 +472,7 @@ static void loop_add_bio(struct loop_device *lo, struct bio *bio)
  */
 static struct bio *loop_get_bio(struct loop_device *lo)
 {
+	lo->lo_bio_count--;
 	return bio_list_pop(&lo->lo_bio_list);
 }

@@ -489,6 +491,14 @@ static void loop_make_request(struct request_queue *q, struct bio *old_bio)
 		goto out;
 	if (unlikely(rw == WRITE && (lo->lo_flags & LO_FLAGS_READ_ONLY)))
 		goto out;
+	if (lo->lo_bio_count >= lo->lo_queue->nr_requests) {
+		spin_unlock_irq(&lo->lo_lock);
+
+		wait_event_interruptible(lo->lo_req_wait,
+				lo->lo_bio_count < lo->lo_queue->nr_requests);
+
+		spin_lock_irq(&lo->lo_lock);
+	}
 	loop_add_bio(lo, old_bio);
 	wake_up(&lo->lo_event);
 	spin_unlock_irq(&lo->lo_lock);

@@ -546,6 +556,7 @@ static int loop_thread(void *data)
 			continue;
 		spin_lock_irq(&lo->lo_lock);
 		bio = loop_get_bio(lo);
+		wake_up(&lo->lo_req_wait);
 		spin_unlock_irq(&lo->lo_lock);
 
 		BUG_ON(!bio);

@@ -873,6 +884,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	lo->transfer = transfer_none;
 	lo->ioctl = NULL;
 	lo->lo_sizelimit = 0;
+	lo->lo_bio_count = 0;
 	lo->old_gfp_mask = mapping_gfp_mask(mapping);
 	mapping_set_gfp_mask(mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));

@@ -1660,6 +1672,7 @@ static int loop_add(struct loop_device **l, int i)
 	lo->lo_number		= i;
 	lo->lo_thread		= NULL;
 	init_waitqueue_head(&lo->lo_event);
+	init_waitqueue_head(&lo->lo_req_wait);
 	spin_lock_init(&lo->lo_lock);
 	disk->major		= LOOP_MAJOR;
 	disk->first_minor	= i << part_shift;

include/linux/loop.h +3 -0

index 11a41a8..e455d84 100644
@@ -57,10 +57,13 @@ struct loop_device {
 
 	spinlock_t		lo_lock;
 	struct bio_list		lo_bio_list;
+	unsigned int		lo_bio_count;
 	int			lo_state;
 	struct mutex		lo_ctl_mutex;
 	struct task_struct	*lo_thread;
 	wait_queue_head_t	lo_event;
+	/* wait queue for incoming requests */
+	wait_queue_head_t	lo_req_wait;
 
 	struct request_queue	*lo_queue;
 	struct gendisk		*lo_disk;

Signature · 1 line

1.7.7.6

Jeff Moyer

2012-10-01 16:52 UTC #1 lore raw

Quoted 10 lines · Lukas Czerner

Currently there is not limitation of number of requests in the loop bio list. This can lead into some nasty situations when the caller spawns tons of bio requests taking huge amount of memory. This is even more obvious with discard where blkdev_issue_discard() will submit all bios for the range and wait for them to finish afterwards. On really big loop devices this can lead to OOM situation as reported by Dave Chinner.
With this patch we will wait in loop_make_request() if the number of bios in the loop bio list would exceed 'nr_requests' number of requests. We'll wake up the process as we process the bios form the list.

I think you might want to do something similar to what is done for request_queues by implementing a congestion on and off threshold. As Jens writes in this commit (predating the conversion to git):

Author: Jens Axboe <[email protected]>
Date:   Wed Nov 3 15:47:37 2004 -0800

[PATCH] queue congestion threshold hysteresis

We need to open the gap between congestion on/off a little bit, or we risk burning many cycles continually putting processes on a wait queue only to wake them up again immediately. This was observed with CFQ at least, which showed way excessive sys time.

Patch is from Arjan.

Signed-off-by Jens Axboe <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

If you feel this isn't necessary, then I think you at least need to justify it with testing. Perhaps Jens can shed some light on the exact workload that triggered the pathological behaviour.

Cheers,
Jeff

Lukáš Czerner

author

2012-10-02 08:52 UTC #2 lore raw

Quoted 16 lines · Jeff Moyer

Date: Mon, 01 Oct 2012 12:52:19 -0400
From: Jeff Moyer <[email protected]>
To: Lukas Czerner <[email protected]>
Cc: Jens Axboe <[email protected]>, [email protected],
Dave Chinner <[email protected]>
Subject: Re: [PATCH] loop: Limit the number of requests in the bio list
Quoted 7 lines · Lukas Czerner
Currently there is not limitation of number of requests in the loop bio list. This can lead into some nasty situations when the caller spawns tons of bio requests taking huge amount of memory. This is even more obvious with discard where blkdev_issue_discard() will submit all bios for the range and wait for them to finish afterwards. On really big loop devices this can lead to OOM situation as reported by Dave Chinner.

Quoted 7 lines

Quoted 3 lines
With this patch we will wait in loop_make_request() if the number of bios in the loop bio list would exceed 'nr_requests' number of requests. We'll wake up the process as we process the bios form the list.
I think you might want to do something similar to what is done for request_queues by implementing a congestion on and off threshold. As Jens writes in this commit (predating the conversion to git):

Right, I've had the same idea. However my first proof-of-concept worked quite well without this and my simple performance testing did not show any regression.

I've basically done just fstrim, and blkdiscard on huge loop device measuring time to finish and dd bs=4k throughput. None of those showed any performance regression. I've chosen those for being quite simple and supposedly issuing quite a lot of bios. Any better recommendation to test this ?

Also I am still unable to reproduce the problem Dave originally experienced and I was hoping that he can test whether this helps or not.

Dave could you give it a try please ? By creating huge (500T, 1000T, 1500T) loop device on machine with 2GB memory I was not able to reproduce that. Maybe it's that xfs punch hole implementation is so damn fast :). Please let me know.

Thanks!
-Lukas

Quoted 23 lines

Author: Jens Axboe <[email protected]>
Date:   Wed Nov 3 15:47:37 2004 -0800
[PATCH] queue congestion threshold hysteresis
We need to open the gap between congestion on/off a little bit, or we risk burning many cycles continually putting processes on a wait queue only to wake them up again immediately. This was observed with CFQ at least, which showed way excessive sys time.
Patch is from Arjan.
Signed-off-by Jens Axboe <[email protected]>
Signed-off-by Linus Torvalds <[email protected]>
If you feel this isn't necessary, then I think you at least need to justify it with testing. Perhaps Jens can shed some light on the exact workload that triggered the pathological behaviour.
Cheers,
Jeff

Dave Chinner

2012-10-02 19:59 UTC #3 lore raw

Quoted 35 lines · Lukáš Czerner

Quoted 16 lines · Jeff Moyer
Date: Mon, 01 Oct 2012 12:52:19 -0400
From: Jeff Moyer <[email protected]>
To: Lukas Czerner <[email protected]>
Cc: Jens Axboe <[email protected]>, [email protected],
Dave Chinner <[email protected]>
Subject: Re: [PATCH] loop: Limit the number of requests in the bio list
Quoted 7 lines · Lukas Czerner
Currently there is not limitation of number of requests in the loop bio list. This can lead into some nasty situations when the caller spawns tons of bio requests taking huge amount of memory. This is even more obvious with discard where blkdev_issue_discard() will submit all bios for the range and wait for them to finish afterwards. On really big loop devices this can lead to OOM situation as reported by Dave Chinner.
Quoted 7 lines
Quoted 3 lines
With this patch we will wait in loop_make_request() if the number of bios in the loop bio list would exceed 'nr_requests' number of requests. We'll wake up the process as we process the bios form the list.
I think you might want to do something similar to what is done for request_queues by implementing a congestion on and off threshold. As Jens writes in this commit (predating the conversion to git):
Right, I've had the same idea. However my first proof-of-concept worked quite well without this and my simple performance testing did not show any regression.
I've basically done just fstrim, and blkdiscard on huge loop device measuring time to finish and dd bs=4k throughput. None of those showed any performance regression. I've chosen those for being quite simple and supposedly issuing quite a lot of bios. Any better recommendation to test this ?

Quoted 8 lines

Also I am still unable to reproduce the problem Dave originally experienced and I was hoping that he can test whether this helps or not.
Dave could you give it a try please ? By creating huge (500T, 1000T, 1500T) loop device on machine with 2GB memory I was not able to reproduce that. Maybe it's that xfs punch hole implementation is so damn fast :). Please let me know.

Try a file with a few hundred thousand extents in it (preallocate them). I found this while testing large block devices on loopback devices, not with empty files.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Jeff Moyer

2012-10-03 14:31 UTC #4 lore raw

Quoted 22 lines · Dave Chinner

Quoted 21 lines · Lukáš Czerner
Quoted 20 lines · Jeff Moyer
Date: Mon, 01 Oct 2012 12:52:19 -0400
From: Jeff Moyer <[email protected]>
To: Lukas Czerner <[email protected]>
Cc: Jens Axboe <[email protected]>, [email protected],
Dave Chinner <[email protected]>
Subject: Re: [PATCH] loop: Limit the number of requests in the bio list
Quoted 10 lines · Lukas Czerner
Currently there is not limitation of number of requests in the loop bio list. This can lead into some nasty situations when the caller spawns tons of bio requests taking huge amount of memory. This is even more obvious with discard where blkdev_issue_discard() will submit all bios for the range and wait for them to finish afterwards. On really big loop devices this can lead to OOM situation as reported by Dave Chinner.
With this patch we will wait in loop_make_request() if the number of bios in the loop bio list would exceed 'nr_requests' number of requests. We'll wake up the process as we process the bios form the list.

Quoted 7 lines

Quoted 7 lines
Quoted 3 lines
I think you might want to do something similar to what is done for request_queues by implementing a congestion on and off threshold. As Jens writes in this commit (predating the conversion to git):
Right, I've had the same idea. However my first proof-of-concept worked quite well without this and my simple performance testing did not show any regression.

Did you look at system time?

-Jeff

Lukáš Czerner

author

2012-10-03 15:01 UTC #5 lore raw

Quoted 35 lines · Jeff Moyer

Date: Wed, 03 Oct 2012 10:30:54 -0400
From: Jeff Moyer <[email protected]>
To: Dave Chinner <[email protected]>
Cc: Lukáš Czerner <[email protected]>, Jens Axboe <[email protected]>,
[email protected]
Subject: Re: [PATCH] loop: Limit the number of requests in the bio list
Quoted 26 lines · Dave Chinner
Quoted 25 lines · Lukáš Czerner
Quoted 23 lines · Jeff Moyer
Date: Mon, 01 Oct 2012 12:52:19 -0400
From: Jeff Moyer <[email protected]>
To: Lukas Czerner <[email protected]>
Cc: Jens Axboe <[email protected]>, [email protected],
Dave Chinner <[email protected]>
Subject: Re: [PATCH] loop: Limit the number of requests in the bio list
Quoted 10 lines · Lukas Czerner
Currently there is not limitation of number of requests in the loop bio list. This can lead into some nasty situations when the caller spawns tons of bio requests taking huge amount of memory. This is even more obvious with discard where blkdev_issue_discard() will submit all bios for the range and wait for them to finish afterwards. On really big loop devices this can lead to OOM situation as reported by Dave Chinner.
With this patch we will wait in loop_make_request() if the number of bios in the loop bio list would exceed 'nr_requests' number of requests. We'll wake up the process as we process the bios form the list.
I think you might want to do something similar to what is done for request_queues by implementing a congestion on and off threshold. As Jens writes in this commit (predating the conversion to git):

Quoted 7 lines

Quoted 3 lines
Quoted 3 lines
Right, I've had the same idea. However my first proof-of-concept worked quite well without this and my simple performance testing did not show any regression.
Did you look at system time?
-Jeff

Hi, none of the times showed any significant difference, there was not any pattern suggesting a problem. Also the system time is included in the real time, so it would show anyway I guess.

-Lukas

Jeff Moyer

2012-10-03 15:05 UTC #6 lore raw

Quoted 7 lines · Lukáš Czerner

Quoted 3 lines
Did you look at system time?
-Jeff
Hi, none of the times showed any significant difference, there was not any pattern suggesting a problem. Also the system time is included in the real time, so it would show anyway I guess.

Only if you're cpu bound. If you aren't, elevated system time may not increase overall runtime.

Cheers,
Jeff