Re: [PATCH v3 2/2] Revert "ocfs2: fix DIO failure due to insufficient transaction credits"

[Heming Zhao]

On Thu, Mar 26, 2026 at 10:38:06AM +0100, Jan Kara wrote:
> On Thu 26-03-26 14:58:18, Heming Zhao wrote:
> > This reverts commit be346c1a6eeb49d8fda827d2a9522124c2f72f36.
> > 
> > With the patch "ocfs2: split transactions in dio completion to avoid
> > credit exhaustion," the jbd2 transaction is now restarted every
> > OCFS2_DIO_MARK_EXTENT_BATCH loops. In theory, this should prevent
> > credit exhaustion issues from occurring.
> 
> I don't see why that would be the case. Look at the commit message of
> be346c1a6eeb49d8fda827d2a9522124c2f72f36. The problem is that
> ocfs2_calc_extend_credits() doesn't (and hardly can) estimate maximum
> number of credits needed for all extent tree manipulations. Some extent tree
> manipulations in ocfs2_mark_extent_written() don't reserve additional
> credits on their own. This ocfs2_assure_trans_credits() call won't be
> needed if you didn't do any batching of extent conversions into a single
> transaction. But if you do that, there's no guarantee that all conversions
> in a single batch can fit in the amount of credits reserved initially.
> Usually it will be the case but I don't see how that would be guaranteed in
> every case.
> 
> 								Honza

Thanks for your careful review. I agree with your point, and I will remove
the revert patch in the v4.

Meanwhile, I have done some tests: the case handled by ocfs2_assure_trans_credits()
is when "old_nblks < nblocks". In my env, old_nblks is a fixed value of 15
("15 < 16"). When writing a fragmented 2GB file, ~400000 loops occur in
ocfs2_dio_end_io_write(), and this condition is triggered about 5 times in total.

- Heming
> 
> > 
> > Signed-off-by: Heming Zhao <heming.zhao@xxxxxxxx>
> > ---
> >  fs/ocfs2/aops.c        |  5 -----
> >  fs/ocfs2/journal.c     | 17 -----------------
> >  fs/ocfs2/journal.h     |  2 --
> >  fs/ocfs2/ocfs2_trace.h |  2 --
> >  4 files changed, 26 deletions(-)
> > 
> > diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> > index 60f1b607022f..e58314c6506a 100644
> > --- a/fs/ocfs2/aops.c
> > +++ b/fs/ocfs2/aops.c
> > @@ -2332,11 +2332,6 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
> >  				break;
> >  			}
> >  		}
> > -		ret = ocfs2_assure_trans_credits(handle, credits);
> > -		if (ret < 0) {
> > -			mlog_errno(ret);
> > -			break;
> > -		}
> >  		ret = ocfs2_mark_extent_written(inode, &et, handle,
> >  						ue->ue_cpos, 1,
> >  						ue->ue_phys,
> > diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
> > index 4c86a9d46870..7e7546e070fb 100644
> > --- a/fs/ocfs2/journal.c
> > +++ b/fs/ocfs2/journal.c
> > @@ -464,23 +464,6 @@ int ocfs2_extend_trans(handle_t *handle, int nblocks)
> >  	return status;
> >  }
> >  
> > -/*
> > - * Make sure handle has at least 'nblocks' credits available. If it does not
> > - * have that many credits available, we will try to extend the handle to have
> > - * enough credits. If that fails, we will restart transaction to have enough
> > - * credits. Similar notes regarding data consistency and locking implications
> > - * as for ocfs2_extend_trans() apply here.
> > - */
> > -int ocfs2_assure_trans_credits(handle_t *handle, int nblocks)
> > -{
> > -	int old_nblks = jbd2_handle_buffer_credits(handle);
> > -
> > -	trace_ocfs2_assure_trans_credits(old_nblks);
> > -	if (old_nblks >= nblocks)
> > -		return 0;
> > -	return ocfs2_extend_trans(handle, nblocks - old_nblks);
> > -}
> > -
> >  /*
> >   * If we have fewer than thresh credits, extend by OCFS2_MAX_TRANS_DATA.
> >   * If that fails, restart the transaction & regain write access for the
> > diff --git a/fs/ocfs2/journal.h b/fs/ocfs2/journal.h
> > index 6397170f302f..9d4763f758ad 100644
> > --- a/fs/ocfs2/journal.h
> > +++ b/fs/ocfs2/journal.h
> > @@ -244,8 +244,6 @@ handle_t		    *ocfs2_start_trans(struct ocfs2_super *osb,
> >  int			     ocfs2_commit_trans(struct ocfs2_super *osb,
> >  						handle_t *handle);
> >  int			     ocfs2_extend_trans(handle_t *handle, int nblocks);
> > -int			     ocfs2_assure_trans_credits(handle_t *handle,
> > -						int nblocks);
> >  int			     ocfs2_allocate_extend_trans(handle_t *handle,
> >  						int thresh);
> >  
> > diff --git a/fs/ocfs2/ocfs2_trace.h b/fs/ocfs2/ocfs2_trace.h
> > index 4b32fb5658ad..74b283875e17 100644
> > --- a/fs/ocfs2/ocfs2_trace.h
> > +++ b/fs/ocfs2/ocfs2_trace.h
> > @@ -2575,8 +2575,6 @@ DEFINE_OCFS2_ULL_UINT_EVENT(ocfs2_commit_cache_end);
> >  
> >  DEFINE_OCFS2_INT_INT_EVENT(ocfs2_extend_trans);
> >  
> > -DEFINE_OCFS2_INT_EVENT(ocfs2_assure_trans_credits);
> > -
> >  DEFINE_OCFS2_INT_EVENT(ocfs2_extend_trans_restart);
> >  
> >  DEFINE_OCFS2_INT_INT_EVENT(ocfs2_allocate_extend_trans);
> > -- 
> > 2.43.0
> > 
> -- 
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR