linux-kernel - Re: [PATCH] block: add timer on blkdev_dequeue_request() not elv_next

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]

Message-ID: <20081030072956.GL31673@kernel.dk>
Date:	Thu, 30 Oct 2008 08:29:56 +0100
From:	Jens Axboe <jens.axboe@...cle.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	James Bottomley <James.Bottomley@...senPartnership.com>,
	linux-scsi <linux-scsi@...r.kernel.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	IDE/ATA development list <linux-ide@...r.kernel.org>
Subject: Re: [PATCH] block: add timer on blkdev_dequeue_request() not elv_next_request()

On Thu, Oct 30 2008, Tejun Heo wrote:
> Block queue supports two usage models - one where block driver peeks
> at the front of queue using elv_next_request(), processes it and
> finishes it and the other where block driver peeks at the front of
> queue, dequeue the request using blkdev_dequeue_request() and finishes
> it.  The latter is more flexible as it allows the driver to process
> multiple commands concurrently.
> 
> These two inconsistent usage models affect the block layer
> implementation confusing.  For some, elv_next_request() is considered
> the issue point while others consider blkdev_dequeue_request() the
> issue point.
> 
> Till now the inconsistency mostly affect only accounting, so it didn't
> really break anything seriously; however, with block layer timeout,
> this inconsistency hits hard.  Block layer considers
> elv_next_request() the issue point and adds timer but SCSI layer
> thinks it was just peeking and when the request can't process the
> command right away, it's just left there without further processing.
> This makes the request dangling on the timer list and, when the timer
> goes off, the request which the SCSI layer and below think is still on
> the block queue ends up in the EH queue, causing various problems - EH
> hang (failed count goes over busy count and EH never wakes up),
> WARN_ON() and oopses as low level driver trying to handle the unknown
> command, etc. depending on the timing.
> 
> As SCSI midlayer is the only user of block layer timer at the moment,
> moving blk_add_timer() to elv_dequeue_request() fixes the problem;
> however, this two usage models definitely need to be cleaned up in the
> future.
> 
> Signed-off-by: Tejun Heo <tj@...nel.org>
> ---
>  block/elevator.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> Index: work/block/elevator.c
> ===================================================================
> --- work.orig/block/elevator.c
> +++ work/block/elevator.c
> @@ -773,12 +773,6 @@ struct request *elv_next_request(struct
>  			 */
>  			rq->cmd_flags |= REQ_STARTED;
>  			blk_add_trace_rq(q, rq, BLK_TA_ISSUE);
> -
> -			/*
> -			 * We are now handing the request to the hardware,
> -			 * add the timeout handler
> -			 */
> -			blk_add_timer(rq);
>  		}
>  
>  		if (!q->boundary_rq || q->boundary_rq == rq) {
> @@ -850,6 +844,12 @@ void elv_dequeue_request(struct request_
>  	 */
>  	if (blk_account_rq(rq))
>  		q->in_flight++;
> +
> +	/*
> +	 * We are now handing the request to the hardware, add the
> +	 * timeout handler.
> +	 */
> +	blk_add_timer(rq);
>  }
>  EXPORT_SYMBOL(elv_dequeue_request);

That's actually a pretty dumb error, I'm surprised it hasn't reared its
ugly face in more ways. Presumably because the timeout is usually so
long, that we'll get to actually issuing and completing it within the
normal timeout anyway.

Thanks, applied!

-- 
Jens Axboe

--
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/