ocfs2: don't clear bh uptodate for block read

PATCH Superseded Junxiao Bi 3 participants 9 messages Nov 14, 2018 – Nov 19, 2018 OCFS2

Share & tools

Junxiao Bi

author

2018-11-14 00:00 UTC #0 lore raw

PATCH v2 1/1 ocfs2: don't clear bh uptodate for block read

For sync io read in ocfs2_read_blocks_sync(), first clear bh uptodate flag and submit the io, second wait io done, last check whether bh uptodate, if not return io error.

If two sync io for the same bh were issued, it could be the first io done and set uptodate flag, but just before check that flag, the second io came in and cleared uptodate, then ocfs2_read_blocks_sync() for the first io will return IO error.

Indeed it's not necessary to clear uptodate flag, as the io end handler end_buffer_read_sync() will set or clear it based on io succeed or failed.

The following message was found from a nfs server but the underlying storage returned no error.

[4106438.567376] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2780 ERROR: read block 1238823695 failed -5
[4106438.567569] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2812 ERROR: status = -5
[4106438.567611] (nfsd,7146,3):ocfs2_test_inode_bit:2894 ERROR: get alloc slot and bit failed -5
[4106438.567643] (nfsd,7146,3):ocfs2_test_inode_bit:2932 ERROR: status = -5
[4106438.567675] (nfsd,7146,3):ocfs2_get_dentry:94 ERROR: test inode bit failed -5

Same issue in non sync read ocfs2_read_blocks(), fixed it as well.

Signed-off-by Junxiao Bi <[email protected]>

Cc Changwei Ge <[email protected]>

---

v1 -> v2:

- fixed non sync read ocfs2_read_blocks() as well.

---
 fs/ocfs2/buffer_head_io.c | 2 --
 1 file changed, 2 deletions(-)

fs/ocfs2/buffer_head_io.c +0 -2

index 4ebbd57cbf84..f9b84f7a3e4b 100644
@@ -161,7 +161,6 @@ int ocfs2_read_blocks_sync(struct ocfs2_super *osb, u64 block,
 #endif
 		}
 
-		clear_buffer_uptodate(bh);
 		get_bh(bh); /* for end_buffer_read_sync() */
 		bh->b_end_io = end_buffer_read_sync;
 		submit_bh(REQ_OP_READ, 0, bh);

@@ -341,7 +340,6 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr,
 				continue;
 			}
 
-			clear_buffer_uptodate(bh);
 			get_bh(bh); /* for end_buffer_read_sync() */
 			if (validate)
 				set_buffer_needs_validate(bh);

Signature · 1 line

2.17.1

Changwei Ge

2018-11-14 01:42 UTC #1 lore raw

PATCH v2 1/1 ocfs2: don't clear bh uptodate for block read

Hi Junxiao,

Actually, I think we still have another race around reading blocks methods. Consider below scenario:

Two parallel reading IO towards the same block.
The first succeeds while the second fails.
The second will clear _UPTODATE_ state in end_buffer_read_sync() thus making the first one failed as well.

So better for us to not use generic end_buffer_read_sync() but ocfs2 own ones. But it has nothing to do with this patch.
I am adding my Reviewed-by.

Thanks,
Changwei

Quoted 21 lines · Junxiao Bi

For sync io read in ocfs2_read_blocks_sync(), first clear bh uptodate flag and submit the io, second wait io done, last check whether bh uptodate, if not return io error.
If two sync io for the same bh were issued, it could be the first io done and set uptodate flag, but just before check that flag, the second io came in and cleared uptodate, then ocfs2_read_blocks_sync() for the first io will return IO error.
Indeed it's not necessary to clear uptodate flag, as the io end handler end_buffer_read_sync() will set or clear it based on io succeed or failed.
The following message was found from a nfs server but the underlying storage returned no error.
[4106438.567376] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2780 ERROR: read block 1238823695 failed -5
[4106438.567569] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2812 ERROR: status = -5
[4106438.567611] (nfsd,7146,3):ocfs2_test_inode_bit:2894 ERROR: get alloc slot and bit failed -5
[4106438.567643] (nfsd,7146,3):ocfs2_test_inode_bit:2932 ERROR: status = -5
[4106438.567675] (nfsd,7146,3):ocfs2_get_dentry:94 ERROR: test inode bit failed -5

Quoted 4 lines

Same issue in non sync read ocfs2_read_blocks(), fixed it as well.
Signed-off-by Junxiao Bi <[email protected]>
Cc Changwei Ge <[email protected]>

Reviewed-by Changwei Ge <[email protected]>

Quoted 31 lines

---

v1 -> v2:

- fixed non sync read ocfs2_read_blocks() as well.

---
  fs/ocfs2/buffer_head_io.c | 2 --
  1 file changed, 2 deletions(-)

fs/ocfs2/buffer_head_io.c +0 -2

index 4ebbd57cbf84..f9b84f7a3e4b 100644
@@ -161,7 +161,6 @@ int ocfs2_read_blocks_sync(struct ocfs2_super *osb, u64 block,
  #endif
  		}
 
-		clear_buffer_uptodate(bh);
  		get_bh(bh); /* for end_buffer_read_sync() */
  		bh->b_end_io = end_buffer_read_sync;
  		submit_bh(REQ_OP_READ, 0, bh);

@@ -341,7 +340,6 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr,
  				continue;
  			}
 
-			clear_buffer_uptodate(bh);
  			get_bh(bh); /* for end_buffer_read_sync() */
  			if (validate)
  				set_buffer_needs_validate(bh);

Junxiao Bi

author

2018-11-14 03:15 UTC #2 lore raw

PATCH v2 1/1 ocfs2: don't clear bh uptodate for block read

Hi Changwei,

Thank you for the review.

See comment inlined

Quoted 10 lines · Changwei Ge

Hi Junxiao,
Actually, I think we still have another race around reading blocks methods. Consider below scenario:
Two parallel reading IO towards the same block.
The first succeeds while the second fails.
The second will clear _UPTODATE_ state in end_buffer_read_sync() thus making the first one failed as well.
So better for us to not use generic end_buffer_read_sync() but ocfs2 own ones.

Is it necessary to do this complicated things? Seemed it will not cause any difference, at last IO error will be returned for this very block.

thanks,

Junxiao.

Quoted 65 lines

But it has nothing to do with this patch. I am adding my Reviewed-by.
Thanks,
Changwei
Quoted 21 lines · Junxiao Bi
For sync io read in ocfs2_read_blocks_sync(), first clear bh uptodate flag and submit the io, second wait io done, last check whether bh uptodate, if not return io error.
If two sync io for the same bh were issued, it could be the first io done and set uptodate flag, but just before check that flag, the second io came in and cleared uptodate, then ocfs2_read_blocks_sync() for the first io will return IO error.
Indeed it's not necessary to clear uptodate flag, as the io end handler end_buffer_read_sync() will set or clear it based on io succeed or failed.
The following message was found from a nfs server but the underlying storage returned no error.
[4106438.567376] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2780 ERROR: read block 1238823695 failed -5
[4106438.567569] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2812 ERROR: status = -5
[4106438.567611] (nfsd,7146,3):ocfs2_test_inode_bit:2894 ERROR: get alloc slot and bit failed -5
[4106438.567643] (nfsd,7146,3):ocfs2_test_inode_bit:2932 ERROR: status = -5
[4106438.567675] (nfsd,7146,3):ocfs2_get_dentry:94 ERROR: test inode bit failed -5
Quoted 4 lines
Same issue in non sync read ocfs2_read_blocks(), fixed it as well.
Signed-off-by Junxiao Bi <[email protected]>
Cc Changwei Ge <[email protected]>
Reviewed-by Changwei Ge <[email protected]>
Quoted 31 lines
---
v1 -> v2:
- fixed non sync read ocfs2_read_blocks() as well.
---
   fs/ocfs2/buffer_head_io.c | 2 --
   1 file changed, 2 deletions(-)
fs/ocfs2/buffer_head_io.c +0 -2
index 4ebbd57cbf84..f9b84f7a3e4b 100644
@@ -161,7 +161,6 @@ int ocfs2_read_blocks_sync(struct ocfs2_super *osb, u64 block,
   #endif
   		}
 
-		clear_buffer_uptodate(bh);
   		get_bh(bh); /* for end_buffer_read_sync() */
   		bh->b_end_io = end_buffer_read_sync;
   		submit_bh(REQ_OP_READ, 0, bh);
@@ -341,7 +340,6 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr,
   				continue;
   			}
 
-			clear_buffer_uptodate(bh);
   			get_bh(bh); /* for end_buffer_read_sync() */
   			if (validate)
   				set_buffer_needs_validate(bh);

jiangyiwen

2018-11-19 03:05 UTC #3 lore raw

PATCH v2 1/1 ocfs2: don't clear bh uptodate for block read

In reply to Junxiao Bi

Quoted 40 lines · Junxiao Bi

For sync io read in ocfs2_read_blocks_sync(), first clear bh uptodate flag and submit the io, second wait io done, last check whether bh uptodate, if not return io error.
If two sync io for the same bh were issued, it could be the first io done and set uptodate flag, but just before check that flag, the second io came in and cleared uptodate, then ocfs2_read_blocks_sync() for the first io will return IO error.
Indeed it's not necessary to clear uptodate flag, as the io end handler end_buffer_read_sync() will set or clear it based on io succeed or failed.
The following message was found from a nfs server but the underlying storage returned no error.
[4106438.567376] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2780 ERROR: read block 1238823695 failed -5
[4106438.567569] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2812 ERROR: status = -5
[4106438.567611] (nfsd,7146,3):ocfs2_test_inode_bit:2894 ERROR: get alloc slot and bit failed -5
[4106438.567643] (nfsd,7146,3):ocfs2_test_inode_bit:2932 ERROR: status = -5
[4106438.567675] (nfsd,7146,3):ocfs2_get_dentry:94 ERROR: test inode bit failed -5
Same issue in non sync read ocfs2_read_blocks(), fixed it as well.
Signed-off-by Junxiao Bi <[email protected]>
Cc Changwei Ge <[email protected]>
---
v1 -> v2:
- fixed non sync read ocfs2_read_blocks() as well.
---
 fs/ocfs2/buffer_head_io.c | 2 --
 1 file changed, 2 deletions(-)
fs/ocfs2/buffer_head_io.c +0 -0
index 4ebbd57cbf84..f9b84f7a3e4b 100644

fs/ocfs2/buffer_head_io.c int ocfs2_read_blocks_sync(struct ocfs2_super *osb, u64 block, · line 164

Quoted 4 lines

fs/ocfs2/buffer_head_io.c +0 -1

@@ -161,7 +161,6 @@ int ocfs2_read_blocks_sync(struct ocfs2_super *osb, u64 block,
 #endif
 		}
 
-		clear_buffer_uptodate(bh);

If we should use mutex_lock to keep sync?

Thanks.

fs/ocfs2/buffer_head_io.c int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr, · line 344

Quoted 8 lines

fs/ocfs2/buffer_head_io.c +0 -1

 		get_bh(bh); /* for end_buffer_read_sync() */
 		bh->b_end_io = end_buffer_read_sync;
 		submit_bh(REQ_OP_READ, 0, bh);

@@ -341,7 +340,6 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr,
 				continue;
 			}
 
-			clear_buffer_uptodate(bh);

Actually, this don't need to change, because we will add ocfs2_metadata_cache_io_lock() and ensure only one thread to read buffer.

Quoted 4 lines

fs/ocfs2/buffer_head_io.c +0 -0

 			get_bh(bh); /* for end_buffer_read_sync() */
 			if (validate)
 				set_buffer_needs_validate(bh);

Junxiao Bi

author

2018-11-19 05:46 UTC #4 lore raw

PATCH v2 1/1 ocfs2: don't clear bh uptodate for block read

Quoted 44 lines · jiangyiwen

Quoted 43 lines · Junxiao Bi
For sync io read in ocfs2_read_blocks_sync(), first clear bh uptodate flag and submit the io, second wait io done, last check whether bh uptodate, if not return io error.
If two sync io for the same bh were issued, it could be the first io done and set uptodate flag, but just before check that flag, the second io came in and cleared uptodate, then ocfs2_read_blocks_sync() for the first io will return IO error.
Indeed it's not necessary to clear uptodate flag, as the io end handler end_buffer_read_sync() will set or clear it based on io succeed or failed.
The following message was found from a nfs server but the underlying storage returned no error.
[4106438.567376] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2780 ERROR: read block 1238823695 failed -5
[4106438.567569] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2812 ERROR: status = -5
[4106438.567611] (nfsd,7146,3):ocfs2_test_inode_bit:2894 ERROR: get alloc slot and bit failed -5
[4106438.567643] (nfsd,7146,3):ocfs2_test_inode_bit:2932 ERROR: status = -5
[4106438.567675] (nfsd,7146,3):ocfs2_get_dentry:94 ERROR: test inode bit failed -5
Same issue in non sync read ocfs2_read_blocks(), fixed it as well.
Signed-off-by Junxiao Bi <[email protected]>
Cc Changwei Ge <[email protected]>
---
v1 -> v2:
- fixed non sync read ocfs2_read_blocks() as well.
---
  fs/ocfs2/buffer_head_io.c | 2 --
  1 file changed, 2 deletions(-)
fs/ocfs2/buffer_head_io.c +0 -0
index 4ebbd57cbf84..f9b84f7a3e4b 100644
@@ -161,7 +161,6 @@ int ocfs2_read_blocks_sync(struct ocfs2_super *osb, u64 block,
  #endif
  		}
 

Quoted 4 lines

fs/ocfs2/buffer_head_io.c int ocfs2_read_blocks_sync(struct ocfs2_super *osb, u64 block, · line 164
Quoted 1 line
fs/ocfs2/buffer_head_io.c +0 -1
-		clear_buffer_uptodate(bh);
If we should use mutex_lock to keep sync?
Thanks.

As my last email in this thread mentioned, it will not cause difference. Here race is between processes accessing the same block. At last the io error will be return for this very block. I don't think we need bother using mutex to sync them.

Quoted 11 lines

fs/ocfs2/buffer_head_io.c int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr, · line 344
Quoted 9 lines
fs/ocfs2/buffer_head_io.c +0 -1
  		get_bh(bh); /* for end_buffer_read_sync() */
  		bh->b_end_io = end_buffer_read_sync;
  		submit_bh(REQ_OP_READ, 0, bh);
@@ -341,7 +340,6 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr,
  				continue;
  			}
 
-			clear_buffer_uptodate(bh);
Actually, this don't need to change, because we will add ocfs2_metadata_cache_io_lock() and ensure only one thread to read buffer.

Not every block has this lock used, only inode and refcount using it.

Thanks,

Junxiao.

Quoted 6 lines

Quoted 5 lines

fs/ocfs2/buffer_head_io.c +0 -0

  			get_bh(bh); /* for end_buffer_read_sync() */
  			if (validate)
  				set_buffer_needs_validate(bh);

jiangyiwen

2018-11-19 06:14 UTC #5 lore raw

PATCH v2 1/1 ocfs2: don't clear bh uptodate for block read

Quoted 48 lines · Junxiao Bi

Quoted 46 lines · jiangyiwen
Quoted 40 lines · Junxiao Bi
For sync io read in ocfs2_read_blocks_sync(), first clear bh uptodate flag and submit the io, second wait io done, last check whether bh uptodate, if not return io error.
If two sync io for the same bh were issued, it could be the first io done and set uptodate flag, but just before check that flag, the second io came in and cleared uptodate, then ocfs2_read_blocks_sync() for the first io will return IO error.
Indeed it's not necessary to clear uptodate flag, as the io end handler end_buffer_read_sync() will set or clear it based on io succeed or failed.
The following message was found from a nfs server but the underlying storage returned no error.
[4106438.567376] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2780 ERROR: read block 1238823695 failed -5
[4106438.567569] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2812 ERROR: status = -5
[4106438.567611] (nfsd,7146,3):ocfs2_test_inode_bit:2894 ERROR: get alloc slot and bit failed -5
[4106438.567643] (nfsd,7146,3):ocfs2_test_inode_bit:2932 ERROR: status = -5
[4106438.567675] (nfsd,7146,3):ocfs2_get_dentry:94 ERROR: test inode bit failed -5
Same issue in non sync read ocfs2_read_blocks(), fixed it as well.
Signed-off-by Junxiao Bi <[email protected]>
Cc Changwei Ge <[email protected]>
---
v1 -> v2:
- fixed non sync read ocfs2_read_blocks() as well.
---
  fs/ocfs2/buffer_head_io.c | 2 --
  1 file changed, 2 deletions(-)
fs/ocfs2/buffer_head_io.c +0 -0
index 4ebbd57cbf84..f9b84f7a3e4b 100644
fs/ocfs2/buffer_head_io.c int ocfs2_read_blocks_sync(struct ocfs2_super *osb, u64 block, · line 163
Quoted 3 lines
fs/ocfs2/buffer_head_io.c +0 -0
@@ -161,7 +161,6 @@ int ocfs2_read_blocks_sync(struct ocfs2_super *osb, u64 block,
  #endif
          }
  -        clear_buffer_uptodate(bh);
If we should use mutex_lock to keep sync?

Quoted 2 lines

Quoted 1 line
Thanks.
As my last email in this thread mentioned, it will not cause difference. Here race is between processes accessing the same block. At last the io error will be return for this very block. I don't think we need bother using mutex to sync them.

I have a question, why doesn't this scenario happen in underlying storage?

Thanks.

Quoted 11 lines

Quoted 10 lines
fs/ocfs2/buffer_head_io.c int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr, · line 342
Quoted 8 lines
fs/ocfs2/buffer_head_io.c +0 -0
          get_bh(bh); /* for end_buffer_read_sync() */
          bh->b_end_io = end_buffer_read_sync;
          submit_bh(REQ_OP_READ, 0, bh);
@@ -341,7 +340,6 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr,
                  continue;
              }
  -            clear_buffer_uptodate(bh);
Actually, this don't need to change, because we will add ocfs2_metadata_cache_io_lock() and ensure only one thread to read buffer.

Quoted 2 lines

Not every block has this lock used, only inode and refcount using it.

Actually ci must be not empty, so ocfs2_read_block() will be called sync for the same block.

Quoted 13 lines

Thanks,
Junxiao.
Quoted 6 lines
fs/ocfs2/buffer_head_io.c int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr, · line 345
Quoted 5 lines
fs/ocfs2/buffer_head_io.c +0 -0
              get_bh(bh); /* for end_buffer_read_sync() */
              if (validate)
                  set_buffer_needs_validate(bh);
.

Junxiao Bi

author

2018-11-19 06:34 UTC #6 lore raw

PATCH v2 1/1 ocfs2: don't clear bh uptodate for block read

Quoted 48 lines · jiangyiwen

Quoted 47 lines · Junxiao Bi
Quoted 46 lines · jiangyiwen
Quoted 40 lines · Junxiao Bi
For sync io read in ocfs2_read_blocks_sync(), first clear bh uptodate flag and submit the io, second wait io done, last check whether bh uptodate, if not return io error.
If two sync io for the same bh were issued, it could be the first io done and set uptodate flag, but just before check that flag, the second io came in and cleared uptodate, then ocfs2_read_blocks_sync() for the first io will return IO error.
Indeed it's not necessary to clear uptodate flag, as the io end handler end_buffer_read_sync() will set or clear it based on io succeed or failed.
The following message was found from a nfs server but the underlying storage returned no error.
[4106438.567376] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2780 ERROR: read block 1238823695 failed -5
[4106438.567569] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2812 ERROR: status = -5
[4106438.567611] (nfsd,7146,3):ocfs2_test_inode_bit:2894 ERROR: get alloc slot and bit failed -5
[4106438.567643] (nfsd,7146,3):ocfs2_test_inode_bit:2932 ERROR: status = -5
[4106438.567675] (nfsd,7146,3):ocfs2_get_dentry:94 ERROR: test inode bit failed -5
Same issue in non sync read ocfs2_read_blocks(), fixed it as well.
Signed-off-by Junxiao Bi <[email protected]>
Cc Changwei Ge <[email protected]>
---
v1 -> v2:
- fixed non sync read ocfs2_read_blocks() as well.
---
   fs/ocfs2/buffer_head_io.c | 2 --
   1 file changed, 2 deletions(-)
fs/ocfs2/buffer_head_io.c +0 -0
index 4ebbd57cbf84..f9b84f7a3e4b 100644
fs/ocfs2/buffer_head_io.c int ocfs2_read_blocks_sync(struct ocfs2_super *osb, u64 block, · line 163
Quoted 3 lines
fs/ocfs2/buffer_head_io.c +0 -0
@@ -161,7 +161,6 @@ int ocfs2_read_blocks_sync(struct ocfs2_super *osb, u64 block,
   #endif
           }
   -        clear_buffer_uptodate(bh);
If we should use mutex_lock to keep sync?

Quoted 4 lines

Quoted 2 lines
Quoted 1 line
Thanks.
As my last email in this thread mentioned, it will not cause difference. Here race is between processes accessing the same block. At last the io error will be return for this very block. I don't think we need bother using mutex to sync them.
I have a question, why doesn't this scenario happen in underlying storage?

didn't follow, could you detail your question?

Quoted 12 lines

Thanks.
Quoted 9 lines
Quoted 9 lines
fs/ocfs2/buffer_head_io.c int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr, · line 342
Quoted 7 lines
fs/ocfs2/buffer_head_io.c +0 -0
           get_bh(bh); /* for end_buffer_read_sync() */
           bh->b_end_io = end_buffer_read_sync;
           submit_bh(REQ_OP_READ, 0, bh);
@@ -341,7 +340,6 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr,
                   continue;
               }
   -            clear_buffer_uptodate(bh);
Actually, this don't need to change, because we will add ocfs2_metadata_cache_io_lock() and ensure only one thread to read buffer.

Quoted 4 lines

Quoted 2 lines
Not every block has this lock used, only inode and refcount using it.
Actually ci must be not empty, so ocfs2_read_block() will be called sync for the same block.

OK.? Any way this will not cause harm and more align with the sync read code.

Thanks,

Junxiao.

Quoted 12 lines

Quoted 11 lines
Thanks,
Junxiao.
Quoted 4 lines
fs/ocfs2/buffer_head_io.c int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr, · line 345
Quoted 4 lines
fs/ocfs2/buffer_head_io.c +0 -0
               get_bh(bh); /* for end_buffer_read_sync() */
               if (validate)
                   set_buffer_needs_validate(bh);
.

jiangyiwen

2018-11-19 07:01 UTC #7 lore raw

PATCH v2 1/1 ocfs2: don't clear bh uptodate for block read

Quoted 26 lines · Junxiao Bi

Quoted 24 lines · jiangyiwen
Quoted 23 lines · Junxiao Bi
Quoted 22 lines · jiangyiwen
Quoted 21 lines · Junxiao Bi
For sync io read in ocfs2_read_blocks_sync(), first clear bh uptodate flag and submit the io, second wait io done, last check whether bh uptodate, if not return io error.
If two sync io for the same bh were issued, it could be the first io done and set uptodate flag, but just before check that flag, the second io came in and cleared uptodate, then ocfs2_read_blocks_sync() for the first io will return IO error.
Indeed it's not necessary to clear uptodate flag, as the io end handler end_buffer_read_sync() will set or clear it based on io succeed or failed.
The following message was found from a nfs server but the underlying storage returned no error.
[4106438.567376] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2780 ERROR: read block 1238823695 failed -5
[4106438.567569] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2812 ERROR: status = -5
[4106438.567611] (nfsd,7146,3):ocfs2_test_inode_bit:2894 ERROR: get alloc slot and bit failed -5
[4106438.567643] (nfsd,7146,3):ocfs2_test_inode_bit:2932 ERROR: status = -5
[4106438.567675] (nfsd,7146,3):ocfs2_get_dentry:94 ERROR: test inode bit failed -5

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Same issue in non sync read ocfs2_read_blocks(), fixed it as well.
Signed-off-by Junxiao Bi <[email protected]>

Reviewed-by Yiwen Jiang <[email protected]>

Quoted 21 lines

Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Quoted 16 lines
Cc Changwei Ge <[email protected]>
---
v1 -> v2:
- fixed non sync read ocfs2_read_blocks() as well.
---
   fs/ocfs2/buffer_head_io.c | 2 --
   1 file changed, 2 deletions(-)
fs/ocfs2/buffer_head_io.c +0 -0
index 4ebbd57cbf84..f9b84f7a3e4b 100644
fs/ocfs2/buffer_head_io.c int ocfs2_read_blocks_sync(struct ocfs2_super *osb, u64 block, · line 163
Quoted 3 lines
fs/ocfs2/buffer_head_io.c +0 -0
@@ -161,7 +161,6 @@ int ocfs2_read_blocks_sync(struct ocfs2_super *osb, u64 block,
   #endif
           }
   -        clear_buffer_uptodate(bh);
If we should use mutex_lock to keep sync?

Quoted 5 lines

Quoted 4 lines
Quoted 2 lines
Quoted 1 line
Thanks.
As my last email in this thread mentioned, it will not cause difference. Here race is between processes accessing the same block. At last the io error will be return for this very block. I don't think we need bother using mutex to sync them.
I have a question, why doesn't this scenario happen in underlying storage?
didn't follow, could you detail your question?

Sorry, It is my misunderstand, I think this
case only happened in the "nfs + ocfs2" before.

Then, there is no problem.

Thanks,
Yiwen.

Quoted 38 lines

Quoted 12 lines
Thanks.
Quoted 9 lines
Quoted 9 lines
fs/ocfs2/buffer_head_io.c int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr, · line 342
Quoted 7 lines
fs/ocfs2/buffer_head_io.c +0 -0
           get_bh(bh); /* for end_buffer_read_sync() */
           bh->b_end_io = end_buffer_read_sync;
           submit_bh(REQ_OP_READ, 0, bh);
@@ -341,7 +340,6 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr,
                   continue;
               }
   -            clear_buffer_uptodate(bh);
Actually, this don't need to change, because we will add ocfs2_metadata_cache_io_lock() and ensure only one thread to read buffer.
Quoted 4 lines
Quoted 2 lines
Not every block has this lock used, only inode and refcount using it.
Actually ci must be not empty, so ocfs2_read_block() will be called sync for the same block.
OK. Any way this will not cause harm and more align with the sync read code.
Thanks,
Junxiao.
Quoted 12 lines
Quoted 11 lines
Thanks,
Junxiao.
Quoted 4 lines
fs/ocfs2/buffer_head_io.c int ocfs2_read_blocks(struct ocfs2_caching_info *ci, u64 block, int nr, · line 345
Quoted 4 lines
fs/ocfs2/buffer_head_io.c +0 -0
               get_bh(bh); /* for end_buffer_read_sync() */
               if (validate)
                   set_buffer_needs_validate(bh);
.
.

Junxiao Bi

author

2018-11-19 07:12 UTC #8 lore raw

PATCH v2 1/1 ocfs2: don't clear bh uptodate for block read

Quoted 27 lines · jiangyiwen

Quoted 26 lines · Junxiao Bi
Quoted 24 lines · jiangyiwen
Quoted 23 lines · Junxiao Bi
Quoted 22 lines · jiangyiwen
Quoted 21 lines · Junxiao Bi
For sync io read in ocfs2_read_blocks_sync(), first clear bh uptodate flag and submit the io, second wait io done, last check whether bh uptodate, if not return io error.
If two sync io for the same bh were issued, it could be the first io done and set uptodate flag, but just before check that flag, the second io came in and cleared uptodate, then ocfs2_read_blocks_sync() for the first io will return IO error.
Indeed it's not necessary to clear uptodate flag, as the io end handler end_buffer_read_sync() will set or clear it based on io succeed or failed.
The following message was found from a nfs server but the underlying storage returned no error.
[4106438.567376] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2780 ERROR: read block 1238823695 failed -5
[4106438.567569] (nfsd,7146,3):ocfs2_get_suballoc_slot_bit:2812 ERROR: status = -5
[4106438.567611] (nfsd,7146,3):ocfs2_test_inode_bit:2894 ERROR: get alloc slot and bit failed -5
[4106438.567643] (nfsd,7146,3):ocfs2_test_inode_bit:2932 ERROR: status = -5
[4106438.567675] (nfsd,7146,3):ocfs2_get_dentry:94 ERROR: test inode bit failed -5

Quoted 4 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Same issue in non sync read ocfs2_read_blocks(), fixed it as well.
Signed-off-by Junxiao Bi<[email protected]>
Reviewed-by Yiwen Jiang<[email protected]>

Yiwen, many thanks for your review.

Quoted 1 line

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20181119/bd47b7f6/attachment.html