* [PATCH] loop: Limit the number of requests in the bio list
@ 2012-09-27 17:33 Lukas Czerner
2012-10-01 16:52 ` Jeff Moyer
0 siblings, 1 reply; 7+ messages in thread
From: Lukas Czerner @ 2012-09-27 17:33 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-kernel, Dave Chinner, 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.
Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Reported-by: Dave Chinner <dchinner@redhat.com>
---
drivers/block/loop.c | 13 +++++++++++++
include/linux/loop.h | 3 +++
2 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 3bba655..2af969c 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -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;
diff --git a/include/linux/loop.h b/include/linux/loop.h
index 11a41a8..e455d84 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -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;
--
1.7.7.6
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] loop: Limit the number of requests in the bio list
2012-09-27 17:33 [PATCH] loop: Limit the number of requests in the bio list Lukas Czerner
@ 2012-10-01 16:52 ` Jeff Moyer
2012-10-02 8:52 ` Lukáš Czerner
0 siblings, 1 reply; 7+ messages in thread
From: Jeff Moyer @ 2012-10-01 16:52 UTC (permalink / raw)
To: Lukas Czerner; +Cc: Jens Axboe, linux-kernel, Dave Chinner
Lukas Czerner <lczerner@redhat.com> writes:
> 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 <axboe@suse.de>
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 <axboe@suse.de>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] loop: Limit the number of requests in the bio list
2012-10-01 16:52 ` Jeff Moyer
@ 2012-10-02 8:52 ` Lukáš Czerner
2012-10-02 19:59 ` Dave Chinner
0 siblings, 1 reply; 7+ messages in thread
From: Lukáš Czerner @ 2012-10-02 8:52 UTC (permalink / raw)
To: Jeff Moyer; +Cc: Lukas Czerner, Jens Axboe, linux-kernel, Dave Chinner
On Mon, 1 Oct 2012, Jeff Moyer wrote:
> Date: Mon, 01 Oct 2012 12:52:19 -0400
> From: Jeff Moyer <jmoyer@redhat.com>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: Jens Axboe <axboe@kernel.dk>, linux-kernel@vger.kernel.org,
> Dave Chinner <dchinner@redhat.com>
> Subject: Re: [PATCH] loop: Limit the number of requests in the bio list
>
> Lukas Czerner <lczerner@redhat.com> writes:
>
> > 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):
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
>
> Author: Jens Axboe <axboe@suse.de>
> 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 <axboe@suse.de>
> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
>
> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] loop: Limit the number of requests in the bio list
2012-10-02 8:52 ` Lukáš Czerner
@ 2012-10-02 19:59 ` Dave Chinner
2012-10-03 14:30 ` Jeff Moyer
0 siblings, 1 reply; 7+ messages in thread
From: Dave Chinner @ 2012-10-02 19:59 UTC (permalink / raw)
To: Lukáš Czerner; +Cc: Jeff Moyer, Jens Axboe, linux-kernel
On Tue, Oct 02, 2012 at 10:52:05AM +0200, Lukáš Czerner wrote:
> On Mon, 1 Oct 2012, Jeff Moyer wrote:
> > Date: Mon, 01 Oct 2012 12:52:19 -0400
> > From: Jeff Moyer <jmoyer@redhat.com>
> > To: Lukas Czerner <lczerner@redhat.com>
> > Cc: Jens Axboe <axboe@kernel.dk>, linux-kernel@vger.kernel.org,
> > Dave Chinner <dchinner@redhat.com>
> > Subject: Re: [PATCH] loop: Limit the number of requests in the bio list
> >
> > Lukas Czerner <lczerner@redhat.com> writes:
> >
> > > 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):
>
> 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.
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.
--
Dave Chinner
dchinner@redhat.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] loop: Limit the number of requests in the bio list
2012-10-02 19:59 ` Dave Chinner
@ 2012-10-03 14:30 ` Jeff Moyer
2012-10-03 15:01 ` Lukáš Czerner
0 siblings, 1 reply; 7+ messages in thread
From: Jeff Moyer @ 2012-10-03 14:30 UTC (permalink / raw)
To: Dave Chinner; +Cc: Lukáš Czerner, Jens Axboe, linux-kernel
Dave Chinner <dchinner@redhat.com> writes:
> On Tue, Oct 02, 2012 at 10:52:05AM +0200, Lukáš Czerner wrote:
>> On Mon, 1 Oct 2012, Jeff Moyer wrote:
>> > Date: Mon, 01 Oct 2012 12:52:19 -0400
>> > From: Jeff Moyer <jmoyer@redhat.com>
>> > To: Lukas Czerner <lczerner@redhat.com>
>> > Cc: Jens Axboe <axboe@kernel.dk>, linux-kernel@vger.kernel.org,
>> > Dave Chinner <dchinner@redhat.com>
>> > Subject: Re: [PATCH] loop: Limit the number of requests in the bio list
>> >
>> > Lukas Czerner <lczerner@redhat.com> writes:
>> >
>> > > 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):
>>
>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] loop: Limit the number of requests in the bio list
2012-10-03 14:30 ` Jeff Moyer
@ 2012-10-03 15:01 ` Lukáš Czerner
2012-10-03 15:05 ` Jeff Moyer
0 siblings, 1 reply; 7+ messages in thread
From: Lukáš Czerner @ 2012-10-03 15:01 UTC (permalink / raw)
To: Jeff Moyer
Cc: Dave Chinner, Lukáš Czerner, Jens Axboe, linux-kernel
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2274 bytes --]
On Wed, 3 Oct 2012, Jeff Moyer wrote:
> Date: Wed, 03 Oct 2012 10:30:54 -0400
> From: Jeff Moyer <jmoyer@redhat.com>
> To: Dave Chinner <dchinner@redhat.com>
> Cc: Lukáš Czerner <lczerner@redhat.com>, Jens Axboe <axboe@kernel.dk>,
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] loop: Limit the number of requests in the bio list
>
> Dave Chinner <dchinner@redhat.com> writes:
>
> > On Tue, Oct 02, 2012 at 10:52:05AM +0200, Lukáš Czerner wrote:
> >> On Mon, 1 Oct 2012, Jeff Moyer wrote:
> >> > Date: Mon, 01 Oct 2012 12:52:19 -0400
> >> > From: Jeff Moyer <jmoyer@redhat.com>
> >> > To: Lukas Czerner <lczerner@redhat.com>
> >> > Cc: Jens Axboe <axboe@kernel.dk>, linux-kernel@vger.kernel.org,
> >> > Dave Chinner <dchinner@redhat.com>
> >> > Subject: Re: [PATCH] loop: Limit the number of requests in the bio list
> >> >
> >> > Lukas Czerner <lczerner@redhat.com> writes:
> >> >
> >> > > 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):
> >>
> >> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] loop: Limit the number of requests in the bio list
2012-10-03 15:01 ` Lukáš Czerner
@ 2012-10-03 15:05 ` Jeff Moyer
0 siblings, 0 replies; 7+ messages in thread
From: Jeff Moyer @ 2012-10-03 15:05 UTC (permalink / raw)
To: Lukáš Czerner; +Cc: Dave Chinner, Jens Axboe, linux-kernel
Lukáš Czerner <lczerner@redhat.com> writes:
>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-10-03 15:05 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-27 17:33 [PATCH] loop: Limit the number of requests in the bio list Lukas Czerner
2012-10-01 16:52 ` Jeff Moyer
2012-10-02 8:52 ` Lukáš Czerner
2012-10-02 19:59 ` Dave Chinner
2012-10-03 14:30 ` Jeff Moyer
2012-10-03 15:01 ` Lukáš Czerner
2012-10-03 15:05 ` Jeff Moyer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox