Skip to contentSkip to conversation

ide-tape: remove pipeline functionality-v2

PATCH Applied Borislav Petkov 2 participants 17 messages Mar 9, 2008 – Mar 13, 2008 libata

lore
Share & tools
lore

Borislav Petkov

author
#0 lore raw

cover ide-tape: remove pipeline functionality-v2

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(-)
author
#1 lore raw

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.

Signed-off-by Borislav Petkov <[email protected]>
---
 drivers/ide/ide-tape.c |   28 ++++------------------------
 1 files changed, 4 insertions(+), 24 deletions(-)
drivers/ide/ide-tape.c +4 -24
index 792c76e..32ba6c8 100644
@@ -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) {
Signature · 1 line
1.5.4.1
#2 lore raw

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

Quoted 5 lines · 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 <[email protected]>

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 <[email protected]>
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 <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-tape.c |   13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)
drivers/ide/ide-tape.c +1 -12
Index: 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) {
author
#3 lore raw

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

Quoted 9 lines · Bartlomiej Zolnierkiewicz
Quoted 5 lines · 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 <[email protected]>

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 :).

Quoted 57 lines

From: Borislav Petkov <[email protected]>
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 <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-tape.c |   13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

Index: b/drivers/ide/ide-tape.c
===================================================================

drivers/ide/ide-tape.c +1 -12
@@ -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) {
Signature · 2 lines
Regards/Gruß,
    Boris.
author
#4 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/ide/ide-tape.c |  103 ++++++++++++------------------------------------
 1 files changed, 26 insertions(+), 77 deletions(-)
drivers/ide/ide-tape.c +26 -77
index 32ba6c8..46a5f95 100644
@@ -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;
Signature · 1 line
1.5.4.1
#5 lore raw

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

Quoted 3 lines · 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().

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

Quoted 6 lines

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?

Quoted 1 line
Signed-off-by Borislav Petkov <[email protected]>

How's about this version?

From: Borislav Petkov <[email protected]>
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 <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-tape.c |   55 +------------------------------------------------
 1 file changed, 2 insertions(+), 53 deletions(-)
drivers/ide/ide-tape.c +2 -53
Index: 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;
 }
 
 /*
author
#6 lore raw

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

Quoted 7 lines · Bartlomiej Zolnierkiewicz
Quoted 3 lines · 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().

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.

Quoted 9 lines
Quoted 6 lines

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.

Quoted 103 lines
Quoted 1 line
Signed-off-by Borislav Petkov <[email protected]>

How's about this version?

From: Borislav Petkov <[email protected]>
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 <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-tape.c |   55 +------------------------------------------------
 1 file changed, 2 insertions(+), 53 deletions(-)

Index: b/drivers/ide/ide-tape.c
===================================================================

drivers/ide/ide-tape.c +2 -53
@@ -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;
 }
 
 /*
Signature · 2 lines
Regards/Gruß,
    Boris.
#7 lore raw

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

Quoted 9 lines · Borislav Petkov
Quoted 7 lines · Bartlomiej Zolnierkiewicz
Quoted 3 lines · 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().

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

Quoted 8 lines

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) ]

Quoted 3 lines

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.

#8 lore raw

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

In reply to Borislav Petkov
Quoted 7 lines · Borislav Petkov
Quoted 7 lines
Quoted 6 lines

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.

Quoted 5 lines
Quoted 2 lines

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.

author
#9 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/ide/ide-tape.c |  131 ------------------------------------------------
 1 files changed, 0 insertions(+), 131 deletions(-)
drivers/ide/ide-tape.c +0 -131
index 46a5f95..a06d83f 100644
@@ -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 */
Signature · 1 line
1.5.4.1
Applied
#10 lore raw

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

Quoted 9 lines · 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

Quoted 8 lines

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 <[email protected]>

applied

author
#11 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/ide/ide-tape.c |   81 +++++++++++++-----------------------------------
 1 files changed, 22 insertions(+), 59 deletions(-)
drivers/ide/ide-tape.c +22 -59
index a06d83f..5afcbf4 100644
@@ -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");
Signature · 1 line
1.5.4.1
#12 lore raw

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

Quoted 8 lines · 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

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 ]

Quoted 3 lines

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

Quoted 1 line
Signed-off-by Borislav Petkov <[email protected]>

Seems like we can get away with:

From: Borislav Petkov <[email protected]>
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 <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-tape.c |   96 ++-----------------------------------------------
 1 file changed, 5 insertions(+), 91 deletions(-)
drivers/ide/ide-tape.c +5 -91
Index: 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)
author
#13 lore raw

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

Quoted 11 lines · Bartlomiej Zolnierkiewicz
Quoted 8 lines · 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

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

yep, this is me being overly cautious :).

Quoted 3 lines

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

Quoted 5 lines
Quoted 3 lines

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?

Quoted 172 lines
Quoted 1 line
Signed-off-by Borislav Petkov <[email protected]>

Seems like we can get away with:

From: Borislav Petkov <[email protected]>
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 <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-tape.c |   96 ++-----------------------------------------------
 1 file changed, 5 insertions(+), 91 deletions(-)

Index: b/drivers/ide/ide-tape.c
===================================================================

drivers/ide/ide-tape.c +5 -91
@@ -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)
Signature · 2 lines
Regards/Gruß,
    Boris.
#14 lore raw

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

Hi,

On Wednesday 12 March 2008, Borislav Petkov wrote:

[...]

Quoted 10 lines
Quoted 5 lines
Quoted 3 lines

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 ]

Quoted 2 lines

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

author
#15 lore raw

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

Quoted 13 lines · Bartlomiej Zolnierkiewicz

Hi,

On Wednesday 12 March 2008, Borislav Petkov wrote:

[...]

Quoted 6 lines
Quoted 5 lines
Quoted 3 lines

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

Quoted 7 lines
Quoted 4 lines

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! :)

Signature · 2 lines
Regards/Gruß,
    Boris.
#16 lore raw
In reply to Borislav Petkov

Hi,

Quoted 9 lines · 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(-)

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