Re: [PATCH v5 7/8] hw/nvme: add basic live migration support

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4720 bytes --]

On Wed, Apr 08, 2026 at 01:31:33PM +0200, Alexander Mikhalitsyn wrote:
> Am Mi., 8. Apr. 2026 um 08:41 Uhr schrieb Klaus Jensen <its@irrelevant.dk>:
> >
> > On Apr  7 21:02, Alexander Mikhalitsyn wrote:
> > > Am Di., 7. Apr. 2026 um 17:48 Uhr schrieb Stefan Hajnoczi <stefanha@redhat.com>:
> > > >
> > > > On Tue, Mar 17, 2026 at 11:27:07AM +0100, Alexander Mikhalitsyn wrote:
> > > > > +    /* wait when all in-flight IO requests (except NVME_ADM_CMD_ASYNC_EV_REQ) are processed */
> > > > > +    for (i = 0; i < n->num_queues; i++) {
> > > > > +        NvmeRequest *req;
> > > > > +        NvmeSQueue *sq = n->sq[i];
> > > > > +
> > > > > +        if (!sq)
> > > > > +            continue;
> > > > > +
> > > > > +        trace_pci_nvme_pre_save_sq_out_req_drain_wait(n, i, sq->head, sq->tail, sq->size);
> > > > > +
> > > > > +wait_out_reqs:
> > > > > +        QTAILQ_FOREACH(req, &sq->out_req_list, entry) {
> > > > > +            if (req->cmd.opcode != NVME_ADM_CMD_ASYNC_EV_REQ) {
> > > > > +                cpu_relax();
> > > > > +                goto wait_out_reqs;
> > > > > +            }
> > > > > +        }
> > > > > +
> > > > > +        trace_pci_nvme_pre_save_sq_out_req_drain_wait_end(n, i, sq->head, sq->tail);
> > > > > +    }
> > > >
> > >
> > > Hi Stefan,
> > >
> > > > Emulated storage controllers usually do not drain requests themselves.
> > > > They rely on core migration code (e.g. migration_completion_precopy())
> > > > to stop vCPUs and call bdrv_drain_all_begin/end() to quiesce I/O. Why
> > > > does NVMe busy wait for requests here?
> > >
> > > I rely on core migration code to stop vCPUs and drain requests, *but*
> > > a challenge here is that
> > > a concept of "in-flight" request in NVMe is not that simple and we
> > > have a few different types of in-flight requests:
> > > - request was written in SQ (sq->head != sq->tail) -> this I don't
> > > even consider as in-flight, because we just stop SQ processing
> > >   and these requests don't require any special handling during migration
> > > - request was taken from SQ by nvme_process_sq() and it now lives in
> > > sq->out_req_list - this means that
> > >   we have also initialized req->aiocb and submitted IO for processing
> > > in QEMU block layer. After request is processed, completion callback
> > >   will be called (for read/write requests it is
> > > nvme_rw_complete_cb()), then nvme_enqueue_req_completion() will be
> > > called and remove
> > >   NvmeRequest from sq->out_req_list and put it into cq->req_list.
> > >   I expect, that by the time when we enter nvme_ctrl_pre_save(),
> > > bdrv_drain_all_begin/end() were called and
> > >   all AIO is finished and sq->out_req_list is empty (except AERs).
> > > *But* to be on a safe side I also added busy loop on
> > >   sq->out_req_list.
> > >
> >
> > Correct. When nvme_rw_complete_cb (and friends) returns, we still have
> > the completion laying around, not posted on the CQ. That is queued up
> > for a BH to handle to coalesce CQE posting.
> >
> > > So, I tend to agree that this busy wait is probably not required, but
> > > I believe that we still need to verify that sq->out_req_list
> > > is in fact empty. Because if we messed up, then it's better to crash
> > > on assert() than to have silent data corruption.
> > >
> > > Then after I have a loop cq->req_list, and this time it is absolutely
> > > required because we need to write all NvmeRequest
> > > results to CQ and free NvmeRequest structure, cause I didn't want to
> > > deal with NvmeRequest serialization.
> > >
> 
> Hi Klaus,
> 
> >
> > There is a subtle catch here. There may not be room in the CQ to post
> > all CQEs. For example, in the extreme, the host has allocated a CQ with
> > room for 1 entry (size 2) and several deep SQs all associated with the
> > same CQ. If the controller has nowhere to post CQEs, then we need to
> > either abort migration (and try again) or migrate the CQEs.
> 
> Exactly. This is why I have a next loop, where I ensure that
> cq->req_list is empty
> and also check nvme_cq_full(cq) to ensure that all CQEs can be posted.

Hi Alexander,
nvme_cq_full(cq) will never become false because the migration thread
holds the Big QEMU Lock while calling .pre_save(). The busy loop will
hang forever. See my reply to an earlier email in this thread for
details.

A qtest test case would be useful here (see tests/qtest/nvme-test.c) to
fill the CQ and live migrate. It might be tricky to deterministically
run .pre_save() (via the `migrate` QMP command) before nvme_post_cqes()
gets called, but then you'd have a test case that covers the CQ full
code path.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]