linux-kernel - Re: [PATCH] ext3: wait on all pending commits in ext3_sync

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 19 Dec 2008 01:27:52 +0100
From:	Jan Kara <jack@...e.cz>
To:	Eric Sandeen <sandeen@...hat.com>
Cc:	Arthur Jones <ajones@...erbed.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
	"sct@...hat.com" <sct@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ext3: wait on all pending commits in ext3_sync_fs

On Thu 18-12-08 17:37:23, Eric Sandeen wrote:
> Jan Kara wrote:
> >   Hello,
> > 
> >   I'm sorry I'm replying late but I got time to react to this only now...
> > 
> 
> <snip>
> 
> >> I tried this and it too fixes the problem.  FWIW I agree it
> >> looks better...
> >   Well, shouldn't we rather fix what journal_start_commit() returns?
> > The interface which returns 1 when a transaction is already committing or
> > a transaction commit has just been started but 0 when we race with
> > somebody staring the commit is fairly unusable. Moreover
> > ext3_force_commit() will unnecessarily create new sync transaction and
> > commit it if there's no transaction running which is quite expensive
> > (even merging empty sync handle is not for free because of sync
> > transaction batching). But this is minor problem since we probably
> > don't care too much about sync() performance - BTW this is probably a
> > cause for bug 12224, isn't it?
> 
> Yep, it is!  :)
> 
> >   BTW: ocfs2 would need fixing as well if done your way since it's
> > sync_fs function has been copied over from ext3.
> >   To summarized I'd rather see a patch like below (untested) going in
> > and your patch reverted... Opinions? I can cookup a JBD2 version of
> > the patch in case we agree to go this way.
> 
> Thanks, I'll look that over.
  Thanks.

> In looking at what we have today, I wonder if we can make things smarter
> so that we don't commit empty transactions in any case?
  Probably it does not make sence to commit such transactions and we might
save some time in sync paths if we do so. So yes, I think skipping empty
transaction commit might be worthwhile and it shouldn't be hard to do
either. But I'd give it serious testing just in case some unexpectedly
relies on this behaviour - wouldn't this interfere e.g. with sync
transaction batching autotuning code? Untested patch below...
								Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
---

>From 3e1a8dc6a64fdce5c99d7edca0a2812178f7463c Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@...e.cz>
Date: Fri, 19 Dec 2008 01:20:47 +0100
Subject: [PATCH] jbd2: Skip commit of a transaction without any buffers

Signed-off-by: Jan Kara <jack@...e.cz>
---
 fs/jbd2/commit.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index ebc667b..798e021 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -401,6 +401,21 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	J_ASSERT (commit_transaction->t_outstanding_credits <=
 			journal->j_max_transaction_buffers);
 
+	/* Skip commit of a transaction without any buffers */
+	spin_lock(&journal->j_list_lock);
+	if (commit_transaction->t_reserved_list == NULL &&
+	    commit_transaction->t_buffers == NULL &&
+	    commit_transaction->t_forget == NULL &&
+	    list_empty(commit_transaction->t_inode_list)) {
+		BUG_ON(commit_transaction->t_checkpoint_list);
+		BUG_ON(commit_transaction->t_checkpoint_io_list);
+		BUG_ON(commit_transaction->t_iobuf_list);
+		BUG_ON(commit_transaction->t_shadow_list);
+		BUG_ON(commit_transaction->t_log_list);
+		goto out_committed;
+	}
+	spin_unlock(&journal->j_list_lock);
+
 	/*
 	 * First thing we are allowed to do is to discard any remaining
 	 * BJ_Reserved buffers.  Note, it is _not_ permissible to assume
@@ -934,6 +949,7 @@ restart_loop:
 
 	/* Done with this transaction! */
 
+out_committed:
 	jbd_debug(3, "JBD: commit phase 7\n");
 
 	J_ASSERT(commit_transaction->t_state == T_COMMIT);
-- 
1.6.0.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/