ocfs2: return -EROFS to upper if inode block is invalid

PATCH New piaojun 4 participants 11 messages Dec 26, 2017 OCFS2

lore

Share & tools

lore

piaojun

author

2017-12-26 02:11 UTC #0 lore raw

PATCH 1/1 ocfs2: return -EROFS to upper if inode block is invalid

If metadata is corrupted such as 'invalid inode block', we will get failed by calling 'mount()' as below:

ocfs2_mount
ocfs2_initialize_super
ocfs2_init_global_system_inodes : return -EINVAL if inode is NULL
ocfs2_get_system_file_inode
_ocfs2_get_system_file_inode : return NULL if inode is errno
ocfs2_iget
ocfs2_read_locked_inode
ocfs2_validate_inode_block

In this situation we need return -EROFS to upper application, so that user can fix it by fsck. And then mount again.

Signed-off-by Jun Piao <[email protected]>

Reviewed-by Alex Chen <[email protected]>

---
 fs/ocfs2/super.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

fs/ocfs2/super.c +8 -2

index 040bbb6..dea21a7 100644
@@ -474,7 +474,10 @@ static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog_errno(status);
 			/* FIXME: Should ERROR_RO_FS */
 			mlog(ML_ERROR, "Unable to load system inode %d, "

@@ -505,7 +508,10 @@ static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog(ML_ERROR, "status=%d, sysfile=%d, slot=%d\n",
 			     status, i, osb->slot_num);
 			goto bail;

Gang He

2017-12-26 02:22 UTC #1 lore raw

PATCH 1/1 ocfs2: return -EROFS to upper if inode block is invalid

Hi Piaojun,

Just one quick question, if the file system is read-only, this can make ocfs2_get_system_file_inode() function invoke failure? If ture, I think this code change make sense.

Thanks
Gang

Quoted 58 lines

Quoted 1 line
Quoted 1 line
If metadata is corrupted such as 'invalid inode block', we will get failed by calling 'mount()' as below:
ocfs2_mount
ocfs2_initialize_super
ocfs2_init_global_system_inodes : return -EINVAL if inode is NULL
ocfs2_get_system_file_inode
_ocfs2_get_system_file_inode : return NULL if inode is errno
ocfs2_iget
ocfs2_read_locked_inode
ocfs2_validate_inode_block
In this situation we need return -EROFS to upper application, so that user can fix it by fsck. And then mount again.
Signed-off-by Jun Piao <[email protected]>
Reviewed-by Alex Chen <[email protected]>
---
 fs/ocfs2/super.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
fs/ocfs2/super.c +4 -1
index 040bbb6..dea21a7 100644
@@ -474,7 +474,10 @@ static int ocfs2_init_global_system_inodes(struct 
 ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog_errno(status);
 			/* FIXME: Should ERROR_RO_FS */
fs/ocfs2/super.c +4 -1
 			mlog(ML_ERROR, "Unable to load system inode %d, "
@@ -505,7 +508,10 @@ static int ocfs2_init_local_system_inodes(struct 
 ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog(ML_ERROR, "status=%d, sysfile=%d, slot=%d\n",
 			     status, i, osb->slot_num);
goto bail;
Signature · 4 lines
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com 
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

piaojun

author

2017-12-26 02:31 UTC #2 lore raw

PATCH 1/1 ocfs2: return -EROFS to upper if inode block is invalid

Hi Gang,

Just like the dumped stack below, ocfs2_validate_inode_block() will find out 'invalid inode' and then return error to upper callers. At last invoke failure of ocfs2_get_system_file_inode().

thanks,
Jun

Quoted 71 lines · Gang He

Hi Piaojun,
Just one quick question, if the file system is read-only, this can make ocfs2_get_system_file_inode() function invoke failure? If ture, I think this code change make sense.
Thanks
Gang
Quoted 51 lines
Quoted 1 line
Quoted 1 line
If metadata is corrupted such as 'invalid inode block', we will get failed by calling 'mount()' as below:
ocfs2_mount
ocfs2_initialize_super
ocfs2_init_global_system_inodes : return -EINVAL if inode is NULL
ocfs2_get_system_file_inode
_ocfs2_get_system_file_inode : return NULL if inode is errno
ocfs2_iget
ocfs2_read_locked_inode
ocfs2_validate_inode_block
In this situation we need return -EROFS to upper application, so that user can fix it by fsck. And then mount again.
Signed-off-by Jun Piao <[email protected]>
Reviewed-by Alex Chen <[email protected]>
---
 fs/ocfs2/super.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
fs/ocfs2/super.c +4 -1
index 040bbb6..dea21a7 100644
@@ -474,7 +474,10 @@ static int ocfs2_init_global_system_inodes(struct 
 ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog_errno(status);
 			/* FIXME: Should ERROR_RO_FS */
fs/ocfs2/super.c +4 -1
 			mlog(ML_ERROR, "Unable to load system inode %d, "
@@ -505,7 +508,10 @@ static int ocfs2_init_local_system_inodes(struct 
 ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog(ML_ERROR, "status=%d, sysfile=%d, slot=%d\n",
 			     status, i, osb->slot_num);
Quoted 7 lines
goto bail;
Signature · 4 lines
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com 
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
.

Joseph Qi

2017-12-26 03:05 UTC #3 lore raw

PATCH 1/1 ocfs2: return -EROFS to upper if inode block is invalid

In reply to piaojun

Quoted 8 lines · piaojun

If metadata is corrupted such as 'invalid inode block', we will get failed by calling 'mount()' as below:
ocfs2_mount
ocfs2_initialize_super
ocfs2_init_global_system_inodes : return -EINVAL if inode is NULL
ocfs2_get_system_file_inode
_ocfs2_get_system_file_inode : return NULL if inode is errno

Do you mean inode is bad?

Quoted 18 lines

ocfs2_iget
ocfs2_read_locked_inode
ocfs2_validate_inode_block
In this situation we need return -EROFS to upper application, so that user can fix it by fsck. And then mount again.
Signed-off-by Jun Piao <[email protected]>
Reviewed-by Alex Chen <[email protected]>
---
 fs/ocfs2/super.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
fs/ocfs2/super.c +0 -0
index 040bbb6..dea21a7 100644

fs/ocfs2/super.c static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb) · line 477

Quoted 5 lines

fs/ocfs2/super.c +1 -1

@@ -474,7 +474,10 @@ static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))

I'm afraid that having bad inode doesn't means ocfs2 is readonly. And the calling application is mount.ocfs2. So do you mean mount.ocfs2 have to handle EROFS like printing corresponding error log?

Quoted 19 lines

fs/ocfs2/super.c +7 -1

+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog_errno(status);
 			/* FIXME: Should ERROR_RO_FS */
 			mlog(ML_ERROR, "Unable to load system inode %d, "

@@ -505,7 +508,10 @@ static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog(ML_ERROR, "status=%d, sysfile=%d, slot=%d\n",
 			     status, i, osb->slot_num);
 			goto bail;

piaojun

author

2017-12-26 05:35 UTC #4 lore raw

PATCH 1/1 ocfs2: return -EROFS to upper if inode block is invalid

Hi Joseph,

Quoted 6 lines · Joseph Qi

Quoted 3 lines · piaojun
If metadata is corrupted such as 'invalid inode block', we will get failed by calling 'mount()' as below:

Quoted 7 lines

Quoted 5 lines
ocfs2_mount
ocfs2_initialize_super
ocfs2_init_global_system_inodes : return -EINVAL if inode is NULL
ocfs2_get_system_file_inode
_ocfs2_get_system_file_inode : return NULL if inode is errno
Do you mean inode is bad?

Here we have to face two abnormal cases:
1. inode is bad;
2. read inode from disk failed due to bad storage link.

Quoted 13 lines

Quoted 13 lines
ocfs2_iget
ocfs2_read_locked_inode
ocfs2_validate_inode_block
In this situation we need return -EROFS to upper application, so that user can fix it by fsck. And then mount again.
Signed-off-by Jun Piao <[email protected]>
Reviewed-by Alex Chen <[email protected]>
---
 fs/ocfs2/super.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Quoted 14 lines

fs/ocfs2/super.c static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb) · line 477
Quoted 10 lines
fs/ocfs2/super.c +1 -1
index 040bbb6..dea21a7 100644
@@ -474,7 +474,10 @@ static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
I'm afraid that having bad inode doesn't means ocfs2 is readonly. And the calling application is mount.ocfs2. So do you mean mount.ocfs2 have to handle EROFS like printing corresponding error log?

I agree that 'bad inode' also means other abnormal cases like 'bad storage link' or 'no memory', but we can distinguish that by ocfs2_is_soft_readonly(). I found that 'mount.ocfs2' did not distinguish any error type and just return 1 for all error cases. I wonder if we should return the exact errno for users?

thanks,
Jun

Quoted 21 lines

Quoted 9 lines

fs/ocfs2/super.c +3 -0

+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog_errno(status);
 			/* FIXME: Should ERROR_RO_FS */
 			mlog(ML_ERROR, "Unable to load system inode %d, "

@@ -505,7 +508,10 @@ static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);

fs/ocfs2/super.c static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb) · line 517

Quoted 10 lines

fs/ocfs2/super.c +4 -1

 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog(ML_ERROR, "status=%d, sysfile=%d, slot=%d\n",
 			     status, i, osb->slot_num);
 			goto bail;

Joseph Qi

2017-12-26 06:10 UTC #5 lore raw

PATCH 1/1 ocfs2: return -EROFS to upper if inode block is invalid

Quoted 46 lines · piaojun

Hi Joseph,
Quoted 6 lines · Joseph Qi
Quoted 3 lines · piaojun
If metadata is corrupted such as 'invalid inode block', we will get failed by calling 'mount()' as below:
Quoted 7 lines
Quoted 5 lines
ocfs2_mount
ocfs2_initialize_super
ocfs2_init_global_system_inodes : return -EINVAL if inode is NULL
ocfs2_get_system_file_inode
_ocfs2_get_system_file_inode : return NULL if inode is errno
Do you mean inode is bad?
Here we have to face two abnormal cases:
1. inode is bad;
2. read inode from disk failed due to bad storage link.
Quoted 13 lines
Quoted 13 lines
ocfs2_iget
ocfs2_read_locked_inode
ocfs2_validate_inode_block
In this situation we need return -EROFS to upper application, so that user can fix it by fsck. And then mount again.
Signed-off-by Jun Piao <[email protected]>
Reviewed-by Alex Chen <[email protected]>
---
 fs/ocfs2/super.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
Quoted 14 lines
fs/ocfs2/super.c static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb) · line 477
Quoted 10 lines
fs/ocfs2/super.c +1 -1
index 040bbb6..dea21a7 100644
@@ -474,7 +474,10 @@ static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
I'm afraid that having bad inode doesn't means ocfs2 is readonly. And the calling application is mount.ocfs2. So do you mean mount.ocfs2 have to handle EROFS like printing corresponding error log?

Quoted 6 lines

I agree that 'bad inode' also means other abnormal cases like 'bad storage link' or 'no memory', but we can distinguish that by ocfs2_is_soft_readonly(). I found that 'mount.ocfs2' did not distinguish any error type and just return 1 for all error cases. I wonder if we should return the exact errno for users? Soft readonly is an in-memory status. The case you described is just

trying to read inode and then check if it is bad. So where to set the status before?

Quoted 24 lines

thanks,
Jun

Quoted 21 lines

Quoted 9 lines

fs/ocfs2/super.c +3 -0

+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog_errno(status);
 			/* FIXME: Should ERROR_RO_FS */
 			mlog(ML_ERROR, "Unable to load system inode %d, "

@@ -505,7 +508,10 @@ static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);

fs/ocfs2/super.c static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb) · line 517

Quoted 10 lines

fs/ocfs2/super.c +4 -1

 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog(ML_ERROR, "status=%d, sysfile=%d, slot=%d\n",
 			     status, i, osb->slot_num);
 			goto bail;

piaojun

author

2017-12-26 06:45 UTC #6 lore raw

PATCH 1/1 ocfs2: return -EROFS to upper if inode block is invalid

Hi Joseph,

Quoted 49 lines · Joseph Qi

Quoted 46 lines · piaojun
Hi Joseph,
Quoted 6 lines · Joseph Qi
Quoted 3 lines · piaojun
If metadata is corrupted such as 'invalid inode block', we will get failed by calling 'mount()' as below:
Quoted 7 lines
Quoted 5 lines
ocfs2_mount
ocfs2_initialize_super
ocfs2_init_global_system_inodes : return -EINVAL if inode is NULL
ocfs2_get_system_file_inode
_ocfs2_get_system_file_inode : return NULL if inode is errno
Do you mean inode is bad?
Here we have to face two abnormal cases:
1. inode is bad;
2. read inode from disk failed due to bad storage link.
Quoted 27 lines
Quoted 18 lines
ocfs2_iget
ocfs2_read_locked_inode
ocfs2_validate_inode_block
In this situation we need return -EROFS to upper application, so that user can fix it by fsck. And then mount again.
Signed-off-by Jun Piao <[email protected]>
Reviewed-by Alex Chen <[email protected]>
---
 fs/ocfs2/super.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
fs/ocfs2/super.c +0 -0
index 040bbb6..dea21a7 100644
fs/ocfs2/super.c static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb) · line 477
Quoted 5 lines
fs/ocfs2/super.c +1 -1
@@ -474,7 +474,10 @@ static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
I'm afraid that having bad inode doesn't means ocfs2 is readonly. And the calling application is mount.ocfs2. So do you mean mount.ocfs2 have to handle EROFS like printing corresponding error log?

Quoted 9 lines

Quoted 6 lines
I agree that 'bad inode' also means other abnormal cases like 'bad storage link' or 'no memory', but we can distinguish that by ocfs2_is_soft_readonly(). I found that 'mount.ocfs2' did not distinguish any error type and just return 1 for all error cases. I wonder if we should return the exact errno for users? Soft readonly is an in-memory status. The case you described is just
trying to read inode and then check if it is bad. So where to set the status before?

we set readonly status in the following process:
ocfs2_validate_inode_block()
ocfs2_error

    ocfs2_handle_error
      ocfs2_set_ro_flag(osb, 0);

I have a suggestion that we could distinguish readonly status in 'mount.ocfs2', and return -EROFS to users so that they can fix it.

Quoted 26 lines

Quoted 24 lines

thanks,
Jun

Quoted 21 lines

Quoted 9 lines

fs/ocfs2/super.c +3 -0

+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog_errno(status);
 			/* FIXME: Should ERROR_RO_FS */
 			mlog(ML_ERROR, "Unable to load system inode %d, "

@@ -505,7 +508,10 @@ static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);

fs/ocfs2/super.c static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb) · line 517

Quoted 10 lines

fs/ocfs2/super.c +4 -1

 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog(ML_ERROR, "status=%d, sysfile=%d, slot=%d\n",
 			     status, i, osb->slot_num);
 			goto bail;

Joseph Qi

2017-12-26 06:59 UTC #7 lore raw

PATCH 1/1 ocfs2: return -EROFS to upper if inode block is invalid

Quoted 61 lines · piaojun

Hi Joseph,
Quoted 49 lines · Joseph Qi
Quoted 46 lines · piaojun
Hi Joseph,
Quoted 6 lines · Joseph Qi
Quoted 3 lines · piaojun
If metadata is corrupted such as 'invalid inode block', we will get failed by calling 'mount()' as below:
Quoted 7 lines
Quoted 5 lines
ocfs2_mount
ocfs2_initialize_super
ocfs2_init_global_system_inodes : return -EINVAL if inode is NULL
ocfs2_get_system_file_inode
_ocfs2_get_system_file_inode : return NULL if inode is errno
Do you mean inode is bad?
Here we have to face two abnormal cases:
1. inode is bad;
2. read inode from disk failed due to bad storage link.
Quoted 27 lines
Quoted 18 lines
ocfs2_iget
ocfs2_read_locked_inode
ocfs2_validate_inode_block
In this situation we need return -EROFS to upper application, so that user can fix it by fsck. And then mount again.
Signed-off-by Jun Piao <[email protected]>
Reviewed-by Alex Chen <[email protected]>
---
 fs/ocfs2/super.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
fs/ocfs2/super.c +0 -0
index 040bbb6..dea21a7 100644
fs/ocfs2/super.c static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb) · line 477
Quoted 5 lines
fs/ocfs2/super.c +1 -1
@@ -474,7 +474,10 @@ static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
I'm afraid that having bad inode doesn't means ocfs2 is readonly. And the calling application is mount.ocfs2. So do you mean mount.ocfs2 have to handle EROFS like printing corresponding error log?
Quoted 9 lines
Quoted 6 lines
I agree that 'bad inode' also means other abnormal cases like 'bad storage link' or 'no memory', but we can distinguish that by ocfs2_is_soft_readonly(). I found that 'mount.ocfs2' did not distinguish any error type and just return 1 for all error cases. I wonder if we should return the exact errno for users? Soft readonly is an in-memory status. The case you described is just
trying to read inode and then check if it is bad. So where to set the status before?

Quoted 8 lines

we set readonly status in the following process:
ocfs2_validate_inode_block()
ocfs2_error
    ocfs2_handle_error
      ocfs2_set_ro_flag(osb, 0);
I have a suggestion that we could distinguish readonly status in 'mount.ocfs2', and return -EROFS to users so that they can fix it.

IC. Please update this information to patch description as well. And suggest just use ternary operator instead of if/else. BTW, so mount.ocfs2 should be updated correspondingly, right?

Thanks,
Joseph

Quoted 26 lines

Quoted 24 lines

thanks,
Jun

Quoted 21 lines

Quoted 9 lines

fs/ocfs2/super.c +3 -0

+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog_errno(status);
 			/* FIXME: Should ERROR_RO_FS */
 			mlog(ML_ERROR, "Unable to load system inode %d, "

@@ -505,7 +508,10 @@ static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);

fs/ocfs2/super.c static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb) · line 517

Quoted 10 lines

fs/ocfs2/super.c +4 -1

 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog(ML_ERROR, "status=%d, sysfile=%d, slot=%d\n",
 			     status, i, osb->slot_num);
 			goto bail;

piaojun

author

2017-12-26 07:05 UTC #8 lore raw

PATCH 1/1 ocfs2: return -EROFS to upper if inode block is invalid

Hi Joseph,

Quoted 70 lines · Joseph Qi

Quoted 67 lines · piaojun
Hi Joseph,
Quoted 49 lines · Joseph Qi
Quoted 46 lines · piaojun
Hi Joseph,
Quoted 6 lines · Joseph Qi
Quoted 3 lines · piaojun
If metadata is corrupted such as 'invalid inode block', we will get failed by calling 'mount()' as below:
Quoted 7 lines
Quoted 5 lines
ocfs2_mount
ocfs2_initialize_super
ocfs2_init_global_system_inodes : return -EINVAL if inode is NULL
ocfs2_get_system_file_inode
_ocfs2_get_system_file_inode : return NULL if inode is errno
Do you mean inode is bad?
Here we have to face two abnormal cases:
1. inode is bad;
2. read inode from disk failed due to bad storage link.
Quoted 27 lines
Quoted 18 lines
ocfs2_iget
ocfs2_read_locked_inode
ocfs2_validate_inode_block
In this situation we need return -EROFS to upper application, so that user can fix it by fsck. And then mount again.
Signed-off-by Jun Piao <[email protected]>
Reviewed-by Alex Chen <[email protected]>
---
 fs/ocfs2/super.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
fs/ocfs2/super.c +0 -0
index 040bbb6..dea21a7 100644
fs/ocfs2/super.c static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb) · line 477
Quoted 5 lines
fs/ocfs2/super.c +1 -1
@@ -474,7 +474,10 @@ static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
I'm afraid that having bad inode doesn't means ocfs2 is readonly. And the calling application is mount.ocfs2. So do you mean mount.ocfs2 have to handle EROFS like printing corresponding error log?
Quoted 9 lines
Quoted 6 lines
I agree that 'bad inode' also means other abnormal cases like 'bad storage link' or 'no memory', but we can distinguish that by ocfs2_is_soft_readonly(). I found that 'mount.ocfs2' did not distinguish any error type and just return 1 for all error cases. I wonder if we should return the exact errno for users? Soft readonly is an in-memory status. The case you described is just
trying to read inode and then check if it is bad. So where to set the status before?
we set readonly status in the following process:
ocfs2_validate_inode_block()
ocfs2_error
    ocfs2_handle_error
      ocfs2_set_ro_flag(osb, 0);

Quoted 8 lines

Quoted 2 lines
I have a suggestion that we could distinguish readonly status in 'mount.ocfs2', and return -EROFS to users so that they can fix it.
IC. Please update this information to patch description as well. And suggest just use ternary operator instead of if/else. BTW, so mount.ocfs2 should be updated correspondingly, right?
Thanks,
Joseph

Thanks for your advices, and I will post a patch for mount.ocfs2 correspondingly.

Quoted 28 lines

Quoted 24 lines

thanks,
Jun

Quoted 21 lines

Quoted 9 lines

fs/ocfs2/super.c +3 -0

+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog_errno(status);
 			/* FIXME: Should ERROR_RO_FS */
 			mlog(ML_ERROR, "Unable to load system inode %d, "

@@ -505,7 +508,10 @@ static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb)
 		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);

fs/ocfs2/super.c static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb) · line 517

Quoted 10 lines

fs/ocfs2/super.c +4 -1

 		if (!new) {
 			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
 			mlog(ML_ERROR, "status=%d, sysfile=%d, slot=%d\n",
 			     status, i, osb->slot_num);
 			goto bail;

Quoted 2 lines

Quoted 2 lines
.

Changwei Ge

2017-12-26 03:34 UTC #9 lore raw

PATCH 1/1 ocfs2: return -EROFS to upper if inode block is invalid

In reply to piaojun

Hi Jun,

What I concern is if we don't return -EROFS to mount.ocfs2, what bad result will come? This patch is a bug fix or something else?
Can you elaborate your intention of this patch?

Thanks,
Changwei

Quoted 50 lines · piaojun

If metadata is corrupted such as 'invalid inode block', we will get failed by calling 'mount()' as below:
ocfs2_mount
ocfs2_initialize_super
ocfs2_init_global_system_inodes : return -EINVAL if inode is NULL
ocfs2_get_system_file_inode
_ocfs2_get_system_file_inode : return NULL if inode is errno
ocfs2_iget
ocfs2_read_locked_inode
ocfs2_validate_inode_block
In this situation we need return -EROFS to upper application, so that user can fix it by fsck. And then mount again.
Signed-off-by Jun Piao <[email protected]>
Reviewed-by Alex Chen <[email protected]>
---
  fs/ocfs2/super.c | 10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)
fs/ocfs2/super.c +8 -2
index 040bbb6..dea21a7 100644
@@ -474,7 +474,10 @@ static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb)
  		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
  		if (!new) {
  			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
  			mlog_errno(status);
  			/* FIXME: Should ERROR_RO_FS */
  			mlog(ML_ERROR, "Unable to load system inode %d, "
@@ -505,7 +508,10 @@ static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb)
  		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
  		if (!new) {
  			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
  			mlog(ML_ERROR, "status=%d, sysfile=%d, slot=%d\n",
  			     status, i, osb->slot_num);
  			goto bail;

piaojun

author

2017-12-26 05:41 UTC #10 lore raw

PATCH 1/1 ocfs2: return -EROFS to upper if inode block is invalid

Hi Changwei,

I just want to return exact errno to users so that they can fix read-only problem rather than doing meaningless retry.

thanks,
Jun

Quoted 63 lines · Changwei Ge

Hi Jun,
What I concern is if we don't return -EROFS to mount.ocfs2, what bad result will come? This patch is a bug fix or something else?
Can you elaborate your intention of this patch?
Thanks,
Changwei
Quoted 40 lines · piaojun
If metadata is corrupted such as 'invalid inode block', we will get failed by calling 'mount()' as below:
ocfs2_mount
ocfs2_initialize_super
ocfs2_init_global_system_inodes : return -EINVAL if inode is NULL
ocfs2_get_system_file_inode
_ocfs2_get_system_file_inode : return NULL if inode is errno
ocfs2_iget
ocfs2_read_locked_inode
ocfs2_validate_inode_block
In this situation we need return -EROFS to upper application, so that user can fix it by fsck. And then mount again.
Signed-off-by Jun Piao <[email protected]>
Reviewed-by Alex Chen <[email protected]>
---
  fs/ocfs2/super.c | 10 ++++++++--
  1 file changed, 8 insertions(+), 2 deletions(-)
fs/ocfs2/super.c +4 -1
index 040bbb6..dea21a7 100644
@@ -474,7 +474,10 @@ static int ocfs2_init_global_system_inodes(struct ocfs2_super *osb)
  		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
  		if (!new) {
  			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
  			mlog_errno(status);
  			/* FIXME: Should ERROR_RO_FS */
  			mlog(ML_ERROR, "Unable to load system inode %d, "
@@ -505,7 +508,10 @@ static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb)
  		new = ocfs2_get_system_file_inode(osb, i, osb->slot_num);
fs/ocfs2/super.c static int ocfs2_init_local_system_inodes(struct ocfs2_super *osb) · line 517
Quoted 10 lines
fs/ocfs2/super.c +4 -1
  		if (!new) {
  			ocfs2_release_system_inodes(osb);
-			status = -EINVAL;
+			if (ocfs2_is_soft_readonly(osb))
+				status = -EROFS;
+			else
+				status = -EINVAL;
  			mlog(ML_ERROR, "status=%d, sysfile=%d, slot=%d\n",
  			     status, i, osb->slot_num);
  			goto bail;
.