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

[ message continues ]

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 ...
[ message continues ]

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 ...
[ message continues ]

I would prefer to keep the original code for now

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

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 ...
[ message continues ]

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

I did this simply to keep behavior changes at minimum - after removing the

-- 
Regards/Gruß,
    Boris.
--

[ message continues ]

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

[ message continues ]

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.

--

[ message continues ]

applied
--

[ message continues ]

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 = ...
[ message continues ]

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

[ message continues ]

-- 
Regards/Gruß,
    Boris.
--

[ message continues ]

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 ...
[ message continues ]

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


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 = ...
[ message continues ]

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

-- 
Regards/Gruß,
    Boris.
--

[ message continues ]

Hi,

On Wednesday 12 March 2008, Borislav Petkov wrote:


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

--

[ message continues ]

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

[ message continues ]

Hi,


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

[ message continues ]