Re: [PATCH 02/10] ide-tape: use single continuous buffer

[Grant Grundler <grundler@google.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 9979 bytes --]

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 devicesand 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 thispoint...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>ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥