Re: [PATCH 1/3] f2fs: avoid calling i_size_read when it's unnecessary

[Jaegeuk Kim <jaegeuk@kernel.org>]

Hi Fan,

On Thu, Dec 31, 2015 at 10:53:32AM +0800, Fan Li wrote:
> 
> 
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> > Sent: Thursday, December 31, 2015 2:28 AM
> > To: Fan Li
> > Cc: linux-f2fs-devel@lists.sourceforge.net
> > Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: avoid calling i_size_read when it's unnecessary
> > 
> > Hi Fan,
> > 
> > On Wed, Dec 30, 2015 at 04:49:56PM +0800, Fan Li wrote:
> > > i_size_read does more than reading a value, it's best that we use it
> > > only when we need it.
> > >
> > > Signed-off-by: Fan li <fanofcode.li@samsung.com>
> > > ---
> > >  fs/f2fs/data.c |    4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index d67c599..a9a4d89
> > > 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -780,7 +780,7 @@ int f2fs_fiemap(struct inode *inode, struct
> > > fiemap_extent_info *fieinfo,  {
> > >  	struct buffer_head map_bh;
> > >  	sector_t start_blk, last_blk;
> > > -	loff_t isize = i_size_read(inode);
> > > +	loff_t isize;
> > >  	u64 logical = 0, phys = 0, size = 0;
> > >  	u32 flags = 0;
> > >  	int ret = 0;
> > > @@ -795,6 +795,8 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
> > >  			return ret;
> > >  	}
> > >
> > > +	isize = i_size_read(inode);
> > 
> > It seems that we need to get isize after grabbing i_mutex below in order to avoid data race.
> 
> See if I got this right, isize should remain unchanged during the entire procedure, so we need
> to add i_mutex upon it, and since there is already a i_mutex,  we can get inode->i_size
> directly instead of calling i_read_size.
> Is that right?

Right, but I prefer to use i_size_read consistently.
TOH, I'm not convincing why you're treating this as a so costly operation.

Thanks,

> 
> > 
> > Thanks,
> > 
> > > +
> > >  	mutex_lock(&inode->i_mutex);
> > >  	if (start >= isize)
> > >  		goto out;
> > > --
> > > 1.7.9.5
> > >

------------------------------------------------------------------------------