lkml.org 
[lkml]   [2009]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 02/10] ide-tape: use single continuous buffer
From
On Wed, Mar 25, 2009 at 7:17 AM, Tejun Heo <tj@kernel.org> wrote:
> Impact: simpler buffer allocation and handling, fix DMA transfers
...
> +               atomic_set(&bh->b_count, bcount);
>                if (atomic_read(&bh->b_count) == bh->b_size)
...

I'm failing to see why bh->b_count is an atomic_t.
I always assumed tapes were exclusive access devices
and would be serialized at a higher level.



>  /*
> - * The function below uses __get_free_pages to allocate a data buffer of size
> - * tape->buffer_size (or a bit more). We attempt to combine sequential pages as
> - * much as possible.
> - *
> - * It returns a pointer to the newly allocated buffer, or NULL in case of
> - * failure.
> + * It returns a pointer to the newly allocated buffer, or NULL in case
> + * of failure.
>  */
>  static struct idetape_bh *ide_tape_kmalloc_buffer(idetape_tape_t *tape,
> -                                                 int full, int clear)
> -{
...
> -abort:
> -       ide_tape_kfree_buffer(tape);
> -       return NULL;
> +       bh->b_size = tape->buffer_size;
> +       atomic_set(&bh->b_count, full ? bh->b_size : 0);

No one else could possibly be referencing bh->count at this
point...I like that it's consistent though.

The use of atomic won't hurt correctness and this patch looks fine to me.
Please add "Reviewed-by: Grant Grundler <grundler@google.com>"

thanks,
grant

...
> @@ -977,30 +872,20 @@ static int idetape_copy_stage_to_user(idetape_tape_t *tape, char __user *buf,
>                                      int n)
>  {
>        struct idetape_bh *bh = tape->bh;
> -       int count;
>        int ret = 0;
>
> -       while (n) {
> -               if (bh == NULL) {
> +       if (n) {
> +               if (bh == NULL || n > tape->b_count) {
>                        printk(KERN_ERR "ide-tape: bh == NULL in %s\n",
>                                        __func__);
>                        return 1;
>                }
> -               count = min(tape->b_count, n);
> -               if  (copy_to_user(buf, tape->b_data, count))
> +               if (copy_to_user(buf, tape->b_data, n))
>                        ret = 1;
> -               n -= count;
> -               tape->b_data += count;
> -               tape->b_count -= count;
> -               buf += count;
> -               if (!tape->b_count) {
> -                       bh = bh->b_reqnext;
> -                       tape->bh = bh;
> -                       if (bh) {
> -                               tape->b_data = bh->b_data;
> -                               tape->b_count = atomic_read(&bh->b_count);
> -                       }
> -               }
> +               tape->b_data += n;
> +               tape->b_count -= n;
> +               if (!tape->b_count)
> +                       tape->bh = NULL;
>        }
>        return ret;
>  }
> @@ -1252,7 +1137,7 @@ static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)
>  static void ide_tape_flush_merge_buffer(ide_drive_t *drive)
>  {
>        idetape_tape_t *tape = drive->driver_data;
> -       int blocks, min;
> +       int blocks;
>        struct idetape_bh *bh;
>
>        if (tape->chrdev_dir != IDETAPE_DIR_WRITE) {
> @@ -1267,31 +1152,16 @@ static void ide_tape_flush_merge_buffer(ide_drive_t *drive)
>        if (tape->merge_bh_size) {
>                blocks = tape->merge_bh_size / tape->blk_size;
>                if (tape->merge_bh_size % tape->blk_size) {
> -                       unsigned int i;
> -
> +                       unsigned int i = tape->blk_size -
> +                               tape->merge_bh_size % tape->blk_size;
>                        blocks++;
> -                       i = tape->blk_size - tape->merge_bh_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)));
> +                       if (bh) {
>                                memset(bh->b_data + atomic_read(&bh->b_count),
> -                                               0, min);
> -                               atomic_add(min, &bh->b_count);
> -                               i -= min;
> -                               bh = bh->b_reqnext;
> -                       }
> +                                      0, i);
> +                               atomic_add(i, &bh->b_count);
> +                       } else
> +                               printk(KERN_INFO "ide-tape: bug, bh NULL\n");
>                }
>                (void) idetape_add_chrdev_write_request(drive, blocks);
>                tape->merge_bh_size = 0;
> @@ -1319,7 +1189,7 @@ static int idetape_init_read(ide_drive_t *drive)
>                                         " 0 now\n");
>                        tape->merge_bh_size = 0;
>                }
> -               tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0, 0);
> +               tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0);
>                if (!tape->merge_bh)
>                        return -ENOMEM;
>                tape->chrdev_dir = IDETAPE_DIR_READ;
> @@ -1366,23 +1236,18 @@ static int idetape_add_chrdev_read_request(ide_drive_t *drive, int blocks)
>  static void idetape_pad_zeros(ide_drive_t *drive, int bcount)
>  {
>        idetape_tape_t *tape = drive->driver_data;
> -       struct idetape_bh *bh;
> +       struct idetape_bh *bh = tape->merge_bh;
>        int blocks;
>
>        while (bcount) {
>                unsigned int count;
>
> -               bh = tape->merge_bh;
>                count = min(tape->buffer_size, bcount);
>                bcount -= count;
>                blocks = count / tape->blk_size;
> -               while (count) {
> -                       atomic_set(&bh->b_count,
> -                                  min(count, (unsigned int)bh->b_size));
> -                       memset(bh->b_data, 0, atomic_read(&bh->b_count));
> -                       count -= atomic_read(&bh->b_count);
> -                       bh = bh->b_reqnext;
> -               }
> +               atomic_set(&bh->b_count, count);
> +               memset(bh->b_data, 0, atomic_read(&bh->b_count));
> +
>                idetape_queue_rw_tail(drive, REQ_IDETAPE_WRITE, blocks,
>                                      tape->merge_bh);
>        }
> @@ -1594,7 +1459,7 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
>                                "should be 0 now\n");
>                        tape->merge_bh_size = 0;
>                }
> -               tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0, 0);
> +               tape->merge_bh = ide_tape_kmalloc_buffer(tape, 0);
>                if (!tape->merge_bh)
>                        return -ENOMEM;
>                tape->chrdev_dir = IDETAPE_DIR_WRITE;
> @@ -1968,7 +1833,7 @@ static void idetape_write_release(ide_drive_t *drive, unsigned int minor)
>        idetape_tape_t *tape = drive->driver_data;
>
>        ide_tape_flush_merge_buffer(drive);
> -       tape->merge_bh = ide_tape_kmalloc_buffer(tape, 1, 0);
> +       tape->merge_bh = ide_tape_kmalloc_buffer(tape, 1);
>        if (tape->merge_bh != NULL) {
>                idetape_pad_zeros(drive, tape->blk_size *
>                                (tape->user_bs_factor - 1));
> @@ -2199,11 +2064,6 @@ 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_buffer = buffer_size / PAGE_SIZE;
> -       if (buffer_size % PAGE_SIZE) {
> -               tape->pages_per_buffer++;
> -               tape->excess_bh_size = PAGE_SIZE - buffer_size % PAGE_SIZE;
> -       }
>
>        /* select the "best" DSC read/write polling freq */
>        speed = max(*(u16 *)&tape->caps[14], *(u16 *)&tape->caps[8]);
> --
> 1.6.0.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
\
 
 \ /
  Last update: 2009-03-25 17:07    [W:0.120 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site