From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.0 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 852FCC4707F for ; Thu, 27 May 2021 08:48:12 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1FFBF6128D for ; Thu, 27 May 2021 08:48:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1FFBF6128D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:46112 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lmBgZ-0002pa-2p for qemu-devel@archiver.kernel.org; Thu, 27 May 2021 04:48:11 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:37816) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lmBfh-0001eX-D0 for qemu-devel@nongnu.org; Thu, 27 May 2021 04:47:17 -0400 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:37624) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lmBfe-0003gD-MT for qemu-devel@nongnu.org; Thu, 27 May 2021 04:47:16 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1622105232; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references; bh=obCSaUqWoPY8cB0S4SdXLBaF3ueMA+hF5AmaJtaqpnk=; b=dZEy2M8QKcd8krfJ3Re7FaCo17Iy4Q+nRgpcwippqJEdz+DVVK47rPmmGrOJJn9GlyOcyU iKs9iULNNmP6Vy0TrPeWSyd65D5JBm2cQPuFDQ1pNNcmbHpfUofRaRhp+pFq6PTZSjdg1T /Wm/8rTcX/Pp3RGipxil/VWP42qAeZA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-141-ZfwvYy87OzqQXnWqBfIlQA-1; Thu, 27 May 2021 04:47:06 -0400 X-MC-Unique: ZfwvYy87OzqQXnWqBfIlQA-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 937F56D5C0; Thu, 27 May 2021 08:47:05 +0000 (UTC) Received: from redhat.com (ovpn-115-54.ams2.redhat.com [10.36.115.54]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C3BD067881; Thu, 27 May 2021 08:46:57 +0000 (UTC) Date: Thu, 27 May 2021 09:46:54 +0100 From: Daniel =?utf-8?B?UC4gQmVycmFuZ8Op?= To: Peter Xu Subject: Re: [PATCH 1/1] yank: Unregister function when using TLS migration Message-ID: References: <20210526200540.1088333-1-leobras.c@gmail.com> <20210526232103.39e2a7d0@gecko.fritz.box> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/2.0.7 (2021-05-04) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=berrange@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Received-SPF: pass client-ip=216.205.24.124; envelope-from=berrange@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -31 X-Spam_score: -3.2 X-Spam_bar: --- X-Spam_report: (-3.2 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.371, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H4=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Daniel =?utf-8?B?UC4gQmVycmFuZ8Op?= Cc: qemu-devel@nongnu.org, Leonardo Bras , Lukas Straub , "Dr. David Alan Gilbert" , Juan Quintela Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On Wed, May 26, 2021 at 05:58:58PM -0400, Peter Xu wrote: > On Wed, May 26, 2021 at 11:21:03PM +0200, Lukas Straub wrote: > > On Wed, 26 May 2021 16:40:35 -0400 > > Peter Xu wrote: > > > > > On Wed, May 26, 2021 at 05:05:40PM -0300, Leonardo Bras wrote: > > > > After yank feature was introduced, whenever migration is started using TLS, > > > > the following error happens in both source and destination hosts: > > > > > > > > (qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance: > > > > Assertion `QLIST_EMPTY(&entry->yankfns)' failed. > > > > > > > > This happens because of a missing yank_unregister_function() when using > > > > qio-channel-tls. > > > > > > > > Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to perform > > > > yank_unregister_function() in channel_close() and multifd_load_cleanup(). > > > > > > > > Fixes: 50186051f ("Introduce yank feature") > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326 > > > > Signed-off-by: Leonardo Bras > > > > > > Leo, > > > > > > Thanks for looking into it! > > > > > > So before looking int the fix... I do have a doubt on why we only enable yank > > > on socket typed, as I think tls should also work with qio_channel_shutdown(). > > > > > > IIUC the confused thing here is we register only for qio-socket, however tls > > > will actually call migration_channel_connect() twice, first with a qio-socket, > > > then with the real tls-socket. For tls I feel like we have registered with the > > > wrong channel - instead of the wrapper socket ioc, we should register to the > > > final tls ioc? > > > > > > Lukas, is there a reason? > > > > > > > Hi, > > There is no specific reason. Both ways work equally well in preventing > > qemu from hanging. shutdown() for tls-channel just makes it abort a > > little sooner (by not attempting to encrypt and send data anymore). > > > > I don't lean either way. I guess registering it on the tls-channel > > makes is a bit more explicit and clearer. > > Agreed, because IMHO logically the migration code should not be aware of > internals of IOChannels, e.g., we shouldn't need to know ioc->master is the > socket ioc of tls ioc to unregister. I think it is atually better to ignore the TLS channel and *always* yank on the undering socket IO channel. The yank functionality is intended to be used in a scenario where we know the channels are broken. If yank calls the high level IO channel it is potentially going to try to do a cleanup shutdown that we know will fail because of the broken network. Conceptually we just want to yank out the socket channel and leave everything above that to just deal with the fallout of the terminated socket. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|