Messages in this thread | | | Date | Wed, 25 Mar 2009 09:04:04 -0700 | Subject | Re: [PATCH 02/10] ide-tape: use single continuous buffer | From | Grant Grundler <> |
| |
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 >
| |