Re: [PATCH 03/16] block: optimise req_bio_endio()

From: Pavel Begunkov <asml.silence@gmail.com>
To: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	Jens Axboe <axboe@kernel.dk>,
	Damien Le Moal <Damien.LeMoal@wdc.com>
Subject: Re: [PATCH 03/16] block: optimise req_bio_endio()
Date: Fri, 22 Oct 2021 11:58:59 +0100	[thread overview]
Message-ID: <7d512f98-d86f-1e5a-dd7d-d8d27cbbd39b@gmail.com> (raw)
In-Reply-To: <20211022095856.2ymvvuz3xrtcuyw3@shindev>

On 10/22/21 10:58, Shinichiro Kawasaki wrote:
> Hello Pavel,
> 
> Recently I tried out for-next branch and observed that simple dd command to
> zonefs files causes an I/O error.
> 
> $ sudo dd if=/dev/zero of=/mnt/seq/0 bs=4096 count=1 oflag=direct
> dd: error writing '/mnt/seq/0': Input/output error
> 1+0 records in
> 0+0 records out
> 0 bytes copied, 0.00409641 s, 0.0 kB/s
> 
> At that time, kernel reported warnings.
> 
> [90713.298721][ T2735] zonefs (nvme0n1) WARNING: inode 1: invalid size 0 (should be 4096)
> [90713.299761][ T2735] zonefs (nvme0n1) WARNING: remounting filesystem read-only
> 
> I bisected and found that this patch triggers the error and warnings. I think
> one liner change is needed in this patch. Please find it below, in line.

[...]
> -	if (req_op(rq) == REQ_OP_ZONE_APPEND && error == BLK_STS_OK) {
>> +	} else if (req_op(rq) == REQ_OP_ZONE_APPEND) {
>>   		/*
>>   		 * Partial zone append completions cannot be supported as the
>>   		 * BIO fragments may end up not being written sequentially.
>>   		 */
>> -		if (bio->bi_iter.bi_size)
>> +		if (bio->bi_iter.bi_size == nbytes)
> 
> I think the line above should be,
> 
> 		if (bio->bi_iter.bi_size != nbytes)

You're right, that was a stupid mistake, thanks!

Jens, will you fold it in or would you prefer a patch?

> Before applying the patch, the if statement checked "bi_size is not zero".
> After applying the patch, bio_advance(bio, nbytes) moved after this check.
> Then bi_size is not decremented by nbytes and the check should be "bi_size is
> not nbytes". With this modification, the I/O error and the warnings go away.
> 
>>   			bio->bi_status = BLK_STS_IOERR;
>>   		else
>>   			bio->bi_iter.bi_sector = rq->__sector;
>>   	}
>>   
>> +	bio_advance(bio, nbytes);
>> +
>> +	if (unlikely(rq->rq_flags & RQF_QUIET))
>> +		bio_set_flag(bio, BIO_QUIET);
>>   	/* don't actually finish bio if it's part of flush sequence */
>>   	if (bio->bi_iter.bi_size == 0 && !(rq->rq_flags & RQF_FLUSH_SEQ))
>>   		bio_endio(bio);
>> -- 
>> 2.33.1
>>
> 

-- 
Pavel Begunkov