From patchwork Wed Jul 6 12:00:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Kara X-Patchwork-Id: 645236 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3rkzsB481gz9sDb for ; Wed, 6 Jul 2016 22:02:38 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754036AbcGFMBA (ORCPT ); Wed, 6 Jul 2016 08:01:00 -0400 Received: from mx2.suse.de ([195.135.220.15]:41939 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753037AbcGFMA7 (ORCPT ); Wed, 6 Jul 2016 08:00:59 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 3EE3EAC5D; Wed, 6 Jul 2016 12:00:51 +0000 (UTC) Received: by quack2.suse.cz (Postfix, from userid 1000) id E59BF1E0F14; Wed, 6 Jul 2016 14:00:49 +0200 (CEST) Date: Wed, 6 Jul 2016 14:00:49 +0200 From: Jan Kara To: Al Viro Cc: Theodore Ts'o , linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: Re: [RFC] weirdness in ext4_sync_file() Message-ID: <20160706120049.GI14067@quack2.suse.cz> References: <20160624043604.GU14480@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20160624043604.GU14480@ZenIV.linux.org.uk> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On Fri 24-06-16 05:36:05, Al Viro wrote: > Could somebody explain when would the second part of that test _not_ be true? > > if (!ret && !hlist_empty(&inode->i_dentry)) > ret = ext4_sync_parent(inode); > > inode is that of an opened file; how could it possibly _not_ have a dentry > alias? Is that code actually supposed to check if the sucker is not > unlinked? Yes, that's what the test was supposed to do I believe. > If so, it's not what we are actually checking - pinned dentry remains > positive (and unhashed) after unlink(2). What's more, the loop in > ext4_sync_parent() is vulnerable to races with rmdir(2) - if you get > unlink and rmdir of ancestors between > > next = igrab(d_inode(dentry->d_parent)); > and > inode = next; > ret = sync_mapping_buffers(inode->i_mapping); > if (ret) > break; > ret = sync_inode_metadata(inode, 1); > if (ret) > break; > you are risking interesting things done in the middle of rmdir and/or > unlink; that might be actually safe, but in that case it's worth a comment > explaining that. You are right and it should be harmless. I'm now testing the patch below. Thanks for your comments. From 3dbea3e4e58d9ad81f38e8f89f9f569daef5a800 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Wed, 6 Jul 2016 13:55:58 +0200 Subject: [PATCH] ext4: Cleanup ext4_sync_parent() A condition !hlist_empty(&inode->i_dentry) is always true for open file. Just remove it. Also ext4_sync_parent() could use some explanation why races with rmdir() are not an issue - add a comment explaining that. Reported-by: Al Viro Signed-off-by: Jan Kara --- fs/ext4/fsync.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c index 8850254136ae..9b9335796fbc 100644 --- a/fs/ext4/fsync.c +++ b/fs/ext4/fsync.c @@ -61,6 +61,13 @@ static int ext4_sync_parent(struct inode *inode) break; iput(inode); inode = next; + /* + * The directory inode may have gone through rmdir by now. But + * the inode itself and its blocks are still allocated (we hold + * a reference to the inode so it didn't go through + * ext4_evict_inode()) and so we are safe to flush metadata + * blocks and the inode. + */ ret = sync_mapping_buffers(inode->i_mapping); if (ret) break; @@ -107,7 +114,7 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync) if (!journal) { ret = generic_file_fsync(file, start, end, datasync); - if (!ret && !hlist_empty(&inode->i_dentry)) + if (!ret) ret = ext4_sync_parent(inode); goto out; }