[PATCH v4 0/3] Support disabling fair tag sharing

[PATCH v4 0/3] Support disabling fair tag sharing

[Bart Van Assche]

Hi Jens,

Performance of UFS devices is reduced significantly by the fair tag sharing
algorithm. This is because UFS devices have multiple logical units and a
limited queue depth (32 for UFS 3.1 devices) and also because it takes time to
give tags back after activity on a request queue has stopped. This patch series
addresses this issue by introducing a flag that allows block drivers to
disable fair sharing.

Please consider this patch series for the next merge window.

Thanks,

Bart.

Changes compared to v3:
 - Instead of disabling fair tag sharing for all block drivers, introduce a flag
   for disabling it conditionally.

Changes between v2 and v3:
 - Rebased on top of the latest kernel.

Changes between v1 and v2:
 - Restored the tags->active_queues variable and thereby fixed the
   "uninitialized variable" warning reported by the kernel test robot.

Bart Van Assche (3):
  block: Introduce flag BLK_MQ_F_DISABLE_FAIR_TAG_SHARING
  scsi: core: Support disabling fair tag sharing
  scsi: ufs: Disable fair tag sharing

 block/blk-mq-debugfs.c    | 1 +
 block/blk-mq.h            | 3 ++-
 drivers/scsi/hosts.c      | 1 +
 drivers/scsi/scsi_lib.c   | 2 ++
 drivers/ufs/core/ufshcd.c | 1 +
 include/linux/blk-mq.h    | 1 +
 include/scsi/scsi_host.h  | 6 ++++++
 7 files changed, 14 insertions(+), 1 deletion(-)

[PATCH v4 1/3] block: Introduce flag BLK_MQ_F_DISABLE_FAIR_TAG_SHARING

[Bart Van Assche]

The fair sharing algorithm has a negative performance impact for storage
devices for which the full queue depth is required to reach peak
performance, e.g. UFS devices. This is because it takes long after a
request queue became inactive until tags are reassigned to the active
request queue(s). Since making tag sharing fair is not needed if the
request processing latency is similar for all request queues, introduce
the hctx / tag set flag BLK_MQ_F_DISABLE_FAIR_TAG_SHARING.

Cc: Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Keith Busch <kbusch@kernel.org>
Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Cc: Yu Kuai <yukuai1@huaweicloud.com>
Cc: Ed Tsai <ed.tsai@mediatek.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-mq-debugfs.c | 1 +
 block/blk-mq.h         | 3 ++-
 include/linux/blk-mq.h | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 5cbeb9344f2f..f41408103106 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -198,6 +198,7 @@ static const char *const hctx_flag_name[] = {
 	HCTX_FLAG_NAME(NO_SCHED),
 	HCTX_FLAG_NAME(STACKING),
 	HCTX_FLAG_NAME(TAG_HCTX_SHARED),
+	HCTX_FLAG_NAME(DISABLE_FAIR_TAG_SHARING),
 };
 #undef HCTX_FLAG_NAME
 
diff --git a/block/blk-mq.h b/block/blk-mq.h
index f75a9ecfebde..eda6bd0611ea 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -416,7 +416,8 @@ static inline bool hctx_may_queue(struct blk_mq_hw_ctx *hctx,
 {
 	unsigned int depth, users;
 
-	if (!hctx || !(hctx->flags & BLK_MQ_F_TAG_QUEUE_SHARED))
+	if (!hctx || !(hctx->flags & BLK_MQ_F_TAG_QUEUE_SHARED) ||
+	    (hctx->flags & BLK_MQ_F_DISABLE_FAIR_TAG_SHARING))
 		return true;
 
 	/*
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 1ab3081c82ed..b12a0be839aa 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -662,6 +662,7 @@ enum {
 	 * or shared hwqs instead of 'mq-deadline'.
 	 */
 	BLK_MQ_F_NO_SCHED_BY_DEFAULT	= 1 << 7,
+	BLK_MQ_F_DISABLE_FAIR_TAG_SHARING = 1 << 8,
 	BLK_MQ_F_ALLOC_POLICY_START_BIT = 8,
 	BLK_MQ_F_ALLOC_POLICY_BITS = 1,
 

[PATCH v4 2/3] scsi: core: Support disabling fair tag sharing

[Bart Van Assche]

Allow SCSI drivers to disable the block layer fair tag sharing algorithm.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Keith Busch <kbusch@kernel.org>
Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Cc: Yu Kuai <yukuai1@huaweicloud.com>
Cc: Ed Tsai <ed.tsai@mediatek.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/hosts.c     | 1 +
 drivers/scsi/scsi_lib.c  | 2 ++
 include/scsi/scsi_host.h | 6 ++++++
 3 files changed, 9 insertions(+)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index d7f51b84f3c7..872f87001374 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -442,6 +442,7 @@ struct Scsi_Host *scsi_host_alloc(const struct scsi_host_template *sht, int priv
 	shost->no_write_same = sht->no_write_same;
 	shost->host_tagset = sht->host_tagset;
 	shost->queuecommand_may_block = sht->queuecommand_may_block;
+	shost->disable_fair_tag_sharing = sht->disable_fair_tag_sharing;
 
 	if (shost_eh_deadline == -1 || !sht->eh_host_reset_handler)
 		shost->eh_deadline = -1;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c2f647a7c1b0..68683988c466 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1985,6 +1985,8 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
 		BLK_ALLOC_POLICY_TO_MQ_FLAG(shost->hostt->tag_alloc_policy);
 	if (shost->queuecommand_may_block)
 		tag_set->flags |= BLK_MQ_F_BLOCKING;
+	if (shost->disable_fair_tag_sharing)
+		tag_set->flags |= BLK_MQ_F_DISABLE_FAIR_TAG_SHARING;
 	tag_set->driver_data = shost;
 	if (shost->host_tagset)
 		tag_set->flags |= BLK_MQ_F_TAG_HCTX_SHARED;
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 49f768d0ff37..2cd8a41b1f1c 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -461,6 +461,9 @@ struct scsi_host_template {
 	/* The queuecommand callback may block. See also BLK_MQ_F_BLOCKING. */
 	unsigned queuecommand_may_block:1;
 
+	/* See also BLK_MQ_F_DISABLE_FAIR_TAG_SHARING. */
+	unsigned disable_fair_tag_sharing:1;
+
 	/*
 	 * Countdown for host blocking with no commands outstanding.
 	 */
@@ -659,6 +662,9 @@ struct Scsi_Host {
 	/* The queuecommand callback may block. See also BLK_MQ_F_BLOCKING. */
 	unsigned queuecommand_may_block:1;
 
+	/* See also BLK_MQ_F_DISABLE_FAIR_TAG_SHARING. */
+	unsigned disable_fair_tag_sharing:1;
+
 	/* Host responded with short (<36 bytes) INQUIRY result */
 	unsigned short_inquiry:1;
 

[PATCH v4 3/3] scsi: ufs: Disable fair tag sharing

[Bart Van Assche]

Disable the block layer fair tag sharing algorithm because it
significantly reduces performance of UFS devices with a maximum queue
depth of 32.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Keith Busch <kbusch@kernel.org>
Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Cc: Yu Kuai <yukuai1@huaweicloud.com>
Cc: Ed Tsai <ed.tsai@mediatek.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index c2df07545f96..ed04d1263e02 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8866,6 +8866,7 @@ static const struct scsi_host_template ufshcd_driver_template = {
 	.max_host_blocked	= 1,
 	.track_queue_depth	= 1,
 	.skip_settle_delay	= 1,
+	.disable_fair_tag_sharing = 1,
 	.sdev_groups		= ufshcd_driver_groups,
 	.rpm_autosuspend_delay	= RPM_AUTOSUSPEND_DELAY_MS,
 };

RE: [PATCH v4 3/3] scsi: ufs: Disable fair tag sharing

[Avri Altman]

 
> Disable the block layer fair tag sharing algorithm because it
> significantly reduces performance of UFS devices with a maximum queue
> depth of 32.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Keith Busch <kbusch@kernel.org>
> Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Cc: Yu Kuai <yukuai1@huaweicloud.com>
> Cc: Ed Tsai <ed.tsai@mediatek.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

> ---
>  drivers/ufs/core/ufshcd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index c2df07545f96..ed04d1263e02 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8866,6 +8866,7 @@ static const struct scsi_host_template
> ufshcd_driver_template = {
>         .max_host_blocked       = 1,
>         .track_queue_depth      = 1,
>         .skip_settle_delay      = 1,
> +       .disable_fair_tag_sharing = 1,
>         .sdev_groups            = ufshcd_driver_groups,
>         .rpm_autosuspend_delay  = RPM_AUTOSUSPEND_DELAY_MS,
>  };

Re: [PATCH v4 0/3] Support disabling fair tag sharing

[Ming Lei]

Hi Bart,

On Mon, Oct 23, 2023 at 01:36:32PM -0700, Bart Van Assche wrote:
> Hi Jens,
> 
> Performance of UFS devices is reduced significantly by the fair tag sharing
> algorithm. This is because UFS devices have multiple logical units and a
> limited queue depth (32 for UFS 3.1 devices) and also because it takes time to
> give tags back after activity on a request queue has stopped. This patch series
> addresses this issue by introducing a flag that allows block drivers to
> disable fair sharing.
> 
> Please consider this patch series for the next merge window.

In previous post[1], you mentioned that the issue is caused by non-IO
queue of WLUN, but in this version, looks there isn't such story any more.

IMO, it isn't reasonable to account non-IO LUN for tag fairness, so
solution could be to not take non-IO queue into account for fair tag
sharing. But disabling fair tag sharing for this whole tagset could be
too over-kill.

And if you mean normal IO LUNs, can you share more details about the
performance drop? such as the test case, how many IO LUNs, and how to
observe performance drop, cause it isn't simple any more since multiple
LUN's perf has to be considered.


[1] https://lore.kernel.org/linux-block/20231018180056.2151711-1-bvanassche@acm.org/

Thanks,
Ming

Re: [PATCH v4 0/3] Support disabling fair tag sharing

[Bart Van Assche]

On 10/23/23 19:28, Ming Lei wrote:
> On Mon, Oct 23, 2023 at 01:36:32PM -0700, Bart Van Assche wrote:
>> Performance of UFS devices is reduced significantly by the fair tag sharing
>> algorithm. This is because UFS devices have multiple logical units and a
>> limited queue depth (32 for UFS 3.1 devices) and also because it takes time to
>> give tags back after activity on a request queue has stopped. This patch series
>> addresses this issue by introducing a flag that allows block drivers to
>> disable fair sharing.
>>
>> Please consider this patch series for the next merge window.
> 
> In previous post[1], you mentioned that the issue is caused by non-IO
> queue of WLUN, but in this version, looks there isn't such story any more.
> 
> IMO, it isn't reasonable to account non-IO LUN for tag fairness, so
> solution could be to not take non-IO queue into account for fair tag
> sharing. But disabling fair tag sharing for this whole tagset could be
> too over-kill.
> 
> And if you mean normal IO LUNs, can you share more details about the
> performance drop? such as the test case, how many IO LUNs, and how to
> observe performance drop, cause it isn't simple any more since multiple
> LUN's perf has to be considered.
> 
> [1] https://lore.kernel.org/linux-block/20231018180056.2151711-1-bvanassche@acm.org/

Hi Ming,

Submitting I/O to a WLUN is only one example of a use case that
activates the fair sharing algorithm for UFS devices. Another use
case is simultaneous activity for multiple data LUNs. Conventional
UFS devices typically have four data LUNs and zoned UFS devices
typically have five data LUNs. From an Android device with a zoned UFS
device:

$ adb shell ls /sys/class/scsi_device
0:0:0:0
0:0:0:1
0:0:0:2
0:0:0:3
0:0:0:4
0:0:0:49456
0:0:0:49476
0:0:0:49488

The first five are data logical units. The last three are WLUNs.

For a block size of 4 KiB, I see 144 K IOPS for queue depth 31 and
107 K IOPS for queue depth 15 (queue depth is reduced from 31 to 15
if I/O is being submitted to two LUNs simultaneously). In other words,
disabling fair sharing results in up to 35% higher IOPS for small reads
and in case two logical units are active simultaneously. I think that's
a very significant performance difference.

Thanks,

Bart.

Re: [PATCH v4 0/3] Support disabling fair tag sharing

[Ming Lei]

On Tue, Oct 24, 2023 at 09:41:50AM -0700, Bart Van Assche wrote:
> On 10/23/23 19:28, Ming Lei wrote:
> > On Mon, Oct 23, 2023 at 01:36:32PM -0700, Bart Van Assche wrote:
> > > Performance of UFS devices is reduced significantly by the fair tag sharing
> > > algorithm. This is because UFS devices have multiple logical units and a
> > > limited queue depth (32 for UFS 3.1 devices) and also because it takes time to
> > > give tags back after activity on a request queue has stopped. This patch series
> > > addresses this issue by introducing a flag that allows block drivers to
> > > disable fair sharing.
> > > 
> > > Please consider this patch series for the next merge window.
> > 
> > In previous post[1], you mentioned that the issue is caused by non-IO
> > queue of WLUN, but in this version, looks there isn't such story any more.
> > 
> > IMO, it isn't reasonable to account non-IO LUN for tag fairness, so
> > solution could be to not take non-IO queue into account for fair tag
> > sharing. But disabling fair tag sharing for this whole tagset could be
> > too over-kill.
> > 
> > And if you mean normal IO LUNs, can you share more details about the
> > performance drop? such as the test case, how many IO LUNs, and how to
> > observe performance drop, cause it isn't simple any more since multiple
> > LUN's perf has to be considered.
> > 
> > [1] https://lore.kernel.org/linux-block/20231018180056.2151711-1-bvanassche@acm.org/
> 
> Hi Ming,
> 
> Submitting I/O to a WLUN is only one example of a use case that
> activates the fair sharing algorithm for UFS devices. Another use
> case is simultaneous activity for multiple data LUNs. Conventional
> UFS devices typically have four data LUNs and zoned UFS devices
> typically have five data LUNs. From an Android device with a zoned UFS
> device:
> 
> $ adb shell ls /sys/class/scsi_device
> 0:0:0:0
> 0:0:0:1
> 0:0:0:2
> 0:0:0:3
> 0:0:0:4
> 0:0:0:49456
> 0:0:0:49476
> 0:0:0:49488
> 
> The first five are data logical units. The last three are WLUNs.
> 
> For a block size of 4 KiB, I see 144 K IOPS for queue depth 31 and
> 107 K IOPS for queue depth 15 (queue depth is reduced from 31 to 15
> if I/O is being submitted to two LUNs simultaneously). In other words,
> disabling fair sharing results in up to 35% higher IOPS for small reads
> and in case two logical units are active simultaneously. I think that's
> a very significant performance difference.

Yeah, performance does drop when queue depth is cut to half if queue
depth is low enough.

However, it isn't enough to just test perf over one LUN, what is the
perf effect when running IOs over the 2 or 5 data LUNs concurrently?

SATA should have similar issue too, and I think the improvement may be
more generic to bypass fair tag sharing in case of low queue depth
(such as < 32) if turns out the fair tag sharing doesn't work well
in case low queue depth.

Also the 'fairness' could be enhanced dynamically by scsi LUN's queue depth,
which can be adjusted dynamically.


Thanks, 
Ming

RE: [PATCH v4 0/3] Support disabling fair tag sharing

[Avri Altman]

Hi,
> On Tue, Oct 24, 2023 at 09:41:50AM -0700, Bart Van Assche wrote:
> > On 10/23/23 19:28, Ming Lei wrote:
> > > On Mon, Oct 23, 2023 at 01:36:32PM -0700, Bart Van Assche wrote:
> > > > Performance of UFS devices is reduced significantly by the fair
> > > > tag sharing algorithm. This is because UFS devices have multiple
> > > > logical units and a limited queue depth (32 for UFS 3.1 devices)
> > > > and also because it takes time to give tags back after activity on
> > > > a request queue has stopped. This patch series addresses this
> > > > issue by introducing a flag that allows block drivers to disable fair
> sharing.
> > > >
> > > > Please consider this patch series for the next merge window.
> > >
> > > In previous post[1], you mentioned that the issue is caused by
> > > non-IO queue of WLUN, but in this version, looks there isn't such story
> any more.
> > >
> > > IMO, it isn't reasonable to account non-IO LUN for tag fairness, so
> > > solution could be to not take non-IO queue into account for fair tag
> > > sharing. But disabling fair tag sharing for this whole tagset could
> > > be too over-kill.
> > >
> > > And if you mean normal IO LUNs, can you share more details about the
> > > performance drop? such as the test case, how many IO LUNs, and how
> > > to observe performance drop, cause it isn't simple any more since
> > > multiple LUN's perf has to be considered.
> > >
> > > [1]
> > > https://lore.kernel.org/linux-block/20231018180056.2151711-1-bvanass
> > > che@acm.org/
> >
> > Hi Ming,
> >
> > Submitting I/O to a WLUN is only one example of a use case that
> > activates the fair sharing algorithm for UFS devices. Another use case
> > is simultaneous activity for multiple data LUNs. Conventional UFS
> > devices typically have four data LUNs and zoned UFS devices typically
> > have five data LUNs. From an Android device with a zoned UFS
> > device:
> >
> > $ adb shell ls /sys/class/scsi_device
> > 0:0:0:0
> > 0:0:0:1
> > 0:0:0:2
> > 0:0:0:3
> > 0:0:0:4
> > 0:0:0:49456
> > 0:0:0:49476
> > 0:0:0:49488
> >
> > The first five are data logical units. The last three are WLUNs.
> >
> > For a block size of 4 KiB, I see 144 K IOPS for queue depth 31 and
> > 107 K IOPS for queue depth 15 (queue depth is reduced from 31 to 15 if
> > I/O is being submitted to two LUNs simultaneously). In other words,
> > disabling fair sharing results in up to 35% higher IOPS for small
> > reads and in case two logical units are active simultaneously. I think
> > that's a very significant performance difference.
This issue is known for a long time now.
Whenever we needed to provide performance measures,
We used to disable that mechanism, hacking the kernel locally - one way or the other.
I know that my peers are doing this as well.

> 
> Yeah, performance does drop when queue depth is cut to half if queue depth
> is low enough.
> 
> However, it isn't enough to just test perf over one LUN, what is the perf
> effect when running IOs over the 2 or 5 data LUNs concurrently?
> 
> SATA should have similar issue too, and I think the improvement may be
> more generic to bypass fair tag sharing in case of low queue depth (such as <
> 32) if turns out the fair tag sharing doesn't work well in case low queue
> depth.
> 
> Also the 'fairness' could be enhanced dynamically by scsi LUN's queue depth,
> which can be adjusted dynamically.
As far our concern as a ufs device manufacturers & users, 
We find the current proposal a clean and elegant solution we like to adopt.
Further, more sophisticated scheme can be adopted on top of disabling tag sharing.

Thanks,
Avri

> 
> 
> Thanks,
> Ming

Re: [PATCH v4 0/3] Support disabling fair tag sharing

[Bart Van Assche]

On 10/25/23 11:50, Avri Altman wrote:
> This issue is known for a long time now.
> Whenever we needed to provide performance measures,
> We used to disable that mechanism, hacking the kernel locally - one way or the other.
> I know that my peers are doing this as well.

The Android 14 kernels have this patch since May (five months ago). None
of the OEMs who use this kernel branch have complained so far.

Thanks,

Bart.

Re: [PATCH v4 0/3] Support disabling fair tag sharing

[Bart Van Assche]

On 10/24/23 18:33, Ming Lei wrote:
> Yeah, performance does drop when queue depth is cut to half if queue
> depth is low enough.
> 
> However, it isn't enough to just test perf over one LUN, what is the
> perf effect when running IOs over the 2 or 5 data LUNs
> concurrently?

I think that the results I shared are sufficient because these show the
worst possible performance impact of fair tag sharing (two active
logical units and much more activity on one logical unit than on the
other).

> SATA should have similar issue too, and I think the improvement may 
> be more generic to bypass fair tag sharing in case of low queue depth
> (such as < 32) if turns out the fair tag sharing doesn't work well in
> case low queue depth.
> 
> Also the 'fairness' could be enhanced dynamically by scsi LUN's
> queue depth, which can be adjusted dynamically.

Most SATA devices are hard disks. Hard disk IOPS are constrained by the
speed with which the head of a hard disk can move. That speed hasn't
changed much during the past 40 years. I'm not sure that hard disks are
impacted as much as SSD devices by fair tag sharing.

Any algorithm that is more complicated than what I posted probably would
have a negative performance impact on storage devices that use NAND
technology, e.g. UFS devices. So I prefer to proceed with this patch
series and solve any issues with ATA devices separately. Once this patch
series has been merged, it could be used as a basis for a solution for
ATA devices. A solution for ATA devices does not have to be implemented
in the block layer core - it could e.g. be implemented in the ATA subsystem.

Thanks,

Bart.

Re: [PATCH v4 0/3] Support disabling fair tag sharing

[Ming Lei]

On Wed, Oct 25, 2023 at 12:01:33PM -0700, Bart Van Assche wrote:
> 
> On 10/24/23 18:33, Ming Lei wrote:
> > Yeah, performance does drop when queue depth is cut to half if queue
> > depth is low enough.
> > 
> > However, it isn't enough to just test perf over one LUN, what is the
> > perf effect when running IOs over the 2 or 5 data LUNs
> > concurrently?
> 
> I think that the results I shared are sufficient because these show the
> worst possible performance impact of fair tag sharing (two active
> logical units and much more activity on one logical unit than on the
> other).

You are talking about multi-lun case, and your change does affect
multi-lun code path, but your test result doesn't cover multi-lun,
is it enough?

At least your patch shouldn't cause performance regression on multi-lun IO
workloads, right?

> 
> > SATA should have similar issue too, and I think the improvement may be
> > more generic to bypass fair tag sharing in case of low queue depth
> > (such as < 32) if turns out the fair tag sharing doesn't work well in
> > case low queue depth.
> > 
> > Also the 'fairness' could be enhanced dynamically by scsi LUN's
> > queue depth, which can be adjusted dynamically.
> 
> Most SATA devices are hard disks. Hard disk IOPS are constrained by the
> speed with which the head of a hard disk can move. That speed hasn't
> changed much during the past 40 years. I'm not sure that hard disks are
> impacted as much as SSD devices by fair tag sharing.

What I meant is that SATA's queue depth is often 32 or 31, and still have
multi-lun cases.

At least from what you shared, the fair tag sharing doesn't work well
just because of low queue depth, nothing is actually related with UFS.

That is why I am wondering that why not force to disable fairing sharing
in case of low queue depth.

> 
> Any algorithm that is more complicated than what I posted probably would
> have a negative performance impact on storage devices that use NAND
> technology, e.g. UFS devices. So I prefer to proceed with this patch
> series and solve any issues with ATA devices separately. Once this patch
> series has been merged, it could be used as a basis for a solution for
> ATA devices. A solution for ATA devices does not have to be implemented
> in the block layer core - it could e.g. be implemented in the ATA subsystem.

I don't object to take the disabling fair sharing first, and I meant that
the fairness may be brought back by adjusting scsi_device's queue depth in
future.


Thanks,
Ming

Re: [PATCH v4 0/3] Support disabling fair tag sharing

[Bart Van Assche]

On 10/25/23 16:37, Ming Lei wrote:
> You are talking about multi-lun case, and your change does affect 
> multi-lun code path, but your test result doesn't cover multi-lun, is
> it enough?
I think that the results I shared are sufficient because these show the
worst possible performance impact of fair tag sharing. If there are two
active logical units and much more I/O is sent to one logical unit than
to the other, then the aggregate performance will be very close to what
I shared in a previous email.

> At least your patch shouldn't cause performance regression on
> multi-lun IO workloads, right?
If blk_mq_get_tag() can't allocate a tag, and if multiple threads are
waiting for a tag, the thread that called blk_mq_get_tag() first is
granted the first tag that is released. I think this guarantees fairness
if all requests have a similar latency. There will be some unfairness if
there are significant differences in latency per logical unit, e.g.
because all requests sent to one logical unit are small and because all
requests sent to another logical unit are large. Whether or not this
matters depends on the use case.

Thanks,

Bart.

Re: [PATCH v4 0/3] Support disabling fair tag sharing

[Yu Kuai]

Hi,

在 2023/10/27 0:29, Bart Van Assche 写道:
> If blk_mq_get_tag() can't allocate a tag, and if multiple threads are
> waiting for a tag, the thread that called blk_mq_get_tag() first is
> granted the first tag that is released. I think this guarantees fairness
> if all requests have a similar latency. There will be some unfairness if
> there are significant differences in latency per logical unit, e.g.
> because all requests sent to one logical unit are small and because all
> requests sent to another logical unit are large. Whether or not this
> matters depends on the use case.

I'm afraid that is not correct, fairness can't be guranteed at all, not
even with just one scsi disk. This is because there are 8 wait queues in
sbitmap, and threads are waiting in roundrobin mode, and each time
wake_batch tags are released, wake_batch threads of one wait queue will
be woke up, regardless that some threads can't grab tag after woken up,
what's worse, thoese thread will be added to the tail of waitqueue
again.

In the case that high io pressure under a slow disk, this behaviour will
cause that io tail latency will be quite bad compared to sq from old
kernel.

AFAIC, disable tag sharing will definitely case some regresion, for
example, one disk will high io pressure, and another disk only issure
one IO at a time, disable tag sharing can improve brandwith of fist
disk, however, for the latter disk, IO latency will definitely be much
worse.

Thanks,
Kuai

> 
> Thanks,
> 
> Bart.
> 
> 
> 
> .
> 

Re: [PATCH v4 0/3] Support disabling fair tag sharing

[Bart Van Assche]

On 10/30/23 19:01, Yu Kuai wrote:
> I'm afraid that is not correct, fairness can't be guranteed at all, not
> even with just one scsi disk. This is because there are 8 wait queues in
> sbitmap, and threads are waiting in roundrobin mode, and each time
> wake_batch tags are released, wake_batch threads of one wait queue will
> be woke up, regardless that some threads can't grab tag after woken up,
> what's worse, thoese thread will be added to the tail of waitqueue
> again.
> 
> In the case that high io pressure under a slow disk, this behaviour will
> cause that io tail latency will be quite bad compared to sq from old
> kernel.
> 
> AFAIC, disable tag sharing will definitely case some regresion, for
> example, one disk will high io pressure, and another disk only issure
> one IO at a time, disable tag sharing can improve brandwith of fist
> disk, however, for the latter disk, IO latency will definitely be much
> worse.

Hi Yu,

All UFS users I know prefer that fair tag sharing is disabled for UFS
devices. So I propose to proceed with this patch series and additionally
to improve fair tag sharing for devices and transport layers that need
an improved sharing algorithm.

Thanks,

Bart.