ocfs2: Flush drive's caches on fdatasync

PATCH Superseded Jan Kara 3 participants 6 messages Jul 29, 2010 – Aug 2, 2010 OCFS2

Share & tools

Jan Kara

author

2010-07-29 12:00 UTC #0 lore raw

PATCH 1/2 ocfs2: Flush drive's caches on fdatasync

We have to issue a cache flush during fdatasync even if inode doesn't have I_DIRTY_DATASYNC set because we still have to get written *data* to disk to observe fdatasync() guarantees.

Signed-off-by Jan Kara <[email protected]>

---
 fs/ocfs2/file.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

fs/ocfs2/file.c +9 -1

index 2b10b36..5659d85 100644
@@ -36,6 +36,7 @@
 #include <linux/writeback.h>
 #include <linux/falloc.h>
 #include <linux/quotaops.h>
+#include <linux/blkdev.h>
 
 #define MLOG_MASK_PREFIX ML_INODE
 #include <cluster/masklog.h>

@@ -190,8 +191,15 @@ static int ocfs2_sync_file(struct file *file, int datasync)
 	if (err)
 		goto bail;
 
-	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
+	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC)) {
+		/*
+		 * Although inode doesn't need writing, we still have to flush
+		 * drive's caches to get data to the platter
+		 */
+		blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL,
+				   BLKDEV_IFL_WAIT);
 		goto bail;
+	}
 
 	journal = osb->journal->j_journal;
 	err = jbd2_journal_force_commit(journal);

Signature · 1 line

1.6.4.2

Jan Kara

author

2010-07-29 12:00 UTC #1 lore raw

PATCH 2/2 ocfs2: Remove ocfs2_sync_inode()

ocfs2_sync_inode() is used only from ocfs2_sync_file(). But all data has already been written before calling ocfs2_sync_file() and ocfs2 doesn't use inode's private_list for tracking metadata buffers thus sync_mapping_buffers() is superfluous as well.

Signed-off-by Jan Kara <[email protected]>

---
 fs/ocfs2/file.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

fs/ocfs2/file.c +0 ~~-10~~

index 5659d85..2e341a2 100644
@@ -64,12 +64,6 @@
 
 #include "buffer_head_io.h"
 
-static int ocfs2_sync_inode(struct inode *inode)
-{
-	filemap_fdatawrite(inode->i_mapping);
-	return sync_mapping_buffers(inode->i_mapping);
-}
-
 static int ocfs2_init_file_private(struct inode *inode, struct file *file)
 {
 	struct ocfs2_file_private *fp;

@@ -187,10 +181,6 @@ static int ocfs2_sync_file(struct file *file, int datasync)
 	mlog_entry("(0x%p, 0x%p, %d, '%.*s')\n", file, dentry, datasync,
 		   dentry->d_name.len, dentry->d_name.name);
 
-	err = ocfs2_sync_inode(dentry->d_inode);
-	if (err)
-		goto bail;
-
 	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC)) {
 		/*
 		 * We still have to flush drive's caches to get data to the

Signature · 1 line

1.6.4.2

Tao Ma

2010-07-30 02:07 UTC #2 lore raw

PATCH 1/2 ocfs2: Flush drive's caches on fdatasync

In reply to Jan Kara

Hi Jan,

Quoted 3 lines · Jan Kara

We have to issue a cache flush during fdatasync even if inode doesn't have I_DIRTY_DATASYNC set because we still have to get written *data* to disk to observe fdatasync() guarantees.

I am fine with the patch from the code's perspective.

But I just noticed the discussion in fsdevel with the subject "relaxed barrier semantics", so with barrier there will be a massive slowdowns according to Christoph. And as ocfs2 is mainly used with some SAN, I guess in most cases the storage will have a battery backed cache, so we may not need this?

Sunil, Joel and Mark, Did you have any user data that most of the ocfs2 system is used on or can we start a survey in ocfs2-users?

Regards,
Tao

Quoted 34 lines

Signed-off-by Jan Kara<[email protected]>

---
  fs/ocfs2/file.c |   10 +++++++++-
  1 files changed, 9 insertions(+), 1 deletions(-)

fs/ocfs2/file.c +9 -1

index 2b10b36..5659d85 100644
@@ -36,6 +36,7 @@
  #include<linux/writeback.h>
  #include<linux/falloc.h>
  #include<linux/quotaops.h>
+#include<linux/blkdev.h>
 
  #define MLOG_MASK_PREFIX ML_INODE
  #include<cluster/masklog.h>

@@ -190,8 +191,15 @@ static int ocfs2_sync_file(struct file *file, int datasync)
  	if (err)
  		goto bail;
 
-	if (datasync&&  !(inode->i_state&  I_DIRTY_DATASYNC))
+	if (datasync&&  !(inode->i_state&  I_DIRTY_DATASYNC)) {
+		/*
+		 * Although inode doesn't need writing, we still have to flush
+		 * drive's caches to get data to the platter
+		 */
+		blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL,
+				   BLKDEV_IFL_WAIT);
  		goto bail;
+	}
 
  	journal = osb->journal->j_journal;
  	err = jbd2_journal_force_commit(journal);

Sunil Mushran

2010-07-30 02:12 UTC #3 lore raw

PATCH 1/2 ocfs2: Flush drive's caches on fdatasync

Quoted 10 lines · Tao Ma

Hi Jan,
Quoted 5 lines · Jan Kara
We have to issue a cache flush during fdatasync even if inode doesn't have
I_DIRTY_DATASYNC set because we still have to get written *data* to disk to
observe fdatasync() guarantees.
I am fine with the patch from the code's perspective.

Quoted 8 lines

But I just noticed the discussion in fsdevel with the subject "relaxed barrier semantics", so with barrier there will be a massive slowdowns according to Christoph. And as ocfs2 is mainly used with some SAN, I guess in most cases the storage will have a battery backed cache, so we may not need this?
Sunil, Joel and Mark, Did you have any user data that most of the ocfs2 system is used on or can we start a survey in ocfs2-users?

A SAN with a battery backed cache is a safe assumption. That's why we don't enable barrier by default.

Jan Kara

author

2010-07-30 14:47 UTC #4 lore raw

PATCH 1/2 ocfs2: Flush drive's caches on fdatasync

In reply to Tao Ma

Quoted 6 lines · Tao Ma

Hi Jan,
Quoted 3 lines · Jan Kara
We have to issue a cache flush during fdatasync even if inode doesn't have I_DIRTY_DATASYNC set because we still have to get written *data* to disk to observe fdatasync() guarantees.

Quoted 7 lines

I am fine with the patch from the code's perspective.
But I just noticed the discussion in fsdevel with the subject "relaxed barrier semantics", so with barrier there will be a massive slowdowns according to Christoph. And as ocfs2 is mainly used with some SAN, I guess in most cases the storage will have a battery backed cache, so we may not need this?

Yes, barriers are rather heavy operation and battery backed SANs don't
need them. In this regard, you are right my patch forgot to check the OCFS2_MOUNT_BARRIER option. But when the option is set, we must send the barrier. Otherwise we risk user's data on crash... Below is a fixed version of the patch.

Honza
---
From 61c46d394bdc9915554b499f7c3154bde13f3ee5 Mon Sep 17 00:00:00 2001
From: Jan Kara <[email protected]>
Date: Thu, 29 Jul 2010 13:32:19 +0200
Subject: [PATCH 1/2] ocfs2: Flush drive's caches on fdatasync

We have to issue a cache flush during fdatasync even if inode doesn't have I_DIRTY_DATASYNC set because we still have to get written *data* to disk to observe fdatasync() guarantees.

Signed-off-by Jan Kara <[email protected]>

---
 fs/ocfs2/file.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

fs/ocfs2/file.c +10 -1

index 2b10b36..3ed8efd 100644
@@ -36,6 +36,7 @@
 #include <linux/writeback.h>
 #include <linux/falloc.h>
 #include <linux/quotaops.h>
+#include <linux/blkdev.h>
 
 #define MLOG_MASK_PREFIX ML_INODE
 #include <cluster/masklog.h>

@@ -190,8 +191,16 @@ static int ocfs2_sync_file(struct file *file, int datasync)
 	if (err)
 		goto bail;
 
-	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
+	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC)) {
+		/*
+		 * We still have to flush drive's caches to get data to the
+		 * platter
+		 */
+		if (osb->s_mount_opt & OCFS2_MOUNT_BARRIER)
+			blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL,
+					   NULL, BLKDEV_IFL_WAIT);
 		goto bail;
+	}
 
 	journal = osb->journal->j_journal;
 	err = jbd2_journal_force_commit(journal);

Signature · 1 line

1.6.4.2

Tao Ma

2010-08-02 05:47 UTC #5 lore raw

PATCH 1/2 ocfs2: Flush drive's caches on fdatasync

Quoted 14 lines · Jan Kara

Quoted 6 lines · Tao Ma
Hi Jan,
Quoted 3 lines · Jan Kara
We have to issue a cache flush during fdatasync even if inode doesn't have I_DIRTY_DATASYNC set because we still have to get written *data* to disk to observe fdatasync() guarantees.
Quoted 7 lines
I am fine with the patch from the code's perspective.
But I just noticed the discussion in fsdevel with the subject "relaxed barrier semantics", so with barrier there will be a massive slowdowns according to Christoph. And as ocfs2 is mainly used with some SAN, I guess in most cases the storage will have a battery backed cache, so we may not need this?

Quoted 5 lines

Yes, barriers are rather heavy operation and battery backed SANs don't
need them. In this regard, you are right my patch forgot to check the OCFS2_MOUNT_BARRIER option. But when the option is set, we must send the barrier. Otherwise we risk user's data on crash... Below is a fixed version of the patch.

oh, yes, so ack.

Regards,
Tao

Quoted 47 lines

Honza
---
From 61c46d394bdc9915554b499f7c3154bde13f3ee5 Mon Sep 17 00:00:00 2001
From: Jan Kara<[email protected]>
Date: Thu, 29 Jul 2010 13:32:19 +0200
Subject: [PATCH 1/2] ocfs2: Flush drive's caches on fdatasync

We have to issue a cache flush during fdatasync even if inode doesn't have I_DIRTY_DATASYNC set because we still have to get written *data* to disk to observe fdatasync() guarantees.

Signed-off-by Jan Kara<[email protected]>

---
  fs/ocfs2/file.c |   11 ++++++++++-
  1 files changed, 10 insertions(+), 1 deletions(-)

fs/ocfs2/file.c +10 -1

index 2b10b36..3ed8efd 100644
@@ -36,6 +36,7 @@
  #include<linux/writeback.h>
  #include<linux/falloc.h>
  #include<linux/quotaops.h>
+#include<linux/blkdev.h>
 
  #define MLOG_MASK_PREFIX ML_INODE
  #include<cluster/masklog.h>

@@ -190,8 +191,16 @@ static int ocfs2_sync_file(struct file *file, int datasync)
  	if (err)
  		goto bail;
 
-	if (datasync&&  !(inode->i_state&  I_DIRTY_DATASYNC))
+	if (datasync&&  !(inode->i_state&  I_DIRTY_DATASYNC)) {
+		/*
+		 * We still have to flush drive's caches to get data to the
+		 * platter
+		 */
+		if (osb->s_mount_opt&  OCFS2_MOUNT_BARRIER)
+			blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL,
+					   NULL, BLKDEV_IFL_WAIT);
  		goto bail;
+	}
 
  	journal = osb->journal->j_journal;
  	err = jbd2_journal_force_commit(journal);