* [PATCH 1/3] f2fs: avoid calling i_size_read when it's unnecessary
@ 2015-12-30 8:49 Fan Li
2015-12-30 18:27 ` Jaegeuk Kim
0 siblings, 1 reply; 5+ messages in thread
From: Fan Li @ 2015-12-30 8:49 UTC (permalink / raw)
To: 'Jaegeuk Kim'; +Cc: linux-f2fs-devel
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);
+
mutex_lock(&inode->i_mutex);
if (start >= isize)
goto out;
--
1.7.9.5
------------------------------------------------------------------------------
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] f2fs: avoid calling i_size_read when it's unnecessary
2015-12-30 8:49 [PATCH 1/3] f2fs: avoid calling i_size_read when it's unnecessary Fan Li
@ 2015-12-30 18:27 ` Jaegeuk Kim
2015-12-31 2:53 ` Fan Li
0 siblings, 1 reply; 5+ messages in thread
From: Jaegeuk Kim @ 2015-12-30 18:27 UTC (permalink / raw)
To: Fan Li; +Cc: linux-f2fs-devel
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.
Thanks,
> +
> mutex_lock(&inode->i_mutex);
> if (start >= isize)
> goto out;
> --
> 1.7.9.5
>
------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] f2fs: avoid calling i_size_read when it's unnecessary
2015-12-30 18:27 ` Jaegeuk Kim
@ 2015-12-31 2:53 ` Fan Li
2016-01-01 3:07 ` Jaegeuk Kim
0 siblings, 1 reply; 5+ messages in thread
From: Fan Li @ 2015-12-31 2:53 UTC (permalink / raw)
To: 'Jaegeuk Kim'; +Cc: linux-f2fs-devel
> -----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?
>
> Thanks,
>
> > +
> > mutex_lock(&inode->i_mutex);
> > if (start >= isize)
> > goto out;
> > --
> > 1.7.9.5
> >
------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] f2fs: avoid calling i_size_read when it's unnecessary
2015-12-31 2:53 ` Fan Li
@ 2016-01-01 3:07 ` Jaegeuk Kim
2016-01-04 6:13 ` Fan Li
0 siblings, 1 reply; 5+ messages in thread
From: Jaegeuk Kim @ 2016-01-01 3:07 UTC (permalink / raw)
To: Fan Li; +Cc: linux-f2fs-devel
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
> > >
------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] f2fs: avoid calling i_size_read when it's unnecessary
2016-01-01 3:07 ` Jaegeuk Kim
@ 2016-01-04 6:13 ` Fan Li
0 siblings, 0 replies; 5+ messages in thread
From: Fan Li @ 2016-01-04 6:13 UTC (permalink / raw)
To: 'Jaegeuk Kim'; +Cc: linux-f2fs-devel
> -----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
> > > >
------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-01-04 6:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-30 8:49 [PATCH 1/3] f2fs: avoid calling i_size_read when it's unnecessary Fan Li
2015-12-30 18:27 ` Jaegeuk Kim
2015-12-31 2:53 ` Fan Li
2016-01-01 3:07 ` Jaegeuk Kim
2016-01-04 6:13 ` Fan Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).