linux-kernel - Re: [patch 1/2]scsi: scsi_run_queue() doesn't use local list to handle starved sdev

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: <1324603045.9282.2.camel@dabdike.cust.hotspot.t-mobile.com>
Date:	Thu, 22 Dec 2011 19:17:25 -0600
From:	James Bottomley <James.Bottomley@...senPartnership.com>
To:	Shaohua Li <shaohua.li@...el.com>
Cc:	lkml <linux-kernel@...r.kernel.org>,
	"linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	Jens Axboe <axboe@...nel.dk>,
	Christoph Hellwig <hch@...radead.org>,
	Ted Ts'o <tytso@....edu>,
	"Wu, Fengguang" <fengguang.wu@...el.com>,
	"Darrick J. Wong" <djwong@...ibm.com>
Subject: Re: [patch 1/2]scsi: scsi_run_queue() doesn't use local list to
 handle starved sdev

On Fri, 2011-12-23 at 08:40 +0800, Shaohua Li wrote:
> On Thu, 2011-12-22 at 18:27 +0000, James Bottomley wrote:
> > On Thu, 2011-12-22 at 11:10 +0800, Shaohua Li wrote:
> > > scsi_run_queue() picks off all sdev from host starved_list to a local list,
> > > then handle them. If there are multiple threads running scsi_run_queue(),
> > > the starved_list will get messed. This is quite common, because request
> > > rq_affinity is on by default.
> > > 
> > > Signed-off-by: Shaohua Li <shaohua.li@...el.com>
> > > ---
> > >  drivers/scsi/scsi_lib.c |   21 ++++++++++++++-------
> > >  1 file changed, 14 insertions(+), 7 deletions(-)
> > > 
> > > Index: linux/drivers/scsi/scsi_lib.c
> > > ===================================================================
> > > --- linux.orig/drivers/scsi/scsi_lib.c	2011-12-21 16:56:23.000000000 +0800
> > > +++ linux/drivers/scsi/scsi_lib.c	2011-12-22 09:33:09.000000000 +0800
> > > @@ -401,9 +401,8 @@ static inline int scsi_host_is_busy(stru
> > >   */
> > >  static void scsi_run_queue(struct request_queue *q)
> > >  {
> > > -	struct scsi_device *sdev = q->queuedata;
> > > +	struct scsi_device *sdev = q->queuedata, *head_sdev = NULL;
> > >  	struct Scsi_Host *shost;
> > > -	LIST_HEAD(starved_list);
> > >  	unsigned long flags;
> > >  
> > >  	/* if the device is dead, sdev will be NULL, so no queue to run */
> > > @@ -415,9 +414,8 @@ static void scsi_run_queue(struct reques
> > >  		scsi_single_lun_run(sdev);
> > >  
> > >  	spin_lock_irqsave(shost->host_lock, flags);
> > > -	list_splice_init(&shost->starved_list, &starved_list);
> > >  
> > > -	while (!list_empty(&starved_list)) {
> > > +	while (!list_empty(&shost->starved_list)) {
> > 
> > The original reason for working from a copy instead of the original list
> > was that the device can end up back on the starved list because of a
> > variety of conditions in the HBA and so this would cause the loop not to
> > exit, so this piece of the patch doesn't look right to me.
> +               /*
> +                * the head sdev is no longer starved and removed from the
> +                * starved list, select a new sdev as head.
> +                */
> +               if (head_sdev == sdev && list_empty(&sdev->starved_entry))
> +                       head_sdev = NULL;
> I had this in the loop, which is to guarantee the loop will exit if a
> device is removed from the starved list.

And the non-head sdevs? which can also get put back onto the list

What's the reason for not just traversing the list once using list
splice?

Basically, the changelog doesn't seem to explain what you're doing and
the logic looks flawed.

James


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