Re: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Aug 19, 2014 at 04:04:11PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@xxxxxxxxxx]
> > Sent: Saturday, August 16, 2014 6:04 AM
> > To: linux-kernel@xxxxxxxxxxxxxxx; linux-fsdevel@xxxxxxxxxxxxxxx;
> > linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Jaegeuk Kim
> > Subject: [f2fs-dev] [PATCH 3/4] f2fs: avoid double lock in truncate_blocks
> > 
> > The init_inode_metadata calls truncate_blocks when error is occurred.
> > The callers holds f2fs_lock_op, so we should not call it again in
> > truncate_blocks.
> 
> Nice catch! Your solution is a good way to fix this issue.
> 
> Previously, in create inode path, I found there are some redundant codes between
> init_inode_metadata and evict_inode, including:
> 	truncate_inode_pages(&inode->i_data, 0);
> 	truncate_blocks(inode, 0);
> 	remove_dirty_dir_inode(inode);
> 	remove_inode_page(inode); 
> 
> So I think there is another way to fix this issue by removing error path handling
> codes in init_inode_metadata, not making the inode bad to left garbage clean work in
> evict_inode. In this way we can avoid adding additional argument for all different
> callers.

Well, possible.
But we need to take a closer look at the race condition on the inode cache.
What can happen if this bad inode is reassigned to the other thread?

> 
> How do you think?
> 
> Thanks,
> Yu
> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
> > ---
> >  fs/f2fs/data.c   |  2 +-
> >  fs/f2fs/dir.c    |  2 +-
> >  fs/f2fs/f2fs.h   |  2 +-
> >  fs/f2fs/file.c   | 13 ++++++++-----
> >  fs/f2fs/inline.c |  2 +-
> >  5 files changed, 12 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 68834e2..14cc3e8 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -935,7 +935,7 @@ static void f2fs_write_failed(struct address_space *mapping, loff_t to)
> > 
> >  	if (to > inode->i_size) {
> >  		truncate_pagecache(inode, inode->i_size);
> > -		truncate_blocks(inode, inode->i_size);
> > +		truncate_blocks(inode, inode->i_size, true);
> >  	}
> >  }
> > 
> > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > index a69bbfa..155fb05 100644
> > --- a/fs/f2fs/dir.c
> > +++ b/fs/f2fs/dir.c
> > @@ -391,7 +391,7 @@ put_error:
> >  error:
> >  	/* once the failed inode becomes a bad inode, i_mode is S_IFREG */
> >  	truncate_inode_pages(&inode->i_data, 0);
> > -	truncate_blocks(inode, 0);
> > +	truncate_blocks(inode, 0, false);
> >  	remove_dirty_dir_inode(inode);
> >  	remove_inode_page(inode);
> >  	return ERR_PTR(err);
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 2723b2d..7f976c1 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -1122,7 +1122,7 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi)
> >   */
> >  int f2fs_sync_file(struct file *, loff_t, loff_t, int);
> >  void truncate_data_blocks(struct dnode_of_data *);
> > -int truncate_blocks(struct inode *, u64);
> > +int truncate_blocks(struct inode *, u64, bool);
> >  void f2fs_truncate(struct inode *);
> >  int f2fs_getattr(struct vfsmount *, struct dentry *, struct kstat *);
> >  int f2fs_setattr(struct dentry *, struct iattr *);
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index ecbdf6a..a8e97f8 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -422,7 +422,7 @@ out:
> >  	f2fs_put_page(page, 1);
> >  }
> > 
> > -int truncate_blocks(struct inode *inode, u64 from)
> > +int truncate_blocks(struct inode *inode, u64 from, bool lock)
> >  {
> >  	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> >  	unsigned int blocksize = inode->i_sb->s_blocksize;
> > @@ -438,14 +438,16 @@ int truncate_blocks(struct inode *inode, u64 from)
> >  	free_from = (pgoff_t)
> >  			((from + blocksize - 1) >> (sbi->log_blocksize));
> > 
> > -	f2fs_lock_op(sbi);
> > +	if (lock)
> > +		f2fs_lock_op(sbi);
> > 
> >  	set_new_dnode(&dn, inode, NULL, NULL, 0);
> >  	err = get_dnode_of_data(&dn, free_from, LOOKUP_NODE);
> >  	if (err) {
> >  		if (err == -ENOENT)
> >  			goto free_next;
> > -		f2fs_unlock_op(sbi);
> > +		if (lock)
> > +			f2fs_unlock_op(sbi);
> >  		trace_f2fs_truncate_blocks_exit(inode, err);
> >  		return err;
> >  	}
> > @@ -463,7 +465,8 @@ int truncate_blocks(struct inode *inode, u64 from)
> >  	f2fs_put_dnode(&dn);
> >  free_next:
> >  	err = truncate_inode_blocks(inode, free_from);
> > -	f2fs_unlock_op(sbi);
> > +	if (lock)
> > +		f2fs_unlock_op(sbi);
> >  done:
> >  	/* lastly zero out the first data page */
> >  	truncate_partial_data_page(inode, from);
> > @@ -480,7 +483,7 @@ void f2fs_truncate(struct inode *inode)
> > 
> >  	trace_f2fs_truncate(inode);
> > 
> > -	if (!truncate_blocks(inode, i_size_read(inode))) {
> > +	if (!truncate_blocks(inode, i_size_read(inode), true)) {
> >  		inode->i_mtime = inode->i_ctime = CURRENT_TIME;
> >  		mark_inode_dirty(inode);
> >  	}
> > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> > index 520758b..4d1f39f 100644
> > --- a/fs/f2fs/inline.c
> > +++ b/fs/f2fs/inline.c
> > @@ -247,7 +247,7 @@ process_inline:
> >  		update_inode(inode, ipage);
> >  		f2fs_put_page(ipage, 1);
> >  	} else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) {
> > -		truncate_blocks(inode, 0);
> > +		truncate_blocks(inode, 0, false);
> >  		set_inode_flag(F2FS_I(inode), FI_INLINE_DATA);
> >  		goto process_inline;
> >  	}
> > --
> > 1.8.5.2 (Apple Git-48)
> > 
> > 
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux