Re: [PATCH v2] loop: Limit the number of requests in the bio list

2012-11-13 Thread Lukáš Czerner 
On Thu, 8 Nov 2012, Andrew Morton wrote:

> Date: Thu, 8 Nov 2012 11:14:18 -0800
> From: Andrew Morton 
> To: Lukas Czerner 
> Cc: ax...@kernel.dk, dchin...@redhat.com, jmo...@redhat.com,
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] loop: Limit the number of requests in the bio list
> 
> On Tue, 16 Oct 2012 11:21:45 +0200
> Lukas Czerner  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.
> > 
> > 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?
> 
> > --- 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_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_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_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_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.

Understood, I'll fix that.

> 
> b) Why is it safe to drop lo_lock here?  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] loop: Limit the number of requests in the bio list

2012-11-13 Thread Lukáš Czerner 
On Thu, 8 Nov 2012, Jeff Moyer wrote:

> Date: Thu, 08 Nov 2012 16:53:01 -0500
> From: Jeff Moyer 
> To: Lukas Czerner 
> Cc: ax...@kernel.dk, dchin...@redhat.com, linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] loop: Limit the number of requests in the bio list
> 
> Lukas Czerner  writes:
> 
> > +   if (lo->lo_bio_count >= lo->lo_queue->nr_requests) {
> > +   unsigned int nr;
> > +   spin_unlock_irq(>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_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?

The reason is that I did not knew about those :) Thanks for pointing
it out I'll take a look at it.

Thanks!
-Lukas

> 
> Cheers,
> Jeff
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] loop: Limit the number of requests in the bio list

2012-11-13 Thread Lukáš Czerner 
On Fri, 9 Nov 2012, Jens Axboe wrote:

> Date: Fri, 09 Nov 2012 08:34:03 +0100
> From: Jens Axboe 
> To: Andrew Morton 
> Cc: Lukas Czerner , dchin...@redhat.com,
> jmo...@redhat.com, linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] loop: Limit the number of requests in the bio list
> 
> On 2012-11-08 20:14, Andrew Morton wrote:
> > On Tue, 16 Oct 2012 11:21:45 +0200
> > Lukas Czerner  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.
> >>
> >> 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?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] loop: Limit the number of requests in the bio list

2012-11-13 Thread Lukáš Czerner 
On Fri, 9 Nov 2012, Jens Axboe wrote:

 Date: Fri, 09 Nov 2012 08:34:03 +0100
 From: Jens Axboe ax...@kernel.dk
 To: Andrew Morton a...@linux-foundation.org
 Cc: Lukas Czerner lczer...@redhat.com, dchin...@redhat.com,
 jmo...@redhat.com, linux-kernel@vger.kernel.org
 Subject: Re: [PATCH v2] loop: Limit the number of requests in the bio list
 
 On 2012-11-08 20:14, Andrew Morton wrote:
  On Tue, 16 Oct 2012 11:21:45 +0200
  Lukas Czerner lczer...@redhat.com 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.
 
  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?

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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] loop: Limit the number of requests in the bio list

2012-11-13 Thread Lukáš Czerner 
On Thu, 8 Nov 2012, Jeff Moyer wrote:

 Date: Thu, 08 Nov 2012 16:53:01 -0500
 From: Jeff Moyer jmo...@redhat.com
 To: Lukas Czerner lczer...@redhat.com
 Cc: ax...@kernel.dk, dchin...@redhat.com, linux-kernel@vger.kernel.org
 Subject: Re: [PATCH v2] loop: Limit the number of requests in the bio list
 
 Lukas Czerner lczer...@redhat.com writes:
 
  +   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?

The reason is that I did not knew about those :) Thanks for pointing
it out I'll take a look at it.

Thanks!
-Lukas

 
 Cheers,
 Jeff
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] loop: Limit the number of requests in the bio list

2012-11-13 Thread Lukáš Czerner 
On Thu, 8 Nov 2012, Andrew Morton wrote:

 Date: Thu, 8 Nov 2012 11:14:18 -0800
 From: Andrew Morton a...@linux-foundation.org
 To: Lukas Czerner lczer...@redhat.com
 Cc: ax...@kernel.dk, dchin...@redhat.com, jmo...@redhat.com,
 linux-kernel@vger.kernel.org
 Subject: Re: [PATCH v2] loop: Limit the number of requests in the bio list
 
 On Tue, 16 Oct 2012 11:21:45 +0200
 Lukas Czerner lczer...@redhat.com 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.
  
  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?
 
  --- 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) {
  +   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.

Understood, I'll fix that.

 
 b) Why is it safe to drop lo_lock here?  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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] loop: Limit the number of requests in the bio list

2012-11-08 Thread Jens Axboe 
On 2012-11-08 20:14, Andrew Morton wrote:
> On Tue, 16 Oct 2012 11:21:45 +0200
> Lukas Czerner  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.
>>
>> 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?

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] loop: Limit the number of requests in the bio list

2012-11-08 Thread Jeff Moyer 
Lukas Czerner  writes:

> + if (lo->lo_bio_count >= lo->lo_queue->nr_requests) {
> + unsigned int nr;
> + spin_unlock_irq(>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_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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] loop: Limit the number of requests in the bio list

2012-11-08 Thread Jeff Moyer 
Andrew Morton  writes:

> On Tue, 16 Oct 2012 11:21:45 +0200
> Lukas Czerner  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.
>> 
>> 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.

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

Good catch, this needs fixing.

> b) Why is it safe to drop lo_lock here?  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] loop: Limit the number of requests in the bio list

2012-11-08 Thread Andrew Morton 
On Tue, 16 Oct 2012 11:21:45 +0200
Lukas Czerner  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.
> 
> 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?

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] loop: Limit the number of requests in the bio list

2012-11-08 Thread Andrew Morton 
On Tue, 16 Oct 2012 11:21:45 +0200
Lukas Czerner lczer...@redhat.com 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.
 
 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?

 --- 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) {
 + 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?


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] loop: Limit the number of requests in the bio list

2012-11-08 Thread Jeff Moyer 
Andrew Morton a...@linux-foundation.org writes:

 On Tue, 16 Oct 2012 11:21:45 +0200
 Lukas Czerner lczer...@redhat.com 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.
 
 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.

 --- 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) {
 +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.

Good catch, this needs fixing.

 b) Why is it safe to drop lo_lock here?  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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] loop: Limit the number of requests in the bio list

2012-11-08 Thread Jeff Moyer 
Lukas Czerner lczer...@redhat.com writes:

 + 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] loop: Limit the number of requests in the bio list

2012-11-08 Thread Jens Axboe 
On 2012-11-08 20:14, Andrew Morton wrote:
 On Tue, 16 Oct 2012 11:21:45 +0200
 Lukas Czerner lczer...@redhat.com 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.

 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?

-- 
Jens Axboe

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] loop: Limit the number of requests in the bio list

2012-10-16 Thread 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 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.

Signed-off-by: Lukas Czerner 
Reported-by: Dave Chinner 
---
v2: add threshold hysteresis

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

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index e9d594f..0589976 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_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_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_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_lock);
+   }
loop_add_bio(lo, old_bio);
wake_up(>lo_event);
spin_unlock_irq(>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_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_req_wait);
spin_unlock_irq(>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_event);
+   init_waitqueue_head(>lo_req_wait);
spin_lock_init(>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 9635116..4ba4789 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 intlo_bio_count;
int lo_state;
struct mutexlo_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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] loop: Limit the number of requests in the bio list

2012-10-16 Thread 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 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.

Signed-off-by: Lukas Czerner lczer...@redhat.com
Reported-by: Dave Chinner dchin...@redhat.com
---
v2: add threshold hysteresis

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

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index e9d594f..0589976 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) {
+   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;
diff --git a/include/linux/loop.h b/include/linux/loop.h
index 9635116..4ba4789 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 intlo_bio_count;
int lo_state;
struct mutexlo_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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/