From owner-svn-src-head@freebsd.org Tue Feb 6 08:15:05 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 07D5DEE17A3; Tue, 6 Feb 2018 08:15:05 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: from mail-it0-f43.google.com (mail-it0-f43.google.com [209.85.214.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 9950D7C694; Tue, 6 Feb 2018 08:15:04 +0000 (UTC) (envelope-from cse.cem@gmail.com) Received: by mail-it0-f43.google.com with SMTP id x128so1497048ite.0; Tue, 06 Feb 2018 00:15:04 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:reply-to:in-reply-to:references :from:date:message-id:subject:to:cc; bh=k9MRwf+VyRTYDtwfJUbsI+jyVKyAWOKthwYkJUTmcjM=; b=IxAsLhzL4g9Iz3RQWUWmwAOlCFp/8OsFwkbZ2jFfjrwWe/vO6GhvbWGrycBsgFkIeK gIghRojaA/pjL8D0TiT7G3RtB/+gv39tRyAJJa+l4D6Q6pM4oJTt24OS3g7pdBhDYvvV e1ow1dPmsasNfGvznjEQnDWxmJ383d9WORXGzGH0UPSxk9m+0xA/4ybljphSNXHT8hBm 91XDhoG6BmPd43ncHKOPeZYRpdojDoutoAEG6/SRgkQ/pL1r1WOagR1483jhUQoXpqjB /X400TmelAOAUn2tMIIVncyRHGqiK7JgubLrzhME5H4XazhkNqzPPxnh6AUNxDfSvlyR QN+w== X-Gm-Message-State: APf1xPA1mnDOm/Dt1AmgNdOOT+Fi9TRt/mnB77aWF4G1WmOkuPjV/DkC zEUyAqANWZD7aLjzIS99fTWBnWqc X-Google-Smtp-Source: AH8x227cLPcUjj4+ArjpnIpQmZlgO7f+8q1PhqgmzDwoBXmIC7Ti7LVKjxXnUZKB2DioGWGD7LJ+fA== X-Received: by 10.36.146.196 with SMTP id l187mr2036733itd.115.1517904544534; Tue, 06 Feb 2018 00:09:04 -0800 (PST) Received: from mail-io0-f170.google.com (mail-io0-f170.google.com. [209.85.223.170]) by smtp.gmail.com with ESMTPSA id u128sm6598213itb.1.2018.02.06.00.09.04 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 06 Feb 2018 00:09:04 -0800 (PST) Received: by mail-io0-f170.google.com with SMTP id p188so1549084ioe.12; Tue, 06 Feb 2018 00:09:04 -0800 (PST) X-Received: by 10.107.15.89 with SMTP id x86mr2049858ioi.38.1517904543916; Tue, 06 Feb 2018 00:09:03 -0800 (PST) MIME-Version: 1.0 Reply-To: cem@freebsd.org Received: by 10.2.141.86 with HTTP; Tue, 6 Feb 2018 00:09:03 -0800 (PST) In-Reply-To: <201802060019.w160Jl1W076251@repo.freebsd.org> References: <201802060019.w160Jl1W076251@repo.freebsd.org> From: Conrad Meyer Date: Tue, 6 Feb 2018 00:09:03 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r328914 - in head/sys: kern ufs/ffs To: Kirk McKusick Cc: src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Feb 2018 08:15:05 -0000 Hi Kirk, On Mon, Feb 5, 2018 at 4:19 PM, Kirk McKusick wrote: > Author: mckusick > Date: Tue Feb 6 00:19:46 2018 > New Revision: 328914 > URL: https://svnweb.freebsd.org/changeset/base/328914 > > Log: > ... > The second problem was that the check hash computed at the end of the > read was incorrect because the calculation of the check hash on > completion of the read was being done too soon. > > - When a read completes we had the following sequence: > > - bufdone() > -- b_ckhashcalc (calculates check hash) > -- bufdone_finish() > --- vfs_vmio_iodone() (replaces bogus pages with the cached ones) > > - When we are reading a buffer where one or more pages are already > in memory (but not all pages, or we wouldn't be doing the read), > the I/O is done with bogus_page mapped in for the pages that exist > in the VM cache. This mapping is done to avoid corrupting the > cached pages if there is any I/O overrun. The vfs_vmio_iodone() > function is responsible for replacing the bogus_page(s) with the > cached ones. But we were calculating the check hash before the > bogus_page(s) were replaced. Hence, when we were calculating the > check hash, we were partly reading from bogus_page, which means > we calculated a bad check hash (e.g., because multiple pages have > been mapped to bogus_page, so its contents are indeterminate). > > The second fix is to move the check-hash calculation from bufdone() > to bufdone_finish() after the call to vfs_vmio_iodone() so that it > computes the check hash over the correct set of pages. Does the b_iodone callback have a very similar potential problem? It is invoked in bufdone(), before bufdone_finish() and vfs_vmio_iodone(). One example b_iodone is the bdone() callback. It seems that b_iodone -> bdone() can then wake bwait()ers before any VMIO cached content has been filled in. I don't know that any specific consumers of b_iodone are currently broken, but it seems like maybe the b_iodone callback should really be in the later location as well. Best, Conrad