ocfs2: fix ocfs2_sync_file() if filesystem is readonly

PATCH New akpm at linux-foundation.org 4 participants 8 messages Jan 24, 2014 – Jan 27, 2014 OCFS2

lore

Share & tools

lore

akpm at linux-foundation.org

author

2014-01-24 20:46 UTC #0 lore raw

PATCH 1/11 ocfs2: fix ocfs2_sync_file() if filesystem is readonly

From: Younger Liu <[email protected]>
Subject: ocfs2: fix ocfs2_sync_file() if filesystem is readonly

If filesystem is readonly, there is no need to flush drive's caches or force any uncommitted transactions.

Signed-off-by Younger Liu <[email protected]>

Cc Joel Becker <[email protected]>

Cc Mark Fasheh <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 fs/ocfs2/file.c |    3 +++
 1 file changed, 3 insertions(+)

fs/ocfs2/file.c renamed from fs/ocfs2/file.c~ocfs2-fix-ocfs2_sync_file-if-filesystem-is-readonly +3 -0

diff -puN fs/ocfs2/file.c~ocfs2-fix-ocfs2_sync_file-if-filesystem-is-readonly fs/ocfs2/file.c
@@ -185,6 +185,9 @@ static int ocfs2_sync_file(struct file *
 			      file->f_path.dentry->d_name.name,
 			      (unsigned long long)datasync);
 
+	if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb))
+		return 0;
+
 	err = filemap_write_and_wait_range(inode->i_mapping, start, end);
 	if (err)
 		return err;

Goldwyn Rodrigues

NAK

2014-01-24 22:02 UTC #1 lore raw

PATCH 1/11 ocfs2: fix ocfs2_sync_file() if filesystem is readonly

Quoted 5 lines · akpm at linux-foundation.org

From: Younger Liu <[email protected]>
Subject: ocfs2: fix ocfs2_sync_file() if filesystem is readonly
If filesystem is readonly, there is no need to flush drive's caches or force any uncommitted transactions.

An ocfs2 filesystem can be set to read-only because of an error, in which case, you should return -EROFS.

Nak.

Signature · 1 line

Goldwyn

Mark Fasheh

2014-01-24 22:21 UTC #2 lore raw

PATCH 1/11 ocfs2: fix ocfs2_sync_file() if filesystem is readonly

Quoted 6 lines · Goldwyn Rodrigues

Quoted 3 lines · akpm at linux-foundation.org
From: Younger Liu <[email protected]>
Subject: ocfs2: fix ocfs2_sync_file() if filesystem is readonly

Quoted 7 lines

Quoted 2 lines
If filesystem is readonly, there is no need to flush drive's caches or force any uncommitted transactions.
An ocfs2 filesystem can be set to read-only because of an error, in which case, you should return -EROFS.
Nak.

Goldwyn's right actually - disregard my sign off for the last one.

Basically the patch does this:

if (we're in some readonly state)
return 0;

What we want, at the top of ocfs2_sync_file() is a return of -EROFS. This will satisfy Goldwyn's requirement that we bubble -EROFS up the stack but at the same time avoiding the extra work of trying to sync on a RO fs.

So the new version of the patch would be:

if (we're in some readonly state)
return -EROFS;

Thanks,

	--Mark

--
Mark Fasheh

Andrew Morton

author

2014-01-24 22:32 UTC #3 lore raw

PATCH 1/11 ocfs2: fix ocfs2_sync_file() if filesystem is readonly

Quoted 22 lines · On Fri, 24 Jan 2014 14:21:19 -0800 Mark Fasheh <[email protected]> wrote:

Quoted 6 lines · Goldwyn Rodrigues
Quoted 3 lines · akpm at linux-foundation.org
From: Younger Liu <[email protected]>
Subject: ocfs2: fix ocfs2_sync_file() if filesystem is readonly
Quoted 7 lines
Quoted 2 lines
If filesystem is readonly, there is no need to flush drive's caches or force any uncommitted transactions.
An ocfs2 filesystem can be set to read-only because of an error, in which case, you should return -EROFS.
Nak.
Goldwyn's right actually - disregard my sign off for the last one.
Basically the patch does this:
if (we're in some readonly state)
return 0;

Quoted 9 lines

What we want, at the top of ocfs2_sync_file() is a return of -EROFS. This will satisfy Goldwyn's requirement that we bubble -EROFS up the stack but at the same time avoiding the extra work of trying to sync on a RO fs.
So the new version of the patch would be:
if (we're in some readonly state)
return -EROFS;

So it's this?

fs/ocfs2/file.c renamed from fs/ocfs2/file.c~ocfs2-fix-ocfs2_sync_file-if-filesystem-is-readonly +3 -0

@@ -185,6 +185,9 @@ static int ocfs2_sync_file(struct file *
 			      file->f_path.dentry->d_name.name,
 			      (unsigned long long)datasync);
 
+	if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb))
+		return -EROFS;
+
 	err = filemap_write_and_wait_range(inode->i_mapping, start, end);
 	if (err)
 		return err;

Younger Liu

2014-01-26 00:51 UTC #4 lore raw

PATCH 1/11 ocfs2: fix ocfs2_sync_file() if filesystem is readonly

Quoted 39 lines · Andrew Morton

Quoted 22 lines · On Fri, 24 Jan 2014 14:21:19 -0800 Mark Fasheh <[email protected]> wrote:
Quoted 6 lines · Goldwyn Rodrigues
Quoted 3 lines · akpm at linux-foundation.org
From: Younger Liu <[email protected]>
Subject: ocfs2: fix ocfs2_sync_file() if filesystem is readonly
Quoted 7 lines
Quoted 2 lines
If filesystem is readonly, there is no need to flush drive's caches or force any uncommitted transactions.
An ocfs2 filesystem can be set to read-only because of an error, in which case, you should return -EROFS.
Nak.
Goldwyn's right actually - disregard my sign off for the last one.
Basically the patch does this:
if (we're in some readonly state)
return 0;
Quoted 9 lines
What we want, at the top of ocfs2_sync_file() is a return of -EROFS. This will satisfy Goldwyn's requirement that we bubble -EROFS up the stack but at the same time avoiding the extra work of trying to sync on a RO fs.
So the new version of the patch would be:
if (we're in some readonly state)
return -EROFS;
So it's this?

fs/ocfs2/file.c static int ocfs2_sync_file(struct file * · line 190

Quoted 6 lines

fs/ocfs2/file.c renamed from fs/ocfs2/file.c~ocfs2-fix-ocfs2_sync_file-if-filesystem-is-readonly +3 -0

@@ -185,6 +185,9 @@ static int ocfs2_sync_file(struct file *
 			      file->f_path.dentry->d_name.name,
 			      (unsigned long long)datasync);
 
+	if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb))
+		return -EROFS;
+

fine, -EROFS shows the status of filesystem.

--Younger

Quoted 6 lines

fs/ocfs2/file.c renamed from fs/ocfs2/file.c~ocfs2-fix-ocfs2_sync_file-if-filesystem-is-readonly +0 -0
 	err = filemap_write_and_wait_range(inode->i_mapping, start, end);
 	if (err)
 		return err;
_

Goldwyn Rodrigues

2014-01-26 01:18 UTC #5 lore raw

PATCH 1/11 ocfs2: fix ocfs2_sync_file() if filesystem is readonly

In reply to Andrew Morton

Quoted 28 lines · Andrew Morton

Quoted 26 lines · On Fri, 24 Jan 2014 14:21:19 -0800 Mark Fasheh <[email protected]> wrote:
Quoted 6 lines · Goldwyn Rodrigues
Quoted 3 lines · akpm at linux-foundation.org
From: Younger Liu <[email protected]>
Subject: ocfs2: fix ocfs2_sync_file() if filesystem is readonly
Quoted 7 lines
Quoted 2 lines
If filesystem is readonly, there is no need to flush drive's caches or force any uncommitted transactions.
An ocfs2 filesystem can be set to read-only because of an error, in which case, you should return -EROFS.
Nak.
Goldwyn's right actually - disregard my sign off for the last one.
Basically the patch does this:
if (we're in some readonly state)
return 0;
What we want, at the top of ocfs2_sync_file() is a return of -EROFS. This will satisfy Goldwyn's requirement that we bubble -EROFS up the stack but at the same time avoiding the extra work of trying to sync on a RO fs.

Quoted 8 lines

Quoted 5 lines
So the new version of the patch would be:
if (we're in some readonly state)
return -EROFS;
So it's this?

Yes.

Acked-by Goldwyn Rodrigues <[email protected]>

Quoted 14 lines

fs/ocfs2/file.c renamed from fs/ocfs2/file.c~ocfs2-fix-ocfs2_sync_file-if-filesystem-is-readonly +3 -0

@@ -185,6 +185,9 @@ static int ocfs2_sync_file(struct file *
  			      file->f_path.dentry->d_name.name,
  			      (unsigned long long)datasync);
 
+	if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb))
+		return -EROFS;
+
  	err = filemap_write_and_wait_range(inode->i_mapping, start, end);
  	if (err)
  		return err;

Signature · 1 line

Goldwyn

Mark Fasheh

2014-01-27 17:22 UTC #6 lore raw

PATCH 1/11 ocfs2: fix ocfs2_sync_file() if filesystem is readonly

In reply to Andrew Morton

Quoted 28 lines · Andrew Morton

Quoted 26 lines · On Fri, 24 Jan 2014 14:21:19 -0800 Mark Fasheh <[email protected]> wrote:
Quoted 6 lines · Goldwyn Rodrigues
Quoted 3 lines · akpm at linux-foundation.org
From: Younger Liu <[email protected]>
Subject: ocfs2: fix ocfs2_sync_file() if filesystem is readonly
Quoted 7 lines
Quoted 2 lines
If filesystem is readonly, there is no need to flush drive's caches or force any uncommitted transactions.
An ocfs2 filesystem can be set to read-only because of an error, in which case, you should return -EROFS.
Nak.
Goldwyn's right actually - disregard my sign off for the last one.
Basically the patch does this:
if (we're in some readonly state)
return 0;
What we want, at the top of ocfs2_sync_file() is a return of -EROFS. This will satisfy Goldwyn's requirement that we bubble -EROFS up the stack but at the same time avoiding the extra work of trying to sync on a RO fs.

Quoted 7 lines

Quoted 5 lines
So the new version of the patch would be:
if (we're in some readonly state)
return -EROFS;
So it's this?

Yes, that's exactly what I was thinking the patch should look like. If you want to change it and include my signoff that's fine with me. Thanks,

	--Mark

Quoted 15 lines

fs/ocfs2/file.c renamed from fs/ocfs2/file.c~ocfs2-fix-ocfs2_sync_file-if-filesystem-is-readonly +3 -0

@@ -185,6 +185,9 @@ static int ocfs2_sync_file(struct file *
 			      file->f_path.dentry->d_name.name,
 			      (unsigned long long)datasync);
 
+	if (ocfs2_is_hard_readonly(osb) || ocfs2_is_soft_readonly(osb))
+		return -EROFS;
+
 	err = filemap_write_and_wait_range(inode->i_mapping, start, end);
 	if (err)
 		return err;

Quoted 4 lines

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

--
Mark Fasheh

Mark Fasheh

2014-01-24 22:02 UTC #7 lore raw

PATCH 1/11 ocfs2: fix ocfs2_sync_file() if filesystem is readonly

In reply to akpm at linux-foundation.org

Quoted 10 lines · akpm at linux-foundation.org

From: Younger Liu <[email protected]>
Subject: ocfs2: fix ocfs2_sync_file() if filesystem is readonly
If filesystem is readonly, there is no need to flush drive's caches or force any uncommitted transactions.
Signed-off-by Younger Liu <[email protected]>
Cc Joel Becker <[email protected]>
Cc Mark Fasheh <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

Looks good, thanks for this.

Signed-off-by Mark Fasheh <[email protected]>

	--Mark

--
Mark Fasheh