ocfs2: bugfix for hard readonly mount

PATCH New Tiger Yang 3 participants 14 messages May 26, 2011 – Jun 3, 2011 OCFS2

lore

Share & tools

lore

Tiger Yang

author

2011-05-26 09:40 UTC #0 lore raw

cover ocfs2: bugfix for hard readonly mount

Hi, All,

These four patches are all related to ocfs2 on hard readonly mount. patch 1 fix oops when umount ocfs2 on hard readonly device. Because ocfs2_dismount_volume() will call ocfs2_cluster_hangup() and then call ocfs2_stack_driver_put(), will hit BUG_ON(active_stack == NULL).

patch 2 fix oops when do ls or cat in ocfs2 on hard readonly device. Because ocfs2_open_lock() will call ocfs2_cluster_lock() and then call ocfs2_dlm_lock(), but active_stack is NULL.

patch 3 fix bug of http://oss.oracle.com/bugzilla/show_bug.cgi?id=1322
ocfs2_statfs, ocfs2_fiemap, ocfs2_get_acl, ocfs2_listxattr and ocfs2_xattr_get need ocfs2_inode_lock to get dinode buffer head, but on hard readonly mount, they did not get they expected, so lead to oops. ocfs2_init_security_and_acl and ocfs2_test_inode_bit also called ocfs2_inode_lock(inode, &bh, 0), but they are safe, because ocfs2_reflink and ocfs2_get_dentry check hard read only before them.

patch 4 fix problem of running ls on hard readonly mount can't get the right result.
Because ocfs2_dentry_lock() return -EROFS. It should return 0.

best regards,
Tiger

Tiger Yang

author

2011-05-26 09:56 UTC #1 lore raw

PATCH 1/4 ocfs2: No need to hangup cluster on hard readonly mount

As no cluster service in hard readonly mount, so skip hangup cluster, otherwise ocfs2_stack_driver_put will hit BUG_ON.

Signed-off-by Tiger Yang <[email protected]>

---
 fs/ocfs2/super.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

fs/ocfs2/super.c +2 -1

index 5a521c7..395b26f 100644
@@ -1973,7 +1973,8 @@ static void ocfs2_dismount_volume(struct super_block *sb, int mnt_err)
 	 * If we failed before we got a uuid_str yet, we can't stop
 	 * heartbeat.  Otherwise, do it.
 	 */
-	if (!mnt_err && !ocfs2_mount_local(osb) && osb->uuid_str)
+	if (!mnt_err && !ocfs2_mount_local(osb) && osb->uuid_str &&
+	    !ocfs2_is_hard_readonly(osb))
 		hangup_needed = 1;
 
 	if (osb->cconn)

Signature · 1 line

1.7.4.4

Tiger Yang

author

2011-05-26 09:57 UTC #2 lore raw

PATCH 2/4 ocfs2: Add hard readonly check in open lock

In reply to Tiger Yang

As no dlm lock in hard readonly mount, so add hard readonly check in open lock, otherwise ocfs2_cluster_lock leads to oops.

Signed-off-by Tiger Yang <[email protected]>

---
 fs/ocfs2/dlmglue.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

fs/ocfs2/dlmglue.c +7 -1

index 7642d7c..09e8b30 100644
@@ -1692,7 +1692,7 @@ int ocfs2_open_lock(struct inode *inode)
 	mlog(0, "inode %llu take PRMODE open lock\n",
 	     (unsigned long long)OCFS2_I(inode)->ip_blkno);
 
-	if (ocfs2_mount_local(osb))
+	if (ocfs2_is_hard_readonly(osb) || ocfs2_mount_local(osb))
 		goto out;
 
 	lockres = &OCFS2_I(inode)->ip_open_lockres;

@@ -1718,6 +1718,12 @@ int ocfs2_try_open_lock(struct inode *inode, int write)
 	     (unsigned long long)OCFS2_I(inode)->ip_blkno,
 	     write ? "EXMODE" : "PRMODE");
 
+	if (ocfs2_is_hard_readonly(osb)) {
+		if (write)
+			status = -EROFS;
+		goto out;
+	}
+
 	if (ocfs2_mount_local(osb))
 		goto out;

Signature · 1 line

1.7.4.4

Tiger Yang

author

2011-05-26 09:57 UTC #3 lore raw

PATCH 3/4 ocfs2: Get dinode buffer head on hard readonly mount

In reply to Tiger Yang

ocfs2_statfs, ocfs2_fiemap, ocfs2_get_acl, ocfs2_listxattr and ocfs2_xattr_get need ocfs2_inode_lock to get dinode buffer head even on hard readonly mount.

Signed-off-by Tiger Yang <[email protected]>

---
 fs/ocfs2/dlmglue.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

fs/ocfs2/dlmglue.c +2 -2

index 09e8b30..717f8d3 100644
@@ -2304,7 +2304,7 @@ int ocfs2_inode_lock_full_nested(struct inode *inode,
 	if (ocfs2_is_hard_readonly(osb)) {
 		if (ex)
 			status = -EROFS;
-		goto bail;
+		goto getbh;
 	}
 
 	if (ocfs2_mount_local(osb))

@@ -2362,7 +2362,7 @@ local:
 			mlog_errno(status);
 		goto bail;
 	}
-
+getbh:
 	if (ret_bh) {
 		status = ocfs2_assign_bh(inode, ret_bh, local_bh);
 		if (status < 0) {

Signature · 1 line

1.7.4.4

Tiger Yang

author

2011-05-26 09:58 UTC #4 lore raw

PATCH 4/4 ocfs2: Get readonly dentry lock on hard readonly mount

In reply to Tiger Yang

As some readonly operations such as ls, cat are allowed to run on hard readonly mount, so change return value to 0 to allow them to get fake readonly dentry lock.

Signed-off-by Tiger Yang <[email protected]>

---
 fs/ocfs2/dlmglue.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

fs/ocfs2/dlmglue.c +6 -3

index 717f8d3..da103f5 100644
@@ -2634,8 +2634,11 @@ int ocfs2_dentry_lock(struct dentry *dentry, int ex)
 
 	BUG_ON(!dl);
 
-	if (ocfs2_is_hard_readonly(osb))
-		return -EROFS;
+	if (ocfs2_is_hard_readonly(osb)) {
+		if (ex)
+			return -EROFS;
+		return 0;
+	}
 
 	if (ocfs2_mount_local(osb))
 		return 0;

@@ -2653,7 +2656,7 @@ void ocfs2_dentry_unlock(struct dentry *dentry, int ex)
 	struct ocfs2_dentry_lock *dl = dentry->d_fsdata;
 	struct ocfs2_super *osb = OCFS2_SB(dentry->d_sb);
 
-	if (!ocfs2_mount_local(osb))
+	if (!ocfs2_is_hard_readonly(osb) && !ocfs2_mount_local(osb))
 		ocfs2_cluster_unlock(osb, &dl->dl_lockres, level);
 }

Signature · 1 line

1.7.4.4

Sunil Mushran

2011-05-26 16:03 UTC #5 lore raw

cover ocfs2: bugfix for hard readonly mount

In reply to Tiger Yang

Quoted 18 lines · Tiger Yang

These four patches are all related to ocfs2 on hard readonly mount. patch 1 fix oops when umount ocfs2 on hard readonly device. Because ocfs2_dismount_volume() will call ocfs2_cluster_hangup() and then call ocfs2_stack_driver_put(), will hit BUG_ON(active_stack == NULL).
patch 2 fix oops when do ls or cat in ocfs2 on hard readonly device. Because ocfs2_open_lock() will call ocfs2_cluster_lock() and then call ocfs2_dlm_lock(), but active_stack is NULL.
patch 3 fix bug of http://oss.oracle.com/bugzilla/show_bug.cgi?id=1322
ocfs2_statfs, ocfs2_fiemap, ocfs2_get_acl, ocfs2_listxattr and ocfs2_xattr_get need ocfs2_inode_lock to get dinode buffer head, but on hard readonly mount, they did not get they expected, so lead to oops. ocfs2_init_security_and_acl and ocfs2_test_inode_bit also called ocfs2_inode_lock(inode, &bh, 0), but they are safe, because ocfs2_reflink and ocfs2_get_dentry check hard read only before them.

Quoted 3 lines

patch 4 fix problem of running ls on hard readonly mount can't get the right result.
Because ocfs2_dentry_lock() return -EROFS. It should return 0.

Tiger, Thanks. Was this tested with all fs-features enabled? I am worried about quotas. If not, please test that.

Tristan, Please can you cross check these patches. Enable all features in a volume, make it readonly and do mount, ls -lR, umount, read, stat, stat -f. This calls for a test script. We should be able to use loopback. losetup -rf.

Thanks
Sunil

Tristan Ye

2011-05-26 16:16 UTC #6 lore raw

cover ocfs2: bugfix for hard readonly mount

Quoted 23 lines · Sunil Mushran

Quoted 18 lines · Tiger Yang
These four patches are all related to ocfs2 on hard readonly mount. patch 1 fix oops when umount ocfs2 on hard readonly device. Because ocfs2_dismount_volume() will call ocfs2_cluster_hangup() and then call ocfs2_stack_driver_put(), will hit BUG_ON(active_stack == NULL).
patch 2 fix oops when do ls or cat in ocfs2 on hard readonly device. Because ocfs2_open_lock() will call ocfs2_cluster_lock() and then call ocfs2_dlm_lock(), but active_stack is NULL.
patch 3 fix bug of http://oss.oracle.com/bugzilla/show_bug.cgi?id=1322
ocfs2_statfs, ocfs2_fiemap, ocfs2_get_acl, ocfs2_listxattr and ocfs2_xattr_get need ocfs2_inode_lock to get dinode buffer head, but on hard readonly mount, they did not get they expected, so lead to oops. ocfs2_init_security_and_acl and ocfs2_test_inode_bit also called ocfs2_inode_lock(inode, &bh, 0), but they are safe, because ocfs2_reflink and ocfs2_get_dentry check hard read only before them.
Quoted 3 lines
patch 4 fix problem of running ls on hard readonly mount can't get the right result.
Because ocfs2_dentry_lock() return -EROFS. It should return 0.

Quoted 7 lines

Tiger, Thanks. Was this tested with all fs-features enabled? I am worried about quotas. If not, please test that.
Tristan, Please can you cross check these patches. Enable all features in a volume, make it readonly and do mount, ls -lR, umount, read, stat, stat -f. This calls for a test script. We should be able to use loopback. losetup -rf.

Sure,

I'm going to write a dedicated script for sanity check, including these generic fs syscalls.

Tristan

Quoted 3 lines

Thanks
Sunil

Tristan Ye

2011-05-26 16:21 UTC #13 lore raw

cover ocfs2: bugfix for hard readonly mount

In reply to Sunil Mushran

Quoted 23 lines · Sunil Mushran

Quoted 18 lines · Tiger Yang
These four patches are all related to ocfs2 on hard readonly mount. patch 1 fix oops when umount ocfs2 on hard readonly device. Because ocfs2_dismount_volume() will call ocfs2_cluster_hangup() and then call ocfs2_stack_driver_put(), will hit BUG_ON(active_stack == NULL).
patch 2 fix oops when do ls or cat in ocfs2 on hard readonly device. Because ocfs2_open_lock() will call ocfs2_cluster_lock() and then call ocfs2_dlm_lock(), but active_stack is NULL.
patch 3 fix bug of http://oss.oracle.com/bugzilla/show_bug.cgi?id=1322
ocfs2_statfs, ocfs2_fiemap, ocfs2_get_acl, ocfs2_listxattr and ocfs2_xattr_get need ocfs2_inode_lock to get dinode buffer head, but on hard readonly mount, they did not get they expected, so lead to oops. ocfs2_init_security_and_acl and ocfs2_test_inode_bit also called ocfs2_inode_lock(inode, &bh, 0), but they are safe, because ocfs2_reflink and ocfs2_get_dentry check hard read only before them.
Quoted 3 lines
patch 4 fix problem of running ls on hard readonly mount can't get the right result.
Because ocfs2_dentry_lock() return -EROFS. It should return 0.

Quoted 7 lines

Tiger, Thanks. Was this tested with all fs-features enabled? I am worried about quotas. If not, please test that.
Tristan, Please can you cross check these patches. Enable all features in a volume, make it readonly and do mount, ls -lR, umount, read, stat, stat -f. This calls for a test script. We should be able to use loopback. losetup -rf.

Sure,

I'm going to write a dedicated script for sanity check, including these generic fs syscalls.

Tristan

Quoted 3 lines

Thanks
Sunil

Sunil Mushran

2011-05-26 16:21 UTC #7 lore raw

cover ocfs2: bugfix for hard readonly mount

In reply to Tristan Ye

Quoted 9 lines · Tristan Ye

Quoted 7 lines · Sunil Mushran
Tiger, Thanks. Was this tested with all fs-features enabled? I am worried about quotas. If not, please test that.
Tristan, Please can you cross check these patches. Enable all features in a volume, make it readonly and do mount, ls -lR, umount, read, stat, stat -f. This calls for a test script. We should be able to use loopback. losetup -rf.

Quoted 4 lines

Sure,
I'm going to write a dedicated script for sanity check, including these generic fs syscalls.

Thanks Tristan.

Tiger, I would suggest you roll up the patch into one. I see no reason for the breakup considering it fixes just one bug and is small enough.

Tristan Ye

2011-06-02 03:09 UTC #8 lore raw

cover ocfs2: bugfix for hard readonly mount

Quoted 13 lines · Sunil Mushran

Quoted 12 lines · Tristan Ye
Quoted 8 lines · Sunil Mushran
Tiger, Thanks. Was this tested with all fs-features enabled? I am worried about quotas. If not, please test that.
Tristan, Please can you cross check these patches. Enable all features in a volume, make it readonly and do mount, ls -lR, umount, read, stat, stat -f. This calls for a test script. We should be able to use loopback.
losetup -rf.
Sure,

Quoted 7 lines

Quoted 2 lines
I'm going to write a dedicated script for sanity check, including these generic fs syscalls.
Thanks Tristan.
Tiger, I would suggest you roll up the patch into one. I see no reason for the breakup considering it fixes just one bug and is small enough.

After running tests on block/loopback device, it shows that:

1. 'ls -lR /kernel/tree' dumps slightly different output:

6c6
< total 436
---

Quoted 1 line

total 428

37c37
< total 52
---

Quoted 1 line

total 44

394c394
< total 252
---

Quoted 1 line

total 228

1408c1408
< total 8
---

Quoted 1 line

total 4

1636c1636
< total 8

while the data is consistent anyway.

2. 'stat -f' also dumps a slightly different map:

4,5c4,5
< Blocks: Total: 176120528 Free: 170080504 Available: 170080504
< Inodes: Total: 22015066 Free: 21260063
---

Quoted 2 lines

Blocks: Total: 176120528 Free: 170088304 Available: 170088304
Inodes: Total: 22015066 Free: 21261038

3. Enabling quota on readonly fs caused kernel oops

4. Otherwise, everything is fine(xattr,reflink,..etc)

Testing script attached.

Tristan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: verify_readonly.sh
Type: application/x-sh
Size: 15228 bytes
Desc: not available
Url : http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20110602/b891e97d/attachment-0001.sh

Sunil Mushran

2011-06-02 18:43 UTC #9 lore raw

cover ocfs2: bugfix for hard readonly mount

Tristan,

Please can you build a newer ocfs2-test rpm. The last one was built on 2009.

The stat -f discrepancy is probably due to localalloc. We shouldn't be reserving space for hard-ro mounts. So should be ok as long as the discrepancy can be accounted for.

# grep "LocalAlloc " /sys/kernel/debug/ocfs2/*/fs_state
The last column * clustersize is the space reserved.

I have no clue what the "total" value comprises off. Can you look into that?

Sunil

Quoted 64 lines · Tristan Ye

Quoted 10 lines · Sunil Mushran
Quoted 9 lines · Tristan Ye
Quoted 8 lines · Sunil Mushran
Tiger, Thanks. Was this tested with all fs-features enabled? I am worried about quotas. If not, please test that.
Tristan, Please can you cross check these patches. Enable all features in a volume, make it readonly and do mount, ls -lR, umount, read, stat, stat -f. This calls for a test script. We should be able to use loopback.
losetup -rf.
Quoted 8 lines
Quoted 4 lines
Sure,
I'm going to write a dedicated script for sanity check, including these generic fs syscalls.
Thanks Tristan.
Tiger, I would suggest you roll up the patch into one. I see no reason for the breakup considering it fixes just one bug and is small enough.
After running tests on block/loopback device, it shows that:
1. 'ls -lR /kernel/tree' dumps slightly different output:
6c6
< total 436
---
Quoted 1 line
total 428
37c37
< total 52
---
Quoted 1 line
total 44
394c394
< total 252
---
Quoted 1 line
total 228
1408c1408
< total 8
---
Quoted 1 line
total 4
1636c1636
< total 8
while the data is consistent anyway.
2. 'stat -f' also dumps a slightly different map:
4,5c4,5
<  Blocks: Total: 176120528  Free: 170080504  Available: 170080504
<  Inodes: Total: 22015066   Free: 21260063
---
Quoted 2 lines
Blocks: Total: 176120528 Free: 170088304 Available: 170088304
Inodes: Total: 22015066 Free: 21261038
3. Enabling quota on readonly fs caused kernel oops
4. Otherwise, everything is fine(xattr,reflink,..etc)
Testing script attached.
Tristan.

Tristan Ye

2011-06-03 01:00 UTC #10 lore raw

cover ocfs2: bugfix for hard readonly mount

Quoted 4 lines · Sunil Mushran

Tristan,
Please can you build a newer ocfs2-test rpm. The last one was built on 2009.

atop el6 with UEK kernel?

Quoted 11 lines

The stat -f discrepancy is probably due to localalloc. We shouldn't be reserving
space for hard-ro mounts. So should be ok as long as the discrepancy can be accounted for.
# grep "LocalAlloc " /sys/kernel/debug/ocfs2/*/fs_state
The last column * clustersize is the space reserved.
I have no clue what the "total" value comprises off. Can you look into that?

Sure

Quoted 70 lines

Sunil
Quoted 65 lines · Tristan Ye
Quoted 11 lines · Sunil Mushran
Quoted 10 lines · Tristan Ye
Quoted 9 lines · Sunil Mushran
Tiger, Thanks. Was this tested with all fs-features enabled? I am worried about quotas. If not, please test that.
Tristan, Please can you cross check these patches. Enable all features in a volume, make it readonly and do mount, ls -lR, umount, read, stat,
stat -f. This calls for a test script. We should be able to use loopback.
losetup -rf.
Quoted 8 lines
Quoted 4 lines
Sure,
I'm going to write a dedicated script for sanity check, including these generic fs syscalls.
Thanks Tristan.
Tiger, I would suggest you roll up the patch into one. I see no reason for the breakup considering it fixes just one bug and is small enough.
After running tests on block/loopback device, it shows that:
1. 'ls -lR /kernel/tree' dumps slightly different output:
6c6
< total 436
---
Quoted 1 line
total 428
37c37
< total 52
---
Quoted 1 line
total 44
394c394
< total 252
---
Quoted 1 line
total 228
1408c1408
< total 8
---
Quoted 1 line
total 4
1636c1636
< total 8
while the data is consistent anyway.
2. 'stat -f' also dumps a slightly different map:
4,5c4,5
<  Blocks: Total: 176120528  Free: 170080504  Available: 170080504
<  Inodes: Total: 22015066   Free: 21260063
---
Quoted 2 lines
Blocks: Total: 176120528 Free: 170088304 Available: 170088304
Inodes: Total: 22015066 Free: 21261038
3. Enabling quota on readonly fs caused kernel oops
4. Otherwise, everything is fine(xattr,reflink,..etc)
Testing script attached.
Tristan.

Sunil Mushran

2011-06-03 01:05 UTC #11 lore raw

cover ocfs2: bugfix for hard readonly mount

Quoted 6 lines · Tristan Ye

Quoted 4 lines · Sunil Mushran
Tristan,
Please can you build a newer ocfs2-test rpm. The last one was built on 2009.
atop el6 with UEK kernel?

It is userspace. So kernel should not matter. I guess both el5 and el6 would be nice.

Tristan Ye

2011-06-03 01:14 UTC #12 lore raw

cover ocfs2: bugfix for hard readonly mount

Quoted 10 lines · Sunil Mushran

Quoted 6 lines · Tristan Ye
Quoted 4 lines · Sunil Mushran
Tristan,
Please can you build a newer ocfs2-test rpm. The last one was built on 2009.
atop el6 with UEK kernel?
It is userspace. So kernel should not matter. I guess both el5 and el6 would be nice.

Yep,
But parts of its testcases were intentional for 1.6 ocfs2, and was
dependent on latest ocfs2-tools-devel, ok, it's also fine, still userspace stuff;-)