From: Kevin Wolf <kwolf@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Cc: Fam Zheng <fam@euphon.net>,
Stefano Stabellini <sstabellini@kernel.org>,
Sergio Lopez <slp@redhat.com>,
qemu-block@nongnu.org, "Michael S. Tsirkin" <mst@redhat.com>,
Paul Durrant <paul@xen.org>,
qemu-devel@nongnu.org, Max Reitz <mreitz@redhat.com>,
Stefan Hajnoczi <stefanha@redhat.com>,
xen-devel@lists.xenproject.org,
Anthony Perard <anthony.perard@citrix.com>,
Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v2 2/4] block: Avoid processing BDS twice in bdrv_set_aio_context_ignore()
Date: Thu, 17 Dec 2020 14:06:02 +0100 [thread overview]
Message-ID: <20201217130602.GB12328@merkur.fritz.box> (raw)
In-Reply-To: <d7c1ee7f-4171-1407-3a71-a7e45708cc4a@virtuozzo.com>
Am 17.12.2020 um 13:50 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 17.12.2020 13:58, Kevin Wolf wrote:
> > Am 17.12.2020 um 10:37 hat Sergio Lopez geschrieben:
> > > On Wed, Dec 16, 2020 at 07:31:02PM +0100, Kevin Wolf wrote:
> > > > Am 16.12.2020 um 15:55 hat Sergio Lopez geschrieben:
> > > > > On Wed, Dec 16, 2020 at 01:35:14PM +0100, Kevin Wolf wrote:
> > > > > > Anyway, trying to reconstruct the block graph with BdrvChild pointers
> > > > > > annotated at the edges:
> > > > > >
> > > > > > BlockBackend
> > > > > > |
> > > > > > v
> > > > > > backup-top ------------------------+
> > > > > > | | |
> > > > > > | +-----------------------+ |
> > > > > > | 0x5655068b8510 | | 0x565505e3c450
> > > > > > | | |
> > > > > > | 0x565505e42090 | |
> > > > > > v | |
> > > > > > qcow2 ---------------------+ | |
> > > > > > | | | |
> > > > > > | 0x565505e52060 | | | ??? [1]
> > > > > > | | | | |
> > > > > > v 0x5655066a34d0 | | | | 0x565505fc7aa0
> > > > > > file v v v v
> > > > > > qcow2 (backing)
> > > > > > |
> > > > > > | 0x565505e41d20
> > > > > > v
> > > > > > file
> > > > > >
> > > > > > [1] This seems to be a BdrvChild with a non-BDS parent. Probably a
> > > > > > BdrvChild directly owned by the backup job.
> > > > > >
> > > > > > > So it seems this is happening:
> > > > > > >
> > > > > > > backup-top (5e48030) <---------| (5)
> > > > > > > | | |
> > > > > > > | | (6) ------------> qcow2 (5fbf660)
> > > > > > > | ^ |
> > > > > > > | (3) | | (4)
> > > > > > > |-> (1) qcow2 (5e5d420) ----- |-> file (6bc0c00)
> > > > > > > |
> > > > > > > |-> (2) file (5e52060)
> > > > > > >
> > > > > > > backup-top (5e48030), the BDS that was passed as argument in the first
> > > > > > > bdrv_set_aio_context_ignore() call, is re-entered when qcow2 (5fbf660)
> > > > > > > is processing its parents, and the latter is also re-entered when the
> > > > > > > first one starts processing its children again.
> > > > > >
> > > > > > Yes, but look at the BdrvChild pointers, it is through different edges
> > > > > > that we come back to the same node. No BdrvChild is used twice.
> > > > > >
> > > > > > If backup-top had added all of its children to the ignore list before
> > > > > > calling into the overlay qcow2, the backing qcow2 wouldn't eventually
> > > > > > have called back into backup-top.
> > > > >
> > > > > I've tested a patch that first adds every child to the ignore list,
> > > > > and then processes those that weren't there before, as you suggested
> > > > > on a previous email. With that, the offending qcow2 is not re-entered,
> > > > > so we avoid the crash, but backup-top is still entered twice:
> > > >
> > > > I think we also need to every parent to the ignore list before calling
> > > > callbacks, though it doesn't look like this is the problem you're
> > > > currently seeing.
> > >
> > > I agree.
> > >
> > > > > bs=0x560db0e3b030 (backup-top) enter
> > > > > bs=0x560db0e3b030 (backup-top) processing children
> > > > > bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e2f450 (child->bs=0x560db0fb2660)
> > > > > bs=0x560db0fb2660 (qcow2) enter
> > > > > bs=0x560db0fb2660 (qcow2) processing children
> > > > > bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db0e34d20 (child->bs=0x560db1bb3c00)
> > > > > bs=0x560db1bb3c00 (file) enter
> > > > > bs=0x560db1bb3c00 (file) processing children
> > > > > bs=0x560db1bb3c00 (file) processing parents
> > > > > bs=0x560db1bb3c00 (file) processing itself
> > > > > bs=0x560db0fb2660 (qcow2) calling bsaci child=0x560db16964d0 (child->bs=0x560db0e50420)
> > > > > bs=0x560db0e50420 (qcow2) enter
> > > > > bs=0x560db0e50420 (qcow2) processing children
> > > > > bs=0x560db0e50420 (qcow2) calling bsaci child=0x560db0e34ea0 (child->bs=0x560db0e45060)
> > > > > bs=0x560db0e45060 (file) enter
> > > > > bs=0x560db0e45060 (file) processing children
> > > > > bs=0x560db0e45060 (file) processing parents
> > > > > bs=0x560db0e45060 (file) processing itself
> > > > > bs=0x560db0e50420 (qcow2) processing parents
> > > > > bs=0x560db0e50420 (qcow2) processing itself
> > > > > bs=0x560db0fb2660 (qcow2) processing parents
> > > > > bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1672860
> > > > > bs=0x560db0fb2660 (qcow2) calling set_aio_ctx child=0x560db1b14a20
> > > > > bs=0x560db0e3b030 (backup-top) enter
> > > > > bs=0x560db0e3b030 (backup-top) processing children
> > > > > bs=0x560db0e3b030 (backup-top) processing parents
> > > > > bs=0x560db0e3b030 (backup-top) calling set_aio_ctx child=0x560db0e332d0
> > > > > bs=0x560db0e3b030 (backup-top) processing itself
> > > > > bs=0x560db0fb2660 (qcow2) processing itself
> > > > > bs=0x560db0e3b030 (backup-top) calling bsaci child=0x560db0e35090 (child->bs=0x560db0e50420)
> > > > > bs=0x560db0e50420 (qcow2) enter
> > > > > bs=0x560db0e3b030 (backup-top) processing parents
> > > > > bs=0x560db0e3b030 (backup-top) processing itself
> > > > >
> > > > > I see that "blk_do_set_aio_context()" passes "blk->root" to
> > > > > "bdrv_child_try_set_aio_context()" so it's already in the ignore list,
> > > > > so I'm not sure what's happening here. Is backup-top is referenced
> > > > > from two different BdrvChild or is "blk->root" not pointing to
> > > > > backup-top's BDS?
> > > >
> > > > The second time that backup-top is entered, it is not as the BDS of
> > > > blk->root, but as the parent node of the overlay qcow2. Which is
> > > > interesting, because last time it was still the backing qcow2, so the
> > > > change did have _some_ effect.
> > > >
> > > > The part that I don't understand is why you still get the line with
> > > > child=0x560db1b14a20, because when you add all children to the ignore
> > > > list first, that should have been put into the ignore list as one of the
> > > > first things in the whole process (when backup-top was first entered).
> > > >
> > > > Is 0x560db1b14a20 a BdrvChild that has backup-top as its opaque value,
> > > > but isn't actually present in backup-top's bs->children?
> > >
> > > Exactly, that line corresponds to this chunk of code:
> > >
> > > <---- begin ---->
> > > QLIST_FOREACH(child, &bs->parents, next_parent) {
> > > if (g_slist_find(*ignore, child)) {
> > > continue;
> > > }
> > > assert(child->klass->set_aio_ctx);
> > > *ignore = g_slist_prepend(*ignore, child);
> > > fprintf(stderr, "bs=%p (%s) calling set_aio_ctx child=%p\n", bs, bs->drv->format_name, child);
> > > child->klass->set_aio_ctx(child, new_context, ignore);
> > > }
> > > <---- end ---->
> > >
> > > Do you think it's safe to re-enter backup-top, or should we look for a
> > > way to avoid this?
> >
> > I think it should be avoided, but I don't understand why putting all
> > children of backup-top into the ignore list doesn't already avoid it. If
> > backup-top is in the parents list of qcow2, then qcow2 should be in the
> > children list of backup-top and therefore the BdrvChild should already
> > be in the ignore list.
> >
> > The only way I can explain this is that backup-top and qcow2 have
> > different ideas about which BdrvChild objects exist that connect them.
> > Or that the graph changes between both places, but I don't see how that
> > could happen in bdrv_set_aio_context_ignore().
> >
>
> bdrv_set_aio_context_ignore() do bdrv_drained_begin().. As I reported
> recently, nothing prevents some job finish and do graph modification
> during some another drained section. It may be the case.
Good point, this might be the same bug then.
If everything worked correctly, a job completion could only happen on
the outer bdrv_set_aio_context_ignore(). But after that, we are already
in a drain section, so the job should be quiesced and a second drain
shouldn't cause any additional graph changes.
I would have to go back to the other discussion, but I think it was
related to block jobs that are already in the completion process and
keep moving forward even though they are supposed to be quiesced.
If I remember correctly, actually pausing them at this point looked
difficult. Maybe what we should then do is letting .drained_poll return
true until they have actually fully completed?
Ah, but was this something that would deadlock because the job
completion callbacks use drain sections themselves?
> If backup-top involved, I can suppose that graph modification is in
> backup_clean, when we remove the filter.. Who is making
> set_aio_context in the issue? I mean, what is the backtrace of
> bdrv_set_aio_context_ignore()?
Sergio, can you provide the backtrace and also test if the theory with a
job completion in the middle of the process is what you actually hit?
Kevin
next prev parent reply other threads:[~2020-12-17 13:08 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-14 17:05 [PATCH v2 0/4] nbd/server: Quiesce coroutines on context switch Sergio Lopez
2020-12-14 17:05 ` [PATCH v2 1/4] block: Honor blk_set_aio_context() context requirements Sergio Lopez
2020-12-15 11:58 ` Kevin Wolf
2020-12-14 17:05 ` [PATCH v2 2/4] block: Avoid processing BDS twice in bdrv_set_aio_context_ignore() Sergio Lopez
2020-12-15 12:12 ` Kevin Wolf
2020-12-15 13:15 ` Sergio Lopez
2020-12-15 15:01 ` Kevin Wolf
2020-12-15 17:23 ` Sergio Lopez
2020-12-16 12:35 ` Kevin Wolf
2020-12-16 14:55 ` Sergio Lopez
2020-12-16 18:31 ` Kevin Wolf
2020-12-17 9:37 ` Sergio Lopez
2020-12-17 10:58 ` Kevin Wolf
2020-12-17 12:50 ` Vladimir Sementsov-Ogievskiy
2020-12-17 13:06 ` Kevin Wolf [this message]
2020-12-17 13:27 ` Sergio Lopez
2020-12-17 14:01 ` Vladimir Sementsov-Ogievskiy
2020-12-17 13:09 ` Sergio Lopez
2020-12-14 17:05 ` [PATCH v2 3/4] nbd/server: Quiesce coroutines on context switch Sergio Lopez
2020-12-14 17:05 ` [PATCH v2 4/4] block: Close block exports in two steps Sergio Lopez
2020-12-15 15:34 ` Kevin Wolf
2020-12-15 17:26 ` Sergio Lopez
2020-12-21 17:07 ` Sergio Lopez
2021-01-20 20:49 ` [PATCH v2 0/4] nbd/server: Quiesce coroutines on context switch Eric Blake
2021-01-21 5:57 ` Sergio Lopez
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201217130602.GB12328@merkur.fritz.box \
--to=kwolf@redhat.com \
--cc=anthony.perard@citrix.com \
--cc=fam@euphon.net \
--cc=mreitz@redhat.com \
--cc=mst@redhat.com \
--cc=paul@xen.org \
--cc=pbonzini@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=slp@redhat.com \
--cc=sstabellini@kernel.org \
--cc=stefanha@redhat.com \
--cc=vsementsov@virtuozzo.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).