From patchwork Mon Jun 25 11:10:31 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Sterba X-Patchwork-Id: 10485573 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id E228D601D5 for ; Mon, 25 Jun 2018 11:13:28 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D2183283B0 for ; Mon, 25 Jun 2018 11:13:28 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C54BB2841E; Mon, 25 Jun 2018 11:13:28 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 90F20283B0 for ; Mon, 25 Jun 2018 11:13:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932245AbeFYLNZ (ORCPT ); Mon, 25 Jun 2018 07:13:25 -0400 Received: from mx2.suse.de ([195.135.220.15]:36807 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932089AbeFYLNY (ORCPT ); Mon, 25 Jun 2018 07:13:24 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (charybdis-ext-too.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 4F427AD52; Mon, 25 Jun 2018 11:13:23 +0000 (UTC) Received: by ds.suse.cz (Postfix, from userid 10065) id E144ADA8D3; Mon, 25 Jun 2018 13:10:31 +0200 (CEST) Date: Mon, 25 Jun 2018 13:10:31 +0200 From: David Sterba To: Chris Mason Cc: linux-btrfs@vger.kernel.org Subject: Re: [PATCH RFC 0/2] Btrfs: fix file data corruptions due to lost dirty bits Message-ID: <20180625111031.GA27958@suse.cz> Reply-To: dsterba@suse.cz Mail-Followup-To: dsterba@suse.cz, Chris Mason , linux-btrfs@vger.kernel.org References: <20180620145612.1644763-1-clm@fb.com> <20180620193310.GG24375@twin.jikos.cz> <36F766B7-6388-4B93-A39F-A977F6F805D3@fb.com> <20180620202431.GH24375@suse.cz> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Fri, Jun 22, 2018 at 05:25:54PM -0400, Chris Mason wrote: > The bug came here: > > commit a528a24150870c5c16cbbbec69dba7e992b08456 > Author: Souptick Joarder > Date: Wed Jun 6 19:54:44 2018 +0530 > > btrfs: change return type of btrfs_page_mkwrite to vm_fault_t > > When page->mapping != mapping, we improperly return success because ret2 > is zero on goto out_unlock. > > I'm running a fix through a full xfstests and I'll post soon. The ret/ret2 pattern is IMO used in the wrong way, the ret2 is some temporary value and it should be set and tested next to each other, not holding the state accross the function. The fix I'd propose is to avoid relying on it and add a separate exit block, similar to out_noreserve: --- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -8981,7 +8981,6 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf) ret = VM_FAULT_SIGBUS; goto out_unlock; } - ret2 = 0; /* page is wholly or partially inside EOF */ if (page_start + PAGE_SIZE > size) @@ -9004,14 +9003,6 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf) BTRFS_I(inode)->last_log_commit = BTRFS_I(inode)->root->last_log_commit; unlock_extent_cached(io_tree, page_start, page_end, &cached_state); - -out_unlock: - if (!ret2) { - btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, true); - sb_end_pagefault(inode->i_sb); - extent_changeset_free(data_reserved); - return VM_FAULT_LOCKED; - } unlock_page(page); out: btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, (ret != 0)); @@ -9021,6 +9012,12 @@ vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf) sb_end_pagefault(inode->i_sb); extent_changeset_free(data_reserved); return ret; + +out_unlock: + btrfs_delalloc_release_extents(BTRFS_I(inode), PAGE_SIZE, true); + sb_end_pagefault(inode->i_sb); + extent_changeset_free(data_reserved); + return VM_FAULT_LOCKED; } static int btrfs_truncate(struct inode *inode, bool skip_writeback)