From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933111AbcKQCwC (ORCPT ); Wed, 16 Nov 2016 21:52:02 -0500 Received: from szxga01-in.huawei.com ([58.251.152.64]:6407 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752377AbcKQCwB (ORCPT ); Wed, 16 Nov 2016 21:52:01 -0500 Subject: Re: [PATCH] f2fs: fix fdatasync To: Jaegeuk Kim References: <20161116121211.11790-1-yuchao0@huawei.com> <20161116191312.GA57609@jaegeuk> CC: , , From: Chao Yu Message-ID: <25a9f225-ba22-d0cd-54ff-4a75031bb161@huawei.com> Date: Thu, 17 Nov 2016 10:51:37 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <20161116191312.GA57609@jaegeuk> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.134.22.195] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jaegeuk, On 2016/11/17 3:13, Jaegeuk Kim wrote: > Hi Chao, > > On Wed, Nov 16, 2016 at 08:12:11PM +0800, Chao Yu wrote: >> For below two cases, we can't guarantee data consistence: >> >> a) >> 1. xfs_io "pwrite 0 4195328" "fsync" >> 2. xfs_io "pwrite 4195328 1024" "fdatasync" >> 3. godown >> 4. umount & mount >> --> isize we updated before fdatasync won't be recovered >> >> b) >> 1. xfs_io "pwrite -S 0xcc 0 4202496" "fsync" >> 2. xfs_io "fpunch 4194304 4096" "fdatasync" >> 3. godown >> 4. umount & mount >> --> dnode we punched before fdatasync won't be recovered >> >> The reason is that normally fdatasync won't be aware of modification >> of metadata in file, e.g. isize changing, dnode updating, so in ->fsync >> we will skip flushing node pages for above cases, result in making >> fdatasynced file being lost during recovery. >> >> Introduce FDATASYNC_INO global ino cache for tracking node changing, >> later fdatasync choose to flush nodes depend on ino cache state. > > We don't need to add this additionally, and would be better to consider other > major metadata as well. > > How about this? Seems it can't track file after evict? Thanks, > > --- > fs/f2fs/f2fs.h | 11 ++++++++++- > fs/f2fs/file.c | 2 +- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index c9672a3..50ffa4f 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1727,8 +1727,17 @@ static inline void f2fs_i_size_write(struct inode *inode, loff_t i_size) > set_inode_flag(inode, FI_AUTO_RECOVER); > } > > -static inline bool f2fs_skip_inode_update(struct inode *inode) > +static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync) > { > + if (dsync) { > + struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > + bool ret; > + > + spin_lock(&sbi->inode_lock[DIRTY_META]); > + ret = list_empty(&F2FS_I(inode)->gdirty_list); > + spin_unlock(&sbi->inode_lock[DIRTY_META]); > + return ret; > + } > if (!is_inode_flag_set(inode, FI_AUTO_RECOVER)) > return false; > return F2FS_I(inode)->last_disk_size == i_size_read(inode); > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c > index d48c120..50123c6 100644 > --- a/fs/f2fs/file.c > +++ b/fs/f2fs/file.c > @@ -212,7 +212,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end, > } > > /* if the inode is dirty, let's recover all the time */ > - if (!datasync && !f2fs_skip_inode_update(inode)) { > + if (!f2fs_skip_inode_update(inode, datasync)) { > f2fs_write_inode(inode, NULL); > goto go_write; > } >