[PATCH 1/2] ext3: optimize ext3_force

linux-ext4.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 1/2] ext3: optimize ext3_force_commit
@ 2013-04-01  9:03 Dmitry Monakhov
  2013-04-01  9:03 ` [PATCH 2/2] ext4: optimize ext4_force_commit Dmitry Monakhov
  2013-04-02 13:20 ` [PATCH 1/2] ext3: optimize ext3_force_commit Jan Kara
  0 siblings, 2 replies; 6+ messages in thread
From: Dmitry Monakhov @ 2013-04-01  9:03 UTC (permalink / raw)
  To: linux-ext4; +Cc: tytso, jack, Dmitry Monakhov

We do not have to use journal_force_commit() because it explicitly
start and stop SYNC transaction. This is very suboptimal because the only
users of ext3_force_commit() are ext3_sync_file and ext3_write_inode().
Both functions just want to commit and wait any uncommitted transaction
similar to ext3_sync_fs().

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext3/super.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index fb5120a..1853031 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -2507,15 +2507,14 @@ static void ext3_clear_journal_err(struct super_block *sb,
  */
 int ext3_force_commit(struct super_block *sb)
 {
-	journal_t *journal;
-	int ret;
+	tid_t target;
 
 	if (sb->s_flags & MS_RDONLY)
 		return 0;
 
-	journal = EXT3_SB(sb)->s_journal;
-	ret = ext3_journal_force_commit(journal);
-	return ret;
+	if (journal_start_commit(EXT3_SB(sb)->s_journal, &target))
+		return log_wait_commit(EXT3_SB(sb)->s_journal, target);
+	return 0;
 }
 
 static int ext3_sync_fs(struct super_block *sb, int wait)
@@ -2530,7 +2529,7 @@ static int ext3_sync_fs(struct super_block *sb, int wait)
 	dquot_writeback_dquots(sb, -1);
 	if (journal_start_commit(EXT3_SB(sb)->s_journal, &target)) {
 		if (wait)
-			log_wait_commit(EXT3_SB(sb)->s_journal, target);
+			return log_wait_commit(EXT3_SB(sb)->s_journal, target);
 	}
 	return 0;
 }
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] ext4: optimize ext4_force_commit
  2013-04-01  9:03 [PATCH 1/2] ext3: optimize ext3_force_commit Dmitry Monakhov
@ 2013-04-01  9:03 ` Dmitry Monakhov
  2013-04-01 18:58   ` Theodore Ts'o
  2013-04-02 13:20 ` [PATCH 1/2] ext3: optimize ext3_force_commit Jan Kara
  1 sibling, 1 reply; 6+ messages in thread
From: Dmitry Monakhov @ 2013-04-01  9:03 UTC (permalink / raw)
  To: linux-ext4; +Cc: tytso, jack, Dmitry Monakhov

We do not have to use jbd2_journal_force_commit() because it explicitly
start and stop SYNC transaction. This is very suboptimal because the only
users of ext4_force_commit() are ext4_sync_file and ext4_write_inode().
Both functions just want to commit and wait any uncommitted transaction
similar to ext4_sync_fs().

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/super.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index e3e6a06..280a918 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4436,13 +4436,14 @@ static void ext4_clear_journal_err(struct super_block *sb,
  */
 int ext4_force_commit(struct super_block *sb)
 {
-	journal_t *journal;
+	tid_t target;
 
 	if (sb->s_flags & MS_RDONLY)
 		return 0;
 
-	journal = EXT4_SB(sb)->s_journal;
-	return ext4_journal_force_commit(journal);
+	if (jbd2_journal_start_commit(EXT4_SB(sb)->s_journal, &target))
+		return jbd2_log_wait_commit(EXT4_SB(sb)->s_journal, target);
+	return 0;
 }
 
 static int ext4_sync_fs(struct super_block *sb, int wait)
@@ -4460,7 +4461,7 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
 	dquot_writeback_dquots(sb, -1);
 	if (jbd2_journal_start_commit(sbi->s_journal, &target)) {
 		if (wait)
-			jbd2_log_wait_commit(sbi->s_journal, target);
+			ret = jbd2_log_wait_commit(sbi->s_journal, target);
 	}
 	return ret;
 }
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] ext4: optimize ext4_force_commit
  2013-04-01  9:03 ` [PATCH 2/2] ext4: optimize ext4_force_commit Dmitry Monakhov
@ 2013-04-01 18:58   ` Theodore Ts'o
       [not found]     ` <CAF5pi0FZ4MUdBMH9A_Wav8V7TX39z1cOtGspJP9k8LN0KFja7g@mail.gmail.com>
  0 siblings, 1 reply; 6+ messages in thread
From: Theodore Ts'o @ 2013-04-01 18:58 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: linux-ext4, jack

On Mon, Apr 01, 2013 at 01:03:57PM +0400, Dmitry Monakhov wrote:
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index e3e6a06..280a918 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4436,13 +4436,14 @@ static void ext4_clear_journal_err(struct super_block *sb,
>   */
>  int ext4_force_commit(struct super_block *sb)
>  {
> -	journal_t *journal;
> +	tid_t target;
>  
>  	if (sb->s_flags & MS_RDONLY)
>  		return 0;
>  
> -	journal = EXT4_SB(sb)->s_journal;
> -	return ext4_journal_force_commit(journal);
> +	if (jbd2_journal_start_commit(EXT4_SB(sb)->s_journal, &target))
> +		return jbd2_log_wait_commit(EXT4_SB(sb)->s_journal, target);

EXT4_SB(sb)->s_journal is NULL in no-journal mode.  So you need to
check for this --- this is something which is done in
ext4_journal_force_commit().

Since this is the only user of ext4_journal_force_commit(), you might
as well get rid of ext4_journal_force_commit() as part of making this
change.

The other possibility is we perhaps we should just change
jbd2_journal_force_commit() to call jbd2_journal_start_commit() and
jbd2_log_wait_commit(); the only other caller of
jbd2_journal_force_commit() is ocfs2_sync_file(), and it would benefit
from this change as well.

					- Ted

^ permalink raw reply	[flat|nested] 6+ messages in thread

[parent not found: <CAF5pi0FZ4MUdBMH9A_Wav8V7TX39z1cOtGspJP9k8LN0KFja7g@mail.gmail.com>]

[parent not found: <20130409133123.GE12050@thunk.org>]

[parent not found: <CAF5pi0GhudB=BmBNGPAp4KLvfuAiw_2zo5bzYVWQ3jhthsj3Xw@mail.gmail.com>]

* Re: [PATCH 2/2] ext4: optimize ext4_force_commit
       [not found]         ` <CAF5pi0GhudB=BmBNGPAp4KLvfuAiw_2zo5bzYVWQ3jhthsj3Xw@mail.gmail.com>
@ 2013-04-09 14:06           ` Dmitry Monakhov
  2013-04-09 14:36             ` Jan Kara
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Monakhov @ 2013-04-09 14:06 UTC (permalink / raw)
  To: tytso; +Cc: Jan Kara, linux-ext4

On Tue, 9 Apr 2013 17:42:33 +0400, Dmitry Monakhov <dmonakhovopenvz@gmail.com> wrote:
Non-text part: multipart/alternative
> ---------- Forwarded message ----------
> From: "Theodore Ts'o" <tytso@mit.edu>
> Date: Apr 9, 2013 5:31 PM
> Subject: Re: [PATCH 2/2] ext4: optimize ext4_force_commit
> To: "Dmitry Monakhov" <dmonakhovopenvz@gmail.com>
> Cc:
> 
> On Mon, Apr 01, 2013 at 11:10:59PM +0400, Dmitry Monakhov wrote:
> > Yes. I think so too.  Even more jbd2_force_commit_nested already does what
> > we want because we are not hold transaction. I'll send patch tomorrow
> 
> Hi Dmitry,
> 
> Did you send an update for this patch?  I can't seem to find it.  I'm
> wondering if we perhaps got distracted by the big endian patch, and we
> didn't get back to this commit.
Ohh It is appeared that it requires more deep analysis.
For example sync(2) is implicitly broken because ext4_sync_fs() may not
send a flush barrier. So following case is possible:

dd if=/dev/zero of=file oflag=direct bs=1M count=1
sync
POWER_FAILURE-> result in data lost. Because:

sys_write()
-> __generic_file_aio_write
   ->file_update_time
    ->update_time
      -> touch metadata -> ext4_update_inode_fsync_trans

  <# A lot of journal_start/journal_stop# >
 ->submit_bio  

 ->dio_complete

sys_sync()
 -> flush_inodes (may not start a journal)
 -> ext4_sync_fs
    if (jbd2_journal_start_commit(sbi->s_journal, &target)) {
                if (wait)
                        jbd2_log_wait_commit(sbi->s_journal, target);

   But no one guarantee us that we start any transaction since dio was
   completed so barrier will not be send. This means we may lose
   our data on power failure.
At this moment I try to figure up sane data integrity model for
fsync/syncfs. Hope I'll send you patches at the end of the week.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] ext4: optimize ext4_force_commit
  2013-04-09 14:06           ` Dmitry Monakhov
@ 2013-04-09 14:36             ` Jan Kara
  0 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2013-04-09 14:36 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: tytso, Jan Kara, linux-ext4

On Tue 09-04-13 18:06:58, Dmitry Monakhov wrote:
> On Tue, 9 Apr 2013 17:42:33 +0400, Dmitry Monakhov <dmonakhovopenvz@gmail.com> wrote:
> Non-text part: multipart/alternative
> > ---------- Forwarded message ----------
> > From: "Theodore Ts'o" <tytso@mit.edu>
> > Date: Apr 9, 2013 5:31 PM
> > Subject: Re: [PATCH 2/2] ext4: optimize ext4_force_commit
> > To: "Dmitry Monakhov" <dmonakhovopenvz@gmail.com>
> > Cc:
> > 
> > On Mon, Apr 01, 2013 at 11:10:59PM +0400, Dmitry Monakhov wrote:
> > > Yes. I think so too.  Even more jbd2_force_commit_nested already does what
> > > we want because we are not hold transaction. I'll send patch tomorrow
> > 
> > Hi Dmitry,
> > 
> > Did you send an update for this patch?  I can't seem to find it.  I'm
> > wondering if we perhaps got distracted by the big endian patch, and we
> > didn't get back to this commit.
> Ohh It is appeared that it requires more deep analysis.
> For example sync(2) is implicitly broken because ext4_sync_fs() may not
> send a flush barrier. So following case is possible:
> 
> dd if=/dev/zero of=file oflag=direct bs=1M count=1
> sync
> POWER_FAILURE-> result in data lost. Because:
> 
> sys_write()
> -> __generic_file_aio_write
>    ->file_update_time
>     ->update_time
>       -> touch metadata -> ext4_update_inode_fsync_trans
> 
>   <# A lot of journal_start/journal_stop# >
  If we are allocating blocks (as you would in the above example), then
we call ext4_update_inode_fsync_trans() for each allocation. But that's not
really important for your sync(2) example.

>  ->submit_bio  
> 
>  ->dio_complete
> 
> sys_sync()
>  -> flush_inodes (may not start a journal)
>  -> ext4_sync_fs
>     if (jbd2_journal_start_commit(sbi->s_journal, &target)) {
>                 if (wait)
>                         jbd2_log_wait_commit(sbi->s_journal, target);
> 
>    But no one guarantee us that we start any transaction since dio was
>    completed so barrier will not be send. This means we may lose
>    our data on power failure.
  So ext4_sync_file() actually handles this fine - if transaction commit
doesn't send the flush we need, it will send it manually. But you are right
that ext4_sync_fs() needs a similar thing.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] ext3: optimize ext3_force_commit
  2013-04-01  9:03 [PATCH 1/2] ext3: optimize ext3_force_commit Dmitry Monakhov
  2013-04-01  9:03 ` [PATCH 2/2] ext4: optimize ext4_force_commit Dmitry Monakhov
@ 2013-04-02 13:20 ` Jan Kara
  1 sibling, 0 replies; 6+ messages in thread
From: Jan Kara @ 2013-04-02 13:20 UTC (permalink / raw)
  To: Dmitry Monakhov; +Cc: linux-ext4, tytso, jack

On Mon 01-04-13 13:03:56, Dmitry Monakhov wrote:
> We do not have to use journal_force_commit() because it explicitly
> start and stop SYNC transaction. This is very suboptimal because the only
> users of ext3_force_commit() are ext3_sync_file and ext3_write_inode().
> Both functions just want to commit and wait any uncommitted transaction
> similar to ext3_sync_fs().
  As Ted mentioned, you can also remove ext3_journal_force_commit(). Also
I'm somewhat curious whether you spotted some particular performance issue
with this call or whether it just seemed better to you this way. Note that
I believe your change isn't going to be worse than what we had but OTOH we
have sync transaction batching code in journal_stop() so there might be
some changes in timings.

								Honza

> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/ext3/super.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext3/super.c b/fs/ext3/super.c
> index fb5120a..1853031 100644
> --- a/fs/ext3/super.c
> +++ b/fs/ext3/super.c
> @@ -2507,15 +2507,14 @@ static void ext3_clear_journal_err(struct super_block *sb,
>   */
>  int ext3_force_commit(struct super_block *sb)
>  {
> -	journal_t *journal;
> -	int ret;
> +	tid_t target;
>  
>  	if (sb->s_flags & MS_RDONLY)
>  		return 0;
>  
> -	journal = EXT3_SB(sb)->s_journal;
> -	ret = ext3_journal_force_commit(journal);
> -	return ret;
> +	if (journal_start_commit(EXT3_SB(sb)->s_journal, &target))
> +		return log_wait_commit(EXT3_SB(sb)->s_journal, target);
> +	return 0;
>  }
>  
>  static int ext3_sync_fs(struct super_block *sb, int wait)
> @@ -2530,7 +2529,7 @@ static int ext3_sync_fs(struct super_block *sb, int wait)
>  	dquot_writeback_dquots(sb, -1);
>  	if (journal_start_commit(EXT3_SB(sb)->s_journal, &target)) {
>  		if (wait)
> -			log_wait_commit(EXT3_SB(sb)->s_journal, target);
> +			return log_wait_commit(EXT3_SB(sb)->s_journal, target);
>  	}
>  	return 0;
>  }
> -- 
> 1.7.1
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-04-09 14:37 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-01  9:03 [PATCH 1/2] ext3: optimize ext3_force_commit Dmitry Monakhov
2013-04-01  9:03 ` [PATCH 2/2] ext4: optimize ext4_force_commit Dmitry Monakhov
2013-04-01 18:58   ` Theodore Ts'o
     [not found]     ` <CAF5pi0FZ4MUdBMH9A_Wav8V7TX39z1cOtGspJP9k8LN0KFja7g@mail.gmail.com>
     [not found]       ` <20130409133123.GE12050@thunk.org>
     [not found]         ` <CAF5pi0GhudB=BmBNGPAp4KLvfuAiw_2zo5bzYVWQ3jhthsj3Xw@mail.gmail.com>
2013-04-09 14:06           ` Dmitry Monakhov
2013-04-09 14:36             ` Jan Kara
2013-04-02 13:20 ` [PATCH 1/2] ext3: optimize ext3_force_commit Jan Kara

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