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

[PATCH 0/2] 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.

								Honza

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

[Jan Kara]

Remome 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")
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/transaction.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 3dccc23cf010..b49a103cff1f 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);
@@ -721,8 +728,10 @@ 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);
+	if (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]

On May 18, 2020, at 03:21, Jan Kara <jack@suse.cz> wrote:
> 
> Remome ext4_journal_free_reserved() function. It is never used.

(typo) "Remove", but otherwise seems fine.

> 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
> 

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

[Andreas Dilger]

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

On May 18, 2020, at 3:21 AM, Jan Kara <jack@suse.cz> 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")
> Signed-off-by: Jan Kara <jack@suse.cz>

Patch looks reasonable, with one minor nit below.

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

> ---
> fs/jbd2/transaction.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 3dccc23cf010..b49a103cff1f 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);
> @@ -721,8 +728,10 @@ 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);
> +	if (handle->h_rsv_handle) {
> +		__jbd2_journal_unreserve_handle(handle->h_rsv_handle,
> +						transaction);
> +	}

There isn't any need for braces {} around this one-line if-block.


Cheers, Andreas






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

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

[Jan Kara]

On Tue 19-05-20 18:44:25, Andreas Dilger wrote:
> On May 18, 2020, at 3:21 AM, Jan Kara <jack@suse.cz> 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")
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Patch looks reasonable, with one minor nit below.
> 
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>

Thanks!


> > @@ -721,8 +728,10 @@ 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);
> > +	if (handle->h_rsv_handle) {
> > +		__jbd2_journal_unreserve_handle(handle->h_rsv_handle,
> > +						transaction);
> > +	}
> 
> There isn't any need for braces {} around this one-line if-block.

Yeah, I'm always undecided about this. I agree that in this case the code
wouldn't be any less readable without the braces. So I'll remove them and
resend with your tag.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 8f7d89f36829 ("jbd2: transaction reservation support").

The bot has tested the following trees: v5.6.13, v5.4.41, v4.19.123, v4.14.180, v4.9.223, v4.4.223.

v5.6.13: Build OK!
v5.4.41: Failed to apply! Possible dependencies:
    5559b2d81b51 ("jbd2: Drop pointless wakeup from jbd2_journal_stop()")
    933f1c1e0b75 ("jbd2: Rename h_buffer_credits to h_total_credits")
    a413036791d0 ("ext4: Provide function to handle transaction restarts")
    a9a8344ee171 ("ext4, jbd2: Provide accessor function for handle credits")
    dfaf5ffda227 ("jbd2: Reorganize jbd2_journal_stop()")
    ec8b6f600e49 ("jbd2: Factor out common parts of stopping and restarting a handle")
    fdc3ef882a5d ("jbd2: Reserve space for revoke descriptor blocks")

v4.19.123: Failed to apply! Possible dependencies:
    0b02f4c0d6d9 ("ext4: fix reserved cluster accounting at delayed write time")
    1dc0aa46e74a ("ext4: add new pending reservation mechanism")
    32ea275008d8 ("jbd2: update locking documentation for transaction_t")
    4c273352bb45 ("jbd2: add missing tracepoint for reserved handle")
    5559b2d81b51 ("jbd2: Drop pointless wakeup from jbd2_journal_stop()")
    933f1c1e0b75 ("jbd2: Rename h_buffer_credits to h_total_credits")
    a413036791d0 ("ext4: Provide function to handle transaction restarts")
    a9a8344ee171 ("ext4, jbd2: Provide accessor function for handle credits")
    ad431025aecd ("ext4: generalize extents status tree search functions")
    dfaf5ffda227 ("jbd2: Reorganize jbd2_journal_stop()")
    ec8b6f600e49 ("jbd2: Factor out common parts of stopping and restarting a handle")
    fdc3ef882a5d ("jbd2: Reserve space for revoke descriptor blocks")

v4.14.180: Failed to apply! Possible dependencies:
    0b02f4c0d6d9 ("ext4: fix reserved cluster accounting at delayed write time")
    19fe5f643f89 ("iomap: Switch from blkno to disk offset")
    1dc0aa46e74a ("ext4: add new pending reservation mechanism")
    32ea275008d8 ("jbd2: update locking documentation for transaction_t")
    4c273352bb45 ("jbd2: add missing tracepoint for reserved handle")
    545052e9e35a ("ext4: Switch to iomap for SEEK_HOLE / SEEK_DATA")
    5559b2d81b51 ("jbd2: Drop pointless wakeup from jbd2_journal_stop()")
    933f1c1e0b75 ("jbd2: Rename h_buffer_credits to h_total_credits")
    a413036791d0 ("ext4: Provide function to handle transaction restarts")
    a9a8344ee171 ("ext4, jbd2: Provide accessor function for handle credits")
    ad431025aecd ("ext4: generalize extents status tree search functions")
    dfaf5ffda227 ("jbd2: Reorganize jbd2_journal_stop()")
    ec8b6f600e49 ("jbd2: Factor out common parts of stopping and restarting a handle")
    fdc3ef882a5d ("jbd2: Reserve space for revoke descriptor blocks")

v4.9.223: Failed to apply! Possible dependencies:
    39bc88e5e38e ("arm64: Disable TTBR0_EL1 during normal kernel execution")
    4c273352bb45 ("jbd2: add missing tracepoint for reserved handle")
    538bcaa6261b ("jbd2: fix race when writing superblock")
    5559b2d81b51 ("jbd2: Drop pointless wakeup from jbd2_journal_stop()")
    783d94854499 ("ext4: add EXT4_IOC_GOINGDOWN ioctl")
    7c0f6ba682b9 ("Replace <asm/uaccess.h> with <linux/uaccess.h> globally")
    81378da64de6 ("jbd2: mark the transaction context with the scope GFP_NOFS context")
    933f1c1e0b75 ("jbd2: Rename h_buffer_credits to h_total_credits")
    9cf09d68b89a ("arm64: xen: Enable user access before a privcmd hvc call")
    b4709067ac09 ("jbd2: preserve original nofs flag during journal restart")
    bd38967d406f ("arm64: Factor out PAN enabling/disabling into separate uaccess_* macros")
    dfaf5ffda227 ("jbd2: Reorganize jbd2_journal_stop()")
    ec8b6f600e49 ("jbd2: Factor out common parts of stopping and restarting a handle")
    fb7c02445c49 ("ext4: pass -ESHUTDOWN code to jbd2 layer")
    fdc3ef882a5d ("jbd2: Reserve space for revoke descriptor blocks")

v4.4.223: Failed to apply! Possible dependencies:
    2a222ca992c3 ("fs: have submit_bh users pass in op and flags separately")
    38f252553300 ("block: add __blkdev_issue_discard")
    4c273352bb45 ("jbd2: add missing tracepoint for reserved handle")
    4e49ea4a3d27 ("block/fs/drivers: remove rw argument from submit_bio")
    538bcaa6261b ("jbd2: fix race when writing superblock")
    5559b2d81b51 ("jbd2: Drop pointless wakeup from jbd2_journal_stop()")
    7a4b188f0c0b ("jbd2: move lockdep instrumentation for jbd2 handles")
    81378da64de6 ("jbd2: mark the transaction context with the scope GFP_NOFS context")
    9082e87bfbf8 ("block: remove struct bio_batch")
    933f1c1e0b75 ("jbd2: Rename h_buffer_credits to h_total_credits")
    ab714aff4f74 ("jbd2: move lockdep tracking to journal_s")
    b4709067ac09 ("jbd2: preserve original nofs flag during journal restart")
    bbd848e0fade ("block: reinstate early return of -EOPNOTSUPP from blkdev_issue_discard")
    d57d611505d9 ("kernel/fs: fix I/O wait not accounted for RW O_DSYNC")
    dfaf5ffda227 ("jbd2: Reorganize jbd2_journal_stop()")
    ec8b6f600e49 ("jbd2: Factor out common parts of stopping and restarting a handle")
    fdc3ef882a5d ("jbd2: Reserve space for revoke descriptor blocks")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

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 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 --]

[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