diff mbox series

[v3,01/30] block: Do not force full zone append completion in req_bio_endio()

Message ID 20240328004409.594888-2-dlemoal@kernel.org (mailing list archive)
State New, archived
Headers show
Series Zone write plugging | expand

Commit Message

Damien Le Moal March 28, 2024, 12:43 a.m. UTC
This reverts commit 748dc0b65ec2b4b7b3dbd7befcc4a54fdcac7988.

Partial zone append completions cannot be supported as there is no
guarantees that the fragmented data will be written sequentially in the
same manner as with a full command. Commit 748dc0b65ec2 ("block: fix
partial zone append completion handling in req_bio_endio()") changed
req_bio_endio() to always advance a partially failed BIO by its full
length, but this can lead to incorrect accounting. So revert this
change and let low level device drivers handle this case by always
failing completely zone append operations. With this revert, users will
still see an IO error for a partially completed zone append BIO.

Fixes: 748dc0b65ec2 ("block: fix partial zone append completion handling in req_bio_endio()")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 block/blk-mq.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Comments

Christoph Hellwig March 28, 2024, 4:10 a.m. UTC | #1
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Bart Van Assche March 28, 2024, 6:14 p.m. UTC | #2
On 3/27/24 17:43, Damien Le Moal wrote:
> This reverts commit 748dc0b65ec2b4b7b3dbd7befcc4a54fdcac7988.
> 
> Partial zone append completions cannot be supported as there is no
> guarantees that the fragmented data will be written sequentially in the
> same manner as with a full command. Commit 748dc0b65ec2 ("block: fix
> partial zone append completion handling in req_bio_endio()") changed
> req_bio_endio() to always advance a partially failed BIO by its full
> length, but this can lead to incorrect accounting. So revert this
> change and let low level device drivers handle this case by always
> failing completely zone append operations. With this revert, users will
> still see an IO error for a partially completed zone append BIO.
> 
> Fixes: 748dc0b65ec2 ("block: fix partial zone append completion handling in req_bio_endio()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   block/blk-mq.c | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 555ada922cf0..32afb87efbd0 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -770,16 +770,11 @@ static void req_bio_endio(struct request *rq, struct bio *bio,
>   		/*
>   		 * Partial zone append completions cannot be supported as the
>   		 * BIO fragments may end up not being written sequentially.
> -		 * For such case, force the completed nbytes to be equal to
> -		 * the BIO size so that bio_advance() sets the BIO remaining
> -		 * size to 0 and we end up calling bio_endio() before returning.
>   		 */
> -		if (bio->bi_iter.bi_size != nbytes) {
> +		if (bio->bi_iter.bi_size != nbytes)
>   			bio->bi_status = BLK_STS_IOERR;
> -			nbytes = bio->bi_iter.bi_size;
> -		} else {
> +		else
>   			bio->bi_iter.bi_sector = rq->__sector;
> -		}
>   	}
>   
>   	bio_advance(bio, nbytes);

Hi Damien,

This patch looks good to me but shouldn't it be separated from this
patch series? I think that will help this patch to get merged sooner.

Thanks,

Bart.
Damien Le Moal March 28, 2024, 10:43 p.m. UTC | #3
On 3/29/24 03:14, Bart Van Assche wrote:
> On 3/27/24 17:43, Damien Le Moal wrote:
>> This reverts commit 748dc0b65ec2b4b7b3dbd7befcc4a54fdcac7988.
>>
>> Partial zone append completions cannot be supported as there is no
>> guarantees that the fragmented data will be written sequentially in the
>> same manner as with a full command. Commit 748dc0b65ec2 ("block: fix
>> partial zone append completion handling in req_bio_endio()") changed
>> req_bio_endio() to always advance a partially failed BIO by its full
>> length, but this can lead to incorrect accounting. So revert this
>> change and let low level device drivers handle this case by always
>> failing completely zone append operations. With this revert, users will
>> still see an IO error for a partially completed zone append BIO.
>>
>> Fixes: 748dc0b65ec2 ("block: fix partial zone append completion handling in req_bio_endio()")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>   block/blk-mq.c | 9 ++-------
>>   1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 555ada922cf0..32afb87efbd0 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -770,16 +770,11 @@ static void req_bio_endio(struct request *rq, struct bio *bio,
>>   		/*
>>   		 * Partial zone append completions cannot be supported as the
>>   		 * BIO fragments may end up not being written sequentially.
>> -		 * For such case, force the completed nbytes to be equal to
>> -		 * the BIO size so that bio_advance() sets the BIO remaining
>> -		 * size to 0 and we end up calling bio_endio() before returning.
>>   		 */
>> -		if (bio->bi_iter.bi_size != nbytes) {
>> +		if (bio->bi_iter.bi_size != nbytes)
>>   			bio->bi_status = BLK_STS_IOERR;
>> -			nbytes = bio->bi_iter.bi_size;
>> -		} else {
>> +		else
>>   			bio->bi_iter.bi_sector = rq->__sector;
>> -		}
>>   	}
>>   
>>   	bio_advance(bio, nbytes);
> 
> Hi Damien,
> 
> This patch looks good to me but shouldn't it be separated from this
> patch series? I think that will help this patch to get merged sooner.

Yes, it can go on its own. But patch 3 depends on it so I kept it in the series.

Jens,

How would you like to proceed with this one ?

> 
> Thanks,
> 
> Bart.
Jens Axboe March 28, 2024, 11:03 p.m. UTC | #4
On 3/28/24 4:43 PM, Damien Le Moal wrote:
> On 3/29/24 03:14, Bart Van Assche wrote:
>> On 3/27/24 17:43, Damien Le Moal wrote:
>>> This reverts commit 748dc0b65ec2b4b7b3dbd7befcc4a54fdcac7988.
>>>
>>> Partial zone append completions cannot be supported as there is no
>>> guarantees that the fragmented data will be written sequentially in the
>>> same manner as with a full command. Commit 748dc0b65ec2 ("block: fix
>>> partial zone append completion handling in req_bio_endio()") changed
>>> req_bio_endio() to always advance a partially failed BIO by its full
>>> length, but this can lead to incorrect accounting. So revert this
>>> change and let low level device drivers handle this case by always
>>> failing completely zone append operations. With this revert, users will
>>> still see an IO error for a partially completed zone append BIO.
>>>
>>> Fixes: 748dc0b65ec2 ("block: fix partial zone append completion handling in req_bio_endio()")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>> ---
>>>   block/blk-mq.c | 9 ++-------
>>>   1 file changed, 2 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index 555ada922cf0..32afb87efbd0 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -770,16 +770,11 @@ static void req_bio_endio(struct request *rq, struct bio *bio,
>>>   		/*
>>>   		 * Partial zone append completions cannot be supported as the
>>>   		 * BIO fragments may end up not being written sequentially.
>>> -		 * For such case, force the completed nbytes to be equal to
>>> -		 * the BIO size so that bio_advance() sets the BIO remaining
>>> -		 * size to 0 and we end up calling bio_endio() before returning.
>>>   		 */
>>> -		if (bio->bi_iter.bi_size != nbytes) {
>>> +		if (bio->bi_iter.bi_size != nbytes)
>>>   			bio->bi_status = BLK_STS_IOERR;
>>> -			nbytes = bio->bi_iter.bi_size;
>>> -		} else {
>>> +		else
>>>   			bio->bi_iter.bi_sector = rq->__sector;
>>> -		}
>>>   	}
>>>   
>>>   	bio_advance(bio, nbytes);
>>
>> Hi Damien,
>>
>> This patch looks good to me but shouldn't it be separated from this
>> patch series? I think that will help this patch to get merged sooner.
> 
> Yes, it can go on its own. But patch 3 depends on it so I kept it in the series.
> 
> Jens,
> 
> How would you like to proceed with this one ?

I can just pick it up separately.
diff mbox series

Patch

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 555ada922cf0..32afb87efbd0 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -770,16 +770,11 @@  static void req_bio_endio(struct request *rq, struct bio *bio,
 		/*
 		 * Partial zone append completions cannot be supported as the
 		 * BIO fragments may end up not being written sequentially.
-		 * For such case, force the completed nbytes to be equal to
-		 * the BIO size so that bio_advance() sets the BIO remaining
-		 * size to 0 and we end up calling bio_endio() before returning.
 		 */
-		if (bio->bi_iter.bi_size != nbytes) {
+		if (bio->bi_iter.bi_size != nbytes)
 			bio->bi_status = BLK_STS_IOERR;
-			nbytes = bio->bi_iter.bi_size;
-		} else {
+		else
 			bio->bi_iter.bi_sector = rq->__sector;
-		}
 	}
 
 	bio_advance(bio, nbytes);