f2fs: avoid calling i_size_read when it's unnecessary

PATCH In review Fan Li 2 participants 5 messages Dec 30, 2015 – Jan 4, 2016 F2FS file system

lore

Share & tools

lore

Fan Li

author

2015-12-30 08:50 UTC #0 lore raw

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

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 <[email protected]>

---
 fs/f2fs/data.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/f2fs/data.c +3 -1

index d67c599..a9a4d89 100644
@@ -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;

Signature · 5 lines

1.7.9.5



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

Jaegeuk Kim

maintainer

2015-12-30 18:27 UTC #1 lore raw

Hi Fan,

Quoted 22 lines · Fan Li

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 <[email protected]>

---
 fs/f2fs/data.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/f2fs/data.c +1 -1

index d67c599..a9a4d89 100644
@@ -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;

fs/f2fs/data.c int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, · line 798

Quoted 4 lines

fs/f2fs/data.c +1 -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,

Quoted 7 lines

fs/f2fs/data.c +1 -0

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

Signature · 1 line

1.7.9.5

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

Fan Li

author

2015-12-31 02:54 UTC #2 lore raw

Quoted 37 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Thursday, December 31, 2015 2:28 AM
To: Fan Li
Cc: [email protected]
Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: avoid calling i_size_read when it's unnecessary
Hi Fan,
Quoted 25 lines · Fan Li
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 <[email protected]>
---
 fs/f2fs/data.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/f2fs/data.c index d67c599..a9a4d89 renamed from fs/f2fs/data.c +0 -0
100644
fs/f2fs/data.c +1 -1
@@ -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;
 	}
 

Quoted 3 lines

fs/f2fs/data.c int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, · line 798
Quoted 1 line
fs/f2fs/data.c +1 -0
+	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?

Quoted 10 lines

Thanks,
Quoted 7 lines
fs/f2fs/data.c +1 -1
+
 	mutex_lock(&inode->i_mutex);
 	if (start >= isize)
 		goto out;
--
1.7.9.5

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

Jaegeuk Kim

maintainer

2016-01-01 03:07 UTC #3 lore raw

Hi Fan,

Quoted 37 lines · Fan Li

Quoted 37 lines
-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Thursday, December 31, 2015 2:28 AM
To: Fan Li
Cc: [email protected]
Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: avoid calling i_size_read when it's unnecessary
Hi Fan,
Quoted 25 lines · Fan Li
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 <[email protected]>
---
 fs/f2fs/data.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/f2fs/data.c index d67c599..a9a4d89 renamed from fs/f2fs/data.c +0 -0
100644
fs/f2fs/data.c +1 -1
@@ -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;
 	}
 

Quoted 8 lines

Quoted 3 lines
fs/f2fs/data.c int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, · line 798
Quoted 1 line
fs/f2fs/data.c +1 -0
+	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,

Quoted 11 lines

Quoted 11 lines
Thanks,
Quoted 7 lines
fs/f2fs/data.c +1 -1
+
 	mutex_lock(&inode->i_mutex);
 	if (start >= isize)
 		goto out;
--
1.7.9.5

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

Fan Li

author

2016-01-04 06:13 UTC #4 lore raw

Quoted 55 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Friday, January 01, 2016 11:08 AM
To: Fan Li
Cc: [email protected]
Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: avoid calling i_size_read when it's unnecessary
Hi Fan,
Quoted 43 lines · Fan Li
Quoted 42 lines
-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Thursday, December 31, 2015 2:28 AM
To: Fan Li
Cc: [email protected]
Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: avoid calling i_size_read when it's unnecessary
Hi Fan,
Quoted 23 lines · Fan Li
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 <[email protected]>
---
 fs/f2fs/data.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/f2fs/data.c index renamed from fs/f2fs/data.c +0 -0
d67c599..a9a4d89
100644
fs/f2fs/data.c +1 -1
@@ -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;
fs/f2fs/data.c int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, · line 798
Quoted 4 lines
fs/f2fs/data.c +1 -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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 14 lines

Thanks,
Quoted 11 lines
Quoted 11 lines
Thanks,
Quoted 7 lines
fs/f2fs/data.c +1 -1
+
 	mutex_lock(&inode->i_mutex);
 	if (start >= isize)
 		goto out;
--
1.7.9.5

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