* [Ocfs2-devel] [PATCH] ocfs2: check the metadate alloc before marking extent written @ 2017-12-05 1:41 alex chen 2017-12-05 2:21 ` Joseph Qi 0 siblings, 1 reply; 6+ messages in thread From: alex chen @ 2017-12-05 1:41 UTC (permalink / raw) To: ocfs2-devel We need to check the free number of the records in each loop to mark extent written, because the last extent block may be changed through many times marking extent written and the 'num_free_extents' also be changed. In the worst case, the 'num_free_extents' may become less than the beginning of the loop. So we should not estimate the free number of the records at the beginning of the loop to mark extent written. Reported-by: John Lightsey <john@nixnuts.net> Signed-off-by: Alex Chen <alex.chen@huawei.com> Reviewed-by: Jun Piao <piaojun@huawei.com> --- fs/ocfs2/aops.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 64 insertions(+), 12 deletions(-) diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c index d151632..702aebc 100644 --- a/fs/ocfs2/aops.c +++ b/fs/ocfs2/aops.c @@ -2272,6 +2272,35 @@ static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock, return ret; } +static int ocfs2_dio_should_restart(struct ocfs2_extent_tree *et, + struct ocfs2_alloc_context *meta_ac, int max_rec_needed) +{ + int status = 0, free_extents; + + free_extents = ocfs2_num_free_extents(et); + if (free_extents < 0) { + status = free_extents; + mlog_errno(status); + return status; + } + + /* + * there are two cases which could cause us to EAGAIN in the + * we-need-more-metadata case: + * 1) we haven't reserved *any* + * 2) we are so fragmented, we've needed to add metadata too + * many times. + */ + if (free_extents < max_rec_needed) { + if (!meta_ac || (ocfs2_alloc_context_bits_left(meta_ac) + < ocfs2_extend_meta_needed(et->et_root_el))) + status = 1; + } + + return status; +} + +#define OCFS2_MAX_REC_NEEDED_SPLIT 2 static int ocfs2_dio_end_io_write(struct inode *inode, struct ocfs2_dio_write_ctxt *dwc, loff_t offset, @@ -2281,14 +2310,13 @@ static int ocfs2_dio_end_io_write(struct inode *inode, struct ocfs2_extent_tree et; struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); struct ocfs2_inode_info *oi = OCFS2_I(inode); - struct ocfs2_unwritten_extent *ue = NULL; + struct ocfs2_unwritten_extent *ue = NULL, *tmp_ue; struct buffer_head *di_bh = NULL; struct ocfs2_dinode *di; - struct ocfs2_alloc_context *data_ac = NULL; struct ocfs2_alloc_context *meta_ac = NULL; handle_t *handle = NULL; loff_t end = offset + bytes; - int ret = 0, credits = 0, locked = 0; + int ret = 0, credits = 0, locked = 0, restart = 0; ocfs2_init_dealloc_ctxt(&dealloc); @@ -2330,8 +2358,9 @@ static int ocfs2_dio_end_io_write(struct inode *inode, ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh); - ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2, - &data_ac, &meta_ac); +restart_all: + ret = ocfs2_lock_allocators(inode, &et, 0, OCFS2_MAX_REC_NEEDED_SPLIT, + NULL, &meta_ac); if (ret) { mlog_errno(ret); goto unlock; @@ -2343,7 +2372,7 @@ static int ocfs2_dio_end_io_write(struct inode *inode, if (IS_ERR(handle)) { ret = PTR_ERR(handle); mlog_errno(ret); - goto unlock; + goto free_ac; } ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh, OCFS2_JOURNAL_ACCESS_WRITE); @@ -2352,7 +2381,17 @@ static int ocfs2_dio_end_io_write(struct inode *inode, goto commit; } - list_for_each_entry(ue, &dwc->dw_zero_list, ue_node) { + list_for_each_entry_safe(ue, tmp_ue, &dwc->dw_zero_list, ue_node) { + ret = ocfs2_dio_should_restart(&et, meta_ac, + OCFS2_MAX_REC_NEEDED_SPLIT * 2); + if (ret < 0) { + mlog_errno(ret); + break; + } else if (ret == 1) { + restart = 1; + break; + } + ret = ocfs2_mark_extent_written(inode, &et, handle, ue->ue_cpos, 1, ue->ue_phys, @@ -2361,24 +2400,37 @@ static int ocfs2_dio_end_io_write(struct inode *inode, mlog_errno(ret); break; } + + dwc->dw_zero_count--; + list_del_init(&ue->ue_node); + spin_lock(&oi->ip_lock); + list_del_init(&ue->ue_ip_node); + spin_unlock(&oi->ip_lock); + kfree(ue); } - if (end > i_size_read(inode)) { + if (!restart && end > i_size_read(inode)) { ret = ocfs2_set_inode_size(handle, inode, di_bh, end); if (ret < 0) mlog_errno(ret); } + commit: ocfs2_commit_trans(osb, handle); +free_ac: + if (meta_ac) { + ocfs2_free_alloc_context(meta_ac); + meta_ac = NULL; + } + if (restart) { + restart = 0; + goto restart_all; + } unlock: up_write(&oi->ip_alloc_sem); ocfs2_inode_unlock(inode, 1); brelse(di_bh); out: - if (data_ac) - ocfs2_free_alloc_context(data_ac); - if (meta_ac) - ocfs2_free_alloc_context(meta_ac); ocfs2_run_deallocs(osb, &dealloc); if (locked) inode_unlock(inode); -- 1.9.5.msysgit.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check the metadate alloc before marking extent written 2017-12-05 1:41 [Ocfs2-devel] [PATCH] ocfs2: check the metadate alloc before marking extent written alex chen @ 2017-12-05 2:21 ` Joseph Qi 2017-12-05 2:36 ` alex chen 0 siblings, 1 reply; 6+ messages in thread From: Joseph Qi @ 2017-12-05 2:21 UTC (permalink / raw) To: ocfs2-devel On 17/12/5 09:41, alex chen wrote: > We need to check the free number of the records in each loop to mark > extent written, because the last extent block may be changed through > many times marking extent written and the 'num_free_extents' also be > changed. In the worst case, the 'num_free_extents' may become less > than the beginning of the loop. So we should not estimate the free > number of the records at the beginning of the loop to mark extent > written. > > Reported-by: John Lightsey <john@nixnuts.net> > Signed-off-by: Alex Chen <alex.chen@huawei.com> > Reviewed-by: Jun Piao <piaojun@huawei.com> > --- > fs/ocfs2/aops.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 64 insertions(+), 12 deletions(-) > > diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c > index d151632..702aebc 100644 > --- a/fs/ocfs2/aops.c > +++ b/fs/ocfs2/aops.c > @@ -2272,6 +2272,35 @@ static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock, > return ret; > } > > +static int ocfs2_dio_should_restart(struct ocfs2_extent_tree *et, > + struct ocfs2_alloc_context *meta_ac, int max_rec_needed) > +{ > + int status = 0, free_extents; > + > + free_extents = ocfs2_num_free_extents(et); > + if (free_extents < 0) { > + status = free_extents; > + mlog_errno(status); > + return status; > + } > + > + /* > + * there are two cases which could cause us to EAGAIN in the > + * we-need-more-metadata case: > + * 1) we haven't reserved *any* > + * 2) we are so fragmented, we've needed to add metadata too > + * many times. > + */ > + if (free_extents < max_rec_needed) { > + if (!meta_ac || (ocfs2_alloc_context_bits_left(meta_ac) > + < ocfs2_extend_meta_needed(et->et_root_el))) > + status = 1; > + } > + > + return status; > +} > + > +#define OCFS2_MAX_REC_NEEDED_SPLIT 2 > static int ocfs2_dio_end_io_write(struct inode *inode, > struct ocfs2_dio_write_ctxt *dwc, > loff_t offset, > @@ -2281,14 +2310,13 @@ static int ocfs2_dio_end_io_write(struct inode *inode, > struct ocfs2_extent_tree et; > struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); > struct ocfs2_inode_info *oi = OCFS2_I(inode); > - struct ocfs2_unwritten_extent *ue = NULL; > + struct ocfs2_unwritten_extent *ue = NULL, *tmp_ue; > struct buffer_head *di_bh = NULL; > struct ocfs2_dinode *di; > - struct ocfs2_alloc_context *data_ac = NULL; > struct ocfs2_alloc_context *meta_ac = NULL; > handle_t *handle = NULL; > loff_t end = offset + bytes; > - int ret = 0, credits = 0, locked = 0; > + int ret = 0, credits = 0, locked = 0, restart = 0; > > ocfs2_init_dealloc_ctxt(&dealloc); > > @@ -2330,8 +2358,9 @@ static int ocfs2_dio_end_io_write(struct inode *inode, > Should we restart here? > ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh); > > - ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2, > - &data_ac, &meta_ac); > +restart_all: > + ret = ocfs2_lock_allocators(inode, &et, 0, OCFS2_MAX_REC_NEEDED_SPLIT, > + NULL, &meta_ac); > if (ret) { > mlog_errno(ret); > goto unlock; > @@ -2343,7 +2372,7 @@ static int ocfs2_dio_end_io_write(struct inode *inode, > if (IS_ERR(handle)) { > ret = PTR_ERR(handle); > mlog_errno(ret); > - goto unlock; > + goto free_ac; I don't think it is a necessary change here. > } > ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh, > OCFS2_JOURNAL_ACCESS_WRITE); > @@ -2352,7 +2381,17 @@ static int ocfs2_dio_end_io_write(struct inode *inode, > goto commit; > } > > - list_for_each_entry(ue, &dwc->dw_zero_list, ue_node) { > + list_for_each_entry_safe(ue, tmp_ue, &dwc->dw_zero_list, ue_node) { > + ret = ocfs2_dio_should_restart(&et, meta_ac, > + OCFS2_MAX_REC_NEEDED_SPLIT * 2); > + if (ret < 0) { > + mlog_errno(ret); > + break; > + } else if (ret == 1) { > + restart = 1; > + break; > + } > + > ret = ocfs2_mark_extent_written(inode, &et, handle, > ue->ue_cpos, 1, > ue->ue_phys, > @@ -2361,24 +2400,37 @@ static int ocfs2_dio_end_io_write(struct inode *inode, > mlog_errno(ret); > break; > } > + > + dwc->dw_zero_count--; > + list_del_init(&ue->ue_node); > + spin_lock(&oi->ip_lock); > + list_del_init(&ue->ue_ip_node); > + spin_unlock(&oi->ip_lock); > + kfree(ue); > } > > - if (end > i_size_read(inode)) { > + if (!restart && end > i_size_read(inode)) { > ret = ocfs2_set_inode_size(handle, inode, di_bh, end); > if (ret < 0) > mlog_errno(ret); > } > + > commit: > ocfs2_commit_trans(osb, handle); > +free_ac: > + if (meta_ac) { > + ocfs2_free_alloc_context(meta_ac); > + meta_ac = NULL; > + } > + if (restart) { > + restart = 0; > + goto restart_all; > + } > unlock: > up_write(&oi->ip_alloc_sem); > ocfs2_inode_unlock(inode, 1); > brelse(di_bh); > out: > - if (data_ac) > - ocfs2_free_alloc_context(data_ac); Agree to cleanup the unused data_ac. > - if (meta_ac) > - ocfs2_free_alloc_context(meta_ac);Just move the restart logic down is enough. Thanks, Joseph > ocfs2_run_deallocs(osb, &dealloc); > if (locked) > inode_unlock(inode); > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check the metadate alloc before marking extent written 2017-12-05 2:21 ` Joseph Qi @ 2017-12-05 2:36 ` alex chen 2017-12-05 2:45 ` Joseph Qi 0 siblings, 1 reply; 6+ messages in thread From: alex chen @ 2017-12-05 2:36 UTC (permalink / raw) To: ocfs2-devel Hi Joseph, Thanks for your reviews. On 2017/12/5 10:21, Joseph Qi wrote: > > > On 17/12/5 09:41, alex chen wrote: >> We need to check the free number of the records in each loop to mark >> extent written, because the last extent block may be changed through >> many times marking extent written and the 'num_free_extents' also be >> changed. In the worst case, the 'num_free_extents' may become less >> than the beginning of the loop. So we should not estimate the free >> number of the records at the beginning of the loop to mark extent >> written. >> >> Reported-by: John Lightsey <john@nixnuts.net> >> Signed-off-by: Alex Chen <alex.chen@huawei.com> >> Reviewed-by: Jun Piao <piaojun@huawei.com> >> --- >> fs/ocfs2/aops.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 64 insertions(+), 12 deletions(-) >> >> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c >> index d151632..702aebc 100644 >> --- a/fs/ocfs2/aops.c >> +++ b/fs/ocfs2/aops.c >> @@ -2272,6 +2272,35 @@ static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock, >> return ret; >> } >> >> +static int ocfs2_dio_should_restart(struct ocfs2_extent_tree *et, >> + struct ocfs2_alloc_context *meta_ac, int max_rec_needed) >> +{ >> + int status = 0, free_extents; >> + >> + free_extents = ocfs2_num_free_extents(et); >> + if (free_extents < 0) { >> + status = free_extents; >> + mlog_errno(status); >> + return status; >> + } >> + >> + /* >> + * there are two cases which could cause us to EAGAIN in the >> + * we-need-more-metadata case: >> + * 1) we haven't reserved *any* >> + * 2) we are so fragmented, we've needed to add metadata too >> + * many times. >> + */ >> + if (free_extents < max_rec_needed) { >> + if (!meta_ac || (ocfs2_alloc_context_bits_left(meta_ac) >> + < ocfs2_extend_meta_needed(et->et_root_el))) >> + status = 1; >> + } >> + >> + return status; >> +} >> + >> +#define OCFS2_MAX_REC_NEEDED_SPLIT 2 >> static int ocfs2_dio_end_io_write(struct inode *inode, >> struct ocfs2_dio_write_ctxt *dwc, >> loff_t offset, >> @@ -2281,14 +2310,13 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >> struct ocfs2_extent_tree et; >> struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); >> struct ocfs2_inode_info *oi = OCFS2_I(inode); >> - struct ocfs2_unwritten_extent *ue = NULL; >> + struct ocfs2_unwritten_extent *ue = NULL, *tmp_ue; >> struct buffer_head *di_bh = NULL; >> struct ocfs2_dinode *di; >> - struct ocfs2_alloc_context *data_ac = NULL; >> struct ocfs2_alloc_context *meta_ac = NULL; >> handle_t *handle = NULL; >> loff_t end = offset + bytes; >> - int ret = 0, credits = 0, locked = 0; >> + int ret = 0, credits = 0, locked = 0, restart = 0; >> >> ocfs2_init_dealloc_ctxt(&dealloc); >> >> @@ -2330,8 +2358,9 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >> > Should we restart here? OK. we should restart before initialized the inode extent tree. > >> ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh); >> >> - ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2, >> - &data_ac, &meta_ac); >> +restart_all: >> + ret = ocfs2_lock_allocators(inode, &et, 0, OCFS2_MAX_REC_NEEDED_SPLIT, >> + NULL, &meta_ac); >> if (ret) { >> mlog_errno(ret); >> goto unlock; >> @@ -2343,7 +2372,7 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >> if (IS_ERR(handle)) { >> ret = PTR_ERR(handle); >> mlog_errno(ret); >> - goto unlock; >> + goto free_ac; > I don't think it is a necessary change here. we need to free the 'meta_ac' which allocated in ocfs2_lock_allocators() when we start transaction failed. Thanks, Alex > >> } >> ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh, >> OCFS2_JOURNAL_ACCESS_WRITE); >> @@ -2352,7 +2381,17 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >> goto commit; >> } >> >> - list_for_each_entry(ue, &dwc->dw_zero_list, ue_node) { >> + list_for_each_entry_safe(ue, tmp_ue, &dwc->dw_zero_list, ue_node) { >> + ret = ocfs2_dio_should_restart(&et, meta_ac, >> + OCFS2_MAX_REC_NEEDED_SPLIT * 2); >> + if (ret < 0) { >> + mlog_errno(ret); >> + break; >> + } else if (ret == 1) { >> + restart = 1; >> + break; >> + } >> + >> ret = ocfs2_mark_extent_written(inode, &et, handle, >> ue->ue_cpos, 1, >> ue->ue_phys, >> @@ -2361,24 +2400,37 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >> mlog_errno(ret); >> break; >> } >> + >> + dwc->dw_zero_count--; >> + list_del_init(&ue->ue_node); >> + spin_lock(&oi->ip_lock); >> + list_del_init(&ue->ue_ip_node); >> + spin_unlock(&oi->ip_lock); >> + kfree(ue); >> } >> >> - if (end > i_size_read(inode)) { >> + if (!restart && end > i_size_read(inode)) { >> ret = ocfs2_set_inode_size(handle, inode, di_bh, end); >> if (ret < 0) >> mlog_errno(ret); >> } >> + >> commit: >> ocfs2_commit_trans(osb, handle); >> +free_ac: >> + if (meta_ac) { >> + ocfs2_free_alloc_context(meta_ac); >> + meta_ac = NULL; >> + } >> + if (restart) { >> + restart = 0; >> + goto restart_all; >> + } >> unlock: >> up_write(&oi->ip_alloc_sem); >> ocfs2_inode_unlock(inode, 1); >> brelse(di_bh); >> out: >> - if (data_ac) >> - ocfs2_free_alloc_context(data_ac); > Agree to cleanup the unused data_ac. > >> - if (meta_ac) >> - ocfs2_free_alloc_context(meta_ac);Just move the restart logic down is enough. > > Thanks, > Joseph > >> ocfs2_run_deallocs(osb, &dealloc); >> if (locked) >> inode_unlock(inode); >> > > . > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check the metadate alloc before marking extent written 2017-12-05 2:36 ` alex chen @ 2017-12-05 2:45 ` Joseph Qi 2017-12-05 3:09 ` alex chen 0 siblings, 1 reply; 6+ messages in thread From: Joseph Qi @ 2017-12-05 2:45 UTC (permalink / raw) To: ocfs2-devel On 17/12/5 10:36, alex chen wrote: > Hi Joseph, > > Thanks for your reviews. > > On 2017/12/5 10:21, Joseph Qi wrote: >> >> >> On 17/12/5 09:41, alex chen wrote: >>> We need to check the free number of the records in each loop to mark >>> extent written, because the last extent block may be changed through >>> many times marking extent written and the 'num_free_extents' also be >>> changed. In the worst case, the 'num_free_extents' may become less >>> than the beginning of the loop. So we should not estimate the free >>> number of the records at the beginning of the loop to mark extent >>> written. >>> >>> Reported-by: John Lightsey <john@nixnuts.net> >>> Signed-off-by: Alex Chen <alex.chen@huawei.com> >>> Reviewed-by: Jun Piao <piaojun@huawei.com> >>> --- >>> fs/ocfs2/aops.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++--------- >>> 1 file changed, 64 insertions(+), 12 deletions(-) >>> >>> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c >>> index d151632..702aebc 100644 >>> --- a/fs/ocfs2/aops.c >>> +++ b/fs/ocfs2/aops.c >>> @@ -2272,6 +2272,35 @@ static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock, >>> return ret; >>> } >>> >>> +static int ocfs2_dio_should_restart(struct ocfs2_extent_tree *et, >>> + struct ocfs2_alloc_context *meta_ac, int max_rec_needed) >>> +{ >>> + int status = 0, free_extents; >>> + >>> + free_extents = ocfs2_num_free_extents(et); >>> + if (free_extents < 0) { >>> + status = free_extents; >>> + mlog_errno(status); >>> + return status; >>> + } >>> + >>> + /* >>> + * there are two cases which could cause us to EAGAIN in the >>> + * we-need-more-metadata case: >>> + * 1) we haven't reserved *any* >>> + * 2) we are so fragmented, we've needed to add metadata too >>> + * many times. >>> + */ >>> + if (free_extents < max_rec_needed) { >>> + if (!meta_ac || (ocfs2_alloc_context_bits_left(meta_ac) >>> + < ocfs2_extend_meta_needed(et->et_root_el))) >>> + status = 1; >>> + } >>> + >>> + return status; >>> +} >>> + >>> +#define OCFS2_MAX_REC_NEEDED_SPLIT 2 >>> static int ocfs2_dio_end_io_write(struct inode *inode, >>> struct ocfs2_dio_write_ctxt *dwc, >>> loff_t offset, >>> @@ -2281,14 +2310,13 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>> struct ocfs2_extent_tree et; >>> struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); >>> struct ocfs2_inode_info *oi = OCFS2_I(inode); >>> - struct ocfs2_unwritten_extent *ue = NULL; >>> + struct ocfs2_unwritten_extent *ue = NULL, *tmp_ue; >>> struct buffer_head *di_bh = NULL; >>> struct ocfs2_dinode *di; >>> - struct ocfs2_alloc_context *data_ac = NULL; >>> struct ocfs2_alloc_context *meta_ac = NULL; >>> handle_t *handle = NULL; >>> loff_t end = offset + bytes; >>> - int ret = 0, credits = 0, locked = 0; >>> + int ret = 0, credits = 0, locked = 0, restart = 0; >>> >>> ocfs2_init_dealloc_ctxt(&dealloc); >>> >>> @@ -2330,8 +2358,9 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>> >> Should we restart here? > > OK. we should restart before initialized the inode extent tree. > >> >>> ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh); >>> >>> - ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2, >>> - &data_ac, &meta_ac); >>> +restart_all: >>> + ret = ocfs2_lock_allocators(inode, &et, 0, OCFS2_MAX_REC_NEEDED_SPLIT, >>> + NULL, &meta_ac); >>> if (ret) { >>> mlog_errno(ret); >>> goto unlock; >>> @@ -2343,7 +2372,7 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>> if (IS_ERR(handle)) { >>> ret = PTR_ERR(handle); >>> mlog_errno(ret); >>> - goto unlock; >>> + goto free_ac; >> I don't think it is a necessary change here. > > we need to free the 'meta_ac' which allocated in ocfs2_lock_allocators() when we > start transaction failed. > As I described in the last mail, just move the restart check after the original free meta_ac check. > Thanks, > Alex >> >>> } >>> ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh, >>> OCFS2_JOURNAL_ACCESS_WRITE); >>> @@ -2352,7 +2381,17 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>> goto commit; >>> } >>> >>> - list_for_each_entry(ue, &dwc->dw_zero_list, ue_node) { >>> + list_for_each_entry_safe(ue, tmp_ue, &dwc->dw_zero_list, ue_node) { >>> + ret = ocfs2_dio_should_restart(&et, meta_ac, >>> + OCFS2_MAX_REC_NEEDED_SPLIT * 2); >>> + if (ret < 0) { >>> + mlog_errno(ret); >>> + break; >>> + } else if (ret == 1) { >>> + restart = 1; >>> + break; >>> + } >>> + >>> ret = ocfs2_mark_extent_written(inode, &et, handle, >>> ue->ue_cpos, 1, >>> ue->ue_phys, >>> @@ -2361,24 +2400,37 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>> mlog_errno(ret); >>> break; >>> } >>> + >>> + dwc->dw_zero_count--; >>> + list_del_init(&ue->ue_node); >>> + spin_lock(&oi->ip_lock); >>> + list_del_init(&ue->ue_ip_node); >>> + spin_unlock(&oi->ip_lock); >>> + kfree(ue); >>> } >>> >>> - if (end > i_size_read(inode)) { >>> + if (!restart && end > i_size_read(inode)) { >>> ret = ocfs2_set_inode_size(handle, inode, di_bh, end); >>> if (ret < 0) >>> mlog_errno(ret); >>> } >>> + >>> commit: >>> ocfs2_commit_trans(osb, handle); >>> +free_ac: >>> + if (meta_ac) { >>> + ocfs2_free_alloc_context(meta_ac); >>> + meta_ac = NULL; >>> + } >>> + if (restart) { >>> + restart = 0; >>> + goto restart_all; >>> + } >>> unlock: >>> up_write(&oi->ip_alloc_sem); >>> ocfs2_inode_unlock(inode, 1); >>> brelse(di_bh); >>> out: >>> - if (data_ac) >>> - ocfs2_free_alloc_context(data_ac); >> Agree to cleanup the unused data_ac. >> >>> - if (meta_ac) >>> - ocfs2_free_alloc_context(meta_ac); Just move the restart logic down is enough. >> >> Thanks, >> Joseph >> >>> ocfs2_run_deallocs(osb, &dealloc); >>> if (locked) >>> inode_unlock(inode); >>> >> >> . >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check the metadate alloc before marking extent written 2017-12-05 2:45 ` Joseph Qi @ 2017-12-05 3:09 ` alex chen 2017-12-05 3:16 ` Joseph Qi 0 siblings, 1 reply; 6+ messages in thread From: alex chen @ 2017-12-05 3:09 UTC (permalink / raw) To: ocfs2-devel On 2017/12/5 10:45, Joseph Qi wrote: > > > On 17/12/5 10:36, alex chen wrote: >> Hi Joseph, >> >> Thanks for your reviews. >> >> On 2017/12/5 10:21, Joseph Qi wrote: >>> >>> >>> On 17/12/5 09:41, alex chen wrote: >>>> We need to check the free number of the records in each loop to mark >>>> extent written, because the last extent block may be changed through >>>> many times marking extent written and the 'num_free_extents' also be >>>> changed. In the worst case, the 'num_free_extents' may become less >>>> than the beginning of the loop. So we should not estimate the free >>>> number of the records at the beginning of the loop to mark extent >>>> written. >>>> >>>> Reported-by: John Lightsey <john@nixnuts.net> >>>> Signed-off-by: Alex Chen <alex.chen@huawei.com> >>>> Reviewed-by: Jun Piao <piaojun@huawei.com> >>>> --- >>>> fs/ocfs2/aops.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++--------- >>>> 1 file changed, 64 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c >>>> index d151632..702aebc 100644 >>>> --- a/fs/ocfs2/aops.c >>>> +++ b/fs/ocfs2/aops.c >>>> @@ -2272,6 +2272,35 @@ static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock, >>>> return ret; >>>> } >>>> >>>> +static int ocfs2_dio_should_restart(struct ocfs2_extent_tree *et, >>>> + struct ocfs2_alloc_context *meta_ac, int max_rec_needed) >>>> +{ >>>> + int status = 0, free_extents; >>>> + >>>> + free_extents = ocfs2_num_free_extents(et); >>>> + if (free_extents < 0) { >>>> + status = free_extents; >>>> + mlog_errno(status); >>>> + return status; >>>> + } >>>> + >>>> + /* >>>> + * there are two cases which could cause us to EAGAIN in the >>>> + * we-need-more-metadata case: >>>> + * 1) we haven't reserved *any* >>>> + * 2) we are so fragmented, we've needed to add metadata too >>>> + * many times. >>>> + */ >>>> + if (free_extents < max_rec_needed) { >>>> + if (!meta_ac || (ocfs2_alloc_context_bits_left(meta_ac) >>>> + < ocfs2_extend_meta_needed(et->et_root_el))) >>>> + status = 1; >>>> + } >>>> + >>>> + return status; >>>> +} >>>> + >>>> +#define OCFS2_MAX_REC_NEEDED_SPLIT 2 >>>> static int ocfs2_dio_end_io_write(struct inode *inode, >>>> struct ocfs2_dio_write_ctxt *dwc, >>>> loff_t offset, >>>> @@ -2281,14 +2310,13 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>>> struct ocfs2_extent_tree et; >>>> struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); >>>> struct ocfs2_inode_info *oi = OCFS2_I(inode); >>>> - struct ocfs2_unwritten_extent *ue = NULL; >>>> + struct ocfs2_unwritten_extent *ue = NULL, *tmp_ue; >>>> struct buffer_head *di_bh = NULL; >>>> struct ocfs2_dinode *di; >>>> - struct ocfs2_alloc_context *data_ac = NULL; >>>> struct ocfs2_alloc_context *meta_ac = NULL; >>>> handle_t *handle = NULL; >>>> loff_t end = offset + bytes; >>>> - int ret = 0, credits = 0, locked = 0; >>>> + int ret = 0, credits = 0, locked = 0, restart = 0; >>>> >>>> ocfs2_init_dealloc_ctxt(&dealloc); >>>> >>>> @@ -2330,8 +2358,9 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>>> >>> Should we restart here? >> >> OK. we should restart before initialized the inode extent tree. >> >>> >>>> ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh); >>>> >>>> - ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2, >>>> - &data_ac, &meta_ac); >>>> +restart_all: >>>> + ret = ocfs2_lock_allocators(inode, &et, 0, OCFS2_MAX_REC_NEEDED_SPLIT, >>>> + NULL, &meta_ac); >>>> if (ret) { >>>> mlog_errno(ret); >>>> goto unlock; >>>> @@ -2343,7 +2372,7 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>>> if (IS_ERR(handle)) { >>>> ret = PTR_ERR(handle); >>>> mlog_errno(ret); >>>> - goto unlock; >>>> + goto free_ac; >>> I don't think it is a necessary change here. >> >> we need to free the 'meta_ac' which allocated in ocfs2_lock_allocators() when we >> start transaction failed. >> > As I described in the last mail, just move the restart check after the > original free meta_ac check. > >> Thanks, >> Alex >>> >>>> } >>>> ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh, >>>> OCFS2_JOURNAL_ACCESS_WRITE); >>>> @@ -2352,7 +2381,17 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>>> goto commit; >>>> } >>>> >>>> - list_for_each_entry(ue, &dwc->dw_zero_list, ue_node) { >>>> + list_for_each_entry_safe(ue, tmp_ue, &dwc->dw_zero_list, ue_node) { >>>> + ret = ocfs2_dio_should_restart(&et, meta_ac, >>>> + OCFS2_MAX_REC_NEEDED_SPLIT * 2); >>>> + if (ret < 0) { >>>> + mlog_errno(ret); >>>> + break; >>>> + } else if (ret == 1) { >>>> + restart = 1; >>>> + break; >>>> + } >>>> + >>>> ret = ocfs2_mark_extent_written(inode, &et, handle, >>>> ue->ue_cpos, 1, >>>> ue->ue_phys, >>>> @@ -2361,24 +2400,37 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>>> mlog_errno(ret); >>>> break; >>>> } >>>> + >>>> + dwc->dw_zero_count--; >>>> + list_del_init(&ue->ue_node); >>>> + spin_lock(&oi->ip_lock); >>>> + list_del_init(&ue->ue_ip_node); >>>> + spin_unlock(&oi->ip_lock); >>>> + kfree(ue); >>>> } >>>> >>>> - if (end > i_size_read(inode)) { >>>> + if (!restart && end > i_size_read(inode)) { >>>> ret = ocfs2_set_inode_size(handle, inode, di_bh, end); >>>> if (ret < 0) >>>> mlog_errno(ret); >>>> } >>>> + >>>> commit: >>>> ocfs2_commit_trans(osb, handle); >>>> +free_ac: >>>> + if (meta_ac) { >>>> + ocfs2_free_alloc_context(meta_ac); >>>> + meta_ac = NULL; >>>> + } >>>> + if (restart) { >>>> + restart = 0; >>>> + goto restart_all; >>>> + } >>>> unlock: >>>> up_write(&oi->ip_alloc_sem); >>>> ocfs2_inode_unlock(inode, 1); >>>> brelse(di_bh); >>>> out: >>>> - if (data_ac) >>>> - ocfs2_free_alloc_context(data_ac); >>> Agree to cleanup the unused data_ac. >>> >>>> - if (meta_ac) >>>> - ocfs2_free_alloc_context(meta_ac); > Just move the restart logic down is enough. If we check the restart here, we will unlock the 'ip_alloc_sem' and release the inode buffer, which we don't need to do. Thanks, Alex >>> >>> Thanks, >>> Joseph >>> >>>> ocfs2_run_deallocs(osb, &dealloc); >>>> if (locked) >>>> inode_unlock(inode); >>>> >>> >>> . >>> >> > > . > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: check the metadate alloc before marking extent written 2017-12-05 3:09 ` alex chen @ 2017-12-05 3:16 ` Joseph Qi 0 siblings, 0 replies; 6+ messages in thread From: Joseph Qi @ 2017-12-05 3:16 UTC (permalink / raw) To: ocfs2-devel On 17/12/5 11:09, alex chen wrote: > > > On 2017/12/5 10:45, Joseph Qi wrote: >> >> >> On 17/12/5 10:36, alex chen wrote: >>> Hi Joseph, >>> >>> Thanks for your reviews. >>> >>> On 2017/12/5 10:21, Joseph Qi wrote: >>>> >>>> >>>> On 17/12/5 09:41, alex chen wrote: >>>>> We need to check the free number of the records in each loop to mark >>>>> extent written, because the last extent block may be changed through >>>>> many times marking extent written and the 'num_free_extents' also be >>>>> changed. In the worst case, the 'num_free_extents' may become less >>>>> than the beginning of the loop. So we should not estimate the free >>>>> number of the records at the beginning of the loop to mark extent >>>>> written. >>>>> >>>>> Reported-by: John Lightsey <john@nixnuts.net> >>>>> Signed-off-by: Alex Chen <alex.chen@huawei.com> >>>>> Reviewed-by: Jun Piao <piaojun@huawei.com> >>>>> --- >>>>> fs/ocfs2/aops.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++--------- >>>>> 1 file changed, 64 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c >>>>> index d151632..702aebc 100644 >>>>> --- a/fs/ocfs2/aops.c >>>>> +++ b/fs/ocfs2/aops.c >>>>> @@ -2272,6 +2272,35 @@ static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock, >>>>> return ret; >>>>> } >>>>> >>>>> +static int ocfs2_dio_should_restart(struct ocfs2_extent_tree *et, >>>>> + struct ocfs2_alloc_context *meta_ac, int max_rec_needed) >>>>> +{ >>>>> + int status = 0, free_extents; >>>>> + >>>>> + free_extents = ocfs2_num_free_extents(et); >>>>> + if (free_extents < 0) { >>>>> + status = free_extents; >>>>> + mlog_errno(status); >>>>> + return status; >>>>> + } >>>>> + >>>>> + /* >>>>> + * there are two cases which could cause us to EAGAIN in the >>>>> + * we-need-more-metadata case: >>>>> + * 1) we haven't reserved *any* >>>>> + * 2) we are so fragmented, we've needed to add metadata too >>>>> + * many times. >>>>> + */ >>>>> + if (free_extents < max_rec_needed) { >>>>> + if (!meta_ac || (ocfs2_alloc_context_bits_left(meta_ac) >>>>> + < ocfs2_extend_meta_needed(et->et_root_el))) >>>>> + status = 1; >>>>> + } >>>>> + >>>>> + return status; >>>>> +} >>>>> + >>>>> +#define OCFS2_MAX_REC_NEEDED_SPLIT 2 >>>>> static int ocfs2_dio_end_io_write(struct inode *inode, >>>>> struct ocfs2_dio_write_ctxt *dwc, >>>>> loff_t offset, >>>>> @@ -2281,14 +2310,13 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>>>> struct ocfs2_extent_tree et; >>>>> struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); >>>>> struct ocfs2_inode_info *oi = OCFS2_I(inode); >>>>> - struct ocfs2_unwritten_extent *ue = NULL; >>>>> + struct ocfs2_unwritten_extent *ue = NULL, *tmp_ue; >>>>> struct buffer_head *di_bh = NULL; >>>>> struct ocfs2_dinode *di; >>>>> - struct ocfs2_alloc_context *data_ac = NULL; >>>>> struct ocfs2_alloc_context *meta_ac = NULL; >>>>> handle_t *handle = NULL; >>>>> loff_t end = offset + bytes; >>>>> - int ret = 0, credits = 0, locked = 0; >>>>> + int ret = 0, credits = 0, locked = 0, restart = 0; >>>>> >>>>> ocfs2_init_dealloc_ctxt(&dealloc); >>>>> >>>>> @@ -2330,8 +2358,9 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>>>> >>>> Should we restart here? >>> >>> OK. we should restart before initialized the inode extent tree. >>> >>>> >>>>> ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh); >>>>> >>>>> - ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2, >>>>> - &data_ac, &meta_ac); >>>>> +restart_all: >>>>> + ret = ocfs2_lock_allocators(inode, &et, 0, OCFS2_MAX_REC_NEEDED_SPLIT, >>>>> + NULL, &meta_ac); >>>>> if (ret) { >>>>> mlog_errno(ret); >>>>> goto unlock; >>>>> @@ -2343,7 +2372,7 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>>>> if (IS_ERR(handle)) { >>>>> ret = PTR_ERR(handle); >>>>> mlog_errno(ret); >>>>> - goto unlock; >>>>> + goto free_ac; >>>> I don't think it is a necessary change here. >>> >>> we need to free the 'meta_ac' which allocated in ocfs2_lock_allocators() when we >>> start transaction failed. >>> >> As I described in the last mail, just move the restart check after the >> original free meta_ac check. >> >>> Thanks, >>> Alex >>>> >>>>> } >>>>> ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh, >>>>> OCFS2_JOURNAL_ACCESS_WRITE); >>>>> @@ -2352,7 +2381,17 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>>>> goto commit; >>>>> } >>>>> >>>>> - list_for_each_entry(ue, &dwc->dw_zero_list, ue_node) { >>>>> + list_for_each_entry_safe(ue, tmp_ue, &dwc->dw_zero_list, ue_node) { >>>>> + ret = ocfs2_dio_should_restart(&et, meta_ac, >>>>> + OCFS2_MAX_REC_NEEDED_SPLIT * 2); >>>>> + if (ret < 0) { >>>>> + mlog_errno(ret); >>>>> + break; >>>>> + } else if (ret == 1) { >>>>> + restart = 1; >>>>> + break; >>>>> + } >>>>> + >>>>> ret = ocfs2_mark_extent_written(inode, &et, handle, >>>>> ue->ue_cpos, 1, >>>>> ue->ue_phys, >>>>> @@ -2361,24 +2400,37 @@ static int ocfs2_dio_end_io_write(struct inode *inode, >>>>> mlog_errno(ret); >>>>> break; >>>>> } >>>>> + >>>>> + dwc->dw_zero_count--; >>>>> + list_del_init(&ue->ue_node); >>>>> + spin_lock(&oi->ip_lock); >>>>> + list_del_init(&ue->ue_ip_node); >>>>> + spin_unlock(&oi->ip_lock); >>>>> + kfree(ue); >>>>> } >>>>> >>>>> - if (end > i_size_read(inode)) { >>>>> + if (!restart && end > i_size_read(inode)) { >>>>> ret = ocfs2_set_inode_size(handle, inode, di_bh, end); >>>>> if (ret < 0) >>>>> mlog_errno(ret); >>>>> } >>>>> + >>>>> commit: >>>>> ocfs2_commit_trans(osb, handle); >>>>> +free_ac: >>>>> + if (meta_ac) { >>>>> + ocfs2_free_alloc_context(meta_ac); >>>>> + meta_ac = NULL; >>>>> + } >>>>> + if (restart) { >>>>> + restart = 0; >>>>> + goto restart_all; >>>>> + } >>>>> unlock: >>>>> up_write(&oi->ip_alloc_sem); >>>>> ocfs2_inode_unlock(inode, 1); >>>>> brelse(di_bh); >>>>> out: >>>>> - if (data_ac) >>>>> - ocfs2_free_alloc_context(data_ac); >>>> Agree to cleanup the unused data_ac. >>>> >>>>> - if (meta_ac) >>>>> - ocfs2_free_alloc_context(meta_ac); >> Just move the restart logic down is enough. > > If we check the restart here, we will unlock the 'ip_alloc_sem' and release the > inode buffer, which we don't need to do. > Oh, my mistake. You are right, we should restart under inode lock as well as ip_alloc_sem. > Thanks, > Alex >>>> >>>> Thanks, >>>> Joseph >>>> >>>>> ocfs2_run_deallocs(osb, &dealloc); >>>>> if (locked) >>>>> inode_unlock(inode); >>>>> >>>> >>>> . >>>> >>> >> >> . >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-05 3:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-12-05 1:41 [Ocfs2-devel] [PATCH] ocfs2: check the metadate alloc before marking extent written alex chen 2017-12-05 2:21 ` Joseph Qi 2017-12-05 2:36 ` alex chen 2017-12-05 2:45 ` Joseph Qi 2017-12-05 3:09 ` alex chen 2017-12-05 3:16 ` Joseph Qi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.