* Re: [PATCH] virtio-blk: Fix WARN_ON_ONCE in virtio_queue_rq() [not found] <20220823145005.26356-1-suwan.kim027@gmail.com> @ 2022-08-23 16:39 ` Christoph Hellwig 2022-08-23 20:56 ` Stefan Hajnoczi 1 sibling, 0 replies; 3+ messages in thread From: Christoph Hellwig @ 2022-08-23 16:39 UTC (permalink / raw) To: Suwan Kim; +Cc: acourbot, mst, virtualization, linux-block, stefanha, pbonzini Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de> _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] virtio-blk: Fix WARN_ON_ONCE in virtio_queue_rq() [not found] <20220823145005.26356-1-suwan.kim027@gmail.com> 2022-08-23 16:39 ` [PATCH] virtio-blk: Fix WARN_ON_ONCE in virtio_queue_rq() Christoph Hellwig @ 2022-08-23 20:56 ` Stefan Hajnoczi [not found] ` <CAFNWusaXc3H78kx1wxUDLht3PuV0A_VxvdmmY-yMJNefvih-1Q@mail.gmail.com> 1 sibling, 1 reply; 3+ messages in thread From: Stefan Hajnoczi @ 2022-08-23 20:56 UTC (permalink / raw) To: Suwan Kim; +Cc: acourbot, mst, virtualization, linux-block, pbonzini [-- Attachment #1.1: Type: text/plain, Size: 641 bytes --] On Tue, Aug 23, 2022 at 11:50:05PM +0900, Suwan Kim wrote: > @@ -409,6 +409,8 @@ static bool virtblk_add_req_batch(struct virtio_blk_vq *vq, > virtblk_unmap_data(req, vbr); > virtblk_cleanup_cmd(req); > rq_list_add(requeue_list, req); > + } else { > + blk_mq_start_request(req); > } The device may see new requests as soon as virtblk_add_req() is called above. Therefore the device may complete the request before blk_mq_start_request() is called. rq->io_start_time_ns = ktime_get_ns() will be after the request was actually submitted. I think blk_mq_start_request() needs to be called before virtblk_add_req(). Stefan [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 183 bytes --] _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <CAFNWusaXc3H78kx1wxUDLht3PuV0A_VxvdmmY-yMJNefvih-1Q@mail.gmail.com>]
* Re: [PATCH] virtio-blk: Fix WARN_ON_ONCE in virtio_queue_rq() [not found] ` <CAFNWusaXc3H78kx1wxUDLht3PuV0A_VxvdmmY-yMJNefvih-1Q@mail.gmail.com> @ 2022-08-24 17:32 ` Stefan Hajnoczi 0 siblings, 0 replies; 3+ messages in thread From: Stefan Hajnoczi @ 2022-08-24 17:32 UTC (permalink / raw) To: Kim Suwan; +Cc: acourbot, mst, virtualization, linux-block, pbonzini [-- Attachment #1.1: Type: text/plain, Size: 2115 bytes --] On Wed, Aug 24, 2022 at 10:16:10PM +0900, Kim Suwan wrote: > Hi Stefan, > > On Wed, Aug 24, 2022 at 5:56 AM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > On Tue, Aug 23, 2022 at 11:50:05PM +0900, Suwan Kim wrote: > > > @@ -409,6 +409,8 @@ static bool virtblk_add_req_batch(struct virtio_blk_vq *vq, > > > virtblk_unmap_data(req, vbr); > > > virtblk_cleanup_cmd(req); > > > rq_list_add(requeue_list, req); > > > + } else { > > > + blk_mq_start_request(req); > > > } > > > > The device may see new requests as soon as virtblk_add_req() is called > > above. Therefore the device may complete the request before > > blk_mq_start_request() is called. > > > > rq->io_start_time_ns = ktime_get_ns() will be after the request was > > actually submitted. > > > > I think blk_mq_start_request() needs to be called before > > virtblk_add_req(). > > > > But if blk_mq_start_request() is called before virtblk_add_req() > and virtblk_add_req() fails, it can trigger WARN_ON_ONCE() at > virtio_queue_rq(). > > With regard to the race condition between virtblk_add_req() and > completion, I think that the race condition can not happen because > virtblk_add_req() holds vq lock with irq saving and completion side > (virtblk_done, virtblk_poll) need to acquire the vq lock also. > Moreover, virtblk_done() is irq context so I think it can not be > executed until virtblk_add_req() releases the lock. I agree there is no race condition regarding the ordering of blk_mq_start_request() and request completion. The spinlock prevents that and I wasn't concerned about that part. The issue is that the timestamp will be garbage. If we capture the timestamp during/after the request is executing, then the collected statistics will be wrong. Can you look for another solution that doesn't break the timestamp? FWIW I see that the rq->state state machine allows returning to the idle state once the request has been started: __blk_mq_requeue_request(). Stefan [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 183 bytes --] _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-08-24 17:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20220823145005.26356-1-suwan.kim027@gmail.com>
2022-08-23 16:39 ` [PATCH] virtio-blk: Fix WARN_ON_ONCE in virtio_queue_rq() Christoph Hellwig
2022-08-23 20:56 ` Stefan Hajnoczi
[not found] ` <CAFNWusaXc3H78kx1wxUDLht3PuV0A_VxvdmmY-yMJNefvih-1Q@mail.gmail.com>
2022-08-24 17:32 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).