Skip to contentSkip to conversation

ide-tape: refit tape data buffer bits/kill pipelining

PATCH In review Borislav Petkov 2 participants 9 messages Mar 29, 2008 – Apr 3, 2008 libata

lore
Share & tools
lore

Borislav Petkov

author
#0 lore raw

cover 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(-)
author
#1 lore raw

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 <[email protected]>
---
 drivers/ide/ide-tape.c |   58 +++++++++++++++++++++++++++++------------------
 1 files changed, 36 insertions(+), 22 deletions(-)
drivers/ide/ide-tape.c +36 -22
index 6752d47..b5ec669 100644
@@ -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));
Signature · 1 line
1.5.4.1
#2 lore raw

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

Hi,

Quoted 6 lines · Borislav Petkov

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 <[email protected]>

applied patches #1-4

author
#3 lore raw

PATCH 2/5 ide-tape: mv tape->stage_size tape->buffer_size

In reply to Borislav Petkov
Signed-off-by Borislav Petkov <[email protected]>
---
 drivers/ide/ide-tape.c |   58 ++++++++++++++++++++++++------------------------
 1 files changed, 29 insertions(+), 29 deletions(-)
drivers/ide/ide-tape.c +29 -29
index b5ec669..7fa0cc0 100644
@@ -308,7 +308,7 @@ typedef struct ide_tape_obj {
 	 */
 
 	/* Data buffer size chosen based on the tape's recommendation */
-	int stage_size;
+	int buffer_size;
 	idetape_stage_t *merge_stage;
 	int merge_stage_size;
 	struct idetape_bh *bh;

@@ -1168,7 +1168,7 @@ static void idetape_create_read_cmd(idetape_tape_t *tape,
 	pc->buf = NULL;
 	pc->buf_size = length * tape->blk_size;
 	pc->req_xfer = pc->buf_size;
-	if (pc->req_xfer == tape->stage_size)
+	if (pc->req_xfer == tape->buffer_size)
 		pc->flags |= PC_FLAG_DMA_RECOMMENDED;
 }
 

@@ -1188,7 +1188,7 @@ static void idetape_create_write_cmd(idetape_tape_t *tape,
 	pc->buf = NULL;
 	pc->buf_size = length * tape->blk_size;
 	pc->req_xfer = pc->buf_size;
-	if (pc->req_xfer == tape->stage_size)
+	if (pc->req_xfer == tape->buffer_size)
 		pc->flags |= PC_FLAG_DMA_RECOMMENDED;
 }
 

@@ -1291,7 +1291,7 @@ out:
 
 /*
  * 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
+ * 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

@@ -1792,9 +1792,9 @@ static void idetape_empty_write_pipeline(ide_drive_t *drive)
 				" but we are not writing.\n");
 		return;
 	}
-	if (tape->merge_stage_size > tape->stage_size) {
+	if (tape->merge_stage_size > tape->buffer_size) {
 		printk(KERN_ERR "ide-tape: bug: merge_buffer too big\n");
-		tape->merge_stage_size = tape->stage_size;
+		tape->merge_stage_size = tape->buffer_size;
 	}
 	if (tape->merge_stage_size) {
 		blocks = tape->merge_stage_size / tape->blk_size;

@@ -1905,7 +1905,7 @@ static void idetape_pad_zeros(ide_drive_t *drive, int bcount)
 		unsigned int count;
 
 		bh = tape->merge_stage->bh;
-		count = min(tape->stage_size, bcount);
+		count = min(tape->buffer_size, bcount);
 		bcount -= count;
 		blocks = count / tape->blk_size;
 		while (count) {

@@ -2074,7 +2074,7 @@ static ssize_t idetape_chrdev_read(struct file *file, char __user *buf,
 		tape->merge_stage_size -= actually_read;
 		count -= actually_read;
 	}
-	while (count >= tape->stage_size) {
+	while (count >= tape->buffer_size) {
 		bytes_read = idetape_add_chrdev_read_request(drive, ctl);
 		if (bytes_read <= 0)
 			goto finish;

@@ -2156,12 +2156,12 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
 	if (count == 0)
 		return (0);
 	if (tape->merge_stage_size) {
-		if (tape->merge_stage_size >= tape->stage_size) {
+		if (tape->merge_stage_size >= tape->buffer_size) {
 			printk(KERN_ERR "ide-tape: bug: merge buf too big\n");
 			tape->merge_stage_size = 0;
 		}
 		actually_written = min((unsigned int)
-				(tape->stage_size - tape->merge_stage_size),
+				(tape->buffer_size - tape->merge_stage_size),
 				(unsigned int)count);
 		if (idetape_copy_stage_from_user(tape, buf, actually_written))
 				ret = -EFAULT;

@@ -2169,7 +2169,7 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
 		tape->merge_stage_size += actually_written;
 		count -= actually_written;
 
-		if (tape->merge_stage_size == tape->stage_size) {
+		if (tape->merge_stage_size == tape->buffer_size) {
 			ssize_t retval;
 			tape->merge_stage_size = 0;
 			retval = idetape_add_chrdev_write_request(drive, ctl);

@@ -2177,14 +2177,14 @@ static ssize_t idetape_chrdev_write(struct file *file, const char __user *buf,
 				return (retval);
 		}
 	}
-	while (count >= tape->stage_size) {
+	while (count >= tape->buffer_size) {
 		ssize_t retval;
-		if (idetape_copy_stage_from_user(tape, buf, tape->stage_size))
+		if (idetape_copy_stage_from_user(tape, buf, tape->buffer_size))
 			ret = -EFAULT;
-		buf += tape->stage_size;
-		count -= tape->stage_size;
+		buf += tape->buffer_size;
+		count -= tape->buffer_size;
 		retval = idetape_add_chrdev_write_request(drive, ctl);
-		actually_written += tape->stage_size;
+		actually_written += tape->buffer_size;
 		if (retval <= 0)
 			return (retval);
 	}

@@ -2678,8 +2678,8 @@ static void idetape_add_settings(ide_drive_t *drive)
 			1, 2, (u16 *)&tape->caps[16], NULL);
 	ide_add_setting(drive, "speed", SETTING_READ, TYPE_SHORT, 0, 0xffff,
 			1, 1, (u16 *)&tape->caps[14], NULL);
-	ide_add_setting(drive, "stage", SETTING_READ, TYPE_INT,	0, 0xffff, 1,
-			1024, &tape->stage_size, NULL);
+	ide_add_setting(drive, "buffer_size", SETTING_READ, TYPE_INT, 0, 0xffff,
+			1, 1024, &tape->buffer_size, NULL);
 	ide_add_setting(drive, "tdsc", SETTING_RW, TYPE_INT, IDETAPE_DSC_RW_MIN,
 			IDETAPE_DSC_RW_MAX, 1000, HZ, &tape->best_dsc_rw_freq,
 			NULL);

@@ -2709,7 +2709,7 @@ static void idetape_setup(ide_drive_t *drive, idetape_tape_t *tape, int minor)
 {
 	unsigned long t;
 	int speed;
-	int stage_size;
+	int buffer_size;
 	u8 gcw[2];
 	u16 *ctl = (u16 *)&tape->caps[12];
 

@@ -2739,23 +2739,23 @@ static void idetape_setup(ide_drive_t *drive, idetape_tape_t *tape, int minor)
 	idetape_get_mode_sense_results(drive);
 	ide_tape_get_bsize_from_bdesc(drive);
 	tape->user_bs_factor = 1;
-	tape->stage_size = *ctl * tape->blk_size;
-	while (tape->stage_size > 0xffff) {
+	tape->buffer_size = *ctl * tape->blk_size;
+	while (tape->buffer_size > 0xffff) {
 		printk(KERN_NOTICE "ide-tape: decreasing stage size\n");
 		*ctl /= 2;
-		tape->stage_size = *ctl * tape->blk_size;
+		tape->buffer_size = *ctl * tape->blk_size;
 	}
-	stage_size = tape->stage_size;
-	tape->pages_per_stage = stage_size / PAGE_SIZE;
-	if (stage_size % PAGE_SIZE) {
+	buffer_size = tape->buffer_size;
+	tape->pages_per_stage = buffer_size / PAGE_SIZE;
+	if (buffer_size % PAGE_SIZE) {
 		tape->pages_per_stage++;
-		tape->excess_bh_size = PAGE_SIZE - stage_size % PAGE_SIZE;
+		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]);
 
-	t = (IDETAPE_FIFO_THRESHOLD * tape->stage_size * HZ) / (speed * 1000);
+	t = (IDETAPE_FIFO_THRESHOLD * tape->buffer_size * HZ) / (speed * 1000);
 
 	/*
 	 * Ensure that the number we got makes sense; limit it within

@@ -2767,8 +2767,8 @@ static void idetape_setup(ide_drive_t *drive, idetape_tape_t *tape, int minor)
 	printk(KERN_INFO "ide-tape: %s <-> %s: %dKBps, %d*%dkB buffer, "
 		"%lums tDSC%s\n",
 		drive->name, tape->name, *(u16 *)&tape->caps[14],
-		(*(u16 *)&tape->caps[16] * 512) / tape->stage_size,
-		tape->stage_size / 1024,
+		(*(u16 *)&tape->caps[16] * 512) / tape->buffer_size,
+		tape->buffer_size / 1024,
 		tape->best_dsc_rw_freq * 1000 / HZ,
 		drive->using_dma ? ", DMA":"");
Signature · 1 line
1.5.4.1
author
#4 lore raw

PATCH 3/5 ide-tape: mv tape->pages_per_stage tape->pages_per_buffer

In reply to Borislav Petkov
Signed-off-by Borislav Petkov <[email protected]>
---
 drivers/ide/ide-tape.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
drivers/ide/ide-tape.c +4 -5
index 7fa0cc0..cf4351c 100644
@@ -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;
 	}
Signature · 1 line
1.5.4.1
author
#5 lore raw

PATCH 4/5 ide-tape: improve buffer pages freeing strategy

In reply to Borislav Petkov

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 <[email protected]>
---
 drivers/ide/ide-tape.c |   37 +++++++++++++++++++------------------
 1 files changed, 19 insertions(+), 18 deletions(-)
drivers/ide/ide-tape.c +19 -18
index cf4351c..dcaefef 100644
@@ -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);
Signature · 1 line
1.5.4.1
#6 lore raw

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

Quoted 34 lines · Borislav Petkov

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 <[email protected]>
---
 drivers/ide/ide-tape.c |   37 +++++++++++++++++++------------------
 1 files changed, 19 insertions(+), 18 deletions(-)
drivers/ide/ide-tape.c +4 -12
index cf4351c..dcaefef 100644
@@ -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) {

drivers/ide/ide-tape.c static void idetape_analyze_error(ide_drive_t *drive, u8 *sense) · line 601

Quoted 10 lines
drivers/ide/ide-tape.c +9 -0
+		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;

author
#7 lore raw

PATCH 5/5 ide-tape: remove the last remains of pipelining

In reply to Borislav Petkov

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 <[email protected]>
---
 drivers/ide/ide-tape.c |  267 ++++++++++++------------------------------------
 1 files changed, 65 insertions(+), 202 deletions(-)
drivers/ide/ide-tape.c +65 -202
index dcaefef..c7507c7 100644
@@ -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;
Signature · 1 line
1.5.4.1
#8 lore raw

Re: [PATCH 5/5] ide-tape: remove the last remains of pipelining

Quoted 8 lines · Borislav Petkov

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).

Quoted 6 lines

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.

Quoted 3 lines

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