linux-kernel - Re: [PATCH 02/24] ide-tape: remove pipeline-specific code from idetape_add_chrdev_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 2 Mar 2008 22:19:53 +0100
From:	Borislav Petkov <petkovbb@...glemail.com>
To:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
Cc:	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/24] ide-tape: remove pipeline-specific code from
	idetape_add_chrdev_write_request

On Sun, Mar 02, 2008 at 07:33:05PM +0100, Bartlomiej Zolnierkiewicz wrote:
> On Saturday 01 March 2008, Borislav Petkov wrote:
> > Instead of plugging the request into the pipeline, queue it straight on the
> > device's request queue through idetape_queue_rw_tail().
> > 
> > Signed-off-by: Borislav Petkov <petkovbb@...il.com>
> > ---
> >  drivers/ide/ide-tape.c |   81 ++---------------------------------------------
> >  1 files changed, 4 insertions(+), 77 deletions(-)
> > 
> > diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
> > index 792c76e..abf3efa 100644
> > --- a/drivers/ide/ide-tape.c
> > +++ b/drivers/ide/ide-tape.c
> > @@ -2123,12 +2123,6 @@ static int idetape_queue_rw_tail(ide_drive_t *drive, int cmd, int blocks,
> >  
> >  	debug_log(DBG_SENSE, "%s: cmd=%d\n", __func__, cmd);
> >  
> > -	if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {
> > -		printk(KERN_ERR "ide-tape: bug: the pipeline is active in %s\n",
> > -				__func__);
> > -		return (0);
> > -	}
> > -
> >  	idetape_init_rq(&rq, cmd);
> >  	rq.rq_disk = tape->disk;
> >  	rq.special = (void *)bh;
> > @@ -2140,10 +2134,9 @@ 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 (rq.errors == IDETAPE_ERROR_GENERAL)
> >  		return -EIO;
> > +
> >  	return (tape->blk_size * (blocks-rq.current_nr_sectors));
> >  }
> 
> These two changes to idetape_queue_rw_tail() don't look correct
> as the pipeline mode is still used by read requests.

Wrt first hunk read rq pipeline functionality is removed in the following
patch. Would it then be better to merge the two patches? Actually, do we need
to keep the driver functional in between the patches of those series for
the purposes of git bisection or only compile-testing it is enough? Cause
after applying the whole series you get pipelined mode ripped out anyway and
intermittent states with partially functional pipeline are of no importance, no?

Wrt the second one you're right, this one should stay in for now until
tape->merge_stage has been removed.

> > @@ -2210,81 +2203,15 @@ static void idetape_wait_first_stage(ide_drive_t *drive)
> >  	spin_unlock_irqrestore(&tape->lock, flags);
> >  }
> >  
> > -/*
> > - * Try to add a character device originated write request to our pipeline. In
> > - * case we don't succeed, we revert to non-pipelined operation mode for this
> > - * request. In order to accomplish that, we
> > - *
> > - * 1. Try to allocate a new pipeline stage.
> > - * 2. If we can't, wait for more and more requests to be serviced and try again
> > - * each time.
> > - * 3. If we still can't allocate a stage, fallback to non-pipelined operation
> > - * mode for this request.
> > - */
> > +/* Queue up a character device originated write request. */
> >  static int idetape_add_chrdev_write_request(ide_drive_t *drive, int blocks)
> >  {
> >  	idetape_tape_t *tape = drive->driver_data;
> > -	idetape_stage_t *new_stage;
> > -	unsigned long flags;
> > -	struct request *rq;
> >  
> >  	debug_log(DBG_CHRDEV, "Enter %s\n", __func__);
> >  
> > -	/* Attempt to allocate a new stage. Beware possible race conditions. */
> > -	while ((new_stage = idetape_kmalloc_stage(tape)) == NULL) {
> > -		spin_lock_irqsave(&tape->lock, flags);
> > -		if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE, &tape->flags)) {
> > -			idetape_wait_for_request(drive, tape->active_data_rq);
> > -			spin_unlock_irqrestore(&tape->lock, flags);
> > -		} else {
> > -			spin_unlock_irqrestore(&tape->lock, flags);
> > -			idetape_plug_pipeline(drive);
> > -			if (test_bit(IDETAPE_FLAG_PIPELINE_ACTIVE,
> > -					&tape->flags))
> > -				continue;
> 
> Can all the above code be safely removed (are you sure that there are no
> hidden interactions)?  Even if so I would prefer that it is left intact by
> this patch to ease the review.

This code does exactly what the comment above explains: it tries to free
the pipeline for yet another request by plugging it with the already queued
ones and if it can't do so it simply queues the request in non-pipelined
mode. What the patch does is remove the plugging and waiting. If we
leave this intact we'll be missing the point of the whole exercise.

-- 
Regards/Gruß,
    Boris.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/