On Thu, Jan 11 2018 at 8:42pm -0500, Ming Lei <ming.lei@xxxxxxxxxx> wrote: > On Thu, Jan 11, 2018 at 10:37:37PM +0000, Bart Van Assche wrote: > > On Thu, 2018-01-11 at 17:07 -0500, Mike Snitzer wrote: > > > Bart, if for some reason we regress for some workload you're able to > > > more readily test we can deal with it. But I'm too encouraged by Ming's > > > performance improvements to hold these changes back any further. > > > > Sorry Mike but I think Ming's measurement results are not sufficient to > > motivate upstreaming of these patches. What I remember from previous versions > > of this patch series is that Ming measured the performance impact of this > > patch series only for the Emulex FC driver (lpfc). That driver limits queue > > depth to 3. Instead of modifying the dm code, that driver needs to be fixed > > such that it allows larger queue depths. > > > > Additionally, some time ago I had explained in detail why I think that patch > > 2/5 in this series is wrong and why it will introduce fairness regressions > > in multi-LUN workloads. I think we need performance measurements for this > > patch series for at least the following two configurations before this patch > > series can be considered for upstream inclusion: > > * The performance impact of this patch series for SCSI devices with a > > realistic queue depth (e.g. 64 or 128). > > Please look at the cover letter or patch 5's commit log, it mentions that > dm-mpath over virtio-scsi is tested, and the default queue depth of virito-scsi > is 128. > > > * The performance impact for this patch series for a SCSI host with which > > multiple LUNs are associated and for which the target system often replies > > with the SCSI "BUSY" status. > > I don't think this patch is related with this case, this patch just provides the > BUSY feedback from underlying queue to blk-mq via dm-rq. Hi Ming, Please see https://www.redhat.com/archives/dm-devel/2017-April/msg00190.html Specifically: "That patch [commit 6077c2d706] can be considered as a first step that can be refined further, namely by modifying the dm-rq code further such that dm-rq queues are only rerun after the busy condition has been cleared. The patch at the start of this thread is easier to review and easier to test than any patch that would only rerun dm-rq queues after the busy condition has been cleared." Do you have time to reason through Bart's previous proposal for how to better address the specific issue that was documented to be fixed in the header for commit 6077c2d706 ? Otherwise I fear we'll just keep hitting a wall with Bart... Thanks, Mike
