Skip to contentSkip to conversation

ocfs2: llseek need to require ocfs2 inode lock for updating the size in SEEK_END

PATCH Superseded shencanquan 2 participants 2 messages Jun 19, 2013 OCFS2

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 ocfs2: llseek need to require ocfs2 inode lock for updating the size in SEEK_END

The test scenario is following:

There are two node, one is nodeA, another is nodeB
On nodeA, the test program open the file and write some data to the file and then close the file.
On nodeB, the another test program open the file and llseek the end of file. we found that the position of file is old.

After apply this patch. it is ok on nodeB. the position of file is update to date.

---
 fs/ocfs2/file.c |    9 +++++++++
 1 file changed, 9 insertions(+)
fs/ocfs2/file.c +9 -0
index ff54014..f2570ba 100644
@@ -2626,7 +2626,16 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)
 	case SEEK_SET:
 		break;
 	case SEEK_END:
+		/* SEEK_END requires the OCFS2 inode lock because
+		 * it uses the inode size.
+		*/
+		ret = ocfs2_inode_lock(inode, NULL, 0);
+		if (ret < 0) {
+			mlog_errno(ret);
+			goto out;
+		}
 		offset += inode->i_size;
+		ocfs2_inode_unlock(inode, 0);
 		break;
 	case SEEK_CUR:
 		if (offset == 0) {
Signature · 1 line
1.7.9.7
#1 lore raw

PATCH v2 1/1 ocfs2: llseek need to require ocfs2 inode lock for updating the size in SEEK_END

Hi Canquan,

Quoted 10 lines · shencanquan

The test scenario is following:

There are two node, one is nodeA, another is nodeB
On nodeA, the test program open the file and write some data to the file and then close the file.
On nodeB, the another test program open the file and llseek the end of file. we found that the position of file is old.

After apply this patch. it is ok on nodeB. the position of file is update to date.

But I still not get a direct viewing. That is to say the currently incorrect behavior as well as the correct result with this fix up. This would be useful not only for the patch reviewer but also can be proved that your fix is really works as expected.

That is not hard to wrap up the test code and results into the patch commit log, e.g,

On nodeA:
---------
1) Record the test file size
2) Append the test file && close it

On nodeB:
---------
1) Open/seek to the end of the test file and record/verify the result.

You can write the test code in any language that you preferred. e.g, use python to verify the SEEK_END result with this patch:

$ python -c "import os;f=open('test_file_path_name', 'r');f.seek(0, 2); \
             fsize=f.tell();print fsize"
Quoted 11 lines
---
 fs/ocfs2/file.c |    9 +++++++++
 1 file changed, 9 insertions(+)
fs/ocfs2/file.c +0 -0
index ff54014..f2570ba 100644

fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) · line 2630

Quoted 5 lines
fs/ocfs2/file.c +2 -0
@@ -2626,7 +2626,16 @@ static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence)
 	case SEEK_SET:
 		break;
 	case SEEK_END:
+		/* SEEK_END requires the OCFS2 inode lock because
+		 * it uses the inode size.

The comments is wrong. In general, the inode size means that how many bytes is used to store an inode in file system blocks.

I'd like to suggest the comments below:

		/* SEEK_END requires the OCFS2 inode lock for the
		 * file because it references the file's size.
		 */

Hi Mark, what's your opinion?

fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) · line 2631

Quoted 1 line
fs/ocfs2/file.c +1 -0
+		*/

^^ <-- No alignment to above lines.

fs/ocfs2/file.c static loff_t ocfs2_file_llseek(struct file *file, loff_t offset, int whence) · line 2641

Quoted 10 lines
fs/ocfs2/file.c +6 -0
+		ret = ocfs2_inode_lock(inode, NULL, 0);
+		if (ret < 0) {
+			mlog_errno(ret);
+			goto out;
+		}
 		offset += inode->i_size;
+		ocfs2_inode_unlock(inode, 0);
 		break;
 	case SEEK_CUR:
 		if (offset == 0) {

Thanks,
-Jeff