Re: [PATCH v4 8/8] migration: Add a wrapper to cleanup migration files

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Aug 16, 2023 at 03:35:24PM -0400, Peter Xu wrote:
>> On Wed, Aug 16, 2023 at 03:47:26PM -0300, Fabiano Rosas wrote:
>> > Peter Xu <peterx@redhat.com> writes:
>> > 
>> > > On Wed, Aug 16, 2023 at 11:25:10AM -0300, Fabiano Rosas wrote:
>> > >> @@ -2003,6 +1980,8 @@ static int open_return_path_on_source(MigrationState *ms)
>> > >>          return -1;
>> > >>      }
>> > >>  
>> > >> +    migration_ioc_register_yank(qemu_file_get_ioc(ms->rp_state.from_dst_file));
>> > >
>> > > I think I didn't really get why it wasn't paired before yesterday.  My
>> > > fault.
>> > >
>> > > Registering from_dst_file, afaict, will register two identical yank objects
>> > > because the ioc is the same.
>> > >
>> > 
>> > Why do we have two QEMUFiles for the same fd again?
>> 
>> Because qemufile has a "direction" (either read / write)?
>> 
>> > 
>> > We're bound to crash at some point by trying to qemu_fclose() the two
>> > QEMUFiles at the same time.
>> 
>> Even with each qemufile holding a reference on the ioc object?  I thought
>> it won't crash, but if it will please point that out; or fix it would be
>> even better.

You're right, it wouldn't crash. But it's still the same ioc object. If
qio_channel_close() is called twice, then we could potentially close the
fd twice. Which would either error out or close a reused fd. The window
is small though, so probably unlikely to ever happen.

>> 
>> > 
>> > > Should we make migration_file_release() not handle the unregister of
>> > > yank(), but leave that to callers?  Then we keep the rule of only register
>> > > yank for each ioc once.
>> > >
>> > 
>> > We need the unregister to be at migration_file_release() so that it
>> > takes benefit of the locking while checking the file for NULL. If it
>> > moves out then the caller will have to do locking as well. Which
>> > defeats the purpose of the patch.
>> > 
>> > I don't understand why you moved the unregister out of channel_close in
>> > commit 39675ffffb ("migration: Move the yank unregister of channel_close
>> > out"). You called it a "hack" at the time, but looking at the current
>> > situation, it seems like a reasonable thing to do: unregister the yank
>> > when the ioc refcount drops to 1.
>> > 
>> > I would go even further and say that qemu_fclose should also avoid
>> > calling qio_channel_close if the ioc refcnt is elevated.
>> 
>> I'd rather not; I still think it's a hack, always open to be corrected.

It's hard to figure out what you mean by hack at times. Even more when
reading a years-old commit message.

>> 
>> I think the problem is yank can register anything so it's separate from
>> iochannels.  If one would like to have ioc close() automatically
>> unregister, then one should also register yank transparently without the
>> ioc user even aware of yank's existance.

Ok, fair point.

>> 
>> Now the condition is the caller register yank itself, then I think the
>> caller should unreg it.. not iochannel itself, silently.

I think the issue is that we're linking the yank with the QEMUFile for
no reason. The migration_yank_iochannel() performs a
qio_channel_shutdown() which is an operation on the fd. The QEMUFile
just happens to hold a pointer to the ioc.

>
> I just noticed this is not really copying the list.. let me add the cc list
> back, assuming it was just forgotten.

I'm sorry, I hit the wrong key while replying.

> One more thing to mention is, now I kind of agree probably we should
> register yank over each qemufile, as you raised the concern in the other
> thread that otherwise qmp_yank() won't set error for the qemufile, which
> seems to be unexpected.

I haven't made up my mind yet, but I think I'd rather stop setting that
error instead of doing it from other places. A shutdown() is mostly a
benign operation intended to end the connection. The fact that we use it
in some cases to kick the thread out of a possible hang doesn't seem
compelling enough to set -EIO.

Of course we currently have no other way to indicate that the file was
shutdown, so the -EIO will have to stay and that's a discussion for
another day.