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

[Fan Li <fanofcode.li@samsung.com>]

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> Sent: Friday, January 01, 2016 11:08 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 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.

Yes, the difference between i_size_read and assignment is very small. I just think it's best
to squeeze every bit of performance out of codes.

If the consistence is a more pressing issue, then i_size_read it is.

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


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