Re: [PATCH v2 5/5] migration: Move the yank unregister of channel_close out

[Peter Xu]

On Thu, Jul 22, 2021 at 06:09:03PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Thu, Jul 22, 2021 at 04:27:35PM +0100, Dr. David Alan Gilbert wrote:
> > > > @@ -3352,6 +3355,8 @@ static MigThrError postcopy_pause(MigrationState 
> > > > *s)
> > > >  
> > > >          /* Current channel is possibly broken. Release it. */
> > > >          assert(s->to_dst_file);
> > > > +        /* Unregister yank for current channel */
> > > > +        migration_ioc_unregister_yank_from_file(s->to_dst_file);
> > > 
> > > Should this go inside the lock?
> > 
> > Shouldn't need to; as we've got the assert() right above so otherwise we'll
> > abrt otherwise :)
> 
> Hmm OK; although with out always having to think about it then you worry
> about something getting in after the assert.

Right; the point is still that to_dst_file shouldn't be changed by other
thread, or it'll bring some more challenge.

If it can be mnodified when reaching this line, it means it can also reach
earlier at assert(), then we coredump.  So we should guaratee it won't happen,
or we'd better also remove that assertion..

I think the challenge here is, we do have a mutex to protect the files and from
that pov it's the same as other mutex.  However it's different in that this
mutex is also used in the oob handlers so we want to "keep it non-blocking and
as small as possible for the critical sections".

I didn't put it into the mutex protection because the yank code will further go
to take the yank_lock so it'll make things slightly more complicated (then the
file mutex is correlated to yank_lock too!).  I don't think that's a problem
because yank_lock is also "non-blocking" (ah! it can still block, got scheduled
out, but there's no explicit things that will proactively sleep..).  So since I
think it's safe without the lock then I kept it outside of the lock then we
don't need to discuss the safely of having it inside the critical section.

(However then I noticed we still need justification on why it can be moved 
out..)

> 
> > The mutex lock/unlock right below this one is not protecting us from someone
> > changing it but really for being able to wait until someone finished using 
> > it
> > then we won't crash someone else.
> > 
> > I think the rational is to_dst_file is managed by migration thread while
> > from_dst_file is managed by rp_thread.
> > 
> > Maybe I add a comment above?
> 
> OK, I almost pushed this further, but then I started to get worried that
> we'd trace off a race with ordering on two locks with yank_lock; so yeh
> lets just add a comment.

Agreed.  I think ordering is not a huge problem as yank_lock is very limitedly
used in yank and protect yank instance/functions only, so there shouldn't be a
path we take file mutex within it.  Will repost shortly, thanks.

-- 
Peter Xu