From owner-freebsd-fs@FreeBSD.ORG Fri Jun 4 15:00:52 2010 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 16995106564A; Fri, 4 Jun 2010 15:00:52 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-gy0-f182.google.com (mail-gy0-f182.google.com [209.85.160.182]) by mx1.freebsd.org (Postfix) with ESMTP id 867698FC1C; Fri, 4 Jun 2010 15:00:51 +0000 (UTC) Received: by gyh20 with SMTP id 20so1258408gyh.13 for ; Fri, 04 Jun 2010 08:00:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:sender:received :in-reply-to:references:date:x-google-sender-auth:message-id:subject :from:to:cc:content-type:content-transfer-encoding; bh=gZq1HKsxkDjzGhenpqpe1WhRW25OocUGqAm2ork1MRk=; b=tYpnuOavmaSyx9b1vnT80o2cwl7LJXTfyVUyjSIes785gDp3/2hDzbXd1W14jxyxLe 4WVjxouxe3o/JexZYwY2cLtlM7TA5O0kHeIwQHY0qkOx+IFE6QTbVe6E+uWdvg1biQOt rHyHxJs3mF3ZTsim0Eljnqf0Zfrt8huKS63Us= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=t6tT+XmmSXVO+Bqm0ACppaxOo/fx73Gt/bmFdNCQsfr4v9dy6bQJPzl4IMNbvKZjRO UZW9ue5jSYrXINmk5opZ4yOf+2H7jG0CYzYvcuG5Qx/yqY2gp5f8VIlp7q8G6pM7Geox nGGDbgDVcRXzCz8/ZjBZco3826hRA+xsjeYe8= MIME-Version: 1.0 Received: by 10.224.93.203 with SMTP id w11mr5432387qam.335.1275663649779; Fri, 04 Jun 2010 08:00:49 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.229.183.213 with HTTP; Fri, 4 Jun 2010 08:00:49 -0700 (PDT) In-Reply-To: <20100603143501.GA3176@a91-153-117-195.elisa-laajakaista.fi> References: <20100603143501.GA3176@a91-153-117-195.elisa-laajakaista.fi> Date: Fri, 4 Jun 2010 17:00:49 +0200 X-Google-Sender-Auth: Tn7NqIGgHlDVr3ZkrEDv8IufQrg Message-ID: From: Attilio Rao To: Jaakko Heinonen Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: freebsd-fs@freebsd.org, kib@freebsd.org Subject: Re: syncer vnode leak because of nmount() race X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Jun 2010 15:00:52 -0000 2010/6/3 Jaakko Heinonen : > > Hi, > > I have been playing with devfs and stress2 recently and I was able to > trigger a syncer vnode leak with devfs.sh. As far as I can tell it is a > nmount(2) (initial) mount race against update mount. The code in > vfs_domount() is protected with vfs_busy()/vfs_unbusy() but it doesn't > protect against update mounts. The protection by Giant is defeated > because devfs_root() may sleep due to sx_xlock(). vfs_domount() calls > VFS_ROOT() before allocating the syncer vnode. VI_MOUNT vnode flag > doesn't provide sufficient protection against update mounts either. Thanks for this bug report. I think that, luckilly, it is not a very common condition to have the mount still in flight and get updates... :) However, I think that the real breakage here is that the check on mnt->mnt_syncer is done lockless and it is unsafe. It might really be protected by the mount interlock but that's tricky because vfs_allocate_syncvnode() sleeps. Also re-checking the condition (after the allocation takes place) is not too simple here. I found also this bug when rewriting the syncer and I resolved that by using a separate flag for that (in my case it was simpler and more beneficial actually for some other reasons, but you may do the same thing with a mnt_kern_flag entry). If you have no time I can work actively on the patch, even if I'm confident, luckilly, this problem is very hard to happen in the real life. Additively, note that vfs_busy() here is not intended to protect against such situations but against unmount. Also, I'm very unsure about what Giant protection might bring here. IMHO we might probabilly remove it at all as long as all the sleeping point present make it completely unuseful if anything (and I don't see a reason why it may be needed). > PS. vfs_unbusy(9) manual page is out of date after r184554 and IMO > =C2=A0 =C2=A0vfs_busy(9) manual page is misleading because it talks about > =C2=A0 =C2=A0synchronizing access to a mount point. May you be more precise on what is misleading please? Thanks, Attilio --=20 Peace can only be achieved by understanding - A. Einstein