1. Patchew
  2. linux
  3. [PATCH v3 0/2] ocfs2: split transactions in dio completion to avoid credit exhaustion
  4. View patch

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

Heming Zhao posted 2 patches 1 week ago
There is a newer version of this series
[PATCH v3 2/2] Revert "ocfs2: fix DIO failure due to insufficient transaction credits"
Posted by Heming Zhao 1 week ago 
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.

Signed-off-by: Heming Zhao <heming.zhao@suse.com>
---
 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
Re: [PATCH v3 2/2] Revert "ocfs2: fix DIO failure due to insufficient transaction credits"
Posted by Jan Kara 1 week ago 
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

> 
> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
> ---
>  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@suse.com>
SUSE Labs, CR
Re: [PATCH v3 2/2] Revert "ocfs2: fix DIO failure due to insufficient transaction credits"
Posted by Heming Zhao 1 week ago 
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@suse.com>
> > ---
> >  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@suse.com>
> SUSE Labs, CR

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.