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

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

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