[PATCH 0/2 v2] jbd2: Fix leaked transaction credits

[PATCH 0/2 v2] jbd2: Fix leaked transaction credits

[Jan Kara]

Hello,

after debugging more the reason why fsmark run is slower in dioread_nolock
mode (which became the default in 5.6), I've found jbd2 actually leaks
reserved credits if we start a transaction with reserved handle and then don't
use it. The first patch in the series is a small (and independent) cleanup in
the area, the second patch fixes the leak.

Then there was another revelation for me that in this workload ext4 actually
starts lots of reserved transaction handles that are unused. This is due to the
way how ext4 writepages code works - it starts a transaction, then inspects
page cache and writes one extent if found. Then starts again a transaction and
checks whether there's more to write. So for single extent files we always
start transaction twice, second time only to find there's nothing more to
write. In case all blocks are already allocated, we wouldn't even have to start
transaction at all.  This probably also deserves to be fixed but a simple fix I
made seems to break page writeback so I need to dig more into it and I want to
push out the obvious jbd2 fix early.

Changes since v1:
* Add reviewed-by tag
* Fixed up one coding style issue pointed out by Andreas

								Honza

[PATCH 1/2] ext4: Drop ext4_journal_free_reserved()

[Jan Kara]

Remove ext4_journal_free_reserved() function. It is never used.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/ext4_jbd2.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
index 4b9002f0e84c..1c926f31d43e 100644
--- a/fs/ext4/ext4_jbd2.h
+++ b/fs/ext4/ext4_jbd2.h
@@ -335,12 +335,6 @@ static inline handle_t *__ext4_journal_start(struct inode *inode,
 handle_t *__ext4_journal_start_reserved(handle_t *handle, unsigned int line,
 					int type);
 
-static inline void ext4_journal_free_reserved(handle_t *handle)
-{
-	if (ext4_handle_valid(handle))
-		jbd2_journal_free_reserved(handle);
-}
-
 static inline handle_t *ext4_journal_current_handle(void)
 {
 	return journal_current_handle();
-- 
2.16.4

[PATCH 2/2] jbd2: Avoid leaking transaction credits when unreserving handle

[Jan Kara]

When reserved transaction handle is unused, we subtract its reserved
credits in __jbd2_journal_unreserve_handle() called from
jbd2_journal_stop(). However this function forgets to remove reserved
credits from transaction->t_outstanding_credits and thus the transaction
space that was reserved remains effectively leaked. The leaked
transaction space can be quite significant in some cases and leads to
unnecessarily small transactions and thus reducing throughput of the
journalling machinery. E.g. fsmark workload creating lots of 4k files
was observed to have about 20% lower throughput due to this when ext4 is
mounted with dioread_nolock mount option.

Subtract reserved credits from t_outstanding_credits as well.

CC: stable@vger.kernel.org
Fixes: 8f7d89f36829 ("jbd2: transaction reservation support")
Reviewed-by: Andreas Dilger <adilger@dilger.ca>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/transaction.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 3dccc23cf010..e91aad3637a2 100644
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -541,17 +541,24 @@ handle_t *jbd2_journal_start(journal_t *journal, int nblocks)
 }
 EXPORT_SYMBOL(jbd2_journal_start);
 
-static void __jbd2_journal_unreserve_handle(handle_t *handle)
+static void __jbd2_journal_unreserve_handle(handle_t *handle, transaction_t *t)
 {
 	journal_t *journal = handle->h_journal;
 
 	WARN_ON(!handle->h_reserved);
 	sub_reserved_credits(journal, handle->h_total_credits);
+	if (t)
+		atomic_sub(handle->h_total_credits, &t->t_outstanding_credits);
 }
 
 void jbd2_journal_free_reserved(handle_t *handle)
 {
-	__jbd2_journal_unreserve_handle(handle);
+	journal_t *journal = handle->h_journal;
+
+	/* Get j_state_lock to pin running transaction if it exists */
+	read_lock(&journal->j_state_lock);
+	__jbd2_journal_unreserve_handle(handle, journal->j_running_transaction);
+	read_unlock(&journal->j_state_lock);
 	jbd2_free_handle(handle);
 }
 EXPORT_SYMBOL(jbd2_journal_free_reserved);
@@ -722,7 +729,8 @@ static void stop_this_handle(handle_t *handle)
 	atomic_sub(handle->h_total_credits,
 		   &transaction->t_outstanding_credits);
 	if (handle->h_rsv_handle)
-		__jbd2_journal_unreserve_handle(handle->h_rsv_handle);
+		__jbd2_journal_unreserve_handle(handle->h_rsv_handle,
+						transaction);
 	if (atomic_dec_and_test(&transaction->t_updates))
 		wake_up(&journal->j_wait_updates);
 
-- 
2.16.4

Re: [PATCH 1/2] ext4: Drop ext4_journal_free_reserved()

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 979 bytes --]

On May 20, 2020, at 7:31 AM, Jan Kara <jack@suse.cz> wrote:
> 
> Remove ext4_journal_free_reserved() function. It is never used.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> fs/ext4/ext4_jbd2.h | 6 ------
> 1 file changed, 6 deletions(-)
> 
> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
> index 4b9002f0e84c..1c926f31d43e 100644
> --- a/fs/ext4/ext4_jbd2.h
> +++ b/fs/ext4/ext4_jbd2.h
> @@ -335,12 +335,6 @@ static inline handle_t *__ext4_journal_start(struct inode *inode,
> handle_t *__ext4_journal_start_reserved(handle_t *handle, unsigned int line,
> 					int type);
> 
> -static inline void ext4_journal_free_reserved(handle_t *handle)
> -{
> -	if (ext4_handle_valid(handle))
> -		jbd2_journal_free_reserved(handle);
> -}
> -
> static inline handle_t *ext4_journal_current_handle(void)
> {
> 	return journal_current_handle();
> --
> 2.16.4
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH 1/2] ext4: Drop ext4_journal_free_reserved()

[Theodore Y. Ts'o]

On Wed, May 20, 2020 at 01:29:01PM -0600, Andreas Dilger wrote:
> On May 20, 2020, at 7:31 AM, Jan Kara <jack@suse.cz> wrote:
> > 
> > Remove ext4_journal_free_reserved() function. It is never used.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>

Thanks, applied.

					- Ted

Re: [PATCH 2/2] jbd2: Avoid leaking transaction credits when unreserving handle

[Theodore Y. Ts'o]

On Wed, May 20, 2020 at 03:31:19PM +0200, Jan Kara wrote:
> When reserved transaction handle is unused, we subtract its reserved
> credits in __jbd2_journal_unreserve_handle() called from
> jbd2_journal_stop(). However this function forgets to remove reserved
> credits from transaction->t_outstanding_credits and thus the transaction
> space that was reserved remains effectively leaked. The leaked
> transaction space can be quite significant in some cases and leads to
> unnecessarily small transactions and thus reducing throughput of the
> journalling machinery. E.g. fsmark workload creating lots of 4k files
> was observed to have about 20% lower throughput due to this when ext4 is
> mounted with dioread_nolock mount option.
> 
> Subtract reserved credits from t_outstanding_credits as well.
> 
> CC: stable@vger.kernel.org
> Fixes: 8f7d89f36829 ("jbd2: transaction reservation support")
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted