Remove filesize checks for sync I/O journal commit

PATCH New Goldwyn Rodrigues 4 participants 5 messages Nov 4, 2014 – Dec 3, 2014 OCFS2

Share & tools

Goldwyn Rodrigues

author

2014-11-04 15:52 UTC #0 lore raw

PATCH 1/1 Remove filesize checks for sync I/O journal commit

Filesize is not a good indication that the file needs to be synced. An example where this breaks is:
1. Open the file in O_SYNC|O_RDWR
2. Read a small portion of the file (say 64 bytes)
3. Lseek to starting of the file
4. Write 64 bytes

If the node crashes, it is not written out to disk because this was not committed in the journal and the other node which reads the file after recovery reads stale data (even if the write on the other node was successful)

---

fs/ocfs2/file.c +1 -3

index 324dc93..69fb9f7 100644
@@ -2381,9 +2381,7 @@ out_dio:
 		if (ret < 0)
 			written = ret;
 
-		if (!ret && ((old_size != i_size_read(inode)) ||
-			     (old_clusters != OCFS2_I(inode)->ip_clusters) ||
-			     has_refcount)) {
+		if (!ret) {
 			ret = jbd2_journal_force_commit(osb->journal->j_journal);
 			if (ret < 0)
 				written = ret;

Andrew Morton

2014-11-04 22:45 UTC #1 lore raw

PATCH 1/1 Remove filesize checks for sync I/O journal commit

Quoted 17 lines · On Tue, 4 Nov 2014 09:52:22 -0600 Goldwyn Rodrigues <[email protected]> wrote:

Filesize is not a good indication that the file needs to be synced. An example where this breaks is:
1. Open the file in O_SYNC|O_RDWR
2. Read a small portion of the file (say 64 bytes)
3. Lseek to starting of the file
4. Write 64 bytes
If the node crashes, it is not written out to disk because this was not committed in the journal and the other node which reads the file after recovery reads stale data (even if the write on the other node was successful)
...

fs/ocfs2/file.c out_dio: · line 2387

Quoted 10 lines

fs/ocfs2/file.c +1 -3

@@ -2381,9 +2381,7 @@ out_dio:
 		if (ret < 0)
 			written = ret;
 
-		if (!ret && ((old_size != i_size_read(inode)) ||
-			     (old_clusters != OCFS2_I(inode)->ip_clusters) ||
-			     has_refcount)) {
+		if (!ret) {
 			ret = jbd2_journal_force_commit(osb->journal->j_journal);
 			if (ret < 0)
 				written = ret;

Can we have a signed-off-by for this, please?

Goldwyn Rodrigues

author

2014-11-05 15:26 UTC #2 lore raw

PATCH 1/1 Remove filesize checks for sync I/O journal commit

Quoted 22 lines · Andrew Morton

Quoted 20 lines · On Tue, 4 Nov 2014 09:52:22 -0600 Goldwyn Rodrigues <[email protected]> wrote:
Filesize is not a good indication that the file needs to be synced. An example where this breaks is:
1. Open the file in O_SYNC|O_RDWR
2. Read a small portion of the file (say 64 bytes)
3. Lseek to starting of the file
4. Write 64 bytes
If the node crashes, it is not written out to disk because this was not committed in the journal and the other node which reads the file after recovery reads stale data (even if the write on the other node was successful)
...
fs/ocfs2/file.c +0 -0
@@ -2381,9 +2381,7 @@ out_dio:
  		if (ret < 0)
  			written = ret;
 

Quoted 10 lines

fs/ocfs2/file.c out_dio: · line 2387

Quoted 7 lines

fs/ocfs2/file.c +1 -3

-		if (!ret && ((old_size != i_size_read(inode)) ||
-			     (old_clusters != OCFS2_I(inode)->ip_clusters) ||
-			     has_refcount)) {
+		if (!ret) {
  			ret = jbd2_journal_force_commit(osb->journal->j_journal);
  			if (ret < 0)
  				written = ret;

Can we have a signed-off-by for this, please?

Oops. Missed that.

Signed-off-by Goldwyn Rodrigues <[email protected]>

Signature · 1 line

Goldwyn

Mark Fasheh

2014-11-05 21:18 UTC #3 lore raw

PATCH 1/1 Remove filesize checks for sync I/O journal commit

In reply to Goldwyn Rodrigues

Quoted 11 lines · Goldwyn Rodrigues

Filesize is not a good indication that the file needs to be synced. An example where this breaks is:
1. Open the file in O_SYNC|O_RDWR
2. Read a small portion of the file (say 64 bytes)
3. Lseek to starting of the file
4. Write 64 bytes
If the node crashes, it is not written out to disk because this was not committed in the journal and the other node which reads the file after recovery reads stale data (even if the write on the other node was successful)

This patch looks good, thanks for sending it over.

Reviewed-by Mark Fasheh <[email protected]>

	--Mark

--
Mark Fasheh

Xue jiufei

2014-12-03 08:20 UTC #4 lore raw

PATCH 1/1 Remove filesize checks for sync I/O journal commit

In reply to Goldwyn Rodrigues

Hi Goldwyn,

Quoted 7 lines · Goldwyn Rodrigues

Filesize is not a good indication that the file needs to be synced. An example where this breaks is:
1. Open the file in O_SYNC|O_RDWR
2. Read a small portion of the file (say 64 bytes)
3. Lseek to starting of the file
4. Write 64 bytes

Quoted 5 lines

If the node crashes, it is not written out to disk because this was not committed in the journal and the other node which reads the file after recovery reads stale data (even if the write on the other node was successful)

I have a question that if user open the file with O_SYNC, it will call filemap_fdatawait_range() after generic_perform_write() to wait until data reaches the disk, why another node may read the stale data?

Thanks,
Xuejiufei

Quoted 22 lines

---

fs/ocfs2/file.c +1 -3

index 324dc93..69fb9f7 100644
@@ -2381,9 +2381,7 @@ out_dio:
 		if (ret < 0)
 			written = ret;
 
-		if (!ret && ((old_size != i_size_read(inode)) ||
-			     (old_clusters != OCFS2_I(inode)->ip_clusters) ||
-			     has_refcount)) {
+		if (!ret) {
 			ret = jbd2_journal_force_commit(osb->journal->j_journal);
 			if (ret < 0)
 				written = ret;

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel