[PATCH 0/3] Improve mq-deadline I/O priority support

[PATCH 0/3] Improve mq-deadline I/O priority support

[Bart Van Assche]

Hi Jens,

This patch series includes the following changes for the mq-deadline I/O
scheduler:
 - Do not modify the request order if prio_aging_expire is zero. This
   change restores compatibility of the mq-deadline scheduler with MMC
   devices if prio_aging_expire is set to zero.
 - Ignore I/O priorities for zoned writes that are not write append
   operations such that zoned writes happen in LBA order per zone even
   if different write requests have different I/O priorities in a single
   zone.

Please consider these patches for the next merge window.

Thanks,

Bart.

Bart Van Assche (3):
  block/mq-deadline: Use dd_rq_ioclass() instead of open-coding it
  block/mq-deadline: Introduce dd_bio_ioclass()
  block/mq-deadline: Disable I/O prioritization in certain cases

 block/mq-deadline.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

[PATCH 1/3] block/mq-deadline: Use dd_rq_ioclass() instead of open-coding it

[Bart Van Assche]

Use an existing helper function instead of open-coding it. No
functionality is changed by this patch.

Cc: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/mq-deadline.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/block/mq-deadline.c b/block/mq-deadline.c
index f958e79277b8..e558f3660357 100644
--- a/block/mq-deadline.c
+++ b/block/mq-deadline.c
@@ -798,8 +798,6 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
 	struct request_queue *q = hctx->queue;
 	struct deadline_data *dd = q->elevator->elevator_data;
 	const enum dd_data_dir data_dir = rq_data_dir(rq);
-	u16 ioprio = req_get_ioprio(rq);
-	u8 ioprio_class = IOPRIO_PRIO_CLASS(ioprio);
 	struct dd_per_prio *per_prio;
 	enum dd_prio prio;
 
@@ -811,7 +809,7 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
 	 */
 	blk_req_zone_write_unlock(rq);
 
-	prio = ioprio_class_to_prio[ioprio_class];
+	prio = ioprio_class_to_prio[dd_rq_ioclass(rq)];
 	per_prio = &dd->per_prio[prio];
 	if (!rq->elv.priv[0]) {
 		per_prio->stats.inserted++;

Re: [PATCH 1/3] block/mq-deadline: Use dd_rq_ioclass() instead of open-coding it

[Damien Le Moal]

On 12/5/23 14:32, Bart Van Assche wrote:
> Use an existing helper function instead of open-coding it. No
> functionality is changed by this patch.
> 
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> ---
>  block/mq-deadline.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
> index f958e79277b8..e558f3660357 100644
> --- a/block/mq-deadline.c
> +++ b/block/mq-deadline.c
> @@ -798,8 +798,6 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>  	struct request_queue *q = hctx->queue;
>  	struct deadline_data *dd = q->elevator->elevator_data;
>  	const enum dd_data_dir data_dir = rq_data_dir(rq);
> -	u16 ioprio = req_get_ioprio(rq);
> -	u8 ioprio_class = IOPRIO_PRIO_CLASS(ioprio);
>  	struct dd_per_prio *per_prio;
>  	enum dd_prio prio;
>  
> @@ -811,7 +809,7 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>  	 */
>  	blk_req_zone_write_unlock(rq);
>  
> -	prio = ioprio_class_to_prio[ioprio_class];
> +	prio = ioprio_class_to_prio[dd_rq_ioclass(rq)];
>  	per_prio = &dd->per_prio[prio];
>  	if (!rq->elv.priv[0]) {
>  		per_prio->stats.inserted++;

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/3] block/mq-deadline: Use dd_rq_ioclass() instead of open-coding it

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/3] block/mq-deadline: Introduce dd_bio_ioclass()

[Bart Van Assche]

Prepare for disabling I/O prioritization in certain cases.

Cc: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/mq-deadline.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/mq-deadline.c b/block/mq-deadline.c
index e558f3660357..fe5da2ade953 100644
--- a/block/mq-deadline.c
+++ b/block/mq-deadline.c
@@ -128,6 +128,11 @@ static u8 dd_rq_ioclass(struct request *rq)
 	return IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
 }
 
+static u8 dd_bio_ioclass(struct bio *bio)
+{
+	return IOPRIO_PRIO_CLASS(bio->bi_ioprio);
+}
+
 /*
  * get the request before `rq' in sector-sorted order
  */
@@ -744,7 +749,7 @@ static int dd_request_merge(struct request_queue *q, struct request **rq,
 			    struct bio *bio)
 {
 	struct deadline_data *dd = q->elevator->elevator_data;
-	const u8 ioprio_class = IOPRIO_PRIO_CLASS(bio->bi_ioprio);
+	const u8 ioprio_class = dd_bio_ioclass(bio);
 	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
 	struct dd_per_prio *per_prio = &dd->per_prio[prio];
 	sector_t sector = bio_end_sector(bio);

Re: [PATCH 2/3] block/mq-deadline: Introduce dd_bio_ioclass()

[Damien Le Moal]

On 12/5/23 14:32, Bart Van Assche wrote:
> Prepare for disabling I/O prioritization in certain cases.
> 
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> ---
>  block/mq-deadline.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
> index e558f3660357..fe5da2ade953 100644
> --- a/block/mq-deadline.c
> +++ b/block/mq-deadline.c
> @@ -128,6 +128,11 @@ static u8 dd_rq_ioclass(struct request *rq)
>  	return IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
>  }
>  
> +static u8 dd_bio_ioclass(struct bio *bio)
> +{
> +	return IOPRIO_PRIO_CLASS(bio->bi_ioprio);
> +}
> +
>  /*
>   * get the request before `rq' in sector-sorted order
>   */
> @@ -744,7 +749,7 @@ static int dd_request_merge(struct request_queue *q, struct request **rq,
>  			    struct bio *bio)
>  {
>  	struct deadline_data *dd = q->elevator->elevator_data;
> -	const u8 ioprio_class = IOPRIO_PRIO_CLASS(bio->bi_ioprio);
> +	const u8 ioprio_class = dd_bio_ioclass(bio);
>  	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
>  	struct dd_per_prio *per_prio = &dd->per_prio[prio];
>  	sector_t sector = bio_end_sector(bio);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/3] block/mq-deadline: Introduce dd_bio_ioclass()

[Christoph Hellwig]

On Mon, Dec 04, 2023 at 09:32:12PM -0800, Bart Van Assche wrote:
> Prepare for disabling I/O prioritization in certain cases.

On it's own this looks pretty useless to be honest, an with just a
single caller it can't really make future patches significantly
simpler either.

Re: [PATCH 2/3] block/mq-deadline: Introduce dd_bio_ioclass()

[Bart Van Assche]

On 12/11/23 08:55, Christoph Hellwig wrote:
> On Mon, Dec 04, 2023 at 09:32:12PM -0800, Bart Van Assche wrote:
>> Prepare for disabling I/O prioritization in certain cases.
> 
> On it's own this looks pretty useless to be honest, an with just a
> single caller it can't really make future patches significantly
> simpler either.

This patch helps because a helper function can have multiple return
statements. The alternative without helper function is using a less
readable ternary conditional.

Bart.

[PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Bart Van Assche]

Fix the following two issues:
- Even with prio_aging_expire set to zero, I/O priorities still affect the
  request order.
- Assigning I/O priorities with the ioprio cgroup policy breaks zoned
  storage support in the mq-deadline scheduler.

This patch fixes both issues by disabling I/O prioritization for these
two cases.

Cc: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/mq-deadline.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/block/mq-deadline.c b/block/mq-deadline.c
index fe5da2ade953..6781cef0109e 100644
--- a/block/mq-deadline.c
+++ b/block/mq-deadline.c
@@ -123,14 +123,16 @@ deadline_rb_root(struct dd_per_prio *per_prio, struct request *rq)
  * Returns the I/O priority class (IOPRIO_CLASS_*) that has been assigned to a
  * request.
  */
-static u8 dd_rq_ioclass(struct request *rq)
+static u8 dd_rq_ioclass(struct deadline_data *dd, struct request *rq)
 {
-	return IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
+	return dd->prio_aging_expire ? IOPRIO_PRIO_CLASS(req_get_ioprio(rq)) :
+				       IOPRIO_CLASS_NONE;
 }
 
-static u8 dd_bio_ioclass(struct bio *bio)
+static u8 dd_bio_ioclass(struct deadline_data *dd, struct bio *bio)
 {
-	return IOPRIO_PRIO_CLASS(bio->bi_ioprio);
+	return dd->prio_aging_expire ? IOPRIO_PRIO_CLASS(bio->bi_ioprio) :
+				       IOPRIO_CLASS_NONE;
 }
 
 /*
@@ -233,7 +235,7 @@ static void dd_request_merged(struct request_queue *q, struct request *req,
 			      enum elv_merge type)
 {
 	struct deadline_data *dd = q->elevator->elevator_data;
-	const u8 ioprio_class = dd_rq_ioclass(req);
+	const u8 ioprio_class = dd_rq_ioclass(dd, req);
 	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
 	struct dd_per_prio *per_prio = &dd->per_prio[prio];
 
@@ -253,7 +255,7 @@ static void dd_merged_requests(struct request_queue *q, struct request *req,
 			       struct request *next)
 {
 	struct deadline_data *dd = q->elevator->elevator_data;
-	const u8 ioprio_class = dd_rq_ioclass(next);
+	const u8 ioprio_class = dd_rq_ioclass(dd, next);
 	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
 
 	lockdep_assert_held(&dd->lock);
@@ -550,7 +552,7 @@ static struct request *__dd_dispatch_request(struct deadline_data *dd,
 	dd->batching++;
 	deadline_move_request(dd, per_prio, rq);
 done:
-	ioprio_class = dd_rq_ioclass(rq);
+	ioprio_class = dd_rq_ioclass(dd, rq);
 	prio = ioprio_class_to_prio[ioprio_class];
 	dd->per_prio[prio].latest_pos[data_dir] = blk_rq_pos(rq);
 	dd->per_prio[prio].stats.dispatched++;
@@ -749,7 +751,7 @@ static int dd_request_merge(struct request_queue *q, struct request **rq,
 			    struct bio *bio)
 {
 	struct deadline_data *dd = q->elevator->elevator_data;
-	const u8 ioprio_class = dd_bio_ioclass(bio);
+	const u8 ioprio_class = dd_bio_ioclass(dd, bio);
 	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
 	struct dd_per_prio *per_prio = &dd->per_prio[prio];
 	sector_t sector = bio_end_sector(bio);
@@ -814,7 +816,7 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
 	 */
 	blk_req_zone_write_unlock(rq);
 
-	prio = ioprio_class_to_prio[dd_rq_ioclass(rq)];
+	prio = ioprio_class_to_prio[dd_rq_ioclass(dd, rq)];
 	per_prio = &dd->per_prio[prio];
 	if (!rq->elv.priv[0]) {
 		per_prio->stats.inserted++;
@@ -923,7 +925,7 @@ static void dd_finish_request(struct request *rq)
 {
 	struct request_queue *q = rq->q;
 	struct deadline_data *dd = q->elevator->elevator_data;
-	const u8 ioprio_class = dd_rq_ioclass(rq);
+	const u8 ioprio_class = dd_rq_ioclass(dd, rq);
 	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
 	struct dd_per_prio *per_prio = &dd->per_prio[prio];
 

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Damien Le Moal]

On 12/5/23 14:32, Bart Van Assche wrote:
> Fix the following two issues:
> - Even with prio_aging_expire set to zero, I/O priorities still affect the
>   request order.
> - Assigning I/O priorities with the ioprio cgroup policy breaks zoned
>   storage support in the mq-deadline scheduler.

Can you provide more details ? E.g. an example of a setup that breaks ordering ?

> This patch fixes both issues by disabling I/O prioritization for these
> two cases.

... when prio_aging_expire is set to 0. Right ? Otherwise, the sentence above
reads as if you are disabling IO priority for zoned devices...

Also,

> 
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  block/mq-deadline.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
> index fe5da2ade953..6781cef0109e 100644
> --- a/block/mq-deadline.c
> +++ b/block/mq-deadline.c
> @@ -123,14 +123,16 @@ deadline_rb_root(struct dd_per_prio *per_prio, struct request *rq)
>   * Returns the I/O priority class (IOPRIO_CLASS_*) that has been assigned to a
>   * request.
>   */
> -static u8 dd_rq_ioclass(struct request *rq)
> +static u8 dd_rq_ioclass(struct deadline_data *dd, struct request *rq)
>  {
> -	return IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
> +	return dd->prio_aging_expire ? IOPRIO_PRIO_CLASS(req_get_ioprio(rq)) :
> +				       IOPRIO_CLASS_NONE;

I personally would prefer the simpler:

	if (!dd->prio_aging_expire)
		return IOPRIO_CLASS_NONE;
	return IOPRIO_PRIO_CLASS(req_get_ioprio(rq));

>  }
>  
> -static u8 dd_bio_ioclass(struct bio *bio)
> +static u8 dd_bio_ioclass(struct deadline_data *dd, struct bio *bio)
>  {
> -	return IOPRIO_PRIO_CLASS(bio->bi_ioprio);
> +	return dd->prio_aging_expire ? IOPRIO_PRIO_CLASS(bio->bi_ioprio) :
> +				       IOPRIO_CLASS_NONE;

Same comment as above.

>  }
>  
>  /*
> @@ -233,7 +235,7 @@ static void dd_request_merged(struct request_queue *q, struct request *req,
>  			      enum elv_merge type)
>  {
>  	struct deadline_data *dd = q->elevator->elevator_data;
> -	const u8 ioprio_class = dd_rq_ioclass(req);
> +	const u8 ioprio_class = dd_rq_ioclass(dd, req);
>  	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
>  	struct dd_per_prio *per_prio = &dd->per_prio[prio];
>  
> @@ -253,7 +255,7 @@ static void dd_merged_requests(struct request_queue *q, struct request *req,
>  			       struct request *next)
>  {
>  	struct deadline_data *dd = q->elevator->elevator_data;
> -	const u8 ioprio_class = dd_rq_ioclass(next);
> +	const u8 ioprio_class = dd_rq_ioclass(dd, next);
>  	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
>  
>  	lockdep_assert_held(&dd->lock);
> @@ -550,7 +552,7 @@ static struct request *__dd_dispatch_request(struct deadline_data *dd,
>  	dd->batching++;
>  	deadline_move_request(dd, per_prio, rq);
>  done:
> -	ioprio_class = dd_rq_ioclass(rq);
> +	ioprio_class = dd_rq_ioclass(dd, rq);
>  	prio = ioprio_class_to_prio[ioprio_class];
>  	dd->per_prio[prio].latest_pos[data_dir] = blk_rq_pos(rq);
>  	dd->per_prio[prio].stats.dispatched++;
> @@ -749,7 +751,7 @@ static int dd_request_merge(struct request_queue *q, struct request **rq,
>  			    struct bio *bio)
>  {
>  	struct deadline_data *dd = q->elevator->elevator_data;
> -	const u8 ioprio_class = dd_bio_ioclass(bio);
> +	const u8 ioprio_class = dd_bio_ioclass(dd, bio);
>  	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
>  	struct dd_per_prio *per_prio = &dd->per_prio[prio];
>  	sector_t sector = bio_end_sector(bio);
> @@ -814,7 +816,7 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>  	 */
>  	blk_req_zone_write_unlock(rq);
>  
> -	prio = ioprio_class_to_prio[dd_rq_ioclass(rq)];
> +	prio = ioprio_class_to_prio[dd_rq_ioclass(dd, rq)];
>  	per_prio = &dd->per_prio[prio];
>  	if (!rq->elv.priv[0]) {
>  		per_prio->stats.inserted++;
> @@ -923,7 +925,7 @@ static void dd_finish_request(struct request *rq)
>  {
>  	struct request_queue *q = rq->q;
>  	struct deadline_data *dd = q->elevator->elevator_data;
> -	const u8 ioprio_class = dd_rq_ioclass(rq);
> +	const u8 ioprio_class = dd_rq_ioclass(dd, rq);
>  	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
>  	struct dd_per_prio *per_prio = &dd->per_prio[prio];

What about the call to dd_dispatch_prio_aged_requests() in
dd_dispatch_request() ? Shouldn't that call be skipped if prio_aging_expire is 0 ?

>  

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Bart Van Assche]

On 12/5/23 16:42, Damien Le Moal wrote:
> On 12/5/23 14:32, Bart Van Assche wrote:
>> Fix the following two issues:
>> - Even with prio_aging_expire set to zero, I/O priorities still affect the
>>    request order.
>> - Assigning I/O priorities with the ioprio cgroup policy breaks zoned
>>    storage support in the mq-deadline scheduler.
> 
> Can you provide more details ? E.g. an example of a setup that breaks ordering ?

Regarding prio_aging_expire that is set to zero: a third party Android
developer reported that request prioritization in
mq-deadline is not compatible with MMC devices
(https://android-review.googlesource.com/c/kernel/common/+/2836235).

Regarding zoned storage and I/O priorities: we are working on modifying
Android such that foreground apps have a higher I/O priority than
background apps. As one can see in
https://android-review.googlesource.com/c/platform/system/core/+/2768906, 
we plan to use the ioprio cgroup policy to implement this. We noticed 
that this CL breaks zoned storage support if the mq-deadline I/O 
scheduler has been selected. I think that this should be solved in the
mq-deadline I/O
scheduler and also that ignoring I/O priorities for zoned writes that
must be submitted in order is a good solution. Respecting the I/O
priority for writes would require to make significant changes in the
layer that submits the zoned writes (F2FS): it would require that that
layer is modified such that it adds writes from foreground apps to the
log before those of background apps.

>> -static u8 dd_rq_ioclass(struct request *rq)
>> +static u8 dd_rq_ioclass(struct deadline_data *dd, struct request *rq)
>>   {
>> -	return IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
>> +	return dd->prio_aging_expire ? IOPRIO_PRIO_CLASS(req_get_ioprio(rq)) :
>> +				       IOPRIO_CLASS_NONE;
> 
> I personally would prefer the simpler:
> 
> 	if (!dd->prio_aging_expire)
> 		return IOPRIO_CLASS_NONE;
> 	return IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
> 
>>   }
>>   
>> -static u8 dd_bio_ioclass(struct bio *bio)
>> +static u8 dd_bio_ioclass(struct deadline_data *dd, struct bio *bio)
>>   {
>> -	return IOPRIO_PRIO_CLASS(bio->bi_ioprio);
>> +	return dd->prio_aging_expire ? IOPRIO_PRIO_CLASS(bio->bi_ioprio) :
>> +				       IOPRIO_CLASS_NONE;
> 
> Same comment as above.

I will make the proposed changes.
> What about the call to dd_dispatch_prio_aged_requests() in
> dd_dispatch_request() ? Shouldn't that call be skipped if prio_aging_expire is 0 ?

That sounds like a useful optimization to me. I will look into this.

Thanks,

Bart.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Bart Van Assche]

On 12/5/23 16:42, Damien Le Moal wrote:
> ... when prio_aging_expire is set to 0. Right ? Otherwise, the sentence above
> reads as if you are disabling IO priority for zoned devices...

Hi Damien,

I just noticed that I posted an old version of this patch (3/3). In the patch
below I/O priorities are ignored for zoned writes.

Thanks,

Bart.


Subject: [PATCH] block/mq-deadline: Disable I/O prioritization in certain cases

Fix the following two issues:
- Even with prio_aging_expire set to zero, I/O priorities still affect the
   request order.
- Assigning I/O priorities with the ioprio cgroup policy breaks zoned
   storage support in the mq-deadline scheduler.

This patch fixes both issues by disabling I/O prioritization for these
two cases.

Cc: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  block/mq-deadline.c | 37 ++++++++++++++++++++++++-------------
  1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/block/mq-deadline.c b/block/mq-deadline.c
index fe5da2ade953..310ff133ce20 100644
--- a/block/mq-deadline.c
+++ b/block/mq-deadline.c
@@ -119,18 +119,27 @@ deadline_rb_root(struct dd_per_prio *per_prio, struct request *rq)
  	return &per_prio->sort_list[rq_data_dir(rq)];
  }

+static bool dd_use_io_priority(struct deadline_data *dd, enum req_op op)
+{
+	return dd->prio_aging_expire != 0 && !op_needs_zoned_write_locking(op);
+}
+
  /*
   * Returns the I/O priority class (IOPRIO_CLASS_*) that has been assigned to a
   * request.
   */
-static u8 dd_rq_ioclass(struct request *rq)
+static u8 dd_rq_ioclass(struct deadline_data *dd, struct request *rq)
  {
-	return IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
+	if (dd_use_io_priority(dd, req_op(rq)))
+		return IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
+	return IOPRIO_CLASS_NONE;
  }

-static u8 dd_bio_ioclass(struct bio *bio)
+static u8 dd_bio_ioclass(struct deadline_data *dd, struct bio *bio)
  {
-	return IOPRIO_PRIO_CLASS(bio->bi_ioprio);
+	if (dd_use_io_priority(dd, bio_op(bio)))
+		return IOPRIO_PRIO_CLASS(bio->bi_ioprio);
+	return IOPRIO_CLASS_NONE;
  }

  /*
@@ -233,7 +242,7 @@ static void dd_request_merged(struct request_queue *q, struct request *req,
  			      enum elv_merge type)
  {
  	struct deadline_data *dd = q->elevator->elevator_data;
-	const u8 ioprio_class = dd_rq_ioclass(req);
+	const u8 ioprio_class = dd_rq_ioclass(dd, req);
  	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
  	struct dd_per_prio *per_prio = &dd->per_prio[prio];

@@ -253,7 +262,7 @@ static void dd_merged_requests(struct request_queue *q, struct request *req,
  			       struct request *next)
  {
  	struct deadline_data *dd = q->elevator->elevator_data;
-	const u8 ioprio_class = dd_rq_ioclass(next);
+	const u8 ioprio_class = dd_rq_ioclass(dd, next);
  	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];

  	lockdep_assert_held(&dd->lock);
@@ -550,7 +559,7 @@ static struct request *__dd_dispatch_request(struct deadline_data *dd,
  	dd->batching++;
  	deadline_move_request(dd, per_prio, rq);
  done:
-	ioprio_class = dd_rq_ioclass(rq);
+	ioprio_class = dd_rq_ioclass(dd, rq);
  	prio = ioprio_class_to_prio[ioprio_class];
  	dd->per_prio[prio].latest_pos[data_dir] = blk_rq_pos(rq);
  	dd->per_prio[prio].stats.dispatched++;
@@ -606,9 +615,11 @@ static struct request *dd_dispatch_request(struct blk_mq_hw_ctx *hctx)
  	enum dd_prio prio;

  	spin_lock(&dd->lock);
-	rq = dd_dispatch_prio_aged_requests(dd, now);
-	if (rq)
-		goto unlock;
+	if (dd->prio_aging_expire != 0) {
+		rq = dd_dispatch_prio_aged_requests(dd, now);
+		if (rq)
+			goto unlock;
+	}

  	/*
  	 * Next, dispatch requests in priority order. Ignore lower priority
@@ -749,7 +760,7 @@ static int dd_request_merge(struct request_queue *q, struct request **rq,
  			    struct bio *bio)
  {
  	struct deadline_data *dd = q->elevator->elevator_data;
-	const u8 ioprio_class = dd_bio_ioclass(bio);
+	const u8 ioprio_class = dd_bio_ioclass(dd, bio);
  	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
  	struct dd_per_prio *per_prio = &dd->per_prio[prio];
  	sector_t sector = bio_end_sector(bio);
@@ -814,7 +825,7 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
  	 */
  	blk_req_zone_write_unlock(rq);

-	prio = ioprio_class_to_prio[dd_rq_ioclass(rq)];
+	prio = ioprio_class_to_prio[dd_rq_ioclass(dd, rq)];
  	per_prio = &dd->per_prio[prio];
  	if (!rq->elv.priv[0]) {
  		per_prio->stats.inserted++;
@@ -923,7 +934,7 @@ static void dd_finish_request(struct request *rq)
  {
  	struct request_queue *q = rq->q;
  	struct deadline_data *dd = q->elevator->elevator_data;
-	const u8 ioprio_class = dd_rq_ioclass(rq);
+	const u8 ioprio_class = dd_rq_ioclass(dd, rq);
  	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
  	struct dd_per_prio *per_prio = &dd->per_prio[prio];

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Damien Le Moal]

On 12/8/23 09:03, Bart Van Assche wrote:
> On 12/5/23 16:42, Damien Le Moal wrote:
>> ... when prio_aging_expire is set to 0. Right ? Otherwise, the sentence above
>> reads as if you are disabling IO priority for zoned devices...
> 
> Hi Damien,
> 
> I just noticed that I posted an old version of this patch (3/3). In the patch
> below I/O priorities are ignored for zoned writes.
> 
> Thanks,
> 
> Bart.
> 
> 
> Subject: [PATCH] block/mq-deadline: Disable I/O prioritization in certain cases
> 
> Fix the following two issues:
> - Even with prio_aging_expire set to zero, I/O priorities still affect the
>    request order.
> - Assigning I/O priorities with the ioprio cgroup policy breaks zoned
>    storage support in the mq-deadline scheduler.
> 
> This patch fixes both issues by disabling I/O prioritization for these
> two cases.
> 
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   block/mq-deadline.c | 37 ++++++++++++++++++++++++-------------
>   1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/block/mq-deadline.c b/block/mq-deadline.c
> index fe5da2ade953..310ff133ce20 100644
> --- a/block/mq-deadline.c
> +++ b/block/mq-deadline.c
> @@ -119,18 +119,27 @@ deadline_rb_root(struct dd_per_prio *per_prio, struct request *rq)
>   	return &per_prio->sort_list[rq_data_dir(rq)];
>   }
> 
> +static bool dd_use_io_priority(struct deadline_data *dd, enum req_op op)
> +{
> +	return dd->prio_aging_expire != 0 && !op_needs_zoned_write_locking(op);
> +}

Hard NACK on this. The reason is that this will disable IO priority also for
sensible use cases that use libaio/io_uring with direct IOs, with an application
that does the right thing for writes, namely assigning the same priority for all
writes to a zone. There are some use cases like this in production.

I do understand that there is a problem when IO priorities come from cgroups and
the user go through a file system. But that should be handled by the file
system. That is, for f2fs, all writes going to the same zone should have the
same priority. Otherwise, priority inversion issues will lead to non sequential
write patterns.

Ideally, we should indeed have a generic solution for the cgroup case. But it
seems that for now, the simplest thing to do is to not allow priorities through
cgroups for writes to zoned devices, unless cgroups is made more intellignet
about it and manage bio priorities per zone to avoid priority inversion within a
zone.

> +
>   /*
>    * Returns the I/O priority class (IOPRIO_CLASS_*) that has been assigned to a
>    * request.
>    */
> -static u8 dd_rq_ioclass(struct request *rq)
> +static u8 dd_rq_ioclass(struct deadline_data *dd, struct request *rq)
>   {
> -	return IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
> +	if (dd_use_io_priority(dd, req_op(rq)))
> +		return IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
> +	return IOPRIO_CLASS_NONE;
>   }
> 
> -static u8 dd_bio_ioclass(struct bio *bio)
> +static u8 dd_bio_ioclass(struct deadline_data *dd, struct bio *bio)
>   {
> -	return IOPRIO_PRIO_CLASS(bio->bi_ioprio);
> +	if (dd_use_io_priority(dd, bio_op(bio)))
> +		return IOPRIO_PRIO_CLASS(bio->bi_ioprio);
> +	return IOPRIO_CLASS_NONE;
>   }
> 
>   /*
> @@ -233,7 +242,7 @@ static void dd_request_merged(struct request_queue *q, struct request *req,
>   			      enum elv_merge type)
>   {
>   	struct deadline_data *dd = q->elevator->elevator_data;
> -	const u8 ioprio_class = dd_rq_ioclass(req);
> +	const u8 ioprio_class = dd_rq_ioclass(dd, req);
>   	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
>   	struct dd_per_prio *per_prio = &dd->per_prio[prio];
> 
> @@ -253,7 +262,7 @@ static void dd_merged_requests(struct request_queue *q, struct request *req,
>   			       struct request *next)
>   {
>   	struct deadline_data *dd = q->elevator->elevator_data;
> -	const u8 ioprio_class = dd_rq_ioclass(next);
> +	const u8 ioprio_class = dd_rq_ioclass(dd, next);
>   	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
> 
>   	lockdep_assert_held(&dd->lock);
> @@ -550,7 +559,7 @@ static struct request *__dd_dispatch_request(struct deadline_data *dd,
>   	dd->batching++;
>   	deadline_move_request(dd, per_prio, rq);
>   done:
> -	ioprio_class = dd_rq_ioclass(rq);
> +	ioprio_class = dd_rq_ioclass(dd, rq);
>   	prio = ioprio_class_to_prio[ioprio_class];
>   	dd->per_prio[prio].latest_pos[data_dir] = blk_rq_pos(rq);
>   	dd->per_prio[prio].stats.dispatched++;
> @@ -606,9 +615,11 @@ static struct request *dd_dispatch_request(struct blk_mq_hw_ctx *hctx)
>   	enum dd_prio prio;
> 
>   	spin_lock(&dd->lock);
> -	rq = dd_dispatch_prio_aged_requests(dd, now);
> -	if (rq)
> -		goto unlock;
> +	if (dd->prio_aging_expire != 0) {
> +		rq = dd_dispatch_prio_aged_requests(dd, now);
> +		if (rq)
> +			goto unlock;
> +	}
> 
>   	/*
>   	 * Next, dispatch requests in priority order. Ignore lower priority
> @@ -749,7 +760,7 @@ static int dd_request_merge(struct request_queue *q, struct request **rq,
>   			    struct bio *bio)
>   {
>   	struct deadline_data *dd = q->elevator->elevator_data;
> -	const u8 ioprio_class = dd_bio_ioclass(bio);
> +	const u8 ioprio_class = dd_bio_ioclass(dd, bio);
>   	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
>   	struct dd_per_prio *per_prio = &dd->per_prio[prio];
>   	sector_t sector = bio_end_sector(bio);
> @@ -814,7 +825,7 @@ static void dd_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,
>   	 */
>   	blk_req_zone_write_unlock(rq);
> 
> -	prio = ioprio_class_to_prio[dd_rq_ioclass(rq)];
> +	prio = ioprio_class_to_prio[dd_rq_ioclass(dd, rq)];
>   	per_prio = &dd->per_prio[prio];
>   	if (!rq->elv.priv[0]) {
>   		per_prio->stats.inserted++;
> @@ -923,7 +934,7 @@ static void dd_finish_request(struct request *rq)
>   {
>   	struct request_queue *q = rq->q;
>   	struct deadline_data *dd = q->elevator->elevator_data;
> -	const u8 ioprio_class = dd_rq_ioclass(rq);
> +	const u8 ioprio_class = dd_rq_ioclass(dd, rq);
>   	const enum dd_prio prio = ioprio_class_to_prio[ioprio_class];
>   	struct dd_per_prio *per_prio = &dd->per_prio[prio];
> 
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Bart Van Assche]

On 12/7/23 17:37, Damien Le Moal wrote:
> On 12/8/23 09:03, Bart Van Assche wrote:
>> +static bool dd_use_io_priority(struct deadline_data *dd, enum req_op op)
>> +{
>> +	return dd->prio_aging_expire != 0 && !op_needs_zoned_write_locking(op);
>> +}
> 
> Hard NACK on this. The reason is that this will disable IO priority also for
> sensible use cases that use libaio/io_uring with direct IOs, with an application
> that does the right thing for writes, namely assigning the same priority for all
> writes to a zone. There are some use cases like this in production.
> 
> I do understand that there is a problem when IO priorities come from cgroups and
> the user go through a file system. But that should be handled by the file
> system. That is, for f2fs, all writes going to the same zone should have the
> same priority. Otherwise, priority inversion issues will lead to non sequential
> write patterns.
> 
> Ideally, we should indeed have a generic solution for the cgroup case. But it
> seems that for now, the simplest thing to do is to not allow priorities through
> cgroups for writes to zoned devices, unless cgroups is made more intellignet
> about it and manage bio priorities per zone to avoid priority inversion within a
> zone.

Hi Damien,

My understanding is that blkcg_set_ioprio() is called from inside submit_bio()
and hence that the reported issue cannot be solved by modifying F2FS. How about
modifying the blk-ioprio policy such that it ignores zoned writes?

Thanks,

Bart.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Damien Le Moal]

On 12/9/23 03:40, Bart Van Assche wrote:
> On 12/7/23 17:37, Damien Le Moal wrote:
>> On 12/8/23 09:03, Bart Van Assche wrote:
>>> +static bool dd_use_io_priority(struct deadline_data *dd, enum req_op op)
>>> +{
>>> +	return dd->prio_aging_expire != 0 && !op_needs_zoned_write_locking(op);
>>> +}
>>
>> Hard NACK on this. The reason is that this will disable IO priority also for
>> sensible use cases that use libaio/io_uring with direct IOs, with an application
>> that does the right thing for writes, namely assigning the same priority for all
>> writes to a zone. There are some use cases like this in production.
>>
>> I do understand that there is a problem when IO priorities come from cgroups and
>> the user go through a file system. But that should be handled by the file
>> system. That is, for f2fs, all writes going to the same zone should have the
>> same priority. Otherwise, priority inversion issues will lead to non sequential
>> write patterns.
>>
>> Ideally, we should indeed have a generic solution for the cgroup case. But it
>> seems that for now, the simplest thing to do is to not allow priorities through
>> cgroups for writes to zoned devices, unless cgroups is made more intellignet
>> about it and manage bio priorities per zone to avoid priority inversion within a
>> zone.
> 
> Hi Damien,
> 
> My understanding is that blkcg_set_ioprio() is called from inside submit_bio()
> and hence that the reported issue cannot be solved by modifying F2FS. How about
> modifying the blk-ioprio policy such that it ignores zoned writes?

I do not see a better solution than that at the moment. So yes, let's do that.
But please add a big comment in the code explaining why we ignore zoned writes.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Bart Van Assche]

On 12/10/23 23:40, Damien Le Moal wrote:
> On 12/9/23 03:40, Bart Van Assche wrote:
>> My understanding is that blkcg_set_ioprio() is called from inside submit_bio()
>> and hence that the reported issue cannot be solved by modifying F2FS. How about
>> modifying the blk-ioprio policy such that it ignores zoned writes?
> 
> I do not see a better solution than that at the moment. So yes, let's do that.
> But please add a big comment in the code explaining why we ignore zoned writes.

Hi Damien,

We tested a patch for the blk-ioprio cgroup policy that makes it skip zoned writes.
We noticed that such a patch is not sufficient to prevent unaligned write errors
because some tasks have been assigned an I/O priority via the ionice command
(ioprio_set() system call). I think it would be wrong to skip the assignment of an
I/O priority for zoned writes in all code that can set an I/O priority. Since the
root cause of this issue is the inability of the mq-deadline I/O scheduler to
preserve the order for zoned writes with different I/O priorities, I think this
issue should be fixed in the mq-deadline I/O scheduler.

Thanks,

Bart.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Damien Le Moal]

On 12/13/23 07:44, Bart Van Assche wrote:
> On 12/10/23 23:40, Damien Le Moal wrote:
>> On 12/9/23 03:40, Bart Van Assche wrote:
>>> My understanding is that blkcg_set_ioprio() is called from inside submit_bio()
>>> and hence that the reported issue cannot be solved by modifying F2FS. How about
>>> modifying the blk-ioprio policy such that it ignores zoned writes?
>>
>> I do not see a better solution than that at the moment. So yes, let's do that.
>> But please add a big comment in the code explaining why we ignore zoned writes.
> 
> Hi Damien,
> 
> We tested a patch for the blk-ioprio cgroup policy that makes it skip zoned writes.
> We noticed that such a patch is not sufficient to prevent unaligned write errors
> because some tasks have been assigned an I/O priority via the ionice command
> (ioprio_set() system call). I think it would be wrong to skip the assignment of an
> I/O priority for zoned writes in all code that can set an I/O priority. Since the
> root cause of this issue is the inability of the mq-deadline I/O scheduler to
> preserve the order for zoned writes with different I/O priorities, I think this
> issue should be fixed in the mq-deadline I/O scheduler.

Not necessarily. When the priority for an IO is set when a BIO is prepared, we
know where that priority come from:
1) The user kiocb through aio_reqprio
2) The process ionice context
3) priority cgroups

We can disable (2) and (3) and leave (1) as is.

Trying to solve this issue in mq-deadline would require keeping track of the io
priority used for a write request that is issued to a zone and use that same
priority for all following write requests for the same zone until there are no
writes pending for that zone. Otherwise, you will get the priority inversion
causing the reordering.

But I think that doing all this without also causing priority inversion for the
user, i.e. a high priority write request ends up waiting for a low priority one,
will be challenging, to say the least.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Bart Van Assche]

On 12/12/23 13:52, Damien Le Moal wrote:
> Trying to solve this issue in mq-deadline would require keeping track of the io
> priority used for a write request that is issued to a zone and use that same
> priority for all following write requests for the same zone until there are no
> writes pending for that zone. Otherwise, you will get the priority inversion
> causing the reordering.
> 
> But I think that doing all this without also causing priority inversion for the
> user, i.e. a high priority write request ends up waiting for a low priority one,
> will be challenging, to say the least.

Hi Damien,

How about the following algorithm?
- If a zoned write refers to the start of a zone or no other writes for
   the same zone occur in the RB-tree, use the I/O priority of the zoned
   write.
- If another write for the same zone occurs in the RB-tree, use the I/O
   priority from that other write.

While this algorithm does not guarantee that all zoned writes for a 
single zone have the same I/O priority, it guarantees that the 
mq-deadline I/O scheduler won't submit zoned writes in the wrong order 
because of their I/O priority.

Thanks,

Bart.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Damien Le Moal]

On 12/13/23 10:02, Bart Van Assche wrote:
> On 12/12/23 13:52, Damien Le Moal wrote:
>> Trying to solve this issue in mq-deadline would require keeping track of the io
>> priority used for a write request that is issued to a zone and use that same
>> priority for all following write requests for the same zone until there are no
>> writes pending for that zone. Otherwise, you will get the priority inversion
>> causing the reordering.
>>
>> But I think that doing all this without also causing priority inversion for the
>> user, i.e. a high priority write request ends up waiting for a low priority one,
>> will be challenging, to say the least.
> 
> Hi Damien,
> 
> How about the following algorithm?
> - If a zoned write refers to the start of a zone or no other writes for
>    the same zone occur in the RB-tree, use the I/O priority of the zoned
>    write.
> - If another write for the same zone occurs in the RB-tree, use the I/O
>    priority from that other write.
> 
> While this algorithm does not guarantee that all zoned writes for a 
> single zone have the same I/O priority, it guarantees that the 
> mq-deadline I/O scheduler won't submit zoned writes in the wrong order 
> because of their I/O priority.

I guess this should work.

> 
> Thanks,
> 
> Bart.
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Christoph Hellwig]

On Mon, Dec 04, 2023 at 09:32:13PM -0800, Bart Van Assche wrote:
> Fix the following two issues:
> - Even with prio_aging_expire set to zero, I/O priorities still affect the
>   request order.
> - Assigning I/O priorities with the ioprio cgroup policy breaks zoned
>   storage support in the mq-deadline scheduler.

Not it doesn't, how would it?  Or do you mean your f2fs hacks where you
assume there is some order kept?  You really need to get rid of them
and make sure f2fs doesn't care about reordering by writing the
metadata that records the data location only at I/O completion time.
Not only does that make zoned I/O trivially right, it also fixes the
stale data exposures you are almost guaranteed to have even on
conventional devices without that.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Bart Van Assche]

On 12/11/23 08:57, Christoph Hellwig wrote:
> Or do you mean your f2fs hacks where you assume there is some order
> kept?

Hi Christoph,

It seems like there is a misunderstanding. The issue addressed by this
patch series is not F2FS-specific. The issue addressed by this patch
series can be encountered by any software that submits REQ_OP_WRITE
operations.

Bart.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Christoph Hellwig]

On Mon, Dec 11, 2023 at 09:20:00AM -0800, Bart Van Assche wrote:
> It seems like there is a misunderstanding. The issue addressed by this
> patch series is not F2FS-specific. The issue addressed by this patch
> series can be encountered by any software that submits REQ_OP_WRITE
> operations.

How so?

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Damien Le Moal]

On 12/12/23 01:57, Christoph Hellwig wrote:
> On Mon, Dec 04, 2023 at 09:32:13PM -0800, Bart Van Assche wrote:
>> Fix the following two issues:
>> - Even with prio_aging_expire set to zero, I/O priorities still affect the
>>   request order.
>> - Assigning I/O priorities with the ioprio cgroup policy breaks zoned
>>   storage support in the mq-deadline scheduler.
> 
> Not it doesn't, how would it?  Or do you mean your f2fs hacks where you
> assume there is some order kept?  You really need to get rid of them
> and make sure f2fs doesn't care about reordering by writing the
> metadata that records the data location only at I/O completion time.
> Not only does that make zoned I/O trivially right, it also fixes the
> stale data exposures you are almost guaranteed to have even on
> conventional devices without that.

Priority CGroups can mess things up I think. If you have 2 processes belonging
to different CGs with different priorities and:
1) The processes do raw block device accesses and write to the same zone,
synchronized to get the WP correctly
2) The processes are writing different files and the FS decides to place the
block for the files in the same zone

Case (1) is clearly "the user is doing very stupid things" and for that case,
the user definitely deserve seeing his writes failing. But case (2) is perfectly
legit I think. That is the one that needs to be addressed. The choices I see
are: every file system supporting zone writes need to be priority CG aware when
writing files, or we ignore priority CG when writing.

The latter is I think better than the former as CGs can change without the FS
being aware (as far as I know), and such support would need to be implemented
for all FSes that support zone writing using regular writes (f2fs and zonefs).

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Christoph Hellwig]

On Tue, Dec 12, 2023 at 07:40:02AM +0900, Damien Le Moal wrote:
> The latter is I think better than the former as CGs can change without the FS
> being aware (as far as I know), and such support would need to be implemented
> for all FSes that support zone writing using regular writes (f2fs and zonefs).

How is cse 2 any kind of problem when using the proper append model?
Except when blk-zoned delays it so much that we really need to close
the zone becaue it's timing out, but that would really be configuration
issue.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Bart Van Assche]

On 12/12/23 07:41, Christoph Hellwig wrote:
> On Tue, Dec 12, 2023 at 07:40:02AM +0900, Damien Le Moal wrote:
>> The latter is I think better than the former as CGs can change without the FS
>> being aware (as far as I know), and such support would need to be implemented
>> for all FSes that support zone writing using regular writes (f2fs and zonefs).
> 
> How is cse 2 any kind of problem when using the proper append model?
> Except when blk-zoned delays it so much that we really need to close
> the zone becaue it's timing out, but that would really be configuration
> issue.

As mentioned before, UFS devices implement the SCSI command set and hence do not
support write append operations. If anyone else standardizes a write append
command for SCSI we can ask UFS vendors to implement that command. However, as
far as I know nobody in T10 is working on standardizing a write append command.

Bart.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Christoph Hellwig]

On Tue, Dec 12, 2023 at 09:15:48AM -0800, Bart Van Assche wrote:
> As mentioned before, UFS devices implement the SCSI command set and hence do not
> support write append operations. If anyone else standardizes a write append
> command for SCSI we can ask UFS vendors to implement that command. However, as
> far as I know nobody in T10 is working on standardizing a write append command.

The actual hardware support does not matter, it's the programming model.
Even with the zone append emulation in sd you don't need to care about
reordering due to I/O priorities.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Bart Van Assche]

On 12/12/23 09:18, Christoph Hellwig wrote:
> On Tue, Dec 12, 2023 at 09:15:48AM -0800, Bart Van Assche wrote:
>> As mentioned before, UFS devices implement the SCSI command set and hence do not
>> support write append operations. If anyone else standardizes a write append
>> command for SCSI we can ask UFS vendors to implement that command. However, as
>> far as I know nobody in T10 is working on standardizing a write append command.
> 
> The actual hardware support does not matter, it's the programming model.
> Even with the zone append emulation in sd you don't need to care about
> reordering due to I/O priorities.

The actual hardware support does matter. Using the zone append emulation from
drivers/scsi/sd_zbc.c (or any other zone append emulation) is not an option for
high performance devices. This is because emulating zone append can only be done
by serializing write operations. Any such serialization negatively affects
performance.

Thanks,

Bart.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Christoph Hellwig]

On Tue, Dec 12, 2023 at 09:42:24AM -0800, Bart Van Assche wrote:
> drivers/scsi/sd_zbc.c (or any other zone append emulation) is not an option for
> high performance devices. This is because emulating zone append can only be done
> by serializing write operations. Any such serialization negatively affects
> performance.

Seriously Bart, we've been through this a few times.  You keep insisting
on using a broken model despite having a major influence on the standards.
Go and fix them, and I bet you'll actually have plenty of support.  And
stop pushing hacks into the block layer and SCSI code to work around your
broken file system code and lack of interest in actually making the
hardware interface sane.

This isn't going to get you anywhere, while you're wasting a lot of
peoples time.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Bart Van Assche]

On 12/12/23 09:48, Christoph Hellwig wrote:
> You keep insisting
> on using a broken model despite having a major influence on the standards.
> Go and fix them, and I bet you'll actually have plenty of support.

Hi Christoph,

I do *not* appreciate what you wrote. You shouldn't tell me what I should do
with regard to standardization. If you want standards to be changed, please
change these yourself.

Bart.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Christoph Hellwig]

On Tue, Dec 12, 2023 at 10:09:49AM -0800, Bart Van Assche wrote:
>
> On 12/12/23 09:48, Christoph Hellwig wrote:
>> You keep insisting
>> on using a broken model despite having a major influence on the standards.
>> Go and fix them, and I bet you'll actually have plenty of support.
>
> Hi Christoph,
>
> I do *not* appreciate what you wrote. You shouldn't tell me what I should do
> with regard to standardization. If you want standards to be changed, please
> change these yourself.

Bart,

I don't appreciate all the work you create by insisting on a fundamentally
broken model either.  If you want zoned devices to work you need something
like zone append, and your insistence on using broken methods is not
helpful.  So if you don't want to change the standard to actually work
for your use case at least don't waste *our* time trying to work around
it badly.


>
> Bart.
---end quoted text---

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Bart Van Assche]

On 12/12/23 10:13, Christoph Hellwig wrote:
> I don't appreciate all the work you create by insisting on a fundamentally
> broken model either.  If you want zoned devices to work you need something
> like zone append, and your insistence on using broken methods is not
> helpful.  So if you don't want to change the standard to actually work
> for your use case at least don't waste *our* time trying to work around
> it badly.

Hi Christoph,

"Fundamentally broken model" is your personal opinion. I don't know anyone
else than you who considers zoned writes as a broken model.

Bart.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Christoph Hellwig]

On Tue, Dec 12, 2023 at 10:19:31AM -0800, Bart Van Assche wrote:
> "Fundamentally broken model" is your personal opinion. I don't know anyone
> else than you who considers zoned writes as a broken model.

No Bart, it is not.  Talk to Damien, talk to Martin, to Jens.  Or just
look at all the patches you're sending to the list that play a never
ending hac-a-mole trying to bandaid over reordering that should be
perfectly fine.  You're playing a long term losing game by trying to
prevent reordering that you can't win.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Jaegeuk Kim]

On 12/12, Christoph Hellwig wrote:
> On Tue, Dec 12, 2023 at 10:19:31AM -0800, Bart Van Assche wrote:
> > "Fundamentally broken model" is your personal opinion. I don't know anyone
> > else than you who considers zoned writes as a broken model.
> 
> No Bart, it is not.  Talk to Damien, talk to Martin, to Jens.  Or just
> look at all the patches you're sending to the list that play a never
> ending hac-a-mole trying to bandaid over reordering that should be
> perfectly fine.  You're playing a long term losing game by trying to
> prevent reordering that you can't win.

As one of users of zoned devices, I disagree this is a broken model, but even
better than the zone append model. When considering the filesystem performance,
it is essential to place the data per file to get better bandwidth. And for
NAND-based storage, filesystem is the right place to deal with the more efficient
garbage collecion based on the known data locations. That's why all the flash
storage vendors adopted it in the JEDEC. Agreed that zone append is nice, but
IMO, it's not practical for production.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Damien Le Moal]

On 12/13/23 04:03, Jaegeuk Kim wrote:
> On 12/12, Christoph Hellwig wrote:
>> On Tue, Dec 12, 2023 at 10:19:31AM -0800, Bart Van Assche wrote:
>>> "Fundamentally broken model" is your personal opinion. I don't know anyone
>>> else than you who considers zoned writes as a broken model.
>>
>> No Bart, it is not.  Talk to Damien, talk to Martin, to Jens.  Or just
>> look at all the patches you're sending to the list that play a never
>> ending hac-a-mole trying to bandaid over reordering that should be
>> perfectly fine.  You're playing a long term losing game by trying to
>> prevent reordering that you can't win.
> 
> As one of users of zoned devices, I disagree this is a broken model, but even
> better than the zone append model. When considering the filesystem performance,
> it is essential to place the data per file to get better bandwidth. And for
> NAND-based storage, filesystem is the right place to deal with the more efficient
> garbage collecion based on the known data locations. That's why all the flash
> storage vendors adopted it in the JEDEC. Agreed that zone append is nice, but
> IMO, it's not practical for production.

The work on btrfs is a counter argument to this statement. The initial zone
support based on regular writes was going nowhere as trying to maintain ordering
was too complex and/or too invasive. Using zone append for the data path solved
and simplified many things.

I do think that zone append has a narrower use case spectrum for applications
relying on the raw block device directly. But for file systems, it definitely is
an easier to use writing model for zoned storage.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Jaegeuk Kim]

On 12/13, Damien Le Moal wrote:
> On 12/13/23 04:03, Jaegeuk Kim wrote:
> > On 12/12, Christoph Hellwig wrote:
> >> On Tue, Dec 12, 2023 at 10:19:31AM -0800, Bart Van Assche wrote:
> >>> "Fundamentally broken model" is your personal opinion. I don't know anyone
> >>> else than you who considers zoned writes as a broken model.
> >>
> >> No Bart, it is not.  Talk to Damien, talk to Martin, to Jens.  Or just
> >> look at all the patches you're sending to the list that play a never
> >> ending hac-a-mole trying to bandaid over reordering that should be
> >> perfectly fine.  You're playing a long term losing game by trying to
> >> prevent reordering that you can't win.
> > 
> > As one of users of zoned devices, I disagree this is a broken model, but even
> > better than the zone append model. When considering the filesystem performance,
> > it is essential to place the data per file to get better bandwidth. And for
> > NAND-based storage, filesystem is the right place to deal with the more efficient
> > garbage collecion based on the known data locations. That's why all the flash
> > storage vendors adopted it in the JEDEC. Agreed that zone append is nice, but
> > IMO, it's not practical for production.
> 
> The work on btrfs is a counter argument to this statement. The initial zone
> support based on regular writes was going nowhere as trying to maintain ordering
> was too complex and/or too invasive. Using zone append for the data path solved
> and simplified many things.

We're in supporting zoned writes, and we don't see huge problem of reordering
issues like you mention. I do agree there're pros and cons between the two, but
I believe using which one depends on user behaviors. If there's a user, why it
should be blocked? IOWs, why not just trying to support both?

> 
> I do think that zone append has a narrower use case spectrum for applications
> relying on the raw block device directly. But for file systems, it definitely is
> an easier to use writing model for zoned storage.
> 
> -- 
> Damien Le Moal
> Western Digital Research

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Damien Le Moal]

On 12/14/23 01:49, Jaegeuk Kim wrote:
> On 12/13, Damien Le Moal wrote:
>> On 12/13/23 04:03, Jaegeuk Kim wrote:
>>> On 12/12, Christoph Hellwig wrote:
>>>> On Tue, Dec 12, 2023 at 10:19:31AM -0800, Bart Van Assche wrote:
>>>>> "Fundamentally broken model" is your personal opinion. I don't know anyone
>>>>> else than you who considers zoned writes as a broken model.
>>>>
>>>> No Bart, it is not.  Talk to Damien, talk to Martin, to Jens.  Or just
>>>> look at all the patches you're sending to the list that play a never
>>>> ending hac-a-mole trying to bandaid over reordering that should be
>>>> perfectly fine.  You're playing a long term losing game by trying to
>>>> prevent reordering that you can't win.
>>>
>>> As one of users of zoned devices, I disagree this is a broken model, but even
>>> better than the zone append model. When considering the filesystem performance,
>>> it is essential to place the data per file to get better bandwidth. And for
>>> NAND-based storage, filesystem is the right place to deal with the more efficient
>>> garbage collecion based on the known data locations. That's why all the flash
>>> storage vendors adopted it in the JEDEC. Agreed that zone append is nice, but
>>> IMO, it's not practical for production.
>>
>> The work on btrfs is a counter argument to this statement. The initial zone
>> support based on regular writes was going nowhere as trying to maintain ordering
>> was too complex and/or too invasive. Using zone append for the data path solved
>> and simplified many things.
> 
> We're in supporting zoned writes, and we don't see huge problem of reordering
> issues like you mention. I do agree there're pros and cons between the two, but
> I believe using which one depends on user behaviors. If there's a user, why it
> should be blocked? IOWs, why not just trying to support both?

We do support both... But:
1) regular writes to zones is a user (= application) facing API. An application
using a block device directly without an FS can directly drive the issuing of
sequential writes to a zone. If there is an FS between the application and the
device, the FS decides what to do (regular writes or zone append, and to which zone)
2) Zone append cannot be directly issued by applications to block devices. I am
working on restoring zone append writes in zonefs as an alternative to this
limitation.

Now, in the context of IO priorities, issuing sequential writes to the same zone
with different priorities really is a silly thing to do. Even if done in the
proper order, that would essentially mean that whoever does that (FS or
application) is creating priority inversion issues for himself and thus negating
any benefit one can achieve with IO priorities (that is, most of the time,
lowering tail latency for a class of IOs).

As I mentioned before, for applications that use the zoned block device
directly, I think we should just leave things as is, that is, let the writes
fail if they are reordered due to a nonsensical IO priority setup. That is a
nice way to warn the user that he/she is doing something silly.

For the FS case, it is a little more difficult given that the user may have a
sensible IO priority setup, e.g. assigning different IO priorities (cgroups,
ioprio_set or ionice) to different processes accessing different files. For that
case, if the FS decides to issue writes to these files to the same zone, then
the problem occur. But back to the previous point: this is a silly thing to do
when writes have to be sequential. That is priority inversion right there.

The difficulty for an FS is, I think, that the FS cannot easily know the IO
priority until the BIO for the write is issued... So that is the problem that
needs fixing.

Bart's proposed fix will, I think, address your issue. However, it will also
hide IO priority setup problems to users accessing the block device directly.
That I do not like. As I stated above, I think it is better to let writes fail
in that case to signal the priority inversion. There are *a lot* of IO priority
SMR HDD users out there. Literally millions of drives running with that, and not
just for read operations. So please understand my concerns.

A better solution may be to introduce a BIO flags that says "ignore IO
priorities". f2fs can use that to avoid reordering writes to the same zone due
to different IO priorities (again, *that* is the issue to fix in the first place
I think, because that is simply silly to do, even with a regular HDD or SSD
since that will break sequential write streams and thus impact performace,
increase device-level GC/WAF etc).

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Christoph Hellwig]

On Tue, Dec 12, 2023 at 11:03:06AM -0800, Jaegeuk Kim wrote:
> As one of users of zoned devices, I disagree this is a broken model,

So you think that chasing potential for reordering all over the I/O
stack in perpetualality, including obscure error handling paths and
disabling features intentended to throttle and delay I/O (like
ioprio and cgroups) is not a broken model?

> it is essential to place the data per file to get better bandwidth. And for
> NAND-based storage, filesystem is the right place to deal with the more efficient
> garbage collecion based on the known data locations.

And that works perfectly fine match for zone append.

> That's why all the flash
> storage vendors adopted it in the JEDEC.

Everyone sucking up to Google to place their product in Android, yes..


> Agreed that zone append is nice, but
> IMO, it's not practical for production.

You've delivered exactly zero arguments for that.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Jaegeuk Kim]

On 12/13, Christoph Hellwig wrote:
> On Tue, Dec 12, 2023 at 11:03:06AM -0800, Jaegeuk Kim wrote:
> > As one of users of zoned devices, I disagree this is a broken model,
> 
> So you think that chasing potential for reordering all over the I/O
> stack in perpetualality, including obscure error handling paths and
> disabling features intentended to throttle and delay I/O (like
> ioprio and cgroups) is not a broken model?

As of now, we don't see any reordering issue except this. I don't have any
concern to keep the same ioprio on writes, since handheld devices are mostly
sensitive to reads. So, if you have other use-cases using zoned writes which
require different ioprio on writes, I think you can suggest a knob to control
it by users.

> 
> > it is essential to place the data per file to get better bandwidth. And for
> > NAND-based storage, filesystem is the right place to deal with the more efficient
> > garbage collecion based on the known data locations.
> 
> And that works perfectly fine match for zone append.

How that works, if the device gives random LBAs back to the adjacent data in
a file? And, how to make the LBAs into the sequential ones back?

> 
> > That's why all the flash
> > storage vendors adopted it in the JEDEC.
> 
> Everyone sucking up to Google to place their product in Android, yes..

Sorry, I needed to stop reading here, as you're totally biased. This is not
the case in JEDEC, as Bart spent multiple years to synchronize the technical
benefitcs that we've seen across UFS vendors as well as OEMs.

> 
> 
> > Agreed that zone append is nice, but
> > IMO, it's not practical for production.
> 
> You've delivered exactly zero arguments for that.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Christoph Hellwig]

On Wed, Dec 13, 2023 at 08:41:32AM -0800, Jaegeuk Kim wrote:
> I don't have any
> concern to keep the same ioprio on writes, since handheld devices are mostly
> sensitive to reads. So, if you have other use-cases using zoned writes which
> require different ioprio on writes, I think you can suggest a knob to control
> it by users.

Get out of your little handheld world.  In Linux we need a generally usable
I/O stack, and any feature exposed by the kernel and will be used quite
differently than you imagine.

Just like people will add reordering to the I/O stack that's not there
right now (in addition to the ones your testing doesn't hit).  That
doensn't mean we should avoid them - you genereally get better performance
by not reordering without a good reason (like thotting), but especially
in error handling paths or resource constrained environment they will
hapen all over.  We've had this whole discussion with the I/O barriers
that did not work for exactly the same reasons.

> 
> > 
> > > it is essential to place the data per file to get better bandwidth. And for
> > > NAND-based storage, filesystem is the right place to deal with the more efficient
> > > garbage collecion based on the known data locations.
> > 
> > And that works perfectly fine match for zone append.
> 
> How that works, if the device gives random LBAs back to the adjacent data in
> a file? And, how to make the LBAs into the sequential ones back?

Why would your device pick random LBAs?  If you send a zone append to
zone it will be written at the write pointer, which is absolutely not
random.  All I/O written in a single write is going to be sequential,
so just like for all other devices doing large sequential writes is
important.  Multiple writes can get reordered, but if you havily hit
the same zone you'd get the same effect in the file system allocator
too.

> Sorry, I needed to stop reading here, as you're totally biased. This is not
> the case in JEDEC, as Bart spent multiple years to synchronize the technical
> benefitcs that we've seen across UFS vendors as well as OEMs.

*lol*  There is no more fucked up corporate pressure standard committee
than the storage standards in JEDEC.  That's why not one actually takes
them seriously.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Jaegeuk Kim]

On 12/14, Christoph Hellwig wrote:
> On Wed, Dec 13, 2023 at 08:41:32AM -0800, Jaegeuk Kim wrote:
> > I don't have any
> > concern to keep the same ioprio on writes, since handheld devices are mostly
> > sensitive to reads. So, if you have other use-cases using zoned writes which
> > require different ioprio on writes, I think you can suggest a knob to control
> > it by users.
> 
> Get out of your little handheld world.  In Linux we need a generally usable
> I/O stack, and any feature exposed by the kernel and will be used quite
> differently than you imagine.
> 
> Just like people will add reordering to the I/O stack that's not there
> right now (in addition to the ones your testing doesn't hit).  That
> doensn't mean we should avoid them - you genereally get better performance
> by not reordering without a good reason (like thotting), but especially
> in error handling paths or resource constrained environment they will
> hapen all over.  We've had this whole discussion with the I/O barriers
> that did not work for exactly the same reasons.
> 
> > 
> > > 
> > > > it is essential to place the data per file to get better bandwidth. And for
> > > > NAND-based storage, filesystem is the right place to deal with the more efficient
> > > > garbage collecion based on the known data locations.
> > > 
> > > And that works perfectly fine match for zone append.
> > 
> > How that works, if the device gives random LBAs back to the adjacent data in
> > a file? And, how to make the LBAs into the sequential ones back?
> 
> Why would your device pick random LBAs?  If you send a zone append to
> zone it will be written at the write pointer, which is absolutely not
> random.  All I/O written in a single write is going to be sequential,
> so just like for all other devices doing large sequential writes is
> important.  Multiple writes can get reordered, but if you havily hit
> the same zone you'd get the same effect in the file system allocator
> too.

How can you guarantee the device does not give any random LBAs? What'd
be the selling point of zone append to end users? Are you sure this can
give the better write trhought forever? Have you considered how to
implement this in device side such as FTL mapping overhead and garbage
collection leading to tail latencies?

My takeaway on the two approaches would be:
                  zone_append        zone_write
		  -----------        ----------
LBA               from FTL           from filesystem
FTL mapping       Page-map           Zone-map
SRAM/DRAM needs   Large              Small
FTL GC            Required           Not required
Tail latencies    Exist              Not exisit
GC Efficience     Worse              Better
Longevity         As-is              Longer
Discard cmd       Required           Not required
Block complexity  Small              Large
Failure cases     Less exist         Exist
Fsck              Don't know         F2FS-TOOLS support
Filesystem        BTRFS support(?)   F2FS support

Given this, I took zone_write, especially for mobile devices, since we can
recover the unaligned writes in the corner cases by fsck. And, most benefit
would be getting rid of FTL mapping overhead which improves random read IOPs
significantly due to the lack of SRAM in low-end storages. And, longer lifetime
by mitigating garbage collection overhead is more important in mobile world.

If there's any flag or knob that we can set, IMO, that'd be enough.

> 
> > Sorry, I needed to stop reading here, as you're totally biased. This is not
> > the case in JEDEC, as Bart spent multiple years to synchronize the technical
> > benefitcs that we've seen across UFS vendors as well as OEMs.
> 
> *lol*  There is no more fucked up corporate pressure standard committee
> than the storage standards in JEDEC.  That's why not one actually takes
> them seriously.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Damien Le Moal]

On 12/15/23 02:22, Jaegeuk Kim wrote:
> On 12/14, Christoph Hellwig wrote:
>> On Wed, Dec 13, 2023 at 08:41:32AM -0800, Jaegeuk Kim wrote:
>>> I don't have any
>>> concern to keep the same ioprio on writes, since handheld devices are mostly
>>> sensitive to reads. So, if you have other use-cases using zoned writes which
>>> require different ioprio on writes, I think you can suggest a knob to control
>>> it by users.
>>
>> Get out of your little handheld world.  In Linux we need a generally usable
>> I/O stack, and any feature exposed by the kernel and will be used quite
>> differently than you imagine.
>>
>> Just like people will add reordering to the I/O stack that's not there
>> right now (in addition to the ones your testing doesn't hit).  That
>> doensn't mean we should avoid them - you genereally get better performance
>> by not reordering without a good reason (like thotting), but especially
>> in error handling paths or resource constrained environment they will
>> hapen all over.  We've had this whole discussion with the I/O barriers
>> that did not work for exactly the same reasons.
>>
>>>
>>>>
>>>>> it is essential to place the data per file to get better bandwidth. And for
>>>>> NAND-based storage, filesystem is the right place to deal with the more efficient
>>>>> garbage collecion based on the known data locations.
>>>>
>>>> And that works perfectly fine match for zone append.
>>>
>>> How that works, if the device gives random LBAs back to the adjacent data in
>>> a file? And, how to make the LBAs into the sequential ones back?
>>
>> Why would your device pick random LBAs?  If you send a zone append to
>> zone it will be written at the write pointer, which is absolutely not
>> random.  All I/O written in a single write is going to be sequential,
>> so just like for all other devices doing large sequential writes is
>> important.  Multiple writes can get reordered, but if you havily hit
>> the same zone you'd get the same effect in the file system allocator
>> too.
> 
> How can you guarantee the device does not give any random LBAs? What'd> be the selling point of zone append to end users? Are you sure this can
> give the better write trhought forever? Have you considered how to
> implement this in device side such as FTL mapping overhead and garbage
> collection leading to tail latencies?

Answers to all your questions, in order:

1) Asking this is to me similar to asking how can one guarantee that the device
gives back the data that was written... You are asking for guarantees that the
device is not buggy. By definition, zone append will return the writen start LBA
within the zone that the zone append command specified. And that start LBA will
always be equal to the zone write pointer value when the device started
executing the zone append command.

2) When there is an FS, the user cannot know if the FS is using zone append or
not, so the user should not care at all. If by "user" you mean "the file
system", then it is a design decision. We already pointed out that generally
speaking, zone append will be easier to use because it does not have ordering
constraints.

3) Yes, because the writes are always sequential, which is the least expensive
pattern for the device internal as that only triggers minimal internal activity
on the FTL, GC, weir leveling etc, at least for a decently designed device.

4) See above. If the device interface forces the device user to always write
sequentially, as mandated by a zoned device, then FTL, GC and weir leveling is
minimized. The actual device internal GC that may or may not happen completely
depend on how the device maps zones to flash super blocks. If the mapping is
1:1, then GC will be nearly non-existent. If the mapping is not 1:1, then GC
overhead may exist. The discussion should then be about the design choices of
the device. The fact that the host chose zone append will not in anyway make
things worse for the device. Even with regular writes the host must write
sequentially, same as what zone append achieves (potentially a lot more easily).

> My takeaway on the two approaches would be:
>                   zone_append        zone_write
> 		  -----------        ----------
> LBA               from FTL           from filesystem
> FTL mapping       Page-map           Zone-map

Not sure what you mean here. zone append always returns an LBA from within the
zone specified by the LBA in the command CDB. So mapping is still per zone. zone
append is *NOT* a random write command. Zone append automatically implements
sequential writing within a zone for the user. In the case of regular writes,
the user must fully control sentimentality. In both cases the write pattern *is*
sequential.

> SRAM/DRAM needs   Large              Small

There are no differences in this area because the FTL is the same for both. No
changes, nothing special for zone append.

> FTL GC            Required           Not required

Incorrect. See above. That depends on the device mapping of zones to flash
superblocks. And GC requirements are the same for both because the write pattern
is identical: it is sequential within each zone being written. The user still
controls which zone it wants to write. Zone append is not a magic command that
chooses a target zone automatically.

> Tail latencies    Exist              Not exisit

Incorrect. They are the same and because of the lack of ordering requirement
with zone append, if anything, zone append can give better latency.

> GC Efficience     Worse              Better

Nope. See above. Same.

> Longevity         As-is              Longer
> Discard cmd       Required           Not required

There is no discard with zone devices. Only zone reset. So both are "not
required" here.

> Block complexity  Small              Large
> Failure cases     Less exist         Exist
> Fsck              Don't know         F2FS-TOOLS support
> Filesystem        BTRFS support(?)   F2FS support

Yes, btrfs data path uses zone append.

> 
> Given this, I took zone_write, especially for mobile devices, since we can
> recover the unaligned writes in the corner cases by fsck. And, most benefit
> would be getting rid of FTL mapping overhead which improves random read IOPs
> significantly due to the lack of SRAM in low-end storages. And, longer lifetime
> by mitigating garbage collection overhead is more important in mobile world.

As mentioned, GC is not an issue, or rather, GC depends on how the device is
designed, not on which type of write command the host uses. Writes are always
sequential for both types !


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Jaegeuk Kim]

On 12/15, Damien Le Moal wrote:
> On 12/15/23 02:22, Jaegeuk Kim wrote:
> > On 12/14, Christoph Hellwig wrote:
> >> On Wed, Dec 13, 2023 at 08:41:32AM -0800, Jaegeuk Kim wrote:
> >>> I don't have any
> >>> concern to keep the same ioprio on writes, since handheld devices are mostly
> >>> sensitive to reads. So, if you have other use-cases using zoned writes which
> >>> require different ioprio on writes, I think you can suggest a knob to control
> >>> it by users.
> >>
> >> Get out of your little handheld world.  In Linux we need a generally usable
> >> I/O stack, and any feature exposed by the kernel and will be used quite
> >> differently than you imagine.
> >>
> >> Just like people will add reordering to the I/O stack that's not there
> >> right now (in addition to the ones your testing doesn't hit).  That
> >> doensn't mean we should avoid them - you genereally get better performance
> >> by not reordering without a good reason (like thotting), but especially
> >> in error handling paths or resource constrained environment they will
> >> hapen all over.  We've had this whole discussion with the I/O barriers
> >> that did not work for exactly the same reasons.
> >>
> >>>
> >>>>
> >>>>> it is essential to place the data per file to get better bandwidth. And for
> >>>>> NAND-based storage, filesystem is the right place to deal with the more efficient
> >>>>> garbage collecion based on the known data locations.
> >>>>
> >>>> And that works perfectly fine match for zone append.
> >>>
> >>> How that works, if the device gives random LBAs back to the adjacent data in
> >>> a file? And, how to make the LBAs into the sequential ones back?
> >>
> >> Why would your device pick random LBAs?  If you send a zone append to
> >> zone it will be written at the write pointer, which is absolutely not
> >> random.  All I/O written in a single write is going to be sequential,
> >> so just like for all other devices doing large sequential writes is
> >> important.  Multiple writes can get reordered, but if you havily hit
> >> the same zone you'd get the same effect in the file system allocator
> >> too.
> > 
> > How can you guarantee the device does not give any random LBAs? What'd> be the selling point of zone append to end users? Are you sure this can
> > give the better write trhought forever? Have you considered how to
> > implement this in device side such as FTL mapping overhead and garbage
> > collection leading to tail latencies?
> 
> Answers to all your questions, in order:
> 
> 1) Asking this is to me similar to asking how can one guarantee that the device
> gives back the data that was written... You are asking for guarantees that the
> device is not buggy. By definition, zone append will return the writen start LBA
> within the zone that the zone append command specified. And that start LBA will
> always be equal to the zone write pointer value when the device started
> executing the zone append command.
> 
> 2) When there is an FS, the user cannot know if the FS is using zone append or
> not, so the user should not care at all. If by "user" you mean "the file
> system", then it is a design decision. We already pointed out that generally
> speaking, zone append will be easier to use because it does not have ordering
> constraints.
> 
> 3) Yes, because the writes are always sequential, which is the least expensive
> pattern for the device internal as that only triggers minimal internal activity
> on the FTL, GC, weir leveling etc, at least for a decently designed device.
> 
> 4) See above. If the device interface forces the device user to always write
> sequentially, as mandated by a zoned device, then FTL, GC and weir leveling is
> minimized. The actual device internal GC that may or may not happen completely
> depend on how the device maps zones to flash super blocks. If the mapping is
> 1:1, then GC will be nearly non-existent. If the mapping is not 1:1, then GC
> overhead may exist. The discussion should then be about the design choices of
> the device. The fact that the host chose zone append will not in anyway make
> things worse for the device. Even with regular writes the host must write
> sequentially, same as what zone append achieves (potentially a lot more easily).
> 
> > My takeaway on the two approaches would be:
> >                   zone_append        zone_write
> > 		  -----------        ----------
> > LBA               from FTL           from filesystem
> > FTL mapping       Page-map           Zone-map
> 
> Not sure what you mean here. zone append always returns an LBA from within the
> zone specified by the LBA in the command CDB. So mapping is still per zone. zone
> append is *NOT* a random write command. Zone append automatically implements
> sequential writing within a zone for the user. In the case of regular writes,
> the user must fully control sentimentality. In both cases the write pattern *is*
> sequential.

Okay, it seems there's first disconnect here, which fails to explain all the
below gaps. Do you think the device supporting zone_append keeps LBAs inline
with PBAs within a zone? E.g., LBA#n guarantees to map to PBA#n in a zone.
If LBA order is exactly matching to the PBA order all the time, the mapping
granularity is zone. Otherwise, it should be page.

> 
> > SRAM/DRAM needs   Large              Small
> 
> There are no differences in this area because the FTL is the same for both. No
> changes, nothing special for zone append.
> 
> > FTL GC            Required           Not required
> 
> Incorrect. See above. That depends on the device mapping of zones to flash
> superblocks. And GC requirements are the same for both because the write pattern
> is identical: it is sequential within each zone being written. The user still
> controls which zone it wants to write. Zone append is not a magic command that
> chooses a target zone automatically.
> 
> > Tail latencies    Exist              Not exisit
> 
> Incorrect. They are the same and because of the lack of ordering requirement
> with zone append, if anything, zone append can give better latency.
> 
> > GC Efficience     Worse              Better
> 
> Nope. See above. Same.
> 
> > Longevity         As-is              Longer
> > Discard cmd       Required           Not required
> 
> There is no discard with zone devices. Only zone reset. So both are "not
> required" here.
> 
> > Block complexity  Small              Large
> > Failure cases     Less exist         Exist
> > Fsck              Don't know         F2FS-TOOLS support
> > Filesystem        BTRFS support(?)   F2FS support
> 
> Yes, btrfs data path uses zone append.
> 
> > 
> > Given this, I took zone_write, especially for mobile devices, since we can
> > recover the unaligned writes in the corner cases by fsck. And, most benefit
> > would be getting rid of FTL mapping overhead which improves random read IOPs
> > significantly due to the lack of SRAM in low-end storages. And, longer lifetime
> > by mitigating garbage collection overhead is more important in mobile world.
> 
> As mentioned, GC is not an issue, or rather, GC depends on how the device is
> designed, not on which type of write command the host uses. Writes are always
> sequential for both types !
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Keith Busch]

On Thu, Dec 14, 2023 at 06:03:11PM -0800, Jaegeuk Kim wrote:
> 
> Okay, it seems there's first disconnect here, which fails to explain all the
> below gaps. Do you think the device supporting zone_append keeps LBAs inline
> with PBAs within a zone? E.g., LBA#n guarantees to map to PBA#n in a zone.
> If LBA order is exactly matching to the PBA order all the time, the mapping
> granularity is zone. Otherwise, it should be page.

Yah, you're describing how 'zone append' works.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Christoph Hellwig]

On Thu, Dec 14, 2023 at 06:20:16PM -0800, Keith Busch wrote:
> On Thu, Dec 14, 2023 at 06:03:11PM -0800, Jaegeuk Kim wrote:
> > 
> > Okay, it seems there's first disconnect here, which fails to explain all the
> > below gaps. Do you think the device supporting zone_append keeps LBAs inline
> > with PBAs within a zone? E.g., LBA#n guarantees to map to PBA#n in a zone.
> > If LBA order is exactly matching to the PBA order all the time, the mapping
> > granularity is zone. Otherwise, it should be page.
> 
> Yah, you're describing how 'zone append' works.

Haha.  I'm glad I read through all the answers as Damien and you already
did the explaining.

Note that in a complex SSD of course there can still be some remapping
due to wear leveling, worn out blocks or similar policy decisions, but
that usuall happens below the superblock level and the SSDs can be much
smarter about the algorithms used for that as it knows all data will
be written sequentially.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Bart Van Assche]

On 12/14/23 00:57, Christoph Hellwig wrote:
> On Wed, Dec 13, 2023 at 08:41:32AM -0800, Jaegeuk Kim wrote:
>> How that works, if the device gives random LBAs back to the adjacent data in
>> a file? And, how to make the LBAs into the sequential ones back?
> 
> Why would your device pick random LBAs?  If you send a zone append to
> zone it will be written at the write pointer, which is absolutely not
> random.  All I/O written in a single write is going to be sequential,
> so just like for all other devices doing large sequential writes is
> important.
Bio splitting is one potential cause of write reordering that can be
triggered even if the filesystem submits large writes. The maximum segment
size in the UFS driver is rather small (512 KiB) to restrict the latency
impact of writing data on reads.

Bart.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Bart Van Assche]

On 12/12/23 10:13, Christoph Hellwig wrote:
> If you want zoned devices to work you need something
> like zone append [ ... ]

If F2FS would submit REQ_OP_ZONE_APPEND operations, the only realistic
approach in the short term would be that sd_zbc.c translates these
operations into WRITE commands. Would it be acceptable to optimize
sd_zbc.c such that it does not restrict the queue depth to one per zone
if the storage device (UFS) preserves the command order per hardware
queue?

Thanks,

Bart.

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Damien Le Moal]

On 12/14/23 09:08, Bart Van Assche wrote:
> On 12/12/23 10:13, Christoph Hellwig wrote:
>> If you want zoned devices to work you need something
>> like zone append [ ... ]
> 
> If F2FS would submit REQ_OP_ZONE_APPEND operations, the only realistic
> approach in the short term would be that sd_zbc.c translates these
> operations into WRITE commands. Would it be acceptable to optimize
> sd_zbc.c such that it does not restrict the queue depth to one per zone
> if the storage device (UFS) preserves the command order per hardware
> queue?

Yes, that can be trivially done with the sd_zbc.c zone append emulation. If you
check the code, you'll see that sd_zbc_prepare_zone_append() returns
BLK_STS_ZONE_RESOURCE if the target zone is already locked. That causes a
requeue and the zone append to be resubmitted later. All you need to do for UFS
devices is tweak that to not requeue the zone append if the write command used
to emulate it can be issued. The completion path will also, of course, need some
tweaks to not attempt to unlock the target zone if it was not locked.

> 
> Thanks,
> 
> Bart.
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/3] block/mq-deadline: Disable I/O prioritization in certain cases

[Christoph Hellwig]

On Thu, Dec 14, 2023 at 09:37:21AM +0900, Damien Le Moal wrote:
> Yes, that can be trivially done with the sd_zbc.c zone append emulation. If you
> check the code, you'll see that sd_zbc_prepare_zone_append() returns
> BLK_STS_ZONE_RESOURCE if the target zone is already locked. That causes a
> requeue and the zone append to be resubmitted later. All you need to do for UFS
> devices is tweak that to not requeue the zone append if the write command used
> to emulate it can be issued. The completion path will also, of course, need some
> tweaks to not attempt to unlock the target zone if it was not locked.

On the Linux side yes.  I still don't see how the hardware can actually
make this scheme work withou the potential of deadlocks.  But compared
to the problems with having a completely in-order I/O stacks that's
peanuts as they say in Germany.