[PATCH 0/5] ide-tape: refit tape data buffer bits/kill pipelining - Kernel

This is a discussion on [PATCH 0/5] ide-tape: refit tape data buffer bits/kill pipelining - Kernel ; Hi Bart, This patchset refits the tape->merge_stage pipeline stage into tape->bh, a singly linked list of idetape_bh's, each of which is a tag attached to one or more pages serving as a buffer for data requests. The original functionality is ...

+ Reply to Thread
Results 1 to 8 of 8

Thread: [PATCH 0/5] ide-tape: refit tape data buffer bits/kill pipelining

  1. [PATCH 0/5] ide-tape: refit tape data buffer bits/kill pipelining

    Hi Bart,

    This patchset refits the tape->merge_stage pipeline stage into tape->bh, a
    singly linked list of idetape_bh's, each of which is a tag attached to one or
    more pages serving as a buffer for data requests. The original functionality
    is kept wrt collecting data in the buffer and flushing it to the hardware
    when full.

    Currently, the data rq's are serialized on the block interface and it
    would probably be better to return immediately after issuing it over
    ide_do_drive_cmd() in order to increase throughput, proper locking and testing
    implied of course.

    drivers/ide/ide-tape.c | 393 +++++++++++++++++-------------------------------
    1 files changed, 135 insertions(+), 258 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 4/5] ide-tape: improve buffer pages freeing strategy

    Instead of freeing pages one by one, free them 2^order-wise. Also, mv
    __idetape_kfree_stage() to ide_tape_kfree_buffer().

    Signed-off-by: Borislav Petkov
    ---
    drivers/ide/ide-tape.c | 37 +++++++++++++++++++------------------
    1 files changed, 19 insertions(+), 18 deletions(-)

    diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
    index cf4351c..dcaefef 100644
    --- a/drivers/ide/ide-tape.c
    +++ b/drivers/ide/ide-tape.c
    @@ -583,20 +583,21 @@ static void idetape_analyze_error(ide_drive_t *drive, u8 *sense)
    }
    }

    -/* Free a stage along with its related buffers completely. */
    -static void __idetape_kfree_stage(idetape_stage_t *stage)
    +/* Free data buffers completely. */
    +static void ide_tape_kfree_buffer(idetape_stage_t *stage)
    {
    struct idetape_bh *prev_bh, *bh = stage->bh;
    - int size;
    -
    - while (bh != NULL) {
    - if (bh->b_data != NULL) {
    - size = (int) bh->b_size;
    - while (size > 0) {
    - free_page((unsigned long) bh->b_data);
    - size -= PAGE_SIZE;
    - bh->b_data += PAGE_SIZE;
    - }
    +
    + while (bh) {
    + u32 size = bh->b_size;
    +
    + while (size) {
    + unsigned int order = fls(size >> PAGE_SHIFT)-1;
    +
    + if (bh->b_data)
    + free_pages((unsigned long)bh->b_data, order);
    +
    + size &= (order-1);
    }
    prev_bh = bh;
    bh = bh->b_reqnext;
    @@ -1373,7 +1374,7 @@ static idetape_stage_t *ide_tape_kmalloc_buffer(idetape_tape_t *tape, int full,
    atomic_sub(tape->excess_bh_size, &bh->b_count);
    return stage;
    abort:
    - __idetape_kfree_stage(stage);
    + ide_tape_kfree_buffer(stage);
    return NULL;
    }

    @@ -1649,7 +1650,7 @@ static int __idetape_discard_read_pipeline(ide_drive_t *drive)
    clear_bit(IDETAPE_FLAG_FILEMARK, &tape->flags);
    tape->merge_stage_size = 0;
    if (tape->merge_stage != NULL) {
    - __idetape_kfree_stage(tape->merge_stage);
    + ide_tape_kfree_buffer(tape->merge_stage);
    tape->merge_stage = NULL;
    }

    @@ -1828,7 +1829,7 @@ static void idetape_empty_write_pipeline(ide_drive_t *drive)
    tape->merge_stage_size = 0;
    }
    if (tape->merge_stage != NULL) {
    - __idetape_kfree_stage(tape->merge_stage);
    + ide_tape_kfree_buffer(tape->merge_stage);
    tape->merge_stage = NULL;
    }
    tape->chrdev_dir = IDETAPE_DIR_NONE;
    @@ -1866,7 +1867,7 @@ static int idetape_init_read(ide_drive_t *drive)
    REQ_IDETAPE_READ, 0,
    tape->merge_stage->bh);
    if (bytes_read < 0) {
    - __idetape_kfree_stage(tape->merge_stage);
    + ide_tape_kfree_buffer(tape->merge_stage);
    tape->merge_stage = NULL;
    tape->chrdev_dir = IDETAPE_DIR_NONE;
    return bytes_read;
    @@ -2145,7 +2146,7 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
    REQ_IDETAPE_WRITE, 0,
    tape->merge_stage->bh);
    if (retval < 0) {
    - __idetape_kfree_stage(tape->merge_stage);
    + ide_tape_kfree_buffer(tape->merge_stage);
    tape->merge_stage = NULL;
    tape->chrdev_dir = IDETAPE_DIR_NONE;
    return retval;
    @@ -2512,7 +2513,7 @@ static void idetape_write_release(ide_drive_t *drive, unsigned int minor)
    if (tape->merge_stage != NULL) {
    idetape_pad_zeros(drive, tape->blk_size *
    (tape->user_bs_factor - 1));
    - __idetape_kfree_stage(tape->merge_stage);
    + ide_tape_kfree_buffer(tape->merge_stage);
    tape->merge_stage = NULL;
    }
    idetape_write_filemark(drive);
    --
    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 5/5] ide-tape: remove the last remains of pipelining

    This patch converts the tape->merge_stage pipeline stage into tape->bh, a singly
    linked list of idetape_bh's, each of which is a tag attached to one or more pages
    serving as a data buffer for chrdev requests. In particular,

    1. makes tape->bh the data buffer of size tape->buffer_size which is computed
    from the Continuous Transfer Limit value in the caps page and the tape block
    size. The chrdev rw routines use this buffer as an intermediary location to
    shuffle data to and from.

    2. mv tape->merge_stage_size => tape->cur_buf_size as it contains the offset
    within tape->bh

    3. get rid of pipeline stage idetape_stage_t, tape->merge_stage
    and pipeline-related functions __idetape_discard_read_pipeline(),
    idetape_discard_read_pipeline(), idetape_empty_write_pipeline()

    4. code chunk "if (tape->merge_stage_size) {...}" in idetape_chrdev_read() is not
    needed since tape->merge_stage_size, tape->cur_buf_size resp., is zeroed out in
    idetape_init_read() couple of lines above.

    Signed-off-by: Borislav Petkov
    ---
    drivers/ide/ide-tape.c | 267 ++++++++++++------------------------------------
    1 files changed, 65 insertions(+), 202 deletions(-)

    diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
    index dcaefef..c7507c7 100644
    --- a/drivers/ide/ide-tape.c
    +++ b/drivers/ide/ide-tape.c
    @@ -215,13 +215,6 @@ enum {
    IDETAPE_FLAG_MEDIUM_PRESENT = (1 << 6),
    };

    -/* A pipeline stage. */
    -typedef struct idetape_stage_s {
    - struct request rq; /* The corresponding request */
    - struct idetape_bh *bh; /* The data buffers */
    - struct idetape_stage_s *next; /* Pointer to the next stage */
    -} idetape_stage_t;
    -
    /*
    * Most of our global data which we need to save even as we leave the driver due
    * to an interrupt or a timer event is stored in the struct defined below.
    @@ -309,8 +302,7 @@ typedef struct ide_tape_obj {

    /* Data buffer size chosen based on the tape's recommendation */
    int buffer_size;
    - idetape_stage_t *merge_stage;
    - int merge_stage_size;
    + int cur_buf_size;
    struct idetape_bh *bh;
    char *b_data;
    int b_count;
    @@ -584,9 +576,9 @@ static void idetape_analyze_error(ide_drive_t *drive, u8 *sense)
    }

    /* Free data buffers completely. */
    -static void ide_tape_kfree_buffer(idetape_stage_t *stage)
    +static void ide_tape_kfree_buffer(idetape_tape_t *tape)
    {
    - struct idetape_bh *prev_bh, *bh = stage->bh;
    + struct idetape_bh *prev_bh, *bh = tape->bh;

    while (bh) {
    u32 size = bh->b_size;
    @@ -603,7 +595,7 @@ static void ide_tape_kfree_buffer(idetape_stage_t *stage)
    bh = bh->b_reqnext;
    kfree(prev_bh);
    }
    - kfree(stage);
    + kfree(bh);
    }

    static int idetape_end_request(ide_drive_t *drive, int uptodate, int nr_sects)
    @@ -1297,22 +1289,16 @@ out:
    * It returns a pointer to the newly allocated buffer, or NULL in case of
    * failure.
    */
    -static idetape_stage_t *ide_tape_kmalloc_buffer(idetape_tape_t *tape, int full,
    - int clear)
    +static struct idetape_bh *ide_tape_kmalloc_buffer(idetape_tape_t *tape,
    + int full, int clear)
    {
    - idetape_stage_t *stage;
    - struct idetape_bh *prev_bh, *bh;
    + struct idetape_bh *prev_bh, *bh, *tape_bh;
    int pages = tape->pages_per_buffer;
    unsigned int order, b_allocd;
    char *b_data = NULL;

    - stage = kmalloc(sizeof(idetape_stage_t), GFP_KERNEL);
    - if (!stage)
    - return NULL;
    - stage->next = NULL;
    -
    - stage->bh = kmalloc(sizeof(struct idetape_bh), GFP_KERNEL);
    - bh = stage->bh;
    + tape_bh = kmalloc(sizeof(struct idetape_bh), GFP_KERNEL);
    + bh = tape_bh;
    if (bh == NULL)
    goto abort;

    @@ -1372,9 +1358,9 @@ static idetape_stage_t *ide_tape_kmalloc_buffer(idetape_tape_t *tape, int full,
    bh->b_size -= tape->excess_bh_size;
    if (full)
    atomic_sub(tape->excess_bh_size, &bh->b_count);
    - return stage;
    + return tape_bh;
    abort:
    - ide_tape_kfree_buffer(stage);
    + ide_tape_kfree_buffer(tape);
    return NULL;
    }

    @@ -1442,9 +1428,9 @@ static int idetape_copy_stage_to_user(idetape_tape_t *tape, char __user *buf,
    return ret;
    }

    -static void idetape_init_merge_stage(idetape_tape_t *tape)
    +static void ide_tape_init_data_buf(idetape_tape_t *tape)
    {
    - struct idetape_bh *bh = tape->merge_stage->bh;
    + struct idetape_bh *bh = tape->bh;

    tape->bh = bh;
    if (tape->chrdev_dir == IDETAPE_DIR_WRITE)
    @@ -1640,25 +1626,6 @@ static int idetape_create_prevent_cmd(ide_drive_t *drive,
    return 1;
    }

    -static int __idetape_discard_read_pipeline(ide_drive_t *drive)
    -{
    - idetape_tape_t *tape = drive->driver_data;
    -
    - if (tape->chrdev_dir != IDETAPE_DIR_READ)
    - return 0;
    -
    - clear_bit(IDETAPE_FLAG_FILEMARK, &tape->flags);
    - tape->merge_stage_size = 0;
    - if (tape->merge_stage != NULL) {
    - ide_tape_kfree_buffer(tape->merge_stage);
    - tape->merge_stage = NULL;
    - }
    -
    - tape->chrdev_dir = IDETAPE_DIR_NONE;
    -
    - return 0;
    -}
    -
    /*
    * Position the tape to the requested block using the LOCATE packet command.
    * A READ POSITION command is then issued to check where we are positioned. Like
    @@ -1668,12 +1635,9 @@ static int __idetape_discard_read_pipeline(ide_drive_t *drive)
    static int idetape_position_tape(ide_drive_t *drive, unsigned int block,
    u8 partition, int skip)
    {
    - idetape_tape_t *tape = drive->driver_data;
    - int retval;
    struct ide_atapi_pc pc;
    + int retval;

    - if (tape->chrdev_dir == IDETAPE_DIR_READ)
    - __idetape_discard_read_pipeline(drive);
    idetape_wait_ready(drive, 60 * 5 * HZ);
    idetape_create_locate_cmd(drive, &pc, block, partition, skip);
    retval = idetape_queue_pc_tail(drive, &pc);
    @@ -1684,25 +1648,6 @@ static int idetape_position_tape(ide_drive_t *drive, unsigned int block,
    return (idetape_queue_pc_tail(drive, &pc));
    }

    -static void idetape_discard_read_pipeline(ide_drive_t *drive,
    - int restore_position)
    -{
    - idetape_tape_t *tape = drive->driver_data;
    - int cnt;
    - int seek, position;
    -
    - cnt = __idetape_discard_read_pipeline(drive);
    - if (restore_position) {
    - position = idetape_read_position(drive);
    - seek = position > cnt ? position - cnt : 0;
    - if (idetape_position_tape(drive, seek, 0, 0)) {
    - printk(KERN_INFO "ide-tape: %s: position_tape failed in"
    - " discard_pipeline()\n", tape->name);
    - return;
    - }
    - }
    -}
    -
    /*
    * Generate a read/write request for the block device interface and wait for it
    * to be serviced.
    @@ -1726,8 +1671,8 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
    if ((cmd & (REQ_IDETAPE_READ | REQ_IDETAPE_WRITE)) == 0)
    return 0;

    - if (tape->merge_stage)
    - idetape_init_merge_stage(tape);
    + if (tape->bh)
    + ide_tape_init_data_buf(tape);
    if (rq.errors == IDETAPE_ERROR_GENERAL)
    return -EIO;
    return (tape->blk_size * (blocks-rq.current_nr_sectors));
    @@ -1778,61 +1723,7 @@ static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)
    debug_log(DBG_CHRDEV, "Enter %s\n", __func__);

    return idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE,
    - blocks, tape->merge_stage->bh);
    -}
    -
    -static void idetape_empty_write_pipeline(ide_drive_t *drive)
    -{
    - idetape_tape_t *tape = drive->driver_data;
    - int blocks, min;
    - struct idetape_bh *bh;
    -
    - if (tape->chrdev_dir != IDETAPE_DIR_WRITE) {
    - printk(KERN_ERR "ide-tape: bug: Trying to empty write pipeline,"
    - " but we are not writing.\n");
    - return;
    - }
    - if (tape->merge_stage_size > tape->buffer_size) {
    - printk(KERN_ERR "ide-tape: bug: merge_buffer too big\n");
    - tape->merge_stage_size = tape->buffer_size;
    - }
    - if (tape->merge_stage_size) {
    - blocks = tape->merge_stage_size / tape->blk_size;
    - if (tape->merge_stage_size % tape->blk_size) {
    - unsigned int i;
    -
    - blocks++;
    - i = tape->blk_size - tape->merge_stage_size %
    - tape->blk_size;
    - bh = tape->bh->b_reqnext;
    - while (bh) {
    - atomic_set(&bh->b_count, 0);
    - bh = bh->b_reqnext;
    - }
    - bh = tape->bh;
    - while (i) {
    - if (bh == NULL) {
    - printk(KERN_INFO "ide-tape: bug,"
    - " bh NULL\n");
    - break;
    - }
    - min = min(i, (unsigned int)(bh->b_size -
    - atomic_read(&bh->b_count)));
    - memset(bh->b_data + atomic_read(&bh->b_count),
    - 0, min);
    - atomic_add(min, &bh->b_count);
    - i -= min;
    - bh = bh->b_reqnext;
    - }
    - }
    - (void) idetape_add_chrdev_write_request(drive, blocks);
    - tape->merge_stage_size = 0;
    - }
    - if (tape->merge_stage != NULL) {
    - ide_tape_kfree_buffer(tape->merge_stage);
    - tape->merge_stage = NULL;
    - }
    - tape->chrdev_dir = IDETAPE_DIR_NONE;
    + blocks, tape->bh);
    }

    static int idetape_init_read(ide_drive_t *drive)
    @@ -1842,17 +1733,16 @@ static int idetape_init_read(ide_drive_t *drive)

    /* Initialize read operation */
    if (tape->chrdev_dir != IDETAPE_DIR_READ) {
    - if (tape->chrdev_dir == IDETAPE_DIR_WRITE) {
    - idetape_empty_write_pipeline(drive);
    + if (tape->chrdev_dir == IDETAPE_DIR_WRITE)
    idetape_flush_tape_buffers(drive);
    +
    + if (tape->cur_buf_size) {
    + printk(KERN_ERR "ide-tape: cur_buf_size should be"
    + " 0 now\n");
    + tape->cur_buf_size = 0;
    }
    - if (tape->merge_stage || tape->merge_stage_size) {
    - printk(KERN_ERR "ide-tape: merge_stage_size should be"
    - " 0 now\n");
    - tape->merge_stage_size = 0;
    - }
    - tape->merge_stage = ide_tape_kmalloc_buffer(tape, 0, 0);
    - if (!tape->merge_stage)
    + tape->bh = ide_tape_kmalloc_buffer(tape, 0, 0);
    + if (!tape->bh)
    return -ENOMEM;
    tape->chrdev_dir = IDETAPE_DIR_READ;

    @@ -1865,10 +1755,10 @@ static int idetape_init_read(ide_drive_t *drive)
    if (drive->dsc_overlap) {
    bytes_read = idetape_queue_rw_tail(drive,
    REQ_IDETAPE_READ, 0,
    - tape->merge_stage->bh);
    + tape->bh);
    if (bytes_read < 0) {
    - ide_tape_kfree_buffer(tape->merge_stage);
    - tape->merge_stage = NULL;
    + ide_tape_kfree_buffer(tape);
    + tape->bh = NULL;
    tape->chrdev_dir = IDETAPE_DIR_NONE;
    return bytes_read;
    }
    @@ -1892,7 +1782,7 @@ static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)
    idetape_init_read(drive);

    return idetape_queue_rw_tail(drive, REQ_IDETAPE_READ, blocks,
    - tape->merge_stage->bh);
    + tape->bh);
    }

    static void idetape_pad_zeros(ide_drive_t *drive, int bcount)
    @@ -1904,7 +1794,7 @@ static void idetape_pad_zeros(ide_drive_t *drive, int bcount)
    while (bcount) {
    unsigned int count;

    - bh = tape->merge_stage->bh;
    + bh = tape->bh;
    count = min(tape->buffer_size, bcount);
    bcount -= count;
    blocks = count / tape->blk_size;
    @@ -1916,7 +1806,7 @@ static void idetape_pad_zeros(ide_drive_t *drive, int bcount)
    bh = bh->b_reqnext;
    }
    idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE, blocks,
    - tape->merge_stage->bh);
    + tape->bh);
    }
    }

    @@ -1995,16 +1885,11 @@ static int idetape_space_over_filemarks(ide_drive_t *drive, short mt_op,
    }

    if (tape->chrdev_dir == IDETAPE_DIR_READ) {
    - tape->merge_stage_size = 0;
    + tape->cur_buf_size = 0;
    if (test_and_clear_bit(IDETAPE_FLAG_FILEMARK, &tape->flags))
    ++count;
    - idetape_discard_read_pipeline(drive, 0);
    }

    - /*
    - * The filemark was not found in our internal pipeline; now we can issue
    - * the space command.
    - */
    switch (mt_op) {
    case MTFSF:
    case MTBSF:
    @@ -2065,15 +1950,7 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
    return rc;
    if (count == 0)
    return (0);
    - if (tape->merge_stage_size) {
    - actually_read = min((unsigned int)(tape->merge_stage_size),
    - (unsigned int)count);
    - if (idetape_copy_stage_to_user(tape, buf, actually_read))
    - ret = -EFAULT;
    - buf += actually_read;
    - tape->merge_stage_size -= actually_read;
    - count -= actually_read;
    - }
    +
    while (count >= tape->buffer_size) {
    bytes_read = idetape_add_chrdev_read_request(drive, ctl);
    if (bytes_read <= 0)
    @@ -2092,7 +1969,7 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
    if (idetape_copy_stage_to_user(tape, buf, temp))
    ret = -EFAULT;
    actually_read += temp;
    - tape->merge_stage_size = bytes_read-temp;
    + tape->cur_buf_size = bytes_read-temp;
    }
    finish:
    if (!actually_read && test_bit(IDETAPE_FLAG_FILEMARK, &tape->flags)) {
    @@ -2122,18 +1999,16 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,

    /* Initialize write operation */
    if (tape->chrdev_dir != IDETAPE_DIR_WRITE) {
    - if (tape->chrdev_dir == IDETAPE_DIR_READ)
    - idetape_discard_read_pipeline(drive, 1);
    - if (tape->merge_stage || tape->merge_stage_size) {
    - printk(KERN_ERR "ide-tape: merge_stage_size "
    - "should be 0 now\n");
    - tape->merge_stage_size = 0;
    + if (tape->cur_buf_size) {
    + printk(KERN_ERR "ide-tape: cur_buf_size should be "
    + "0 now\n");
    + tape->cur_buf_size = 0;
    }
    - tape->merge_stage = ide_tape_kmalloc_buffer(tape, 0, 0);
    - if (!tape->merge_stage)
    + tape->bh = ide_tape_kmalloc_buffer(tape, 0, 0);
    + if (!tape->bh)
    return -ENOMEM;
    tape->chrdev_dir = IDETAPE_DIR_WRITE;
    - idetape_init_merge_stage(tape);
    + ide_tape_init_data_buf(tape);

    /*
    * Issue a write 0 command to ensure that DSC handshake is
    @@ -2144,39 +2019,42 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
    if (drive->dsc_overlap) {
    ssize_t retval = idetape_queue_rw_tail(drive,
    REQ_IDETAPE_WRITE, 0,
    - tape->merge_stage->bh);
    + tape->bh);
    if (retval < 0) {
    - ide_tape_kfree_buffer(tape->merge_stage);
    - tape->merge_stage = NULL;
    + ide_tape_kfree_buffer(tape);
    + tape->bh = NULL;
    tape->chrdev_dir = IDETAPE_DIR_NONE;
    return retval;
    }
    }
    }
    if (count == 0)
    - return (0);
    - if (tape->merge_stage_size) {
    - if (tape->merge_stage_size >= tape->buffer_size) {
    + return 0;
    +
    + if (tape->cur_buf_size) {
    + if (tape->cur_buf_size >= tape->buffer_size) {
    printk(KERN_ERR "ide-tape: bug: merge buf too big\n");
    - tape->merge_stage_size = 0;
    + tape->cur_buf_size = 0;
    }
    actually_written = min((unsigned int)
    - (tape->buffer_size - tape->merge_stage_size),
    + (tape->buffer_size - tape->cur_buf_size),
    (unsigned int)count);
    if (idetape_copy_stage_from_user(tape, buf, actually_written))
    ret = -EFAULT;
    buf += actually_written;
    - tape->merge_stage_size += actually_written;
    + tape->cur_buf_size += actually_written;
    count -= actually_written;

    - if (tape->merge_stage_size == tape->buffer_size) {
    + /* buffer full, flush data to tape */
    + if (tape->cur_buf_size == tape->buffer_size) {
    ssize_t retval;
    - tape->merge_stage_size = 0;
    + tape->cur_buf_size = 0;
    retval = idetape_add_chrdev_write_request(drive, ctl);
    if (retval <= 0)
    return (retval);
    }
    }
    +
    while (count >= tape->buffer_size) {
    ssize_t retval;
    if (idetape_copy_stage_from_user(tape, buf, tape->buffer_size))
    @@ -2188,11 +2066,12 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
    if (retval <= 0)
    return (retval);
    }
    +
    if (count) {
    actually_written += count;
    if (idetape_copy_stage_from_user(tape, buf, count))
    ret = -EFAULT;
    - tape->merge_stage_size += count;
    + tape->cur_buf_size += count;
    }
    return ret ? ret : actually_written;
    }
    @@ -2248,7 +2127,6 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)
    case MTWEOF:
    if (tape->write_prot)
    return -EACCES;
    - idetape_discard_read_pipeline(drive, 1);
    for (i = 0; i < mt_count; i++) {
    retval = idetape_write_filemark(drive);
    if (retval)
    @@ -2256,12 +2134,10 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)
    }
    return 0;
    case MTREW:
    - idetape_discard_read_pipeline(drive, 0);
    if (idetape_rewind_tape(drive))
    return -EIO;
    return 0;
    case MTLOAD:
    - idetape_discard_read_pipeline(drive, 0);
    idetape_create_load_unload_cmd(drive, &pc,
    IDETAPE_LU_LOAD_MASK);
    return idetape_queue_pc_tail(drive, &pc);
    @@ -2276,7 +2152,6 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)
    if (!idetape_queue_pc_tail(drive, &pc))
    tape->door_locked = DOOR_UNLOCKED;
    }
    - idetape_discard_read_pipeline(drive, 0);
    idetape_create_load_unload_cmd(drive, &pc,
    !IDETAPE_LU_LOAD_MASK);
    retval = idetape_queue_pc_tail(drive, &pc);
    @@ -2284,10 +2159,8 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)
    clear_bit(IDETAPE_FLAG_MEDIUM_PRESENT, &tape->flags);
    return retval;
    case MTNOP:
    - idetape_discard_read_pipeline(drive, 0);
    return idetape_flush_tape_buffers(drive);
    case MTRETEN:
    - idetape_discard_read_pipeline(drive, 0);
    idetape_create_load_unload_cmd(drive, &pc,
    IDETAPE_LU_RETENSION_MASK | IDETAPE_LU_LOAD_MASK);
    return idetape_queue_pc_tail(drive, &pc);
    @@ -2309,11 +2182,9 @@ static int idetape_mtioctop(ide_drive_t *drive, short mt_op, int mt_count)
    set_bit(IDETAPE_FLAG_DETECT_BS, &tape->flags);
    return 0;
    case MTSEEK:
    - idetape_discard_read_pipeline(drive, 0);
    return idetape_position_tape(drive,
    mt_count * tape->user_bs_factor, tape->partition, 0);
    case MTSETPART:
    - idetape_discard_read_pipeline(drive, 0);
    return idetape_position_tape(drive, 0, mt_count, 0);
    case MTFSR:
    case MTBSR:
    @@ -2358,12 +2229,11 @@ static int idetape_chrdev_ioctl(struct inode *inode, struct file *file,

    debug_log(DBG_CHRDEV, "Enter %s, cmd=%u\n", __func__, cmd);

    - if (tape->chrdev_dir == IDETAPE_DIR_WRITE) {
    - idetape_empty_write_pipeline(drive);
    + if (tape->chrdev_dir == IDETAPE_DIR_WRITE)
    idetape_flush_tape_buffers(drive);
    - }
    +
    if (cmd == MTIOCGET || cmd == MTIOCPOS) {
    - block_offset = tape->merge_stage_size /
    + block_offset = tape->cur_buf_size /
    (tape->blk_size * tape->user_bs_factor);
    position = idetape_read_position(drive);
    if (position < 0)
    @@ -2394,8 +2264,6 @@ static int idetape_chrdev_ioctl(struct inode *inode, struct file *file,
    return -EFAULT;
    return 0;
    default:
    - if (tape->chrdev_dir == IDETAPE_DIR_READ)
    - idetape_discard_read_pipeline(drive, 1);
    return idetape_blkdev_ioctl(drive, cmd, arg);
    }
    }
    @@ -2508,13 +2376,12 @@ static void idetape_write_release(ide_drive_t *drive, unsigned int minor)
    {
    idetape_tape_t *tape = drive->driver_data;

    - idetape_empty_write_pipeline(drive);
    - tape->merge_stage = ide_tape_kmalloc_buffer(tape, 1, 0);
    - if (tape->merge_stage != NULL) {
    + tape->bh = ide_tape_kmalloc_buffer(tape, 1, 0);
    + if (tape->bh) {
    idetape_pad_zeros(drive, tape->blk_size *
    (tape->user_bs_factor - 1));
    - ide_tape_kfree_buffer(tape->merge_stage);
    - tape->merge_stage = NULL;
    + ide_tape_kfree_buffer(tape);
    + tape->bh = NULL;
    }
    idetape_write_filemark(drive);
    idetape_flush_tape_buffers(drive);
    @@ -2535,10 +2402,6 @@ static int idetape_chrdev_release(struct inode *inode, struct file *filp)

    if (tape->chrdev_dir == IDETAPE_DIR_WRITE)
    idetape_write_release(drive, minor);
    - if (tape->chrdev_dir == IDETAPE_DIR_READ) {
    - if (minor < 128)
    - idetape_discard_read_pipeline(drive, 1);
    - }

    if (minor < 128 && test_bit(IDETAPE_FLAG_MEDIUM_PRESENT, &tape->flags))
    (void) idetape_rewind_tape(drive);
    @@ -2792,7 +2655,7 @@ static void ide_tape_release(struct kref *kref)
    ide_drive_t *drive = tape->drive;
    struct gendisk *g = tape->disk;

    - BUG_ON(tape->merge_stage_size);
    + BUG_ON(tape->cur_buf_size);

    drive->dsc_overlap = 0;
    drive->driver_data = NULL;
    --
    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. [PATCH 3/5] ide-tape: mv tape->pages_per_stage tape->pages_per_buffer

    Signed-off-by: Borislav Petkov
    ---
    drivers/ide/ide-tape.c | 9 ++++-----
    1 files changed, 4 insertions(+), 5 deletions(-)

    diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
    index 7fa0cc0..cf4351c 100644
    --- a/drivers/ide/ide-tape.c
    +++ b/drivers/ide/ide-tape.c
    @@ -315,8 +315,7 @@ typedef struct ide_tape_obj {
    char *b_data;
    int b_count;

    - /* Pipeline parameters. */
    - int pages_per_stage;
    + int pages_per_buffer;
    /* Wasted space in each stage */
    int excess_bh_size;

    @@ -1302,7 +1301,7 @@ static idetape_stage_t *ide_tape_kmalloc_buffer(idetape_tape_t *tape, int full,
    {
    idetape_stage_t *stage;
    struct idetape_bh *prev_bh, *bh;
    - int pages = tape->pages_per_stage;
    + int pages = tape->pages_per_buffer;
    unsigned int order, b_allocd;
    char *b_data = NULL;

    @@ -2746,9 +2745,9 @@ static void idetape_setup(ide_drive_t *drive, idetape_tape_t *tape, int minor)
    tape->buffer_size = *ctl * tape->blk_size;
    }
    buffer_size = tape->buffer_size;
    - tape->pages_per_stage = buffer_size / PAGE_SIZE;
    + tape->pages_per_buffer = buffer_size / PAGE_SIZE;
    if (buffer_size % PAGE_SIZE) {
    - tape->pages_per_stage++;
    + tape->pages_per_buffer++;
    tape->excess_bh_size = PAGE_SIZE - buffer_size % PAGE_SIZE;
    }

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

  5. [PATCH 1/5] ide-tape: improve buffer allocation strategy

    Instead of allocating pages for the buffer one by one, take advantage of the
    buddy alloc system and request them 2^order at a time. This increases the chance
    for bigger buffer parts to be contigious and reduces loop iteration count. While
    at it, rename function __idetape_kmalloc_stage() to ide_tape_kmalloc_buffer().

    Signed-off-by: Borislav Petkov
    ---
    drivers/ide/ide-tape.c | 58 +++++++++++++++++++++++++++++------------------
    1 files changed, 36 insertions(+), 22 deletions(-)

    diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
    index 6752d47..b5ec669 100644
    --- a/drivers/ide/ide-tape.c
    +++ b/drivers/ide/ide-tape.c
    @@ -1290,20 +1290,20 @@ out:
    }

    /*
    - * The function below uses __get_free_page to allocate a pipeline stage, along
    - * with all the necessary small buffers which together make a buffer of size
    + * The function below uses __get_free_pages to allocate a data buffer of size
    * tape->stage_size (or a bit more). We attempt to combine sequential pages as
    * much as possible.
    *
    - * It returns a pointer to the new allocated stage, or NULL if we can't (or
    - * don't want to) allocate a stage.
    + * It returns a pointer to the newly allocated buffer, or NULL in case of
    + * failure.
    */
    -static idetape_stage_t *__idetape_kmalloc_stage(idetape_tape_t *tape, int full,
    +static idetape_stage_t *ide_tape_kmalloc_buffer(idetape_tape_t *tape, int full,
    int clear)
    {
    idetape_stage_t *stage;
    struct idetape_bh *prev_bh, *bh;
    int pages = tape->pages_per_stage;
    + unsigned int order, b_allocd;
    char *b_data = NULL;

    stage = kmalloc(sizeof(idetape_stage_t), GFP_KERNEL);
    @@ -1315,32 +1315,43 @@ static idetape_stage_t *__idetape_kmalloc_stage(idetape_tape_t *tape, int full,
    bh = stage->bh;
    if (bh == NULL)
    goto abort;
    - bh->b_reqnext = NULL;
    - bh->b_data = (char *) __get_free_page(GFP_KERNEL);
    +
    + order = fls(pages) - 1;
    + bh->b_data = (char *) __get_free_pages(GFP_KERNEL, order);
    if (!bh->b_data)
    goto abort;
    + b_allocd = (1 << order) * PAGE_SIZE;
    + pages &= (order-1);
    +
    if (clear)
    - memset(bh->b_data, 0, PAGE_SIZE);
    - bh->b_size = PAGE_SIZE;
    + memset(bh->b_data, 0, b_allocd);
    + bh->b_reqnext = NULL;
    + bh->b_size = b_allocd;
    atomic_set(&bh->b_count, full ? bh->b_size : 0);

    - while (--pages) {
    - b_data = (char *) __get_free_page(GFP_KERNEL);
    + while (pages) {
    + order = fls(pages) - 1;
    + b_data = (char *) __get_free_pages(GFP_KERNEL, order);
    if (!b_data)
    goto abort;
    + b_allocd = (1 << order) * PAGE_SIZE;
    +
    if (clear)
    - memset(b_data, 0, PAGE_SIZE);
    - if (bh->b_data == b_data + PAGE_SIZE) {
    - bh->b_size += PAGE_SIZE;
    - bh->b_data -= PAGE_SIZE;
    + memset(b_data, 0, b_allocd);
    +
    + /* newly allocated page frames below buffer header or ...*/
    + if (bh->b_data == b_data + b_allocd) {
    + bh->b_size += b_allocd;
    + bh->b_data -= b_allocd;
    if (full)
    - atomic_add(PAGE_SIZE, &bh->b_count);
    + atomic_add(b_allocd, &bh->b_count);
    continue;
    }
    + /* they are above the header */
    if (b_data == bh->b_data + bh->b_size) {
    - bh->b_size += PAGE_SIZE;
    + bh->b_size += b_allocd;
    if (full)
    - atomic_add(PAGE_SIZE, &bh->b_count);
    + atomic_add(b_allocd, &bh->b_count);
    continue;
    }
    prev_bh = bh;
    @@ -1351,10 +1362,13 @@ static idetape_stage_t *__idetape_kmalloc_stage(idetape_tape_t *tape, int full,
    }
    bh->b_reqnext = NULL;
    bh->b_data = b_data;
    - bh->b_size = PAGE_SIZE;
    + bh->b_size = b_allocd;
    atomic_set(&bh->b_count, full ? bh->b_size : 0);
    prev_bh->b_reqnext = bh;
    +
    + pages &= (order-1);
    }
    +
    bh->b_size -= tape->excess_bh_size;
    if (full)
    atomic_sub(tape->excess_bh_size, &bh->b_count);
    @@ -1837,7 +1851,7 @@ static int idetape_init_read(ide_drive_t *drive)
    " 0 now\n");
    tape->merge_stage_size = 0;
    }
    - tape->merge_stage = __idetape_kmalloc_stage(tape, 0, 0);
    + tape->merge_stage = ide_tape_kmalloc_buffer(tape, 0, 0);
    if (!tape->merge_stage)
    return -ENOMEM;
    tape->chrdev_dir = IDETAPE_DIR_READ;
    @@ -2115,7 +2129,7 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
    "should be 0 now\n");
    tape->merge_stage_size = 0;
    }
    - tape->merge_stage = __idetape_kmalloc_stage(tape, 0, 0);
    + tape->merge_stage = ide_tape_kmalloc_buffer(tape, 0, 0);
    if (!tape->merge_stage)
    return -ENOMEM;
    tape->chrdev_dir = IDETAPE_DIR_WRITE;
    @@ -2495,7 +2509,7 @@ static void idetape_write_release(ide_drive_t *drive, unsigned int minor)
    idetape_tape_t *tape = drive->driver_data;

    idetape_empty_write_pipeline(drive);
    - tape->merge_stage = __idetape_kmalloc_stage(tape, 1, 0);
    + tape->merge_stage = ide_tape_kmalloc_buffer(tape, 1, 0);
    if (tape->merge_stage != NULL) {
    idetape_pad_zeros(drive, tape->blk_size *
    (tape->user_bs_factor - 1));
    --
    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/

  6. Re: [PATCH 1/5] ide-tape: improve buffer allocation strategy


    Hi,

    On Saturday 29 March 2008, Borislav Petkov wrote:
    > Instead of allocating pages for the buffer one by one, take advantage of the
    > buddy alloc system and request them 2^order at a time. This increases the chance
    > for bigger buffer parts to be contigious and reduces loop iteration count. While
    > at it, rename function __idetape_kmalloc_stage() to ide_tape_kmalloc_buffer().
    >
    > Signed-off-by: Borislav Petkov


    applied patches #1-4
    --
    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 5/5] ide-tape: remove the last remains of pipelining


    On Saturday 29 March 2008, Borislav Petkov wrote:
    > This patch converts the tape->merge_stage pipeline stage into tape->bh, a singly
    > linked list of idetape_bh's, each of which is a tag attached to one or more pages
    > serving as a data buffer for chrdev requests. In particular,
    >
    > 1. makes tape->bh the data buffer of size tape->buffer_size which is computed
    > from the Continuous Transfer Limit value in the caps page and the tape block
    > size. The chrdev rw routines use this buffer as an intermediary location to
    > shuffle data to and from.


    ide-tape supports write buffering (if number of bytes written to character
    device is < tape->buffer_size) and it seems to be the main reason behind
    tape->merge_stage. It seems that this feature gets killed by the above
    change (we may actually want to kill it later in order to implement direct
    mapping of user pages but this shouldn't be done unless really necessary
    since it may negatively affect performance of some "poorly" written apps).

    Also:
    tape->bh serves as the current bh pointer in idetape_chrdev_{read,write}()
    so it doesn't seem like we can mix it with merge_stage->bh (OTOH we should
    be fine with having tape->merge_bh).

    > 2. mv tape->merge_stage_size => tape->cur_buf_size as it contains the offset
    > within tape->bh
    >
    > 3. get rid of pipeline stage idetape_stage_t, tape->merge_stage
    > and pipeline-related functions __idetape_discard_read_pipeline(),
    > idetape_discard_read_pipeline(), idetape_empty_write_pipeline()


    Existing tape->merge_stage is first taken care of at the beggining of
    idetape_chrdev_{read,write}() and only then the new one is allocated,
    the above functions play important part in handling write buffering
    (which is _independent_ from pipelined-mode) and idetape_discard...()
    seems to play some part in tape positioning for some ioctls - I worry
    that they can't be just simply deleted.

    > 4. code chunk "if (tape->merge_stage_size) {...}" in idetape_chrdev_read() is not
    > needed since tape->merge_stage_size, tape->cur_buf_size resp., is zeroed out in
    > idetape_init_read() couple of lines above


    Unless the previous user request was also idetape_chrdev_read() since in this
    case idetape_init_read() returns early and existing ->merge_stage is re-used.

    [...]

    I like very much the general direction that this patch is going but the above
    details need to be taken care of. I suggest splitting the patch on many
    smaller ones (i.e. starting with 'tape->merge_stage -> tape->merge_bh', then
    inlining __idetape_discard_read_pipeline() etc.) so we may closely review
    such fine-grained and _obvious_ steps.

    [ ide-tape got _much_ better after your rewrites but some old parts are still
    quite puzzling & mined with gotchas - we shouldn't let guard down yet. ]

    Thanks,
    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/

  8. Re: [PATCH 4/5] ide-tape: improve buffer pages freeing strategy

    On Saturday 29 March 2008, Borislav Petkov wrote:
    > Instead of freeing pages one by one, free them 2^order-wise. Also, mv
    > __idetape_kfree_stage() to ide_tape_kfree_buffer().
    >
    > Signed-off-by: Borislav Petkov
    > ---
    > drivers/ide/ide-tape.c | 37 +++++++++++++++++++------------------
    > 1 files changed, 19 insertions(+), 18 deletions(-)
    >
    > diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
    > index cf4351c..dcaefef 100644
    > --- a/drivers/ide/ide-tape.c
    > +++ b/drivers/ide/ide-tape.c
    > @@ -583,20 +583,21 @@ static void idetape_analyze_error(ide_drive_t *drive, u8 *sense)
    > }
    > }
    >
    > -/* Free a stage along with its related buffers completely. */
    > -static void __idetape_kfree_stage(idetape_stage_t *stage)
    > +/* Free data buffers completely. */
    > +static void ide_tape_kfree_buffer(idetape_stage_t *stage)
    > {
    > struct idetape_bh *prev_bh, *bh = stage->bh;
    > - int size;
    > -
    > - while (bh != NULL) {
    > - if (bh->b_data != NULL) {
    > - size = (int) bh->b_size;
    > - while (size > 0) {
    > - free_page((unsigned long) bh->b_data);
    > - size -= PAGE_SIZE;
    > - bh->b_data += PAGE_SIZE;
    > - }
    > +
    > + while (bh) {
    > + u32 size = bh->b_size;
    > +
    > + while (size) {
    > + unsigned int order = fls(size >> PAGE_SHIFT)-1;
    > +
    > + if (bh->b_data)
    > + free_pages((unsigned long)bh->b_data, order);
    > +
    > + size &= (order-1);
    > }


    Hmmm, don't we also need to update bh->b_data?

    [ I added the change below to the merged patch version for now: ]

    @@ -598,6 +598,7 @@
    free_pages((unsigned long)bh->b_data, order);

    size &= (order-1);
    + bh->b_data += (1 << order) * PAGE_SIZE;
    }
    prev_bh = bh;
    bh = bh->b_reqnext;
    --
    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