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

This is a discussion on [PATCH 0/4] ide-tape: remove pipeline functionality-v2 - Kernel ; 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 ...

+ Reply to Thread
Results 1 to 14 of 14

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

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

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

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

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

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

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


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

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

    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


    How's about this version?

    From: Borislav Petkov
    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
    Signed-off-by: Bartlomiej Zolnierkiewicz
    ---
    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;
    }

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

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

    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


    Seems like we can get away with:

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

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

    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


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

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

    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


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

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

    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

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

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

    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

    >
    > How's about this version?
    >
    > From: Borislav Petkov
    > 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
    > Signed-off-by: Bartlomiej Zolnierkiewicz
    > ---
    > 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.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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.

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

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

    [...]

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

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

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

    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.

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

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

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

+ Reply to Thread