| Message ID | 20240729080454.12771-3-heming.zhao@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | give ocfs2 the ability to reclaim suballocator free bg | expand |
On 2024/7/29 16:04, Heming Zhao wrote: > After ocfs2 has the ability to reclaim suballoc free bg, the > suballocator block group may be released. This change makes xfstest > case 426 failed. > > The existed code call stack: > > ocfs2_fh_to_dentry //or ocfs2_fh_to_parent > ocfs2_get_dentry > ocfs2_test_inode_bit > ocfs2_test_suballoc_bit > ocfs2_read_group_descriptor > + read released bg, triggers validate fails, then cause -EROFS > > how to fix: > The read bg failure is expectation, we should ignore this error. > > Signed-off-by: Heming Zhao <heming.zhao@suse.com> LGTM: Reviewed-by: Su Yue <glass.su@suse.com> > --- > fs/ocfs2/suballoc.c | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c > index 1b64f4c87607..dc421f55ed8f 100644 > --- a/fs/ocfs2/suballoc.c > +++ b/fs/ocfs2/suballoc.c > @@ -3037,7 +3037,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb, > struct ocfs2_group_desc *group; > struct buffer_head *group_bh = NULL; > u64 bg_blkno; > - int status; > + int status, quiet = 0; > > trace_ocfs2_test_suballoc_bit((unsigned long long)blkno, > (unsigned int)bit); > @@ -3053,11 +3053,16 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb, > > bg_blkno = group_blkno ? group_blkno : > ocfs2_which_suballoc_group(blkno, bit); > - status = ocfs2_read_group_descriptor(suballoc, alloc_di, bg_blkno, > + status = ocfs2_read_hint_group_descriptor(suballoc, alloc_di, bg_blkno, > &group_bh); > if (status < 0) { > - mlog(ML_ERROR, "read group %llu failed %d\n", > - (unsigned long long)bg_blkno, status); > + if (status == -EIDRM) { > + quiet = 1; > + status = -EINVAL; > + } else { > + mlog(ML_ERROR, "read group %llu failed %d\n", > + (unsigned long long)bg_blkno, status); > + } > goto bail; > } > > @@ -3067,7 +3072,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb, > bail: > brelse(group_bh); > > - if (status) > + if (status && (!quiet)) > mlog_errno(status); > return status; > } > @@ -3087,7 +3092,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb, > */ > int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res) > { > - int status; > + int status, quiet = 0; > u64 group_blkno = 0; > u16 suballoc_bit = 0, suballoc_slot = 0; > struct inode *inode_alloc_inode; > @@ -3129,8 +3134,12 @@ int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res) > > status = ocfs2_test_suballoc_bit(osb, inode_alloc_inode, alloc_bh, > group_blkno, blkno, suballoc_bit, res); > - if (status < 0) > - mlog(ML_ERROR, "test suballoc bit failed %d\n", status); > + if (status < 0) { > + if (status == -EINVAL) > + quiet = 1; > + else > + mlog(ML_ERROR, "test suballoc bit failed %d\n", status); > + } > > ocfs2_inode_unlock(inode_alloc_inode, 0); > inode_unlock(inode_alloc_inode); > @@ -3138,7 +3147,7 @@ int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res) > iput(inode_alloc_inode); > brelse(alloc_bh); > bail: > - if (status) > + if (status && !quiet) > mlog_errno(status); > return status; > }
On 2024/7/30 09:24, Su Yue wrote: > > > On 2024/7/29 16:04, Heming Zhao wrote: >> After ocfs2 has the ability to reclaim suballoc free bg, the Since the patch fixes a regression which patch[1] causes, better to document the patch explicitly. e.g. After commit "ocfs2: give ocfs2 the ability to reclaim suballoc free bg", ocfs2 has the ability to reclaim suballoc free bg, the suballocator block group may be released. No need to resend I think. Joseph, could you amend the message if the patch is fine to you? -- Su >> suballocator block group may be released. This change makes xfstest >> case 426 failed. >> >> The existed code call stack: >> >> ocfs2_fh_to_dentry //or ocfs2_fh_to_parent >> ocfs2_get_dentry >> ocfs2_test_inode_bit >> ocfs2_test_suballoc_bit >> ocfs2_read_group_descriptor >> + read released bg, triggers validate fails, then cause -EROFS >> >> how to fix: >> The read bg failure is expectation, we should ignore this error. >> >> Signed-off-by: Heming Zhao <heming.zhao@suse.com> > > LGTM: > Reviewed-by: Su Yue <glass.su@suse.com> >> --- >> fs/ocfs2/suballoc.c | 27 ++++++++++++++++++--------- >> 1 file changed, 18 insertions(+), 9 deletions(-) >> >> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c >> index 1b64f4c87607..dc421f55ed8f 100644 >> --- a/fs/ocfs2/suballoc.c >> +++ b/fs/ocfs2/suballoc.c >> @@ -3037,7 +3037,7 @@ static int ocfs2_test_suballoc_bit(struct >> ocfs2_super *osb, >> struct ocfs2_group_desc *group; >> struct buffer_head *group_bh = NULL; >> u64 bg_blkno; >> - int status; >> + int status, quiet = 0; >> trace_ocfs2_test_suballoc_bit((unsigned long long)blkno, >> (unsigned int)bit); >> @@ -3053,11 +3053,16 @@ static int ocfs2_test_suballoc_bit(struct >> ocfs2_super *osb, >> bg_blkno = group_blkno ? group_blkno : >> ocfs2_which_suballoc_group(blkno, bit); >> - status = ocfs2_read_group_descriptor(suballoc, alloc_di, bg_blkno, >> + status = ocfs2_read_hint_group_descriptor(suballoc, alloc_di, >> bg_blkno, >> &group_bh); >> if (status < 0) { >> - mlog(ML_ERROR, "read group %llu failed %d\n", >> - (unsigned long long)bg_blkno, status); >> + if (status == -EIDRM) { >> + quiet = 1; >> + status = -EINVAL; >> + } else { >> + mlog(ML_ERROR, "read group %llu failed %d\n", >> + (unsigned long long)bg_blkno, status); >> + } >> goto bail; >> } >> @@ -3067,7 +3072,7 @@ static int ocfs2_test_suballoc_bit(struct >> ocfs2_super *osb, >> bail: >> brelse(group_bh); >> - if (status) >> + if (status && (!quiet)) >> mlog_errno(status); >> return status; >> } >> @@ -3087,7 +3092,7 @@ static int ocfs2_test_suballoc_bit(struct >> ocfs2_super *osb, >> */ >> int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res) >> { >> - int status; >> + int status, quiet = 0; >> u64 group_blkno = 0; >> u16 suballoc_bit = 0, suballoc_slot = 0; >> struct inode *inode_alloc_inode; >> @@ -3129,8 +3134,12 @@ int ocfs2_test_inode_bit(struct ocfs2_super >> *osb, u64 blkno, int *res) >> status = ocfs2_test_suballoc_bit(osb, inode_alloc_inode, alloc_bh, >> group_blkno, blkno, suballoc_bit, res); >> - if (status < 0) >> - mlog(ML_ERROR, "test suballoc bit failed %d\n", status); >> + if (status < 0) { >> + if (status == -EINVAL) >> + quiet = 1; >> + else >> + mlog(ML_ERROR, "test suballoc bit failed %d\n", status); >> + } >> ocfs2_inode_unlock(inode_alloc_inode, 0); >> inode_unlock(inode_alloc_inode); >> @@ -3138,7 +3147,7 @@ int ocfs2_test_inode_bit(struct ocfs2_super >> *osb, u64 blkno, int *res) >> iput(inode_alloc_inode); >> brelse(alloc_bh); >> bail: >> - if (status) >> + if (status && !quiet) >> mlog_errno(status); >> return status; >> }
On 7/29/24 4:04 PM, Heming Zhao wrote: > After ocfs2 has the ability to reclaim suballoc free bg, the > suballocator block group may be released. This change makes xfstest > case 426 failed. > > The existed code call stack: > > ocfs2_fh_to_dentry //or ocfs2_fh_to_parent > ocfs2_get_dentry > ocfs2_test_inode_bit > ocfs2_test_suballoc_bit > ocfs2_read_group_descriptor > + read released bg, triggers validate fails, then cause -EROFS > > how to fix: > The read bg failure is expectation, we should ignore this error. > > Signed-off-by: Heming Zhao <heming.zhao@suse.com> > --- > fs/ocfs2/suballoc.c | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c > index 1b64f4c87607..dc421f55ed8f 100644 > --- a/fs/ocfs2/suballoc.c > +++ b/fs/ocfs2/suballoc.c > @@ -3037,7 +3037,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb, > struct ocfs2_group_desc *group; > struct buffer_head *group_bh = NULL; > u64 bg_blkno; > - int status; > + int status, quiet = 0; > > trace_ocfs2_test_suballoc_bit((unsigned long long)blkno, > (unsigned int)bit); > @@ -3053,11 +3053,16 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb, > > bg_blkno = group_blkno ? group_blkno : > ocfs2_which_suballoc_group(blkno, bit); > - status = ocfs2_read_group_descriptor(suballoc, alloc_di, bg_blkno, > + status = ocfs2_read_hint_group_descriptor(suballoc, alloc_di, bg_blkno, > &group_bh); > if (status < 0) { > - mlog(ML_ERROR, "read group %llu failed %d\n", > - (unsigned long long)bg_blkno, status); > + if (status == -EIDRM) { I don't think EIDRM is a proper error code in this case. We can pass a output parameter or a flag to indicate a released one. And please explicitly specify that this can be treated normally in the related code logic of ocfs2_read_hint_group_descriptor(). Thanks, Joseph > + quiet = 1; > + status = -EINVAL; > + } else { > + mlog(ML_ERROR, "read group %llu failed %d\n", > + (unsigned long long)bg_blkno, status); > + } > goto bail; > } > > @@ -3067,7 +3072,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb, > bail: > brelse(group_bh); > > - if (status) > + if (status && (!quiet)) > mlog_errno(status); > return status; > } > @@ -3087,7 +3092,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb, > */ > int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res) > { > - int status; > + int status, quiet = 0; > u64 group_blkno = 0; > u16 suballoc_bit = 0, suballoc_slot = 0; > struct inode *inode_alloc_inode; > @@ -3129,8 +3134,12 @@ int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res) > > status = ocfs2_test_suballoc_bit(osb, inode_alloc_inode, alloc_bh, > group_blkno, blkno, suballoc_bit, res); > - if (status < 0) > - mlog(ML_ERROR, "test suballoc bit failed %d\n", status); > + if (status < 0) { > + if (status == -EINVAL) > + quiet = 1; > + else > + mlog(ML_ERROR, "test suballoc bit failed %d\n", status); > + } > > ocfs2_inode_unlock(inode_alloc_inode, 0); > inode_unlock(inode_alloc_inode); > @@ -3138,7 +3147,7 @@ int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res) > iput(inode_alloc_inode); > brelse(alloc_bh); > bail: > - if (status) > + if (status && !quiet) > mlog_errno(status); > return status; > }
diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c index 1b64f4c87607..dc421f55ed8f 100644 --- a/fs/ocfs2/suballoc.c +++ b/fs/ocfs2/suballoc.c @@ -3037,7 +3037,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb, struct ocfs2_group_desc *group; struct buffer_head *group_bh = NULL; u64 bg_blkno; - int status; + int status, quiet = 0; trace_ocfs2_test_suballoc_bit((unsigned long long)blkno, (unsigned int)bit); @@ -3053,11 +3053,16 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb, bg_blkno = group_blkno ? group_blkno : ocfs2_which_suballoc_group(blkno, bit); - status = ocfs2_read_group_descriptor(suballoc, alloc_di, bg_blkno, + status = ocfs2_read_hint_group_descriptor(suballoc, alloc_di, bg_blkno, &group_bh); if (status < 0) { - mlog(ML_ERROR, "read group %llu failed %d\n", - (unsigned long long)bg_blkno, status); + if (status == -EIDRM) { + quiet = 1; + status = -EINVAL; + } else { + mlog(ML_ERROR, "read group %llu failed %d\n", + (unsigned long long)bg_blkno, status); + } goto bail; } @@ -3067,7 +3072,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb, bail: brelse(group_bh); - if (status) + if (status && (!quiet)) mlog_errno(status); return status; } @@ -3087,7 +3092,7 @@ static int ocfs2_test_suballoc_bit(struct ocfs2_super *osb, */ int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res) { - int status; + int status, quiet = 0; u64 group_blkno = 0; u16 suballoc_bit = 0, suballoc_slot = 0; struct inode *inode_alloc_inode; @@ -3129,8 +3134,12 @@ int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res) status = ocfs2_test_suballoc_bit(osb, inode_alloc_inode, alloc_bh, group_blkno, blkno, suballoc_bit, res); - if (status < 0) - mlog(ML_ERROR, "test suballoc bit failed %d\n", status); + if (status < 0) { + if (status == -EINVAL) + quiet = 1; + else + mlog(ML_ERROR, "test suballoc bit failed %d\n", status); + } ocfs2_inode_unlock(inode_alloc_inode, 0); inode_unlock(inode_alloc_inode); @@ -3138,7 +3147,7 @@ int ocfs2_test_inode_bit(struct ocfs2_super *osb, u64 blkno, int *res) iput(inode_alloc_inode); brelse(alloc_bh); bail: - if (status) + if (status && !quiet) mlog_errno(status); return status; }
After ocfs2 has the ability to reclaim suballoc free bg, the suballocator block group may be released. This change makes xfstest case 426 failed. The existed code call stack: ocfs2_fh_to_dentry //or ocfs2_fh_to_parent ocfs2_get_dentry ocfs2_test_inode_bit ocfs2_test_suballoc_bit ocfs2_read_group_descriptor + read released bg, triggers validate fails, then cause -EROFS how to fix: The read bg failure is expectation, we should ignore this error. Signed-off-by: Heming Zhao <heming.zhao@suse.com> --- fs/ocfs2/suballoc.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)