* [PATCH 2/2] jbd2: Avoid leaking transaction credits when unreserving handle
[not found] <20200518092120.10322-1-jack@suse.cz>
@ 2020-05-18 9:21 ` Jan Kara
2020-05-20 0:44 ` Andreas Dilger
2020-05-22 0:12 ` Sasha Levin
0 siblings, 2 replies; 6+ messages in thread
From: Jan Kara @ 2020-05-18 9:21 UTC (permalink / raw)
To: Ted Tso; +Cc: linux-ext4, Jan Kara, stable
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] jbd2: Avoid leaking transaction credits when unreserving handle
2020-05-18 9:21 ` [PATCH 2/2] jbd2: Avoid leaking transaction credits when unreserving handle Jan Kara
@ 2020-05-20 0:44 ` Andreas Dilger
2020-05-20 13:27 ` Jan Kara
2020-05-22 0:12 ` Sasha Levin
1 sibling, 1 reply; 6+ messages in thread
From: Andreas Dilger @ 2020-05-20 0:44 UTC (permalink / raw)
To: Jan Kara; +Cc: Ted Tso, linux-ext4, stable
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] jbd2: Avoid leaking transaction credits when unreserving handle
2020-05-20 0:44 ` Andreas Dilger
@ 2020-05-20 13:27 ` Jan Kara
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2020-05-20 13:27 UTC (permalink / raw)
To: Andreas Dilger; +Cc: Jan Kara, Ted Tso, linux-ext4, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] jbd2: Avoid leaking transaction credits when unreserving handle
2020-05-18 9:21 ` [PATCH 2/2] jbd2: Avoid leaking transaction credits when unreserving handle Jan Kara
2020-05-20 0:44 ` Andreas Dilger
@ 2020-05-22 0:12 ` Sasha Levin
1 sibling, 0 replies; 6+ messages in thread
From: Sasha Levin @ 2020-05-22 0:12 UTC (permalink / raw)
To: Sasha Levin, Jan Kara, Ted Tso; +Cc: linux-ext4, Jan Kara, stable, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] jbd2: Avoid leaking transaction credits when unreserving handle
2020-05-20 13:31 ` Jan Kara
@ 2020-05-29 2:44 ` Theodore Y. Ts'o
0 siblings, 0 replies; 6+ messages in thread
From: Theodore Y. Ts'o @ 2020-05-29 2:44 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-ext4, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] jbd2: Avoid leaking transaction credits when unreserving handle
[not found] <20200520133119.1383-1-jack@suse.cz>
@ 2020-05-20 13:31 ` Jan Kara
2020-05-29 2:44 ` Theodore Y. Ts'o
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2020-05-20 13:31 UTC (permalink / raw)
To: Ted Tso; +Cc: linux-ext4, Jan Kara, stable
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-29 2:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20200518092120.10322-1-jack@suse.cz>
2020-05-18 9:21 ` [PATCH 2/2] jbd2: Avoid leaking transaction credits when unreserving handle Jan Kara
2020-05-20 0:44 ` Andreas Dilger
2020-05-20 13:27 ` Jan Kara
2020-05-22 0:12 ` Sasha Levin
[not found] <20200520133119.1383-1-jack@suse.cz>
2020-05-20 13:31 ` Jan Kara
2020-05-29 2:44 ` Theodore Y. Ts'o
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).