loop: Limit the number of requests in the bio list
PATCH v2 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 and slow backing file system 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. Some threshold hysteresis is in place to avoid high frequency oscillation.
---
v2: add threshold hysteresis
drivers/block/loop.c | 16 ++++++++++++++++ include/linux/loop.h | 3 +++ 2 files changed, 19 insertions(+), 0 deletions(-)
drivers/block/loop.c +16 -0
@@ -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) { +unsigned int nr; +spin_unlock_irq (&lo->lo_lock); + nr = lo->lo_queue->nr_requests - (lo->lo_queue->nr_requests/8 ); +wait_event_interruptible (lo->lo_req_wait, + lo->lo_bio_count < nr); +spin_lock_irq (&lo->lo_lock); + }loop_add_bio (lo, old_bio);wake_up (&lo->lo_event);spin_unlock_irq (&lo->lo_lock);
@@ -532,6 +542,7 @@ static inline void loop_handle_bio(struct loop_device *lo, struct bio *bio)static int loop_thread (void *data) {struct loop_device *lo = data; +unsigned int nr;struct bio *bio;set_user_nice (current,-20 );
@@ -546,6 +557,9 @@ static int loop_thread(void *data)continue ;spin_lock_irq (&lo->lo_lock); bio =loop_get_bio (lo); + nr = lo->lo_queue->nr_requests - (lo->lo_queue->nr_requests/8 ); +if (lo->lo_bio_count < nr) +wake_up (&lo->lo_req_wait);spin_unlock_irq (&lo->lo_lock);BUG_ON (!bio);
@@ -873,6 +887,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 +1675,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
@@ -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
Quoted 8 lines · On Tue, 16 Oct 2012 11:21:45 +0200 Lukas Czerner <[email protected]> wrote:
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 and slow backing file system this can lead to OOM situation as reported by Dave Chinner.
Quoted 5 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. Some threshold hysteresis is in place to avoid high frequency oscillation.
What's happening with this?
Quoted 20 lines
drivers/block/loop.c +2
-0@@ -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;
drivers/block/loop.c static void loop_make_request(struct request_queue *q, struct bio *old_bio) · line 501
Quoted 10 lines
drivers/block/loop.c +8
-0if (unlikely (rw ==WRITE && (lo->lo_flags &LO_FLAGS_READ_ONLY ))) goto out; +if (lo->lo_bio_count >= lo->lo_queue->nr_requests) { +unsigned int nr; +spin_unlock_irq (&lo->lo_lock); + nr = lo->lo_queue->nr_requests - (lo->lo_queue->nr_requests/8 ); +wait_event_interruptible (lo->lo_req_wait, + lo->lo_bio_count < nr); +spin_lock_irq (&lo->lo_lock); + }
Two things.
a) wait_event_interruptible() will return immediately if a signal is pending (eg, someone hit ^C). This is not the behaviour you want. If the calling process is always a kernel thread then wait_event_interruptible() is OK and is the correct thing to use. Otherwise, it will need to be an uninterruptible sleep.
b) Why is it safe to drop lo_lock here? What data is that lock protecting?
Quoted 11 lines · Andrew Morton
Quoted 8 lines · On Tue, 16 Oct 2012 11:21:45 +0200 Lukas Czerner <[email protected]> wrote:
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 and slow backing file system this can lead to OOM situation as reported by Dave Chinner.
Quoted 7 lines
Quoted 5 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. Some threshold hysteresis is in place to avoid high frequency oscillation.What's happening with this?
Still waiting for review, I guess. I'll have a look.
Quoted 33 lines
Quoted 20 lines
drivers/block/loop.c +2
-0@@ -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;drivers/block/loop.c static void loop_make_request(struct request_queue *q, struct bio *old_bio) · line 501
Quoted 10 lines
drivers/block/loop.c +8
-0if (unlikely (rw ==WRITE && (lo->lo_flags &LO_FLAGS_READ_ONLY ))) goto out; +if (lo->lo_bio_count >= lo->lo_queue->nr_requests) { +unsigned int nr; +spin_unlock_irq (&lo->lo_lock); + nr = lo->lo_queue->nr_requests - (lo->lo_queue->nr_requests/8 ); +wait_event_interruptible (lo->lo_req_wait, + lo->lo_bio_count < nr); +spin_lock_irq (&lo->lo_lock); + }Two things.
Quoted 5 lines
a)
wait_event_interruptible()will return immediately if a signal is pending (eg, someone hit ^C). This is not the behaviour you want. If the calling process is always a kernel thread thenwait_event_interruptible()is OK and is the correct thing to use. Otherwise, it will need to be an uninterruptible sleep.
Good catch, this needs fixing.
Quoted 1 line
b) Why is it safe to drop
lo_lockhere? What data is that lock protecting?
lo_lock is protecting access to state and the bio list. Dropping the lock looks okay to me.
Cheers,
Jeff
Jens Axboe
maintainerQuoted 11 lines · Andrew Morton
Quoted 8 lines · On Tue, 16 Oct 2012 11:21:45 +0200 Lukas Czerner <[email protected]> wrote:
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 and slow backing file system this can lead to OOM situation as reported by Dave Chinner.
Quoted 7 lines
Quoted 5 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. Some threshold hysteresis is in place to avoid high frequency oscillation.What's happening with this?
Sorry I didn't reply to this yet. My initial thought is that we had something like this for loop back in the 2.4 days, and it was deadlock prone. Can't seem to remember all the details on that yet.
v2 is a nice improvement, though. With 1:1 bio and wakeups, you would get tons of context switches. The batched approach is much better.
Lukas, have you beaten on this with a file backed loop and heavy traffic on a file system on top?
Signature · 1 line
Jens Axboe
Quoted 31 lines · Jens Axboe
Date: Fri, 09 Nov 2012 08:34:03 +0100
From: Jens Axboe <[email protected]>
To: Andrew Morton <[email protected]>
Cc: Lukas Czerner <[email protected]>, [email protected],
[email protected], [email protected]
Subject: Re: [PATCH v2] loop: Limit the number of requests in the bio listQuoted 11 lines · Andrew Morton
Quoted 8 lines · On Tue, 16 Oct 2012 11:21:45 +0200 Lukas Czerner <[email protected]> wrote:
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 and slow backing file system this can lead to OOM situation as reported by Dave Chinner.Quoted 7 lines
Quoted 5 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. Some threshold hysteresis is in place to avoid high frequency oscillation.What's happening with this?
Sorry I didn't reply to this yet. My initial thought is that we had something like this for loop back in the 2.4 days, and it was deadlock prone. Can't seem to remember all the details on that yet.
Quoted 5 lines
v2 is a nice improvement, though. With 1:1 bio and wakeups, you would get tons of context switches. The batched approach is much better.
Lukas, have you beaten on this with a file backed loop and heavy traffic on a file system on top?
Hi Jeff,
sorry for the delay. Yes, I've tested this with xfstests using loop driver for both test and scratch disk.
I'll resend the updated version hopefully later today.
Thanks!
-Lukas
Quoted 59 lines · Andrew Morton
Date: Thu, 8 Nov 2012 11:14:18 -0800
From: Andrew Morton <[email protected]>
To: Lukas Czerner <[email protected]>
Cc: [email protected], [email protected], [email protected],
[email protected]
Subject: Re: [PATCH v2] loop: Limit the number of requests in the bio listQuoted 8 lines · On Tue, 16 Oct 2012 11:21:45 +0200 Lukas Czerner <[email protected]> wrote:
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 and slow backing file system this can lead to OOM situation as reported by Dave Chinner.Quoted 5 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. Some threshold hysteresis is in place to avoid high frequency oscillation.What's happening with this?
Quoted 20 lines
drivers/block/loop.c +2
-0@@ -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;drivers/block/loop.c static void loop_make_request(struct request_queue *q, struct bio *old_bio) · line 501
Quoted 10 lines
drivers/block/loop.c +8
-0if (unlikely (rw ==WRITE && (lo->lo_flags &LO_FLAGS_READ_ONLY ))) goto out; +if (lo->lo_bio_count >= lo->lo_queue->nr_requests) { +unsigned int nr; +spin_unlock_irq (&lo->lo_lock); + nr = lo->lo_queue->nr_requests - (lo->lo_queue->nr_requests/8 ); +wait_event_interruptible (lo->lo_req_wait, + lo->lo_bio_count < nr); +spin_lock_irq (&lo->lo_lock); + }Two things.
Quoted 5 lines
a)
wait_event_interruptible()will return immediately if a signal is pending (eg, someone hit ^C). This is not the behaviour you want. If the calling process is always a kernel thread thenwait_event_interruptible()is OK and is the correct thing to use. Otherwise, it will need to be an uninterruptible sleep.
Understood, I'll fix that.
Quoted 3 lines
b) Why is it safe to drop
lo_lockhere? What data is that lock protecting?
It is protecting the bio list, lo state, backing file so I think it is perfectly safe to drop the lock there.
Thanks!
-Lukas
drivers/block/loop.c static void loop_make_request(struct request_queue *q, struct bio *old_bio) · line 501
Quoted 8 lines · Lukas Czerner
drivers/block/loop.c +8
-0+if (lo->lo_bio_count >= lo->lo_queue->nr_requests) { +unsigned int nr; +spin_unlock_irq (&lo->lo_lock); + nr = lo->lo_queue->nr_requests - (lo->lo_queue->nr_requests/8 ); +wait_event_interruptible (lo->lo_req_wait, + lo->lo_bio_count < nr); +spin_lock_irq (&lo->lo_lock); + }
So, blk_queue_make_request already initialized q->nr_congestion_on and q->nr_congestion_off. Is there a reason you didn't simply use queue_congestion_on_threshold and queue_congestion_off_threshold?
Cheers,
Jeff
Quoted 8 lines · Jeff Moyer
Date: Thu, 08 Nov 2012 16:53:01 -0500
From: Jeff Moyer <[email protected]>
To: Lukas Czerner <[email protected]>
Cc: [email protected], [email protected], [email protected]
Subject: Re: [PATCH v2] loop: Limit the number of requests in the bio listLukas Czerner <[email protected]> writes:
Quoted 12 lines
drivers/block/loop.c static void loop_make_request(struct request_queue *q, struct bio *old_bio) · line 501
Quoted 8 lines
drivers/block/loop.c +8
-0+if (lo->lo_bio_count >= lo->lo_queue->nr_requests) { +unsigned int nr; +spin_unlock_irq (&lo->lo_lock); + nr = lo->lo_queue->nr_requests - (lo->lo_queue->nr_requests/8 ); +wait_event_interruptible (lo->lo_req_wait, + lo->lo_bio_count < nr); +spin_lock_irq (&lo->lo_lock); + }So,
blk_queue_make_requestalready initializedq->nr_congestion_onandq->nr_congestion_off. Is there a reason you didn't simply usequeue_congestion_on_thresholdandqueue_congestion_off_threshold?
The reason is that I did not knew about those :) Thanks for pointing it out I'll take a look at it.
Thanks!
-Lukas
Quoted 4 lines
Cheers,
Jeff