On Thu, Jan 11, 2018 at 08:57:21PM -0500, Mike Snitzer wrote: > 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 ? Hi Mike, Recently we fixed one such issue in blk-mq (eb619fdb2d4cb8b3d34), and I highly guess that may fix Bart's case. Let's see this commit 6077c2d706 again, I don't know the idea behind the fix, which just adds random of 100ms, and this delay degrades performance a lot, since no hctx can be scheduled any more during the delay. So again it is definitely a workaround, no any reasoning, no any theory, just say it is a fix. Are there anyone who can explain the story? IMO, the blk_get_request() in dm-mpath is just like a kmalloc(ATOMIC) which is used in other blk-mq drivers' IO path too, so don't know why dm-mq is so strange to require such ugly 100ms delay. So I suggest to remove it, let's see if there are reports, and if there are, I am quite confident we can root cause that and fix that. > > Otherwise I fear we'll just keep hitting a wall with Bart... I do test dm-mq over scsi-debug which is setup as two LUNs, and set can_queue as 1, and this patchset just works well, not any IO hang. And anyone can try and play with it. Thanks, Ming -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
