From patchwork Fri Mar 31 01:26:35 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Li Qiang X-Patchwork-Id: 745466 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3vvP442Lk6z9ryb for ; Fri, 31 Mar 2017 12:27:00 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="rgyEOglE"; dkim-atps=neutral Received: from localhost ([::1]:38425 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ctlL7-00035G-N2 for incoming@patchwork.ozlabs.org; Thu, 30 Mar 2017 21:26:57 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46284) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ctlKo-00034z-RM for qemu-devel@nongnu.org; Thu, 30 Mar 2017 21:26:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ctlKn-0006Xd-9Y for qemu-devel@nongnu.org; Thu, 30 Mar 2017 21:26:38 -0400 Received: from mail-it0-x244.google.com ([2607:f8b0:4001:c0b::244]:34495) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1ctlKn-0006XA-3c for qemu-devel@nongnu.org; Thu, 30 Mar 2017 21:26:37 -0400 Received: by mail-it0-x244.google.com with SMTP id e75so784273itd.1 for ; Thu, 30 Mar 2017 18:26:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=gU8xKf9Uw6Cyr4HwRDKD6ULeO6dfJXFGljE5EOH3WZw=; b=rgyEOglECD6odnWTEtntozPE8jXKUS5B0PsQ8pqN1CGBBBGQGSuW3k5bKhuI9K5H+j ux8k4UTB9sEDlEzRnrUOBa4EKp0B04thTEdKRQaRftYC1L97XGyDkFSls/AWIO7MNrbH 1Y4leiMh/TPpdVDn0frJMhxPFJk5KVEKsmf86+daMbjoKWd/NqiFnJXcmEu9ift1fT3+ uVsEOjOFTUv9FN5JDrjKGTb0ivHUeTZb51ZoAU+DY8tXaZY+Sj+8VYan69QcF+4NkNOk dqKrhp6dSOI9p21Jxuys293qRxKoo+U4GMpRomkZawuGkE/1NWn/c7DNttoHuOWjbnRy 2Z/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=gU8xKf9Uw6Cyr4HwRDKD6ULeO6dfJXFGljE5EOH3WZw=; b=LqEf5auDddZqCzMVZf4lz7lBwKhc1KXwUB2GLlImNgNqhmmGx643HlLvNSEK4UpCL1 SDdTnknYuL822tpGJsA8uDt1baKNhVHodw4CHU6hz8MwJqvmcDOb4DkPjzFHfYI85Hiy EsJTvQfGfqtMDzfsMx/JYh6fHX94y6gd8YvOH7jrGbQTUv65mz3lwc4I3fQDLDn6G2hF 0d8okMGMnTqzvDpwUlZ/2C/qJVrri/jKMSUeEe+ybkxEQ0Q+PptO579Wj9cWuJ2Hip1s VvC4CQ0YCte2Cvg+BCOFWquTF0TgXXacRTTPqdlouDxvbJDscPmuXUlG71XMZPNVEjsZ c5Lw== X-Gm-Message-State: AFeK/H2UW7ZfBrYeHMjK3KVjxHRJD6gO0VPqrpchF/KvTi4MdwEJw7JgrVsYqAz9yqqhHFxKq2O1+l22wg16GQ== X-Received: by 10.36.178.29 with SMTP id u29mr1404716ite.41.1490923596462; Thu, 30 Mar 2017 18:26:36 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.6.140 with HTTP; Thu, 30 Mar 2017 18:26:35 -0700 (PDT) In-Reply-To: <20170330174608.0ed57618@bahia.lab.toulouse-stg.fr.ibm.com> References: <1490876834-61392-1-git-send-email-liqiang6-s@360.cn> <20170330174608.0ed57618@bahia.lab.toulouse-stg.fr.ibm.com> From: Li Qiang Date: Fri, 31 Mar 2017 09:26:35 +0800 Message-ID: To: Greg Kurz X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4001:c0b::244 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [Qemu-devel] [PATCH for-2.9?] 9pfs: fix migration_block leak X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Li Qiang , Qemu Developers , P J P Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" Hello, 2017-03-30 23:46 GMT+08:00 Greg Kurz : > On Thu, 30 Mar 2017 08:25:25 -0500 > Eric Blake wrote: > > > On 03/30/2017 07:27 AM, Li Qiang wrote: > > > The guest can leave the pdu->s->migration_blocker exists by attach > > > > s/exists/in place/ > > s/attach/attaching/ > > > Eric, Thanks for pointing my mistakes! > > > but not remove a fid. Then if we hot unplug the 9pfs device, the > > > > In theory you're right, but the current 9p client in linux won't let you > hot > unplug the device unless you unmount the 9p share first, hence freeing the > blocker. > > I think we should consider every possible situation. > > s/remove/removing/ > > > > > v9fs_reset() just free the fids, but not free the migration_blocker. > > > This will leak a memory leak. This patch avoid this. > > I had a similar issue sitting my TODO list for quite a time: the blocker > survives a system_reset. It doesn't cause a memory leak but it prevents > migration until the guest mounts/unmounts the 9p share again. > > This boils down to virtfs_reset() calling free_fid() instead of put_fid() > IIRC. > > > > > s/leak a/cause a/ > > s/avoid/avoids/ > > > > > > > > Signed-off-by: Li Qiang > > > --- > > > hw/9pfs/9p.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > Probably worth including in 2.9 as a bug fix. > > > > Reviewed-by: Eric Blake > > > > > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > > index 48babce..b55c02d 100644 > > > --- a/hw/9pfs/9p.c > > > +++ b/hw/9pfs/9p.c > > > @@ -548,6 +548,12 @@ static void coroutine_fn virtfs_reset(V9fsPDU > *pdu) > > > free_fid(pdu, fidp); > > > } > > > } > > > + > > > + if (pdu->s->migration_blocker) { > > > + migrate_del_blocker(pdu->s->migration_blocker); > > > + error_free(pdu->s->migration_blocker); > > > + pdu->s->migration_blocker = NULL; > > > + } > > I'd prefer to drain all PDUs in virtfs_reset() and have the loop above > to call put_fid() instead of free_fid(). If this isn't doable for 2.9, > I'll apply this patch with a comment. > > Yes, I have considered to use put_fid() to fix this. But I'm not sure the 'fidp->ref' is at most 1 in virtfs_reset() function(I think it is). IIUC I think omit the 'else' branch, and call put_fid() directly like this. If you agree, I will send a formal patch. > > > } > > > > > > #define P9_QID_TYPE_DIR 0x80 > > > > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 48babce..ae97e79 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -544,9 +544,8 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu) if (fidp->ref) { fidp->clunked = 1; - } else { - free_fid(pdu, fidp); } + put_fid(pdu, fidp); } }