[PATCH 0/4] ide-tape: remove pipeline functionality-v2

[PATCH 0/4] ide-tape: remove pipeline functionality-v2

[Borislav Petkov]

Hi Bart,

here are some patches removing the code for adding r/w requests to the pipeline.
Instead, they are being queued straight onto the device request queue now. In
addition, pipeline speed/control calculation has been removed since it becomes
also unused.

 drivers/ide/ide-tape.c |  334 +++++++-----------------------------------------
 1 files changed, 46 insertions(+), 288 deletions(-)

[PATCH 1/4] ide-tape: remove tape->cache_stage

[Borislav Petkov]

Prior to allocating a new pipeline stage, the code checked for the existence of
a cached pipeline stage to use. Do away with and stick to normal pipeline
stages only.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-tape.c |   28 ++++------------------------
 1 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 792c76e..32ba6c8 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -365,8 +365,6 @@ typedef struct ide_tape_obj {
 	idetape_stage_t *next_stage;
 	/* New requests will be added to the pipeline here */
 	idetape_stage_t *last_stage;
-	/* Optional free stage which we can use */
-	idetape_stage_t *cache_stage;
 	int pages_per_stage;
 	/* Wasted space in each stage */
 	int excess_bh_size;
@@ -1684,21 +1682,6 @@ abort:
 	return NULL;
 }
 
-static idetape_stage_t *idetape_kmalloc_stage(idetape_tape_t *tape)
-{
-	idetape_stage_t *cache_stage = tape->cache_stage;
-
-	debug_log(DBG_PROCS, "Enter %s\n", __func__);
-
-	if (tape->nr_stages >= tape->max_stages)
-		return NULL;
-	if (cache_stage != NULL) {
-		tape->cache_stage = NULL;
-		return cache_stage;
-	}
-	return __idetape_kmalloc_stage(tape, 0, 0);
-}
-
 static int idetape_copy_stage_from_user(idetape_tape_t *tape,
 		idetape_stage_t *stage, const char __user *buf, int n)
 {
@@ -2231,7 +2214,7 @@ static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)
 	debug_log(DBG_CHRDEV, "Enter %s\n", __func__);
 
 	/* Attempt to allocate a new stage. Beware possible race conditions. */
-	while ((new_stage = idetape_kmalloc_stage(tape)) == NULL) {
+	while ((new_stage = __idetape_kmalloc_stage(tape, 0, 0)) == NULL) {
 		spin_lock_irqsave(&tape->lock, flags);
 		if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {
 			idetape_wait_for_request(drive, tape->active_data_rq);
@@ -2448,13 +2431,13 @@ static int idetape_init_read(ide_drive_t *drive, int max_stages)
 	rq.current_nr_sectors = blocks;
 	if (!test_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags) &&
 	    tape->nr_stages < max_stages) {
-		new_stage = idetape_kmalloc_stage(tape);
+		new_stage = __idetape_kmalloc_stage(tape, 0, 0);
 		while (new_stage != NULL) {
 			new_stage->rq = rq;
 			idetape_add_stage_tail(drive, new_stage);
 			if (tape->nr_stages >= max_stages)
 				break;
-			new_stage = idetape_kmalloc_stage(tape);
+			new_stage = __idetape_kmalloc_stage(tape, 0, 0);
 		}
 	}
 	if (!test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {
@@ -3245,10 +3228,7 @@ static int idetape_chrdev_release(struct inode *inode, struct file *filp)
 		else
 			idetape_wait_for_pipeline(drive);
 	}
-	if (tape->cache_stage != NULL) {
-		__idetape_kfree_stage(tape->cache_stage);
-		tape->cache_stage = NULL;
-	}
+
 	if (minor < 128 && test_bit(IDETAPE_FLAG_MEDIUM_PRESENT, &tape->flags))
 		(void) idetape_rewind_tape(drive);
 	if (tape->chrdev_dir == IDETAPE_DIR_NONE) {
-- 
1.5.4.1

Re: [PATCH 1/4] ide-tape: remove tape->cache_stage

[Bartlomiej Zolnierkiewicz]

On Sunday 09 March 2008, Borislav Petkov wrote:
> Prior to allocating a new pipeline stage, the code checked for the existence of
> a cached pipeline stage to use. Do away with and stick to normal pipeline
> stages only.
> 
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>

I modified it slightly while merging since AFAICS we still need to check
'tape->nr_stages >= tape_max_stages' for idetape_add_chrdev_write_request().

From: Borislav Petkov <petkovbb@googlemail.com>
Subject: [PATCH 1/4] ide-tape: remove tape->cache_stage

Prior to allocating a new pipeline stage, the code checked for the existence of
a cached pipeline stage to use. Do away with and stick to normal pipeline
stages only.

[bart: keep idetape_kmalloc_stage() for now]

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-tape.c |   13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

Index: b/drivers/ide/ide-tape.c
===================================================================
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -365,8 +365,6 @@ typedef struct ide_tape_obj {
 	idetape_stage_t *next_stage;
 	/* New requests will be added to the pipeline here */
 	idetape_stage_t *last_stage;
-	/* Optional free stage which we can use */
-	idetape_stage_t *cache_stage;
 	int pages_per_stage;
 	/* Wasted space in each stage */
 	int excess_bh_size;
@@ -1686,16 +1684,10 @@ abort:
 
 static idetape_stage_t *idetape_kmalloc_stage(idetape_tape_t *tape)
 {
-	idetape_stage_t *cache_stage = tape->cache_stage;
-
 	debug_log(DBG_PROCS, "Enter %s\n", __func__);
 
 	if (tape->nr_stages >= tape->max_stages)
 		return NULL;
-	if (cache_stage != NULL) {
-		tape->cache_stage = NULL;
-		return cache_stage;
-	}
 	return __idetape_kmalloc_stage(tape, 0, 0);
 }
 
@@ -3245,10 +3237,7 @@ static int idetape_chrdev_release(struct
 		else
 			idetape_wait_for_pipeline(drive);
 	}
-	if (tape->cache_stage != NULL) {
-		__idetape_kfree_stage(tape->cache_stage);
-		tape->cache_stage = NULL;
-	}
+
 	if (minor < 128 && test_bit(IDETAPE_FLAG_MEDIUM_PRESENT, &tape->flags))
 		(void) idetape_rewind_tape(drive);
 	if (tape->chrdev_dir == IDETAPE_DIR_NONE) {

Re: [PATCH 1/4] ide-tape: remove tape->cache_stage

[Borislav Petkov]

On Tue, Mar 11, 2008 at 12:24:51AM +0100, Bartlomiej Zolnierkiewicz wrote:
> On Sunday 09 March 2008, Borislav Petkov wrote:
> > Prior to allocating a new pipeline stage, the code checked for the existence of
> > a cached pipeline stage to use. Do away with and stick to normal pipeline
> > stages only.
> > 
> > Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> 
> I modified it slightly while merging since AFAICS we still need to check
> 'tape->nr_stages >= tape_max_stages' for idetape_add_chrdev_write_request().

Yep, thanks for spotting that landmine. By the way this driver is full of it :).

> From: Borislav Petkov <petkovbb@googlemail.com>
> Subject: [PATCH 1/4] ide-tape: remove tape->cache_stage
> 
> Prior to allocating a new pipeline stage, the code checked for the existence of
> a cached pipeline stage to use. Do away with and stick to normal pipeline
> stages only.
> 
> [bart: keep idetape_kmalloc_stage() for now]
> 
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
>  drivers/ide/ide-tape.c |   13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> Index: b/drivers/ide/ide-tape.c
> ===================================================================
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -365,8 +365,6 @@ typedef struct ide_tape_obj {
>  	idetape_stage_t *next_stage;
>  	/* New requests will be added to the pipeline here */
>  	idetape_stage_t *last_stage;
> -	/* Optional free stage which we can use */
> -	idetape_stage_t *cache_stage;
>  	int pages_per_stage;
>  	/* Wasted space in each stage */
>  	int excess_bh_size;
> @@ -1686,16 +1684,10 @@ abort:
>  
>  static idetape_stage_t *idetape_kmalloc_stage(idetape_tape_t *tape)
>  {
> -	idetape_stage_t *cache_stage = tape->cache_stage;
> -
>  	debug_log(DBG_PROCS, "Enter %s\n", __func__);
>  
>  	if (tape->nr_stages >= tape->max_stages)
>  		return NULL;
> -	if (cache_stage != NULL) {
> -		tape->cache_stage = NULL;
> -		return cache_stage;
> -	}
>  	return __idetape_kmalloc_stage(tape, 0, 0);
>  }
>  
> @@ -3245,10 +3237,7 @@ static int idetape_chrdev_release(struct
>  		else
>  			idetape_wait_for_pipeline(drive);
>  	}
> -	if (tape->cache_stage != NULL) {
> -		__idetape_kfree_stage(tape->cache_stage);
> -		tape->cache_stage = NULL;
> -	}
> +
>  	if (minor < 128 && test_bit(IDETAPE_FLAG_MEDIUM_PRESENT, &tape->flags))
>  		(void) idetape_rewind_tape(drive);
>  	if (tape->chrdev_dir == IDETAPE_DIR_NONE) {

-- 
Regards/Gruß,
    Boris.

[PATCH 2/4] ide-tape: remove pipeline-specific code from idetape_add_chrdev_write_request

[Borislav Petkov]

Refrain from adding more write requests to the pipeline and queue them
directly on the device's request queue instead. Prior to that flush all
penging stages in the pipeline through idetape_wait_for_pipeline().

The remaining pipeline stage allocation code is used for the next current
pipeline stage (tape->merge_stage) and data buffer for an upcoming
request. The so allocated pipeline stage is rewired into the tape struct
thru idetape_switch_buffers() and used during the next request for
copying user data into it (see e.g. idetape_chrdev_write()). In case the
allocation fails, the current request is still attempted prior to failing.

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-tape.c |  103 ++++++++++++------------------------------------
 1 files changed, 26 insertions(+), 77 deletions(-)

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 32ba6c8..46a5f95 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -2194,83 +2194,6 @@ static void idetape_wait_first_stage(ide_drive_t *drive)
 }
 
 /*
- * Try to add a character device originated write request to our pipeline. In
- * case we don't succeed, we revert to non-pipelined operation mode for this
- * request. In order to accomplish that, we
- *
- * 1. Try to allocate a new pipeline stage.
- * 2. If we can't, wait for more and more requests to be serviced and try again
- * each time.
- * 3. If we still can't allocate a stage, fallback to non-pipelined operation
- * mode for this request.
- */
-static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)
-{
-	idetape_tape_t *tape = drive->driver_data;
-	idetape_stage_t *new_stage;
-	unsigned long flags;
-	struct request *rq;
-
-	debug_log(DBG_CHRDEV, "Enter %s\n", __func__);
-
-	/* Attempt to allocate a new stage. Beware possible race conditions. */
-	while ((new_stage = __idetape_kmalloc_stage(tape, 0, 0)) == NULL) {
-		spin_lock_irqsave(&tape->lock, flags);
-		if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {
-			idetape_wait_for_request(drive, tape->active_data_rq);
-			spin_unlock_irqrestore(&tape->lock, flags);
-		} else {
-			spin_unlock_irqrestore(&tape->lock, flags);
-			idetape_plug_pipeline(drive);
-			if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,
-					&tape->flags))
-				continue;
-			/*
-			 * The machine is short on memory. Fallback to non-
-			 * pipelined operation mode for this request.
-			 */
-			return idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE,
-						blocks, tape->merge_stage->bh);
-		}
-	}
-	rq = &new_stage->rq;
-	idetape_init_rq(rq, REQ_IDETAPE_WRITE);
-	/* Doesn't actually matter - We always assume sequential access */
-	rq->sector = tape->first_frame;
-	rq->current_nr_sectors = blocks;
-	rq->nr_sectors = blocks;
-
-	idetape_switch_buffers(tape, new_stage);
-	idetape_add_stage_tail(drive, new_stage);
-	tape->pipeline_head++;
-	idetape_calculate_speeds(drive);
-
-	/*
-	 * Estimate whether the tape has stopped writing by checking if our
-	 * write pipeline is currently empty. If we are not writing anymore,
-	 * wait for the pipeline to be almost completely full (90%) before
-	 * starting to service requests, so that we will be able to keep up with
-	 * the higher speeds of the tape.
-	 */
-	if (!test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {
-		if (tape->nr_stages >= tape->max_stages * 9 / 10 ||
-			tape->nr_stages >= tape->max_stages -
-			tape->uncontrolled_pipeline_head_speed * 3 * 1024 /
-			tape->blk_size) {
-			tape->measure_insert_time = 1;
-			tape->insert_time = jiffies;
-			tape->insert_size = 0;
-			tape->insert_speed = 0;
-			idetape_plug_pipeline(drive);
-		}
-	}
-	if (test_and_clear_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags))
-		/* Return a deferred error */
-		return -EIO;
-	return blocks;
-}
-
-/*
  * Wait until all pending pipeline requests are serviced. Typically called on
  * device close.
  */
@@ -2289,6 +2212,32 @@ static void idetape_wait_for_pipeline(ide_drive_t *drive)
 	}
 }
 
+/* Queue up a character device originated write request. */
+static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)
+{
+	idetape_tape_t *tape = drive->driver_data;
+	idetape_stage_t *new_stage;
+
+	debug_log(DBG_CHRDEV, "Enter %s\n", __func__);
+
+	idetape_wait_for_pipeline(drive);
+
+	idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE, blocks,
+			      tape->merge_stage->bh);
+	__idetape_kfree_stage(tape->merge_stage);
+
+	new_stage = __idetape_kmalloc_stage(tape, 0, 0);
+	if (!new_stage) {
+		printk(KERN_ERR "ide-tape: %s: cannot alloc request buffer.\n",
+			__func__);
+		return -ENOMEM;
+	}
+
+	idetape_switch_buffers(tape, new_stage);
+
+	return blocks;
+}
+
 static void idetape_empty_write_pipeline(ide_drive_t *drive)
 {
 	idetape_tape_t *tape = drive->driver_data;
-- 
1.5.4.1

Re: [PATCH 2/4] ide-tape: remove pipeline-specific code from idetape_add_chrdev_write_request

[Bartlomiej Zolnierkiewicz]

On Sunday 09 March 2008, Borislav Petkov wrote:
> Refrain from adding more write requests to the pipeline and queue them
> directly on the device's request queue instead. Prior to that flush all
> penging stages in the pipeline through idetape_wait_for_pipeline().

I would prefer to keep the original code for now
(it has some subtle differences).

> The remaining pipeline stage allocation code is used for the next current
> pipeline stage (tape->merge_stage) and data buffer for an upcoming
> request. The so allocated pipeline stage is rewired into the tape struct
> thru idetape_switch_buffers() and used during the next request for
> copying user data into it (see e.g. idetape_chrdev_write()). In case the
> allocation fails, the current request is still attempted prior to failing.

Is this really needed now that we've removed pipeline operation for write
requests?

> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>

How's about this version?

From: Borislav Petkov <petkovbb@googlemail.com>
Subject: [PATCH 2/4] ide-tape: remove pipeline-specific code from idetape_add_chrdev_write_request

Refrain from adding more write requests to the pipeline and queue them
directly on the device's request queue instead.

[bart: re-do for minimal behavior changes]

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-tape.c |   55 +------------------------------------------------
 1 file changed, 2 insertions(+), 53 deletions(-)

Index: b/drivers/ide/ide-tape.c
===================================================================
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -2202,28 +2202,16 @@ static void idetape_wait_first_stage(ide
 	spin_unlock_irqrestore(&tape->lock, flags);
 }
 
-/*
- * Try to add a character device originated write request to our pipeline. In
- * case we don't succeed, we revert to non-pipelined operation mode for this
- * request. In order to accomplish that, we
- *
- * 1. Try to allocate a new pipeline stage.
- * 2. If we can't, wait for more and more requests to be serviced and try again
- * each time.
- * 3. If we still can't allocate a stage, fallback to non-pipelined operation
- * mode for this request.
- */
+/* Queue up a character device originated write request. */
 static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)
 {
 	idetape_tape_t *tape = drive->driver_data;
-	idetape_stage_t *new_stage;
 	unsigned long flags;
-	struct request *rq;
 
 	debug_log(DBG_CHRDEV, "Enter %s\n", __func__);
 
 	/* Attempt to allocate a new stage. Beware possible race conditions. */
-	while ((new_stage = idetape_kmalloc_stage(tape)) == NULL) {
+	while (1) {
 		spin_lock_irqsave(&tape->lock, flags);
 		if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {
 			idetape_wait_for_request(drive, tape->active_data_rq);
@@ -2234,49 +2222,10 @@ static int idetape_add_chrdev_write_requ
 			if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,
 					&tape->flags))
 				continue;
-			/*
-			 * The machine is short on memory. Fallback to non-
-			 * pipelined operation mode for this request.
-			 */
 			return idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE,
 						blocks, tape->merge_stage->bh);
 		}
 	}
-	rq = &new_stage->rq;
-	idetape_init_rq(rq, REQ_IDETAPE_WRITE);
-	/* Doesn't actually matter - We always assume sequential access */
-	rq->sector = tape->first_frame;
-	rq->current_nr_sectors = blocks;
-	rq->nr_sectors = blocks;
-
-	idetape_switch_buffers(tape, new_stage);
-	idetape_add_stage_tail(drive, new_stage);
-	tape->pipeline_head++;
-	idetape_calculate_speeds(drive);
-
-	/*
-	 * Estimate whether the tape has stopped writing by checking if our
-	 * write pipeline is currently empty. If we are not writing anymore,
-	 * wait for the pipeline to be almost completely full (90%) before
-	 * starting to service requests, so that we will be able to keep up with
-	 * the higher speeds of the tape.
-	 */
-	if (!test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {
-		if (tape->nr_stages >= tape->max_stages * 9 / 10 ||
-			tape->nr_stages >= tape->max_stages -
-			tape->uncontrolled_pipeline_head_speed * 3 * 1024 /
-			tape->blk_size) {
-			tape->measure_insert_time = 1;
-			tape->insert_time = jiffies;
-			tape->insert_size = 0;
-			tape->insert_speed = 0;
-			idetape_plug_pipeline(drive);
-		}
-	}
-	if (test_and_clear_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags))
-		/* Return a deferred error */
-		return -EIO;
-	return blocks;
 }
 
 /*

Re: [PATCH 2/4] ide-tape: remove pipeline-specific code from idetape_add_chrdev_write_request

[Borislav Petkov]

On Tue, Mar 11, 2008 at 12:25:19AM +0100, Bartlomiej Zolnierkiewicz wrote:
> On Sunday 09 March 2008, Borislav Petkov wrote:
> > Refrain from adding more write requests to the pipeline and queue them
> > directly on the device's request queue instead. Prior to that flush all
> > penging stages in the pipeline through idetape_wait_for_pipeline().
> 
> I would prefer to keep the original code for now
> (it has some subtle differences).

Well, if you mean by this the while-loop below, the original code offloads
the pipeline gradually, stage-wise, until allocation succeeds, in contrast to
idetape_wait_for_pipeline() which iterates over all pending stages and flushes
them all in one go.

At a certain in point in time, however, the driver might land at the unlikely
state of still having some stages left in the pipeline while queueing all
incoming requests on the rq queue. Therefore, i'd prefer to make sure the
pipeline is empty before queueing. What is more, it is flushed only once, if
ever, so idetape_wait_for_pipeline() simply returns in subsequent calls and no
considerable performance penalties are imposed here.

> > The remaining pipeline stage allocation code is used for the next current
> > pipeline stage (tape->merge_stage) and data buffer for an upcoming
> > request. The so allocated pipeline stage is rewired into the tape struct
> > thru idetape_switch_buffers() and used during the next request for
> > copying user data into it (see e.g. idetape_chrdev_write()). In case the
> > allocation fails, the current request is still attempted prior to failing.
> 
> Is this really needed now that we've removed pipeline operation for write
> requests?

I did this simply to keep behavior changes at minimum - after removing the
pipeline code completely this'll be simplified too.

> > Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> 
> How's about this version?
> 
> From: Borislav Petkov <petkovbb@googlemail.com>
> Subject: [PATCH 2/4] ide-tape: remove pipeline-specific code from idetape_add_chrdev_write_request
> 
> Refrain from adding more write requests to the pipeline and queue them
> directly on the device's request queue instead.
> 
> [bart: re-do for minimal behavior changes]
> 
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
>  drivers/ide/ide-tape.c |   55 +------------------------------------------------
>  1 file changed, 2 insertions(+), 53 deletions(-)
> 
> Index: b/drivers/ide/ide-tape.c
> ===================================================================
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -2202,28 +2202,16 @@ static void idetape_wait_first_stage(ide
>  	spin_unlock_irqrestore(&tape->lock, flags);
>  }
>  
> -/*
> - * Try to add a character device originated write request to our pipeline. In
> - * case we don't succeed, we revert to non-pipelined operation mode for this
> - * request. In order to accomplish that, we
> - *
> - * 1. Try to allocate a new pipeline stage.
> - * 2. If we can't, wait for more and more requests to be serviced and try again
> - * each time.
> - * 3. If we still can't allocate a stage, fallback to non-pipelined operation
> - * mode for this request.
> - */
> +/* Queue up a character device originated write request. */
>  static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)
>  {
>  	idetape_tape_t *tape = drive->driver_data;
> -	idetape_stage_t *new_stage;
>  	unsigned long flags;
> -	struct request *rq;
>  
>  	debug_log(DBG_CHRDEV, "Enter %s\n", __func__);
>  
>  	/* Attempt to allocate a new stage. Beware possible race conditions. */
> -	while ((new_stage = idetape_kmalloc_stage(tape)) == NULL) {
> +	while (1) {
>  		spin_lock_irqsave(&tape->lock, flags);
>  		if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {
>  			idetape_wait_for_request(drive, tape->active_data_rq);
> @@ -2234,49 +2222,10 @@ static int idetape_add_chrdev_write_requ
>  			if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,
>  					&tape->flags))
>  				continue;
> -			/*
> -			 * The machine is short on memory. Fallback to non-
> -			 * pipelined operation mode for this request.
> -			 */
>  			return idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE,
>  						blocks, tape->merge_stage->bh);
>  		}
>  	}
> -	rq = &new_stage->rq;
> -	idetape_init_rq(rq, REQ_IDETAPE_WRITE);
> -	/* Doesn't actually matter - We always assume sequential access */
> -	rq->sector = tape->first_frame;
> -	rq->current_nr_sectors = blocks;
> -	rq->nr_sectors = blocks;
> -
> -	idetape_switch_buffers(tape, new_stage);
> -	idetape_add_stage_tail(drive, new_stage);
> -	tape->pipeline_head++;
> -	idetape_calculate_speeds(drive);
> -
> -	/*
> -	 * Estimate whether the tape has stopped writing by checking if our
> -	 * write pipeline is currently empty. If we are not writing anymore,
> -	 * wait for the pipeline to be almost completely full (90%) before
> -	 * starting to service requests, so that we will be able to keep up with
> -	 * the higher speeds of the tape.
> -	 */
> -	if (!test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {
> -		if (tape->nr_stages >= tape->max_stages * 9 / 10 ||
> -			tape->nr_stages >= tape->max_stages -
> -			tape->uncontrolled_pipeline_head_speed * 3 * 1024 /
> -			tape->blk_size) {
> -			tape->measure_insert_time = 1;
> -			tape->insert_time = jiffies;
> -			tape->insert_size = 0;
> -			tape->insert_speed = 0;
> -			idetape_plug_pipeline(drive);
> -		}
> -	}
> -	if (test_and_clear_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags))
> -		/* Return a deferred error */
> -		return -EIO;
> -	return blocks;
>  }
>  
>  /*

-- 
Regards/Gruß,
    Boris.

Re: [PATCH 2/4] ide-tape: remove pipeline-specific code from idetape_add_chrdev_write_request

[Bartlomiej Zolnierkiewicz]

On Wednesday 12 March 2008, Borislav Petkov wrote:
> On Tue, Mar 11, 2008 at 12:25:19AM +0100, Bartlomiej Zolnierkiewicz wrote:
> > On Sunday 09 March 2008, Borislav Petkov wrote:
> > > Refrain from adding more write requests to the pipeline and queue them
> > > directly on the device's request queue instead. Prior to that flush all
> > > penging stages in the pipeline through idetape_wait_for_pipeline().
> > 
> > I would prefer to keep the original code for now
> > (it has some subtle differences).
> 
> Well, if you mean by this the while-loop below, the original code offloads
> the pipeline gradually, stage-wise, until allocation succeeds, in contrast to
> idetape_wait_for_pipeline() which iterates over all pending stages and flushes
> them all in one go.
> 
> At a certain in point in time, however, the driver might land at the unlikely
> state of still having some stages left in the pipeline while queueing all
> incoming requests on the rq queue. Therefore, i'd prefer to make sure the

This is what could happen with the unmodified driver code also.

[ thus given that pipeline code goes away completely soon there is no point
  in changing the original behavior (unless of course it is buggy and I just
  fail to see it) ]

> pipeline is empty before queueing. What is more, it is flushed only once, if
> ever, so idetape_wait_for_pipeline() simply returns in subsequent calls and no
> considerable performance penalties are imposed here.

Re: [PATCH 2/4] ide-tape: remove pipeline-specific code from idetape_add_chrdev_write_request

[Bartlomiej Zolnierkiewicz]

On Wednesday 12 March 2008, Borislav Petkov wrote:

> > > The remaining pipeline stage allocation code is used for the next current
> > > pipeline stage (tape->merge_stage) and data buffer for an upcoming
> > > request. The so allocated pipeline stage is rewired into the tape struct
> > > thru idetape_switch_buffers() and used during the next request for
> > > copying user data into it (see e.g. idetape_chrdev_write()). In case the
> > > allocation fails, the current request is still attempted prior to failing.
> > 
> > Is this really needed now that we've removed pipeline operation for write
> > requests?
> 
> I did this simply to keep behavior changes at minimum - after removing the
> pipeline code completely this'll be simplified too.

BTW I see now how poorly I explained things the last time. :(

[ Sorry for that! ]

Our goal is not "pure" minimal behavior changes but minimal "behavior + code"
changes - IOW we are searching for the best balance for keeping both the old
behavior and code changes (and thus patch _complexity_) at the minimal level.

[PATCH 3/4] ide-tape remove pipeline speed/control calculations

[Borislav Petkov]

Pipeline handling calculations in idetape_calculate_speeds() can
go since they do not have any effect on other functionality besides:

1. info is only being exported through /proc as a read-only item
(controlled_pipeline_head_speed, uncontrolled_pipeline_head_speed)

2. used in idetape_restart_speed_control() which, in turn, is unrelated to
other code

3. used only for pipeline frames number accounting (tape->pipeline_head),
also unused elsewhere.

4.some variables are:
        only written to: tape->buffer_head;
        unused: tape->tape_head, tape->last_tape_head

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-tape.c |  131 ------------------------------------------------
 1 files changed, 0 insertions(+), 131 deletions(-)

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index 46a5f95..a06d83f 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -84,9 +84,6 @@ enum {
  * We start from MIN maximum stages (we will not even use MIN stages if we don't
  * need them), increment it by RATE*(MAX-MIN) whenever we sense that the
  * pipeline is empty, until we reach the optimum value or until we reach MAX.
- *
- * Setting the following parameter to 0 is illegal: the pipelined mode cannot be
- * disabled (idetape_calculate_speeds() divides by tape->max_stages.)
  */
 #define IDETAPE_MIN_PIPELINE_STAGES	  1
 #define IDETAPE_MAX_PIPELINE_STAGES	400
@@ -392,39 +389,12 @@ typedef struct ide_tape_obj {
 	 */
 	int postpone_cnt;
 
-	/*
-	 * Measures number of frames:
-	 *
-	 * 1. written/read to/from the driver pipeline (pipeline_head).
-	 * 2. written/read to/from the tape buffers (idetape_bh).
-	 * 3. written/read by the tape to/from the media (tape_head).
-	 */
-	int pipeline_head;
-	int buffer_head;
-	int tape_head;
-	int last_tape_head;
-
 	/* Speed control at the tape buffers input/output */
 	unsigned long insert_time;
 	int insert_size;
 	int insert_speed;
-	int max_insert_speed;
 	int measure_insert_time;
 
-	/* Speed regulation negative feedback loop */
-	int speed_control;
-	int pipeline_head_speed;
-	int controlled_pipeline_head_speed;
-	int uncontrolled_pipeline_head_speed;
-	int controlled_last_pipeline_head;
-	unsigned long uncontrolled_pipeline_head_time;
-	unsigned long controlled_pipeline_head_time;
-	int controlled_previous_pipeline_head;
-	int uncontrolled_previous_pipeline_head;
-	unsigned long controlled_previous_head_time;
-	unsigned long uncontrolled_previous_head_time;
-	int restart_speed_control_req;
-
 	u32 debug_mask;
 } idetape_tape_t;
 
@@ -1333,69 +1303,6 @@ static void idetape_create_mode_sense_cmd(struct ide_atapi_pc *pc, u8 page_code)
 	pc->idetape_callback = &idetape_pc_callback;
 }
 
-static void idetape_calculate_speeds(ide_drive_t *drive)
-{
-	idetape_tape_t *tape = drive->driver_data;
-
-	if (time_after(jiffies,
-			tape->controlled_pipeline_head_time + 120 * HZ)) {
-		tape->controlled_previous_pipeline_head =
-			tape->controlled_last_pipeline_head;
-		tape->controlled_previous_head_time =
-			tape->controlled_pipeline_head_time;
-		tape->controlled_last_pipeline_head = tape->pipeline_head;
-		tape->controlled_pipeline_head_time = jiffies;
-	}
-	if (time_after(jiffies, tape->controlled_pipeline_head_time + 60 * HZ))
-		tape->controlled_pipeline_head_speed = (tape->pipeline_head -
-				tape->controlled_last_pipeline_head) * 32 * HZ /
-				(jiffies - tape->controlled_pipeline_head_time);
-	else if (time_after(jiffies, tape->controlled_previous_head_time))
-		tape->controlled_pipeline_head_speed = (tape->pipeline_head -
-				tape->controlled_previous_pipeline_head) * 32 *
-			HZ / (jiffies - tape->controlled_previous_head_time);
-
-	if (tape->nr_pending_stages < tape->max_stages/*- 1 */) {
-		/* -1 for read mode error recovery */
-		if (time_after(jiffies, tape->uncontrolled_previous_head_time +
-					10 * HZ)) {
-			tape->uncontrolled_pipeline_head_time = jiffies;
-			tape->uncontrolled_pipeline_head_speed =
-				(tape->pipeline_head -
-				 tape->uncontrolled_previous_pipeline_head) *
-				32 * HZ / (jiffies -
-					tape->uncontrolled_previous_head_time);
-		}
-	} else {
-		tape->uncontrolled_previous_head_time = jiffies;
-		tape->uncontrolled_previous_pipeline_head = tape->pipeline_head;
-		if (time_after(jiffies, tape->uncontrolled_pipeline_head_time +
-					30 * HZ))
-			tape->uncontrolled_pipeline_head_time = jiffies;
-
-	}
-	tape->pipeline_head_speed = max(tape->uncontrolled_pipeline_head_speed,
-					tape->controlled_pipeline_head_speed);
-
-	if (tape->speed_control == 1) {
-		if (tape->nr_pending_stages >= tape->max_stages / 2)
-			tape->max_insert_speed = tape->pipeline_head_speed +
-				(1100 - tape->pipeline_head_speed) * 2 *
-				(tape->nr_pending_stages - tape->max_stages / 2)
-				/ tape->max_stages;
-		else
-			tape->max_insert_speed = 500 +
-				(tape->pipeline_head_speed - 500) * 2 *
-				tape->nr_pending_stages / tape->max_stages;
-
-		if (tape->nr_pending_stages >= tape->max_stages * 99 / 100)
-			tape->max_insert_speed = 5000;
-	} else
-		tape->max_insert_speed = tape->speed_control;
-
-	tape->max_insert_speed = max(tape->max_insert_speed, 500);
-}
-
 static ide_startstop_t idetape_media_access_finished(ide_drive_t *drive)
 {
 	idetape_tape_t *tape = drive->driver_data;
@@ -1548,7 +1455,6 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 	if (time_after(jiffies, tape->insert_time))
 		tape->insert_speed = tape->insert_size / 1024 * HZ /
 					(jiffies - tape->insert_time);
-	idetape_calculate_speeds(drive);
 	if (!test_and_clear_bit(IDETAPE_FLAG_IGNORE_DSC, &tape->flags) &&
 	    (stat & SEEK_STAT) == 0) {
 		if (postponed_rq == NULL) {
@@ -1572,7 +1478,6 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 		return ide_stopped;
 	}
 	if (rq->cmd[0] & REQ_IDETAPE_READ) {
-		tape->buffer_head++;
 		tape->postpone_cnt = 0;
 		pc = idetape_next_pc_storage(drive);
 		idetape_create_read_cmd(tape, pc, rq->current_nr_sectors,
@@ -1580,7 +1485,6 @@ static ide_startstop_t idetape_do_request(ide_drive_t *drive,
 		goto out;
 	}
 	if (rq->cmd[0] & REQ_IDETAPE_WRITE) {
-		tape->buffer_head++;
 		tape->postpone_cnt = 0;
 		pc = idetape_next_pc_storage(drive);
 		idetape_create_write_cmd(tape, pc, rq->current_nr_sectors,
@@ -2312,24 +2216,6 @@ static void idetape_empty_write_pipeline(ide_drive_t *drive)
 	}
 }
 
-static void idetape_restart_speed_control(ide_drive_t *drive)
-{
-	idetape_tape_t *tape = drive->driver_data;
-
-	tape->restart_speed_control_req = 0;
-	tape->pipeline_head = 0;
-	tape->controlled_last_pipeline_head = 0;
-	tape->controlled_previous_pipeline_head = 0;
-	tape->uncontrolled_previous_pipeline_head = 0;
-	tape->controlled_pipeline_head_speed = 5000;
-	tape->pipeline_head_speed = 5000;
-	tape->uncontrolled_pipeline_head_speed = 0;
-	tape->controlled_pipeline_head_time =
-		tape->uncontrolled_pipeline_head_time = jiffies;
-	tape->controlled_previous_head_time =
-		tape->uncontrolled_previous_head_time = jiffies;
-}
-
 static int idetape_init_read(ide_drive_t *drive, int max_stages)
 {
 	idetape_tape_t *tape = drive->driver_data;
@@ -2372,8 +2258,6 @@ static int idetape_init_read(ide_drive_t *drive, int max_stages)
 			}
 		}
 	}
-	if (tape->restart_speed_control_req)
-		idetape_restart_speed_control(drive);
 	idetape_init_rq(&rq, REQ_IDETAPE_READ);
 	rq.sector = tape->first_frame;
 	rq.nr_sectors = blocks;
@@ -2442,8 +2326,6 @@ static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)
 		spin_lock_irqsave(&tape->lock, flags);
 		idetape_remove_stage_head(drive);
 		spin_unlock_irqrestore(&tape->lock, flags);
-		tape->pipeline_head++;
-		idetape_calculate_speeds(drive);
 	}
 	if (bytes_read > blocks * tape->blk_size) {
 		printk(KERN_ERR "ide-tape: bug: trying to return more bytes"
@@ -2778,8 +2660,6 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
 	}
 	if (count == 0)
 		return (0);
-	if (tape->restart_speed_control_req)
-		idetape_restart_speed_control(drive);
 	if (tape->merge_stage_size) {
 		if (tape->merge_stage_size >= tape->stage_size) {
 			printk(KERN_ERR "ide-tape: bug: merge buf too big\n");
@@ -2988,7 +2868,6 @@ static int idetape_chrdev_ioctl(struct inode *inode, struct file *file,
 
 	debug_log(DBG_CHRDEV, "Enter %s, cmd=%u\n", __func__, cmd);
 
-	tape->restart_speed_control_req = 1;
 	if (tape->chrdev_dir == IDETAPE_DIR_WRITE) {
 		idetape_empty_write_pipeline(drive);
 		idetape_flush_tape_buffers(drive);
@@ -3131,8 +3010,6 @@ static int idetape_chrdev_open(struct inode *inode, struct file *filp)
 			}
 		}
 	}
-	idetape_restart_speed_control(drive);
-	tape->restart_speed_control_req = 0;
 	return 0;
 
 out_put_tape:
@@ -3335,12 +3212,6 @@ static void idetape_add_settings(ide_drive_t *drive)
 			NULL);
 	ide_add_setting(drive, "dsc_overlap", SETTING_RW, TYPE_BYTE, 0, 1, 1,
 			1, &drive->dsc_overlap, NULL);
-	ide_add_setting(drive, "pipeline_head_speed_c", SETTING_READ, TYPE_INT,
-			0, 0xffff, 1, 1, &tape->controlled_pipeline_head_speed,
-			NULL);
-	ide_add_setting(drive, "pipeline_head_speed_u", SETTING_READ, TYPE_INT,
-			0, 0xffff, 1, 1,
-			&tape->uncontrolled_pipeline_head_speed, NULL);
 	ide_add_setting(drive, "avg_speed", SETTING_READ, TYPE_INT, 0, 0xffff,
 			1, 1, &tape->avg_speed, NULL);
 	ide_add_setting(drive, "debug_mask", SETTING_RW, TYPE_INT, 0, 0xffff, 1,
@@ -3386,8 +3257,6 @@ static void idetape_setup(ide_drive_t *drive, idetape_tape_t *tape, int minor)
 	tape->name[2] = '0' + minor;
 	tape->chrdev_dir = IDETAPE_DIR_NONE;
 	tape->pc = tape->pc_stack;
-	tape->max_insert_speed = 10000;
-	tape->speed_control = 1;
 	*((unsigned short *) &gcw) = drive->id->config;
 
 	/* Command packet DRQ type */
-- 
1.5.4.1

Re: [PATCH 3/4] ide-tape remove pipeline speed/control calculations

[Bartlomiej Zolnierkiewicz]

On Sunday 09 March 2008, Borislav Petkov wrote:
> Pipeline handling calculations in idetape_calculate_speeds() can
> go since they do not have any effect on other functionality besides:
> 
> 1. info is only being exported through /proc as a read-only item
> (controlled_pipeline_head_speed, uncontrolled_pipeline_head_speed)
> 
> 2. used in idetape_restart_speed_control() which, in turn, is unrelated to
> other code
> 
> 3. used only for pipeline frames number accounting (tape->pipeline_head),
> also unused elsewhere.
> 
> 4.some variables are:
>         only written to: tape->buffer_head;
>         unused: tape->tape_head, tape->last_tape_head
> 
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>

applied

[PATCH 4/4] ide-tape: remove pipeline-specific code from idetape_add_chrdev_read_request()

[Borislav Petkov]

In order to do away with queueing read requests on the pipeline, several things
have to be done:

1. Do not allocate additional pipeline stages in idetape_init_read() until
(tape->nr_stages < max_stages) and do only read operation preparations. As a
collateral result, idetape_add_stage_tail() becomes unused so remove it.

2. Wait for all queued pipeline requests to complete before queueing
the read request's buffer directly thru idetape_queue_rw_tail()

3. Do next request buffer allocation (tape->merge_stage)

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
---
 drivers/ide/ide-tape.c |   81 +++++++++++++-----------------------------------
 1 files changed, 22 insertions(+), 59 deletions(-)

diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index a06d83f..5afcbf4 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -1673,29 +1673,6 @@ static void idetape_switch_buffers(idetape_tape_t *tape, idetape_stage_t *stage)
 	idetape_init_merge_stage(tape);
 }
 
-/* Add a new stage at the end of the pipeline. */
-static void idetape_add_stage_tail(ide_drive_t *drive, idetape_stage_t *stage)
-{
-	idetape_tape_t *tape = drive->driver_data;
-	unsigned long flags;
-
-	debug_log(DBG_PROCS, "Enter %s\n", __func__);
-
-	spin_lock_irqsave(&tape->lock, flags);
-	stage->next = NULL;
-	if (tape->last_stage != NULL)
-		tape->last_stage->next = stage;
-	else
-		tape->first_stage = stage;
-		tape->next_stage  = stage;
-	tape->last_stage = stage;
-	if (tape->next_stage == NULL)
-		tape->next_stage = tape->last_stage;
-	tape->nr_stages++;
-	tape->nr_pending_stages++;
-	spin_unlock_irqrestore(&tape->lock, flags);
-}
-
 /* Install a completion in a pending request and sleep until it is serviced. The
  * caller should ensure that the request will not be serviced before we install
  * the completion (usually by disabling interrupts).
@@ -2219,10 +2196,7 @@ static void idetape_empty_write_pipeline(ide_drive_t *drive)
 static int idetape_init_read(ide_drive_t *drive, int max_stages)
 {
 	idetape_tape_t *tape = drive->driver_data;
-	idetape_stage_t *new_stage;
-	struct request rq;
 	int bytes_read;
-	u16 blocks = *(u16 *)&tape->caps[12];
 
 	/* Initialize read operation */
 	if (tape->chrdev_dir != IDETAPE_DIR_READ) {
@@ -2258,21 +2232,7 @@ static int idetape_init_read(ide_drive_t *drive, int max_stages)
 			}
 		}
 	}
-	idetape_init_rq(&rq, REQ_IDETAPE_READ);
-	rq.sector = tape->first_frame;
-	rq.nr_sectors = blocks;
-	rq.current_nr_sectors = blocks;
-	if (!test_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags) &&
-	    tape->nr_stages < max_stages) {
-		new_stage = __idetape_kmalloc_stage(tape, 0, 0);
-		while (new_stage != NULL) {
-			new_stage->rq = rq;
-			idetape_add_stage_tail(drive, new_stage);
-			if (tape->nr_stages >= max_stages)
-				break;
-			new_stage = __idetape_kmalloc_stage(tape, 0, 0);
-		}
-	}
+
 	if (!test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {
 		if (tape->nr_pending_stages >= 3 * max_stages / 4) {
 			tape->measure_insert_time = 1;
@@ -2292,7 +2252,7 @@ static int idetape_init_read(ide_drive_t *drive, int max_stages)
 static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)
 {
 	idetape_tape_t *tape = drive->driver_data;
-	unsigned long flags;
+	idetape_stage_t *new_stage;
 	struct request *rq_ptr;
 	int bytes_read;
 
@@ -2302,31 +2262,34 @@ static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)
 	if (test_bit(IDETAPE_FLAG_FILEMARK, &tape->flags))
 		return 0;
 
-	/* Wait for the next block to reach the head of the pipeline. */
 	idetape_init_read(drive, tape->max_stages);
-	if (tape->first_stage == NULL) {
-		if (test_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags))
-			return 0;
-		return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks,
-					tape->merge_stage->bh);
-	}
-	idetape_wait_first_stage(drive);
-	rq_ptr = &tape->first_stage->rq;
-	bytes_read = tape->blk_size * (rq_ptr->nr_sectors -
-					rq_ptr->current_nr_sectors);
-	rq_ptr->nr_sectors = 0;
-	rq_ptr->current_nr_sectors = 0;
 
+	idetape_wait_for_pipeline(drive);
+
+	if (test_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags))
+		return 0;
+
+	bytes_read = idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks,
+					   tape->merge_stage->bh);
+
+	rq_ptr = &tape->merge_stage->rq;
 	if (rq_ptr->errors == IDETAPE_ERROR_EOD)
 		return 0;
 	else {
-		idetape_switch_buffers(tape, tape->first_stage);
 		if (rq_ptr->errors == IDETAPE_ERROR_FILEMARK)
 			set_bit(IDETAPE_FLAG_FILEMARK, &tape->flags);
-		spin_lock_irqsave(&tape->lock, flags);
-		idetape_remove_stage_head(drive);
-		spin_unlock_irqrestore(&tape->lock, flags);
+
+		__idetape_kfree_stage(tape->merge_stage);
+
+		new_stage = __idetape_kmalloc_stage(tape, 0, 0);
+		if (!new_stage) {
+			printk(KERN_ERR "ide-tape: %s: cannot alloc request "
+					"buffer.\n", __func__);
+			return -ENOMEM;
+		}
+		idetape_switch_buffers(tape, new_stage);
 	}
+
 	if (bytes_read > blocks * tape->blk_size) {
 		printk(KERN_ERR "ide-tape: bug: trying to return more bytes"
 				" than requested\n");
-- 
1.5.4.1

Re: [PATCH 4/4] ide-tape: remove pipeline-specific code from idetape_add_chrdev_read_request()

[Bartlomiej Zolnierkiewicz]

On Sunday 09 March 2008, Borislav Petkov wrote:
> In order to do away with queueing read requests on the pipeline, several things
> have to be done:
> 
> 1. Do not allocate additional pipeline stages in idetape_init_read() until
> (tape->nr_stages < max_stages) and do only read operation preparations. As a
> collateral result, idetape_add_stage_tail() becomes unused so remove it.
> 
> 2. Wait for all queued pipeline requests to complete before queueing

Hmm, but we've just removed all pipeline requests with this patch?

[ ->first_stage and ->next_stage are always NULL after this patch which makes
  it possible to remove the rest of (now never executed) code for pipeline
  support, same for idetape_plug_pipeline() and IDETAPE_FLAG_PIPELINE* flags ]

> the read request's buffer directly thru idetape_queue_rw_tail()
> 
> 3. Do next request buffer allocation (tape->merge_stage)

Isn't idetape_init_read() taking care of 3.?

> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>

Seems like we can get away with:

From: Borislav Petkov <petkovbb@googlemail.com>
Subject: [PATCH 4/4] ide-tape: remove pipeline-specific code from idetape_add_chrdev_read_request()

In order to do away with queueing read requests on the pipeline, several things
have to be done:

1. Do not allocate additional pipeline stages in idetape_init_read() until
(tape->nr_stages < max_stages) and do only read operation preparations. As a
collateral result, idetape_add_stage_tail() becomes unused so remove it.

2. Queue the read request's buffer directly thru idetape_queue_rw_tail().

3. Remove now unused idetape_kmalloc_stage() and idetape_switch_buffers().

[bart: simplify the original patch]

Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-tape.c |   96 ++-----------------------------------------------
 1 file changed, 5 insertions(+), 91 deletions(-)

Index: b/drivers/ide/ide-tape.c
===================================================================
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -1586,15 +1586,6 @@ abort:
 	return NULL;
 }
 
-static idetape_stage_t *idetape_kmalloc_stage(idetape_tape_t *tape)
-{
-	debug_log(DBG_PROCS, "Enter %s\n", __func__);
-
-	if (tape->nr_stages >= tape->max_stages)
-		return NULL;
-	return __idetape_kmalloc_stage(tape, 0, 0);
-}
-
 static int idetape_copy_stage_from_user(idetape_tape_t *tape,
 		idetape_stage_t *stage, const char __user *buf, int n)
 {
@@ -1672,39 +1663,6 @@ static void idetape_init_merge_stage(ide
 	}
 }
 
-static void idetape_switch_buffers(idetape_tape_t *tape, idetape_stage_t *stage)
-{
-	struct idetape_bh *tmp;
-
-	tmp = stage->bh;
-	stage->bh = tape->merge_stage->bh;
-	tape->merge_stage->bh = tmp;
-	idetape_init_merge_stage(tape);
-}
-
-/* Add a new stage at the end of the pipeline. */
-static void idetape_add_stage_tail(ide_drive_t *drive, idetape_stage_t *stage)
-{
-	idetape_tape_t *tape = drive->driver_data;
-	unsigned long flags;
-
-	debug_log(DBG_PROCS, "Enter %s\n", __func__);
-
-	spin_lock_irqsave(&tape->lock, flags);
-	stage->next = NULL;
-	if (tape->last_stage != NULL)
-		tape->last_stage->next = stage;
-	else
-		tape->first_stage = stage;
-		tape->next_stage  = stage;
-	tape->last_stage = stage;
-	if (tape->next_stage == NULL)
-		tape->next_stage = tape->last_stage;
-	tape->nr_stages++;
-	tape->nr_pending_stages++;
-	spin_unlock_irqrestore(&tape->lock, flags);
-}
-
 /* Install a completion in a pending request and sleep until it is serviced. The
  * caller should ensure that the request will not be serviced before we install
  * the completion (usually by disabling interrupts).
@@ -2228,10 +2186,7 @@ static void idetape_empty_write_pipeline
 static int idetape_init_read(ide_drive_t *drive, int max_stages)
 {
 	idetape_tape_t *tape = drive->driver_data;
-	idetape_stage_t *new_stage;
-	struct request rq;
 	int bytes_read;
-	u16 blocks = *(u16 *)&tape->caps[12];
 
 	/* Initialize read operation */
 	if (tape->chrdev_dir != IDETAPE_DIR_READ) {
@@ -2267,21 +2222,7 @@ static int idetape_init_read(ide_drive_t
 			}
 		}
 	}
-	idetape_init_rq(&rq, REQ_IDETAPE_READ);
-	rq.sector = tape->first_frame;
-	rq.nr_sectors = blocks;
-	rq.current_nr_sectors = blocks;
-	if (!test_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags) &&
-	    tape->nr_stages < max_stages) {
-		new_stage = idetape_kmalloc_stage(tape);
-		while (new_stage != NULL) {
-			new_stage->rq = rq;
-			idetape_add_stage_tail(drive, new_stage);
-			if (tape->nr_stages >= max_stages)
-				break;
-			new_stage = idetape_kmalloc_stage(tape);
-		}
-	}
+
 	if (!test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {
 		if (tape->nr_pending_stages >= 3 * max_stages / 4) {
 			tape->measure_insert_time = 1;
@@ -2301,9 +2242,6 @@ static int idetape_init_read(ide_drive_t
 static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)
 {
 	idetape_tape_t *tape = drive->driver_data;
-	unsigned long flags;
-	struct request *rq_ptr;
-	int bytes_read;
 
 	debug_log(DBG_PROCS, "Enter %s, %d blocks\n", __func__, blocks);
 
@@ -2311,37 +2249,13 @@ static int idetape_add_chrdev_read_reque
 	if (test_bit(IDETAPE_FLAG_FILEMARK, &tape->flags))
 		return 0;
 
-	/* Wait for the next block to reach the head of the pipeline. */
 	idetape_init_read(drive, tape->max_stages);
-	if (tape->first_stage == NULL) {
-		if (test_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags))
-			return 0;
-		return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks,
-					tape->merge_stage->bh);
-	}
-	idetape_wait_first_stage(drive);
-	rq_ptr = &tape->first_stage->rq;
-	bytes_read = tape->blk_size * (rq_ptr->nr_sectors -
-					rq_ptr->current_nr_sectors);
-	rq_ptr->nr_sectors = 0;
-	rq_ptr->current_nr_sectors = 0;
 
-	if (rq_ptr->errors == IDETAPE_ERROR_EOD)
+	if (test_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags))
 		return 0;
-	else {
-		idetape_switch_buffers(tape, tape->first_stage);
-		if (rq_ptr->errors == IDETAPE_ERROR_FILEMARK)
-			set_bit(IDETAPE_FLAG_FILEMARK, &tape->flags);
-		spin_lock_irqsave(&tape->lock, flags);
-		idetape_remove_stage_head(drive);
-		spin_unlock_irqrestore(&tape->lock, flags);
-	}
-	if (bytes_read > blocks * tape->blk_size) {
-		printk(KERN_ERR "ide-tape: bug: trying to return more bytes"
-				" than requested\n");
-		bytes_read = blocks * tape->blk_size;
-	}
-	return (bytes_read);
+
+	return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks,
+				     tape->merge_stage->bh);
 }
 
 static void idetape_pad_zeros(ide_drive_t *drive, int bcount)

Re: [PATCH 4/4] ide-tape: remove pipeline-specific code from idetape_add_chrdev_read_request()

[Borislav Petkov]

On Tue, Mar 11, 2008 at 12:25:50AM +0100, Bartlomiej Zolnierkiewicz wrote:
> On Sunday 09 March 2008, Borislav Petkov wrote:
> > In order to do away with queueing read requests on the pipeline, several things
> > have to be done:
> > 
> > 1. Do not allocate additional pipeline stages in idetape_init_read() until
> > (tape->nr_stages < max_stages) and do only read operation preparations. As a
> > collateral result, idetape_add_stage_tail() becomes unused so remove it.
> > 
> > 2. Wait for all queued pipeline requests to complete before queueing
> 
> Hmm, but we've just removed all pipeline requests with this patch?

yep, this is me being overly cautious :).

> [ ->first_stage and ->next_stage are always NULL after this patch which makes
>   it possible to remove the rest of (now never executed) code for pipeline
>   support, same for idetape_plug_pipeline() and IDETAPE_FLAG_PIPELINE* flags ]

this'll happen next.

> > the read request's buffer directly thru idetape_queue_rw_tail()
> > 
> > 3. Do next request buffer allocation (tape->merge_stage)
> 
> Isn't idetape_init_read() taking care of 3.?

i wanted to have the whole handling at one place and let _init_read() only
prepare the read. Now we don't allocate any new tape->merge_stage anymore,
which is wrong. Originally, this happened in _init_read(), however, if we do
idetape_queue_rw_tail(), we should alloc the new stage _after_ queueing the
request, which means it cannot happen _init_read() now and should take place
afterwards, i.e. as it was in the original patch, or?

> > Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> 
> Seems like we can get away with:
> 
> From: Borislav Petkov <petkovbb@googlemail.com>
> Subject: [PATCH 4/4] ide-tape: remove pipeline-specific code from idetape_add_chrdev_read_request()
> 
> In order to do away with queueing read requests on the pipeline, several things
> have to be done:
> 
> 1. Do not allocate additional pipeline stages in idetape_init_read() until
> (tape->nr_stages < max_stages) and do only read operation preparations. As a
> collateral result, idetape_add_stage_tail() becomes unused so remove it.
> 
> 2. Queue the read request's buffer directly thru idetape_queue_rw_tail().
> 
> 3. Remove now unused idetape_kmalloc_stage() and idetape_switch_buffers().
> 
> [bart: simplify the original patch]
> 
> Signed-off-by: Borislav Petkov <petkovbb@gmail.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
>  drivers/ide/ide-tape.c |   96 ++-----------------------------------------------
>  1 file changed, 5 insertions(+), 91 deletions(-)
> 
> Index: b/drivers/ide/ide-tape.c
> ===================================================================
> --- a/drivers/ide/ide-tape.c
> +++ b/drivers/ide/ide-tape.c
> @@ -1586,15 +1586,6 @@ abort:
>  	return NULL;
>  }
>  
> -static idetape_stage_t *idetape_kmalloc_stage(idetape_tape_t *tape)
> -{
> -	debug_log(DBG_PROCS, "Enter %s\n", __func__);
> -
> -	if (tape->nr_stages >= tape->max_stages)
> -		return NULL;
> -	return __idetape_kmalloc_stage(tape, 0, 0);
> -}
> -
>  static int idetape_copy_stage_from_user(idetape_tape_t *tape,
>  		idetape_stage_t *stage, const char __user *buf, int n)
>  {
> @@ -1672,39 +1663,6 @@ static void idetape_init_merge_stage(ide
>  	}
>  }
>  
> -static void idetape_switch_buffers(idetape_tape_t *tape, idetape_stage_t *stage)
> -{
> -	struct idetape_bh *tmp;
> -
> -	tmp = stage->bh;
> -	stage->bh = tape->merge_stage->bh;
> -	tape->merge_stage->bh = tmp;
> -	idetape_init_merge_stage(tape);
> -}
> -
> -/* Add a new stage at the end of the pipeline. */
> -static void idetape_add_stage_tail(ide_drive_t *drive, idetape_stage_t *stage)
> -{
> -	idetape_tape_t *tape = drive->driver_data;
> -	unsigned long flags;
> -
> -	debug_log(DBG_PROCS, "Enter %s\n", __func__);
> -
> -	spin_lock_irqsave(&tape->lock, flags);
> -	stage->next = NULL;
> -	if (tape->last_stage != NULL)
> -		tape->last_stage->next = stage;
> -	else
> -		tape->first_stage = stage;
> -		tape->next_stage  = stage;
> -	tape->last_stage = stage;
> -	if (tape->next_stage == NULL)
> -		tape->next_stage = tape->last_stage;
> -	tape->nr_stages++;
> -	tape->nr_pending_stages++;
> -	spin_unlock_irqrestore(&tape->lock, flags);
> -}
> -
>  /* Install a completion in a pending request and sleep until it is serviced. The
>   * caller should ensure that the request will not be serviced before we install
>   * the completion (usually by disabling interrupts).
> @@ -2228,10 +2186,7 @@ static void idetape_empty_write_pipeline
>  static int idetape_init_read(ide_drive_t *drive, int max_stages)
>  {
>  	idetape_tape_t *tape = drive->driver_data;
> -	idetape_stage_t *new_stage;
> -	struct request rq;
>  	int bytes_read;
> -	u16 blocks = *(u16 *)&tape->caps[12];
>  
>  	/* Initialize read operation */
>  	if (tape->chrdev_dir != IDETAPE_DIR_READ) {
> @@ -2267,21 +2222,7 @@ static int idetape_init_read(ide_drive_t
>  			}
>  		}
>  	}
> -	idetape_init_rq(&rq, REQ_IDETAPE_READ);
> -	rq.sector = tape->first_frame;
> -	rq.nr_sectors = blocks;
> -	rq.current_nr_sectors = blocks;
> -	if (!test_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags) &&
> -	    tape->nr_stages < max_stages) {
> -		new_stage = idetape_kmalloc_stage(tape);
> -		while (new_stage != NULL) {
> -			new_stage->rq = rq;
> -			idetape_add_stage_tail(drive, new_stage);
> -			if (tape->nr_stages >= max_stages)
> -				break;
> -			new_stage = idetape_kmalloc_stage(tape);
> -		}
> -	}
> +
>  	if (!test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {
>  		if (tape->nr_pending_stages >= 3 * max_stages / 4) {
>  			tape->measure_insert_time = 1;
> @@ -2301,9 +2242,6 @@ static int idetape_init_read(ide_drive_t
>  static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)
>  {
>  	idetape_tape_t *tape = drive->driver_data;
> -	unsigned long flags;
> -	struct request *rq_ptr;
> -	int bytes_read;
>  
>  	debug_log(DBG_PROCS, "Enter %s, %d blocks\n", __func__, blocks);
>  
> @@ -2311,37 +2249,13 @@ static int idetape_add_chrdev_read_reque
>  	if (test_bit(IDETAPE_FLAG_FILEMARK, &tape->flags))
>  		return 0;
>  
> -	/* Wait for the next block to reach the head of the pipeline. */
>  	idetape_init_read(drive, tape->max_stages);
> -	if (tape->first_stage == NULL) {
> -		if (test_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags))
> -			return 0;
> -		return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks,
> -					tape->merge_stage->bh);
> -	}
> -	idetape_wait_first_stage(drive);
> -	rq_ptr = &tape->first_stage->rq;
> -	bytes_read = tape->blk_size * (rq_ptr->nr_sectors -
> -					rq_ptr->current_nr_sectors);
> -	rq_ptr->nr_sectors = 0;
> -	rq_ptr->current_nr_sectors = 0;
>  
> -	if (rq_ptr->errors == IDETAPE_ERROR_EOD)
> +	if (test_bit(IDETAPE_FLAG_PIPELINE_ERR, &tape->flags))
>  		return 0;
> -	else {
> -		idetape_switch_buffers(tape, tape->first_stage);
> -		if (rq_ptr->errors == IDETAPE_ERROR_FILEMARK)
> -			set_bit(IDETAPE_FLAG_FILEMARK, &tape->flags);
> -		spin_lock_irqsave(&tape->lock, flags);
> -		idetape_remove_stage_head(drive);
> -		spin_unlock_irqrestore(&tape->lock, flags);
> -	}
> -	if (bytes_read > blocks * tape->blk_size) {
> -		printk(KERN_ERR "ide-tape: bug: trying to return more bytes"
> -				" than requested\n");
> -		bytes_read = blocks * tape->blk_size;
> -	}
> -	return (bytes_read);
> +
> +	return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks,
> +				     tape->merge_stage->bh);
>  }
>  
>  static void idetape_pad_zeros(ide_drive_t *drive, int bcount)

-- 
Regards/Gruß,
    Boris.

Re: [PATCH 4/4] ide-tape: remove pipeline-specific code from idetape_add_chrdev_read_request()

[Bartlomiej Zolnierkiewicz]

Hi,

On Wednesday 12 March 2008, Borislav Petkov wrote:

[...]

> > > the read request's buffer directly thru idetape_queue_rw_tail()
> > > 
> > > 3. Do next request buffer allocation (tape->merge_stage)
> > 
> > Isn't idetape_init_read() taking care of 3.?
> 
> i wanted to have the whole handling at one place and let _init_read() only
> prepare the read. Now we don't allocate any new tape->merge_stage anymore,
> which is wrong. Originally, this happened in _init_read(), however, if we do
> idetape_queue_rw_tail(), we should alloc the new stage _after_ queueing the

The original driver doesn't do this - it just calls idetape_queue_rw_tail(),
could it be a bug in the original driver?

[ ditto for idetape_queue_rw_tail() for writes ]

> request, which means it cannot happen _init_read() now and should take place
> afterwards, i.e. as it was in the original patch, or?

Re: [PATCH 4/4] ide-tape: remove pipeline-specific code from idetape_add_chrdev_read_request()

[Borislav Petkov]

On Wed, Mar 12, 2008 at 02:51:23PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Wednesday 12 March 2008, Borislav Petkov wrote:
> 
> [...]
> 
> > > > the read request's buffer directly thru idetape_queue_rw_tail()
> > > > 
> > > > 3. Do next request buffer allocation (tape->merge_stage)
> > > 
> > > Isn't idetape_init_read() taking care of 3.?
> > 
> > i wanted to have the whole handling at one place and let _init_read() only
> > prepare the read. Now we don't allocate any new tape->merge_stage anymore,
> > which is wrong. Originally, this happened in _init_read(), however, if we do
> > idetape_queue_rw_tail(), we should alloc the new stage _after_ queueing the
> 
> The original driver doesn't do this - it just calls idetape_queue_rw_tail(),
> could it be a bug in the original driver?

Damn, i see it now, idetape_queue_rw_tail() queues the request and then simply
_reuses_ the tape->merge_stage buffer by doing

        if (tape->merge_stage)
		idetape_init_merge_stage(tape);

so no need for reallocation. Whew! :)

-- 
Regards/Gruß,
    Boris.

Re: [PATCH 0/4] ide-tape: remove pipeline functionality-v2

[Bartlomiej Zolnierkiewicz]

Hi,

On Sunday 09 March 2008, Borislav Petkov wrote:
> Hi Bart,
> 
> here are some patches removing the code for adding r/w requests to the pipeline.
> Instead, they are being queued straight onto the device request queue now. In
> addition, pipeline speed/control calculation has been removed since it becomes
> also unused.
> 
>  drivers/ide/ide-tape.c |  334 +++++++-----------------------------------------
>  1 files changed, 46 insertions(+), 288 deletions(-)

Thanks for re-doing the patches, I applied everything (it seems that thanks
to the recast the changes become more obvious and we may now simplify patches
even more - please see the other mails).

Bart