| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20110712123041.GC1293@redhat.com>
Date: Tue, 12 Jul 2011 08:30:41 -0400
From: Vivek Goyal <vgoyal@...hat.com>
To: Tao Ma <tm@....ma>
Cc: linux-ext4@...r.kernel.org, stable@...nel.org,
Jan Kara <jack@...e.cz>, Jeff Moyer <jmoyer@...hat.com>,
Corrado Zoccolo <czoccolo@...il.com>,
Jens Axboe <jaxboe@...ionio.com>
Subject: Re: [PATCH] jbd/2[stable only]: Use WRITE_SYNC_PLUG in
journal_commit_transaction.
On Tue, Jul 12, 2011 at 06:43:51PM +0800, Tao Ma wrote:
> From: Tao Ma <boyu.mt@...bao.com>
>
> In commit 749ef9f8423, we use WRITE_SYNC instead of WRITE in
> journal_commit_transaction. It causes a much heavy burden for
> the disk as now the seqenctial write can't be merged(see the blktrace below).
Tao Ma,
Few queries.
- What's the workload you are using for this test.
- Do you see any performance improvement by switching to WRITE_SYNC_PLUG.
- Why writes are not being merged? Because request got dispatched
immediately? Do you have logs for insertion of requests also.
- WRITE_SYNC_PLUG will plug the queue and expects explicity unplug. Who
is doing unplug in this case?
- I am not sure in how many cases we are expecting to submit multiple
sequential write here.
Thanks
Vivek
>
> Given the description of that commit 749ef9f8423, the reason why
> we use WRITE_SYNC is that it wants to use REQ_NOIDLE and WRITE_SYNC_PLUG
> also has that flag, so use WRITE_SYNC_PLUG instead. From blktrace,
> we can get that:
>
> without the patch:
> 8,0 6 18 0.016058423 3342 D W 461101317 + 4 [jbd2/sda11-8]
> 8,0 6 19 0.016065473 3342 D W 461101321 + 4 [jbd2/sda11-8]
> 8,0 6 20 0.016070751 3342 D W 461101325 + 4 [jbd2/sda11-8]
> 8,0 6 21 0.016076180 3342 D W 461101329 + 4 [jbd2/sda11-8]
> 8,0 6 22 0.016081255 3342 D W 461101333 + 4 [jbd2/sda11-8]
> 8,0 6 23 0.016085963 3342 D W 461101337 + 4 [jbd2/sda11-8]
> 8,0 6 24 0.016182048 0 C W 461101317 + 4 [0]
> 8,0 6 25 0.016190820 0 C W 461101325 + 4 [0]
> 8,0 6 26 0.016193927 0 C W 461101321 + 4 [0]
> 8,0 6 27 0.016196532 0 C W 461101333 + 4 [0]
> 8,0 6 28 0.016199180 0 C W 461101337 + 4 [0]
> 8,0 6 29 0.016206180 0 C W 461101329 + 4 [0]
>
> with this patch:
> 8,0 4 23 4.320315739 3129 D W 461101317 + 24 [jbd2/sda11-8]
> 8,0 4 24 4.320364518 0 C W 461101317 + 24 [0]
>
> This only affects .37 and .38 since Jens' new plug patches are included
> in .39 and the bug is removed as a side effect. But I think it is needed
> anyway for the stable. And RHEL6 needs this also I guess.
>
> Cc: stable@...nel.org # 2.6.37 and 2.6.38
> Cc: Jan Kara <jack@...e.cz>
> Cc: Vivek Goyal <vgoyal@...hat.com>
> Cc: Jeff Moyer <jmoyer@...hat.com>
> Cc: Corrado Zoccolo <czoccolo@...il.com>
> Cc: Jens Axboe <jaxboe@...ionio.com>
> Signed-off-by: Tao Ma <boyu.mt@...bao.com>
> ---
> fs/jbd/commit.c | 9 +--------
> fs/jbd2/commit.c | 9 +--------
> 2 files changed, 2 insertions(+), 16 deletions(-)
>
> diff --git a/fs/jbd/commit.c b/fs/jbd/commit.c
> index 34a4861..6d13df5 100644
> --- a/fs/jbd/commit.c
> +++ b/fs/jbd/commit.c
> @@ -294,7 +294,7 @@ void journal_commit_transaction(journal_t *journal)
> int first_tag = 0;
> int tag_flag;
> int i;
> - int write_op = WRITE_SYNC;
> + int write_op = WRITE_SYNC_PLUG;
>
> /*
> * First job: lock down the current transaction and wait for
> @@ -327,13 +327,6 @@ void journal_commit_transaction(journal_t *journal)
> spin_lock(&journal->j_state_lock);
> commit_transaction->t_state = T_LOCKED;
>
> - /*
> - * Use plugged writes here, since we want to submit several before
> - * we unplug the device. We don't do explicit unplugging in here,
> - * instead we rely on sync_buffer() doing the unplug for us.
> - */
> - if (commit_transaction->t_synchronous_commit)
> - write_op = WRITE_SYNC_PLUG;
> spin_lock(&commit_transaction->t_handle_lock);
> while (commit_transaction->t_updates) {
> DEFINE_WAIT(wait);
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index f3ad159..fc3840f 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -329,7 +329,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
> int tag_bytes = journal_tag_bytes(journal);
> struct buffer_head *cbh = NULL; /* For transactional checksums */
> __u32 crc32_sum = ~0;
> - int write_op = WRITE_SYNC;
> + int write_op = WRITE_SYNC_PLUG;
>
> /*
> * First job: lock down the current transaction and wait for
> @@ -363,13 +363,6 @@ void jbd2_journal_commit_transaction(journal_t *journal)
> write_lock(&journal->j_state_lock);
> commit_transaction->t_state = T_LOCKED;
>
> - /*
> - * Use plugged writes here, since we want to submit several before
> - * we unplug the device. We don't do explicit unplugging in here,
> - * instead we rely on sync_buffer() doing the unplug for us.
> - */
> - if (commit_transaction->t_synchronous_commit)
> - write_op = WRITE_SYNC_PLUG;
> trace_jbd2_commit_locking(journal, commit_transaction);
> stats.run.rs_wait = commit_transaction->t_max_wait;
> stats.run.rs_locked = jiffies;
> --
> 1.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists