[PATCH] loop: use BIO list management functions

[PATCH] loop: use BIO list management functions

[Akinobu Mita]

Impact: cleanup

Cc:  Jens Axboe <axboe@kernel.dk>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/block/loop.c |   26 +++++++-------------------
 include/linux/bio.h  |    2 +-
 include/linux/loop.h |    3 +--
 3 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ddae808..9ca4bb0 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -511,11 +511,7 @@ out:
  */
 static void loop_add_bio(struct loop_device *lo, struct bio *bio)
 {
-	if (lo->lo_biotail) {
-		lo->lo_biotail->bi_next = bio;
-		lo->lo_biotail = bio;
-	} else
-		lo->lo_bio = lo->lo_biotail = bio;
+	bio_list_add(&lo->lo_bio_list, bio);
 }
 
 /*
@@ -523,16 +519,7 @@ static void loop_add_bio(struct loop_device *lo, struct bio *bio)
  */
 static struct bio *loop_get_bio(struct loop_device *lo)
 {
-	struct bio *bio;
-
-	if ((bio = lo->lo_bio)) {
-		if (bio == lo->lo_biotail)
-			lo->lo_biotail = NULL;
-		lo->lo_bio = bio->bi_next;
-		bio->bi_next = NULL;
-	}
-
-	return bio;
+	return bio_list_pop(&lo->lo_bio_list);
 }
 
 static int loop_make_request(struct request_queue *q, struct bio *old_bio)
@@ -609,12 +596,13 @@ static int loop_thread(void *data)
 
 	set_user_nice(current, -20);
 
-	while (!kthread_should_stop() || lo->lo_bio) {
+	while (!kthread_should_stop() || !bio_list_empty(&lo->lo_bio_list)) {
 
 		wait_event_interruptible(lo->lo_event,
-				lo->lo_bio || kthread_should_stop());
+				!bio_list_empty(&lo->lo_bio_list) ||
+				kthread_should_stop());
 
-		if (!lo->lo_bio)
+		if (bio_list_empty(&lo->lo_bio_list))
 			continue;
 		spin_lock_irq(&lo->lo_lock);
 		bio = loop_get_bio(lo);
@@ -841,7 +829,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	lo->old_gfp_mask = mapping_gfp_mask(mapping);
 	mapping_set_gfp_mask(mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));
 
-	lo->lo_bio = lo->lo_biotail = NULL;
+	bio_list_init(&lo->lo_bio_list);
 
 	/*
 	 * set queue make_request_fn, and add limits based on lower level
diff --git a/include/linux/bio.h b/include/linux/bio.h
index b89cf2d..631153b 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -505,7 +505,7 @@ static inline int bio_has_data(struct bio *bio)
 }
 
 /*
- * BIO list managment for use by remapping drivers (e.g. DM or MD).
+ * BIO list management for use by remapping drivers (e.g. DM or MD) and loop.
  *
  * A bio_list anchors a singly-linked list of bios chained through the bi_next
  * member of the bio.  The bio_list also caches the last list member to allow
diff --git a/include/linux/loop.h b/include/linux/loop.h
index 4072544..66c194e 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -56,8 +56,7 @@ struct loop_device {
 	gfp_t		old_gfp_mask;
 
 	spinlock_t		lo_lock;
-	struct bio 		*lo_bio;
-	struct bio		*lo_biotail;
+	struct bio_list		lo_bio_list;
 	int			lo_state;
 	struct mutex		lo_ctl_mutex;
 	struct task_struct	*lo_thread;

Re: [PATCH] loop: use BIO list management functions

[Christoph Hellwig]

On Fri, Apr 17, 2009 at 03:32:17PM +0900, Akinobu Mita wrote:
> Impact: cleanup

I though we had pretty broad agreement that these lines are utter crap?
:)

>  static void loop_add_bio(struct loop_device *lo, struct bio *bio)
>  {
> -	if (lo->lo_biotail) {
> -		lo->lo_biotail->bi_next = bio;
> -		lo->lo_biotail = bio;
> -	} else
> -		lo->lo_bio = lo->lo_biotail = bio;
> +	bio_list_add(&lo->lo_bio_list, bio);
>  }

This function only has one caller, no need to keep it.

>  static struct bio *loop_get_bio(struct loop_device *lo)
>  {
> -	struct bio *bio;
> -
> -	if ((bio = lo->lo_bio)) {
> -		if (bio == lo->lo_biotail)
> -			lo->lo_biotail = NULL;
> -		lo->lo_bio = bio->bi_next;
> -		bio->bi_next = NULL;
> -	}
> -
> -	return bio;
> +	return bio_list_pop(&lo->lo_bio_list);
>  }

Same here.

> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -505,7 +505,7 @@ static inline int bio_has_data(struct bio *bio)
>  }
>  
>  /*
> - * BIO list managment for use by remapping drivers (e.g. DM or MD).
> + * BIO list management for use by remapping drivers (e.g. DM or MD) and loop.
>   *


I don't think this needs changing, that was just examples.

Re: [PATCH] loop: use BIO list management functions

[Jens Axboe]

On Fri, Apr 17 2009, Akinobu Mita wrote:
> Impact: cleanup

This looks good, but please provide a better description than just a
silly impact notice. I've filled it in for you for this one, but in the
future the description should say what the patch does.

It's queued for 2.6.31.


> 
> Cc:  Jens Axboe <axboe@kernel.dk>
> Cc: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/block/loop.c |   26 +++++++-------------------
>  include/linux/bio.h  |    2 +-
>  include/linux/loop.h |    3 +--
>  3 files changed, 9 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index ddae808..9ca4bb0 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -511,11 +511,7 @@ out:
>   */
>  static void loop_add_bio(struct loop_device *lo, struct bio *bio)
>  {
> -	if (lo->lo_biotail) {
> -		lo->lo_biotail->bi_next = bio;
> -		lo->lo_biotail = bio;
> -	} else
> -		lo->lo_bio = lo->lo_biotail = bio;
> +	bio_list_add(&lo->lo_bio_list, bio);
>  }
>  
>  /*
> @@ -523,16 +519,7 @@ static void loop_add_bio(struct loop_device *lo, struct bio *bio)
>   */
>  static struct bio *loop_get_bio(struct loop_device *lo)
>  {
> -	struct bio *bio;
> -
> -	if ((bio = lo->lo_bio)) {
> -		if (bio == lo->lo_biotail)
> -			lo->lo_biotail = NULL;
> -		lo->lo_bio = bio->bi_next;
> -		bio->bi_next = NULL;
> -	}
> -
> -	return bio;
> +	return bio_list_pop(&lo->lo_bio_list);
>  }
>  
>  static int loop_make_request(struct request_queue *q, struct bio *old_bio)
> @@ -609,12 +596,13 @@ static int loop_thread(void *data)
>  
>  	set_user_nice(current, -20);
>  
> -	while (!kthread_should_stop() || lo->lo_bio) {
> +	while (!kthread_should_stop() || !bio_list_empty(&lo->lo_bio_list)) {
>  
>  		wait_event_interruptible(lo->lo_event,
> -				lo->lo_bio || kthread_should_stop());
> +				!bio_list_empty(&lo->lo_bio_list) ||
> +				kthread_should_stop());
>  
> -		if (!lo->lo_bio)
> +		if (bio_list_empty(&lo->lo_bio_list))
>  			continue;
>  		spin_lock_irq(&lo->lo_lock);
>  		bio = loop_get_bio(lo);
> @@ -841,7 +829,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>  	lo->old_gfp_mask = mapping_gfp_mask(mapping);
>  	mapping_set_gfp_mask(mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));
>  
> -	lo->lo_bio = lo->lo_biotail = NULL;
> +	bio_list_init(&lo->lo_bio_list);
>  
>  	/*
>  	 * set queue make_request_fn, and add limits based on lower level
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index b89cf2d..631153b 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -505,7 +505,7 @@ static inline int bio_has_data(struct bio *bio)
>  }
>  
>  /*
> - * BIO list managment for use by remapping drivers (e.g. DM or MD).
> + * BIO list management for use by remapping drivers (e.g. DM or MD) and loop.
>   *
>   * A bio_list anchors a singly-linked list of bios chained through the bi_next
>   * member of the bio.  The bio_list also caches the last list member to allow
> diff --git a/include/linux/loop.h b/include/linux/loop.h
> index 4072544..66c194e 100644
> --- a/include/linux/loop.h
> +++ b/include/linux/loop.h
> @@ -56,8 +56,7 @@ struct loop_device {
>  	gfp_t		old_gfp_mask;
>  
>  	spinlock_t		lo_lock;
> -	struct bio 		*lo_bio;
> -	struct bio		*lo_biotail;
> +	struct bio_list		lo_bio_list;
>  	int			lo_state;
>  	struct mutex		lo_ctl_mutex;
>  	struct task_struct	*lo_thread;

-- 
Jens Axboe

Re: [PATCH] loop: use BIO list management functions

[Jens Axboe]

On Fri, Apr 17 2009, Christoph Hellwig wrote:
> On Fri, Apr 17, 2009 at 03:32:17PM +0900, Akinobu Mita wrote:
> > Impact: cleanup
> 
> I though we had pretty broad agreement that these lines are utter crap?
> :)

Agree on that...

-- 
Jens Axboe

Re: [PATCH] loop: use BIO list management functions

[Akinobu Mita]

On Fri, Apr 17, 2009 at 08:40:35AM +0200, Jens Axboe wrote:
> On Fri, Apr 17 2009, Akinobu Mita wrote:
> > Impact: cleanup
> 
> This looks good, but please provide a better description than just a
> silly impact notice. I've filled it in for you for this one, but in the
> future the description should say what the patch does.
> 
> It's queued for 2.6.31.

OK, I added the description and Christoph's feedbacks.

Subject: loop: use BIO list management functions

There are BIO list management helpers in bio.h.
This uses them to reduce code duplication in loopback driver.

Cc:  Jens Axboe <axboe@kernel.dk>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/block/loop.c |   42 +++++++-----------------------------------
 include/linux/bio.h  |    2 +-
 include/linux/loop.h |    3 +--
 3 files changed, 9 insertions(+), 38 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ddae808..2b70545 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -506,35 +506,6 @@ out:
 	return ret;
 }
 
-/*
- * Add bio to back of pending list
- */
-static void loop_add_bio(struct loop_device *lo, struct bio *bio)
-{
-	if (lo->lo_biotail) {
-		lo->lo_biotail->bi_next = bio;
-		lo->lo_biotail = bio;
-	} else
-		lo->lo_bio = lo->lo_biotail = bio;
-}
-
-/*
- * Grab first pending buffer
- */
-static struct bio *loop_get_bio(struct loop_device *lo)
-{
-	struct bio *bio;
-
-	if ((bio = lo->lo_bio)) {
-		if (bio == lo->lo_biotail)
-			lo->lo_biotail = NULL;
-		lo->lo_bio = bio->bi_next;
-		bio->bi_next = NULL;
-	}
-
-	return bio;
-}
-
 static int loop_make_request(struct request_queue *q, struct bio *old_bio)
 {
 	struct loop_device *lo = q->queuedata;
@@ -550,7 +521,7 @@ static int 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;
-	loop_add_bio(lo, old_bio);
+	bio_list_add(&lo->lo_bio_list, old_bio);
 	wake_up(&lo->lo_event);
 	spin_unlock_irq(&lo->lo_lock);
 	return 0;
@@ -609,15 +580,16 @@ static int loop_thread(void *data)
 
 	set_user_nice(current, -20);
 
-	while (!kthread_should_stop() || lo->lo_bio) {
+	while (!kthread_should_stop() || !bio_list_empty(&lo->lo_bio_list)) {
 
 		wait_event_interruptible(lo->lo_event,
-				lo->lo_bio || kthread_should_stop());
+				!bio_list_empty(&lo->lo_bio_list) ||
+				kthread_should_stop());
 
-		if (!lo->lo_bio)
+		if (bio_list_empty(&lo->lo_bio_list))
 			continue;
 		spin_lock_irq(&lo->lo_lock);
-		bio = loop_get_bio(lo);
+		bio = bio_list_pop(&lo->lo_bio_list);
 		spin_unlock_irq(&lo->lo_lock);
 
 		BUG_ON(!bio);
@@ -841,7 +813,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
 	lo->old_gfp_mask = mapping_gfp_mask(mapping);
 	mapping_set_gfp_mask(mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));
 
-	lo->lo_bio = lo->lo_biotail = NULL;
+	bio_list_init(&lo->lo_bio_list);
 
 	/*
 	 * set queue make_request_fn, and add limits based on lower level
diff --git a/include/linux/bio.h b/include/linux/bio.h
index b89cf2d..9388c55 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -505,7 +505,7 @@ static inline int bio_has_data(struct bio *bio)
 }
 
 /*
- * BIO list managment for use by remapping drivers (e.g. DM or MD).
+ * BIO list management for use by remapping drivers (e.g. DM or MD).
  *
  * A bio_list anchors a singly-linked list of bios chained through the bi_next
  * member of the bio.  The bio_list also caches the last list member to allow
diff --git a/include/linux/loop.h b/include/linux/loop.h
index 4072544..66c194e 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -56,8 +56,7 @@ struct loop_device {
 	gfp_t		old_gfp_mask;
 
 	spinlock_t		lo_lock;
-	struct bio 		*lo_bio;
-	struct bio		*lo_biotail;
+	struct bio_list		lo_bio_list;
 	int			lo_state;
 	struct mutex		lo_ctl_mutex;
 	struct task_struct	*lo_thread;

Re: [PATCH] loop: use BIO list management functions

[Akinobu Mita]

On Fri, Apr 17, 2009 at 02:37:02AM -0400, Christoph Hellwig wrote:
> >  /*
> > - * BIO list managment for use by remapping drivers (e.g. DM or MD).
> > + * BIO list management for use by remapping drivers (e.g. DM or MD) and loop.
> >   *
> 
> 
> I don't think this needs changing, that was just examples.

OK. I kept only s/managment/management/ typo in new patch.

Re: [PATCH] loop: use BIO list management functions

[Jens Axboe]

On Fri, Apr 17 2009, Akinobu Mita wrote:
> On Fri, Apr 17, 2009 at 08:40:35AM +0200, Jens Axboe wrote:
> > On Fri, Apr 17 2009, Akinobu Mita wrote:
> > > Impact: cleanup
> > 
> > This looks good, but please provide a better description than just a
> > silly impact notice. I've filled it in for you for this one, but in the
> > future the description should say what the patch does.
> > 
> > It's queued for 2.6.31.
> 
> OK, I added the description and Christoph's feedbacks.

Technically, you should not include the bio.h change with this patch, as
it has nothing to do with loop at all.

> 
> Subject: loop: use BIO list management functions
> 
> There are BIO list management helpers in bio.h.
> This uses them to reduce code duplication in loopback driver.
> 
> Cc:  Jens Axboe <axboe@kernel.dk>
> Cc: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/block/loop.c |   42 +++++++-----------------------------------
>  include/linux/bio.h  |    2 +-
>  include/linux/loop.h |    3 +--
>  3 files changed, 9 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index ddae808..2b70545 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -506,35 +506,6 @@ out:
>  	return ret;
>  }
>  
> -/*
> - * Add bio to back of pending list
> - */
> -static void loop_add_bio(struct loop_device *lo, struct bio *bio)
> -{
> -	if (lo->lo_biotail) {
> -		lo->lo_biotail->bi_next = bio;
> -		lo->lo_biotail = bio;
> -	} else
> -		lo->lo_bio = lo->lo_biotail = bio;
> -}
> -
> -/*
> - * Grab first pending buffer
> - */
> -static struct bio *loop_get_bio(struct loop_device *lo)
> -{
> -	struct bio *bio;
> -
> -	if ((bio = lo->lo_bio)) {
> -		if (bio == lo->lo_biotail)
> -			lo->lo_biotail = NULL;
> -		lo->lo_bio = bio->bi_next;
> -		bio->bi_next = NULL;
> -	}
> -
> -	return bio;
> -}
> -
>  static int loop_make_request(struct request_queue *q, struct bio *old_bio)
>  {
>  	struct loop_device *lo = q->queuedata;
> @@ -550,7 +521,7 @@ static int 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;
> -	loop_add_bio(lo, old_bio);
> +	bio_list_add(&lo->lo_bio_list, old_bio);
>  	wake_up(&lo->lo_event);
>  	spin_unlock_irq(&lo->lo_lock);
>  	return 0;
> @@ -609,15 +580,16 @@ static int loop_thread(void *data)
>  
>  	set_user_nice(current, -20);
>  
> -	while (!kthread_should_stop() || lo->lo_bio) {
> +	while (!kthread_should_stop() || !bio_list_empty(&lo->lo_bio_list)) {
>  
>  		wait_event_interruptible(lo->lo_event,
> -				lo->lo_bio || kthread_should_stop());
> +				!bio_list_empty(&lo->lo_bio_list) ||
> +				kthread_should_stop());
>  
> -		if (!lo->lo_bio)
> +		if (bio_list_empty(&lo->lo_bio_list))
>  			continue;
>  		spin_lock_irq(&lo->lo_lock);
> -		bio = loop_get_bio(lo);
> +		bio = bio_list_pop(&lo->lo_bio_list);
>  		spin_unlock_irq(&lo->lo_lock);
>  
>  		BUG_ON(!bio);
> @@ -841,7 +813,7 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode,
>  	lo->old_gfp_mask = mapping_gfp_mask(mapping);
>  	mapping_set_gfp_mask(mapping, lo->old_gfp_mask & ~(__GFP_IO|__GFP_FS));
>  
> -	lo->lo_bio = lo->lo_biotail = NULL;
> +	bio_list_init(&lo->lo_bio_list);
>  
>  	/*
>  	 * set queue make_request_fn, and add limits based on lower level
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index b89cf2d..9388c55 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -505,7 +505,7 @@ static inline int bio_has_data(struct bio *bio)
>  }
>  
>  /*
> - * BIO list managment for use by remapping drivers (e.g. DM or MD).
> + * BIO list management for use by remapping drivers (e.g. DM or MD).
>   *
>   * A bio_list anchors a singly-linked list of bios chained through the bi_next
>   * member of the bio.  The bio_list also caches the last list member to allow
> diff --git a/include/linux/loop.h b/include/linux/loop.h
> index 4072544..66c194e 100644
> --- a/include/linux/loop.h
> +++ b/include/linux/loop.h
> @@ -56,8 +56,7 @@ struct loop_device {
>  	gfp_t		old_gfp_mask;
>  
>  	spinlock_t		lo_lock;
> -	struct bio 		*lo_bio;
> -	struct bio		*lo_biotail;
> +	struct bio_list		lo_bio_list;
>  	int			lo_state;
>  	struct mutex		lo_ctl_mutex;
>  	struct task_struct	*lo_thread;

-- 
Jens Axboe