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

[Grant Grundler]

On Wed, Mar 25, 2009 at 7:17 AM, Tejun Heo <tj@xxxxxxxxxx> 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@xxxxxxxxxx>"

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@xxxxxxxxxxxxxxx
> More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
>
èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—