| Message ID | 20240328004409.594888-4-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Zone write plugging | expand |
On Thu, Mar 28, 2024 at 09:43:42AM +0900, Damien Le Moal wrote: > Moving req_bio_endio() code into its only caller, blk_update_request(), > allows reducing accesses to and tests of bio and request fields. Also, > given that partial completions of zone append operations is not > possible and that zone append operations cannot be merged, the update > of the BIO sector using the request sector for these operations can be > moved directly before the call to bio_endio(). Note that we should actually be able to support merging zone appends. And this patch actually moves the assignment to the right place to be able to deal with it, we'll just need to track the start offset to the start of the request so that we can assign the right bi_sector. Reviewed-by: Christoph Hellwig <hch@lst.de>
On 3/27/24 5:43 PM, Damien Le Moal wrote: > Moving req_bio_endio() code into its only caller, blk_update_request(), > allows reducing accesses to and tests of bio and request fields. Also, > given that partial completions of zone append operations is not > possible and that zone append operations cannot be merged, the update > of the BIO sector using the request sector for these operations can be > moved directly before the call to bio_endio(). Reviewed-by: Bart Van Assche <bvanassche@acm.org> > - if (unlikely(error && !blk_rq_is_passthrough(req) && > - !(req->rq_flags & RQF_QUIET)) && > - !test_bit(GD_DEAD, &req->q->disk->state)) { > + if (unlikely(error && !blk_rq_is_passthrough(req) && !quiet) && > + !test_bit(GD_DEAD, &req->q->disk->state)) { A question that is independent of this patch series: is it a bug or is it a feature that the GD_DEAD bit test is not marked as "unlikely"? Thanks, Bart.
On 3/29/24 06:28, Bart Van Assche wrote: > On 3/27/24 5:43 PM, Damien Le Moal wrote: >> Moving req_bio_endio() code into its only caller, blk_update_request(), >> allows reducing accesses to and tests of bio and request fields. Also, >> given that partial completions of zone append operations is not >> possible and that zone append operations cannot be merged, the update >> of the BIO sector using the request sector for these operations can be >> moved directly before the call to bio_endio(). > > Reviewed-by: Bart Van Assche <bvanassche@acm.org> > >> - if (unlikely(error && !blk_rq_is_passthrough(req) && >> - !(req->rq_flags & RQF_QUIET)) && >> - !test_bit(GD_DEAD, &req->q->disk->state)) { >> + if (unlikely(error && !blk_rq_is_passthrough(req) && !quiet) && >> + !test_bit(GD_DEAD, &req->q->disk->state)) { > > A question that is independent of this patch series: is it a bug or is > it a feature that the GD_DEAD bit test is not marked as "unlikely"? likely/unlikely are optimizations... I guess that bit test could be under unlikely() as well. Though if we are dealing with a removable media device, this may not be appropriate, which may be why it is not under unlikely(). Not sure. > > Thanks, > > Bart.
diff --git a/block/blk-mq.c b/block/blk-mq.c index 32afb87efbd0..e55af6058cbf 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -761,31 +761,6 @@ void blk_dump_rq_flags(struct request *rq, char *msg) } EXPORT_SYMBOL(blk_dump_rq_flags); -static void req_bio_endio(struct request *rq, struct bio *bio, - unsigned int nbytes, blk_status_t error) -{ - if (unlikely(error)) { - bio->bi_status = error; - } 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 != nbytes) - 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); -} - static void blk_account_io_completion(struct request *req, unsigned int bytes) { if (req->part && blk_do_io_stat(req)) { @@ -889,6 +864,8 @@ static void blk_complete_request(struct request *req) bool blk_update_request(struct request *req, blk_status_t error, unsigned int nr_bytes) { + bool is_flush = req->rq_flags & RQF_FLUSH_SEQ; + bool quiet = req->rq_flags & RQF_QUIET; int total_bytes; trace_block_rq_complete(req, error, nr_bytes); @@ -909,9 +886,8 @@ bool blk_update_request(struct request *req, blk_status_t error, if (blk_crypto_rq_has_keyslot(req) && nr_bytes >= blk_rq_bytes(req)) __blk_crypto_rq_put_keyslot(req); - if (unlikely(error && !blk_rq_is_passthrough(req) && - !(req->rq_flags & RQF_QUIET)) && - !test_bit(GD_DEAD, &req->q->disk->state)) { + if (unlikely(error && !blk_rq_is_passthrough(req) && !quiet) && + !test_bit(GD_DEAD, &req->q->disk->state)) { blk_print_req_error(req, error); trace_block_rq_error(req, error, nr_bytes); } @@ -923,12 +899,34 @@ bool blk_update_request(struct request *req, blk_status_t error, struct bio *bio = req->bio; unsigned bio_bytes = min(bio->bi_iter.bi_size, nr_bytes); - if (bio_bytes == bio->bi_iter.bi_size) + if (unlikely(error)) + bio->bi_status = error; + + if (bio_bytes == bio->bi_iter.bi_size) { req->bio = bio->bi_next; + } else if (req_op(req) == REQ_OP_ZONE_APPEND && + error == BLK_STS_OK) { + /* + * Partial zone append completions cannot be supported + * as the BIO fragments may end up not being written + * sequentially. + */ + bio->bi_status = BLK_STS_IOERR; + } /* Completion has already been traced */ bio_clear_flag(bio, BIO_TRACE_COMPLETION); - req_bio_endio(req, bio, bio_bytes, error); + if (unlikely(quiet)) + bio_set_flag(bio, BIO_QUIET); + + bio_advance(bio, bio_bytes); + + /* Don't actually finish bio if it's part of flush sequence */ + if (!bio->bi_iter.bi_size && !is_flush) { + if (req_op(req) == REQ_OP_ZONE_APPEND) + bio->bi_iter.bi_sector = req->__sector; + bio_endio(bio); + } total_bytes += bio_bytes; nr_bytes -= bio_bytes;
Moving req_bio_endio() code into its only caller, blk_update_request(), allows reducing accesses to and tests of bio and request fields. Also, given that partial completions of zone append operations is not possible and that zone append operations cannot be merged, the update of the BIO sector using the request sector for these operations can be moved directly before the call to bio_endio(). Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- block/blk-mq.c | 58 ++++++++++++++++++++++++-------------------------- 1 file changed, 28 insertions(+), 30 deletions(-)