Messages in this thread |  | | | Subject | Re: Perfromance drop on SCSI hard disk | | From | "Alex,Shi" <> | | Date | Fri, 20 May 2011 13:17:43 +0800 |
| |
On Fri, 2011-05-20 at 08:40 +0800, Shaohua Li wrote: > 2011/5/20 Alex,Shi <alex.shi@intel.com>: > > On Fri, 2011-05-20 at 02:27 +0800, Jens Axboe wrote: > >> On 2011-05-19 10:26, Alex,Shi wrote: > >> > > >> >> I will queue up the combined patch, it looks fine from here as well. > >> >> > >> > > >> > When I have some time to study Jens and shaohua's patch today. I find a > >> > simpler way to resolved the re-enter issue on starved_list. Following > >> > Jens' idea, we can just put the starved_list device into kblockd if it > >> > come from __scsi_queue_insert(). > >> > It can resolve the re-enter issue and recover performance totally, and > >> > need not a work_struct in every scsi_device. The logic/code also looks a > >> > bit simpler. > >> > What's your opinion of this? > >> > >> Isn't this _identical_ to my original patch, with the added async run of > >> the queue passed in (which is important, an oversight)? > > > > Not exactly same. It bases on your patch, but added a bypass way for > > starved_list device. If a starved_list device come from > > __scsi_queue_insert(), that may caused by our talking recursion, kblockd > > with take over the process. Maybe you oversight this point in original > > patch. :) > > > > The different part from yours is below: > > --- > > static void __scsi_run_queue(struct request_queue *q, bool async) > > { > > struct scsi_device *sdev = q->queuedata; > > struct Scsi_Host *shost; > > @@ -435,30 +437,35 @@ static void scsi_run_queue(struct request_queue > > *q) > > &shost->starved_list); > > continue; > > } > > - > > - spin_unlock(shost->host_lock); > > - spin_lock(sdev->request_queue->queue_lock); > > - __blk_run_queue(sdev->request_queue); > > - spin_unlock(sdev->request_queue->queue_lock); > > - spin_lock(shost->host_lock); > > + if (async) > > + blk_run_queue_async(sdev->request_queue); > > + else { > > + spin_unlock(shost->host_lock); > > + spin_lock(sdev->request_queue->queue_lock); > > + __blk_run_queue(sdev->request_queue); > > + spin_unlock(sdev->request_queue->queue_lock); > > + spin_lock(shost->host_lock); > >> > I don't quite like this approach. blk_run_queue_async() could > introduce fairness issue as I said in previous mail, because we drop > the sdev from starved list but didn't run its queue immediately. The > issue exists before, but it's a bug to me.
I understand what's your worried. But not quite clear of the trigger scenario. anyway, it is still a potential issue of fairness exist. So forget my patch.
|  |