LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Borislav Petkov <petkovbb@googlemail.com>
To: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 02/24] ide-tape: remove pipeline-specific code from idetape_add_chrdev_write_request
Date: Sun, 2 Mar 2008 22:19:53 +0100	[thread overview]
Message-ID: <20080302211953.GD4836@gollum.tnic> (raw)
In-Reply-To: <200803021933.06012.bzolnier@gmail.com>

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@gmail.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.

  reply	other threads:[~2008-03-02 21:20 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-03-01  8:58 [PATCH 00/24] ide-tape: remove pipelined mode operation Borislav Petkov
2008-03-01  8:58 ` [PATCH 01/24] ide-tape: remove idetape_pipeline_active() Borislav Petkov
2008-03-02 18:21   ` Bartlomiej Zolnierkiewicz
2008-03-01  8:58 ` [PATCH 02/24] ide-tape: remove pipeline-specific code from idetape_add_chrdev_write_request Borislav Petkov
2008-03-02 18:33   ` Bartlomiej Zolnierkiewicz
2008-03-02 21:19     ` Borislav Petkov [this message]
2008-03-02 23:16       ` Bartlomiej Zolnierkiewicz
2008-03-03  6:43         ` Borislav Petkov
2008-03-03 22:32           ` Bartlomiej Zolnierkiewicz
2008-03-01  8:58 ` [PATCH 03/24] ide-tape: remove pipeline-specific code from idetape_add_chrdev_read_request Borislav Petkov
2008-03-02 18:36   ` Bartlomiej Zolnierkiewicz
2008-03-02 21:28     ` Borislav Petkov
2008-03-01  8:58 ` [PATCH 04/24] ide-tape: remove pipeline-specific code from idetape_chrdev_write Borislav Petkov
2008-03-02 18:41   ` Bartlomiej Zolnierkiewicz
2008-03-02 21:31     ` Borislav Petkov
2008-03-02 23:17       ` Bartlomiej Zolnierkiewicz
2008-03-01  8:58 ` [PATCH 05/24] ide-tape: cleanup pipeline-specific code from idetape_init_read Borislav Petkov
2008-03-02 18:48   ` Bartlomiej Zolnierkiewicz
2008-03-01  8:58 ` [PATCH 06/24] ide-tape: remove unused stage-parameter from idetape_copy_stage_to_user Borislav Petkov
2008-03-02 19:15   ` Bartlomiej Zolnierkiewicz
2008-03-01  8:58 ` [PATCH 07/24] ide-tape: remove pipeline-specific code bits from idetape_chrdev_read Borislav Petkov
2008-03-01  8:58 ` [PATCH 08/24] ide-tape: remove pipeline-specific code from idetape_space_over_filemarks Borislav Petkov
2008-03-01  8:58 ` [PATCH 09/24] ide-tape: remove pipeline-specific code from idetape_mtioctop Borislav Petkov
2008-03-01  8:58 ` [RFCPATCH 10/24] ide-tape: remove pipeline-specific code from idetape_chrdev_ioctl Borislav Petkov
2008-03-01  8:58 ` [PATCH 11/24] ide-tape: remove pipeline-specific code from idetape_blkdev_ioctl Borislav Petkov
2008-03-01  8:58 ` [PATCH 12/24] ide-tape: remove idetape_empty_write_pipeline Borislav Petkov
2008-03-01  8:58 ` [PATCH 13/24] ide-tape: remove pipeline-specific code from idetape_chrdev_release Borislav Petkov
2008-03-01  8:58 ` [PATCH 14/24] ide-tape: remove __idetape_discard_read_pipeline Borislav Petkov
2008-03-01  8:58 ` [PATCH 15/24] ide-tape: remove pipeline-specific code from idetape_end_request Borislav Petkov
2008-03-01  8:58 ` [PATCH 16/24] ide-tape: remove idetape_calculate_speeds Borislav Petkov
2008-03-01  8:58 ` [PATCH 17/24] ide-tape: remove pipeline-specific code from idetape_chrdev_open Borislav Petkov
2008-03-01  8:58 ` [PATCH 18/24] ide-tape: remove pipeline-specific code from idetape_setup Borislav Petkov
2008-03-01  8:58 ` [PATCH 19/24] ide-tape: remove pipelined mode parameters Borislav Petkov
2008-03-01  8:58 ` [PATCH 20/24] ide-tape: remove pipelined mode tape control flags Borislav Petkov
2008-03-01  8:58 ` [PATCH 21/24] ide-tape: remove pipeline-specific members from struct ide_tape_obj Borislav Petkov
2008-03-01  8:58 ` [PATCH 22/24] ide-tape: remove misc references to pipelined operation in the comments Borislav Petkov
2008-03-01  8:58 ` [PATCH 23/24] ide-tape: remove pipelined mode description from Documentation/ide/ide-tape.txt Borislav Petkov
2008-03-01  8:58 ` [PATCH 24/24] ide-tape: remove comments markup " Borislav Petkov
2008-03-01  9:55 ` [PATCH 00/24] ide-tape: remove pipelined mode operation Jens Axboe
2008-03-01 15:45   ` Borislav Petkov
2008-03-01 18:36     ` Jens Axboe
2008-03-01 10:20 ` Adrian Bunk
2008-03-01 15:37   ` Borislav Petkov
2008-03-22 16:09     ` Bartlomiej Zolnierkiewicz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080302211953.GD4836@gollum.tnic \
    --to=petkovbb@googlemail.com \
    --cc=bzolnier@gmail.com \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=petkovbb@gmail.com \
    --subject='Re: [PATCH 02/24] ide-tape: remove pipeline-specific code from idetape_add_chrdev_write_request' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).