[PATCH 0/2] Fix dirtying of journalled buffers in data=journal mode

[PATCH 0/2] Fix dirtying of journalled buffers in data=journal mode

[Jan Kara]

  Hi,

  this is a second attempt to fix "JBD: Spotted dirty metadata buffer"
warnings from JBD/JBD2. This time I've found a much less hacky fix so
I'm now satisfied. fsx-linux in data=journal mode now runs without warning
for me. Ted, will you merge the ext4 patch, please? I'll merge the ext3 patch
if noone objects...

								Honza

[PATCH 1/2] ext4: Fix dirtying of journalled buffers in data=journal mode

[Jan Kara]

In data=journal mode, we still use block_write_begin() to prepare
page for writing. This function can occasionally mark buffer dirty
which violates journalling assumptions - when a buffer is part of
a transaction, it should be dirty and a buffer can be already part
of a forget list of some transaction when block_write_begin()
gets called. This violation of journalling assumptions then results
in "JBD: Spotted dirty metadata buffer..." warnings.

In fact, temporary dirtying the buffer while the page is still locked
does not really cause problems to the journalling because we won't write
the buffer until the page gets unlocked. So we just have to make sure
to clear dirty bits before unlocking the page.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 42272d6..702cfeb 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1519,9 +1519,25 @@ static int walk_page_buffers(handle_t *handle,
 static int do_journal_get_write_access(handle_t *handle,
 				       struct buffer_head *bh)
 {
+	int dirty = buffer_dirty(bh);
+	int ret;
+
 	if (!buffer_mapped(bh) || buffer_freed(bh))
 		return 0;
-	return ext4_journal_get_write_access(handle, bh);
+	/*
+	 * __block_prepare_write() could have dirtied some buffers. Clean
+	 * the dirty bit as jbd2_journal_get_write_access() could complain
+	 * otherwise about fs integrity issues. Setting of the dirty bit
+	 * by __block_prepare_write() isn't a real problem here as we clear
+	 * the bit before releasing a page lock and thus writeback cannot
+	 * ever write the buffer.
+	 */
+	if (dirty)
+		clear_buffer_dirty(bh);
+	ret = ext4_journal_get_write_access(handle, bh);
+	if (!ret && dirty)
+		ret = ext4_handle_dirty_metadata(handle, NULL, bh);
+	return ret;
 }
 
 /*
-- 
1.6.4.2

[PATCH 2/2] ext3: Fix dirtying of journalled buffers in data=journal mode

[Jan Kara]

In data=journal mode, we still use block_write_begin() to prepare page for
writing. This function can occasionally mark buffer dirty which violates
journalling assumptions - when a buffer is part of a transaction, it should be
dirty and a buffer can be already part of a forget list of some transaction
when block_write_begin() gets called. This violation of journalling assumptions
then results in "JBD: Spotted dirty metadata buffer..." warnings.

In fact, temporary dirtying the buffer while the page is still locked does not
really cause problems to the journalling because we won't write the buffer
until the page gets unlocked. So we just have to make sure to clear dirty bits
before unlocking the page.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext3/inode.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
index 735f019..1a84abb 100644
--- a/fs/ext3/inode.c
+++ b/fs/ext3/inode.c
@@ -1149,9 +1149,25 @@ static int walk_page_buffers(	handle_t *handle,
 static int do_journal_get_write_access(handle_t *handle,
 					struct buffer_head *bh)
 {
+	int dirty = buffer_dirty(bh);
+	int ret;
+
 	if (!buffer_mapped(bh) || buffer_freed(bh))
 		return 0;
-	return ext3_journal_get_write_access(handle, bh);
+	/*
+	 * __block_prepare_write() could have dirtied some buffers. Clean
+	 * the dirty bit as jbd2_journal_get_write_access() could complain
+	 * otherwise about fs integrity issues. Setting of the dirty bit
+	 * by __block_prepare_write() isn't a real problem here as we clear
+	 * the bit before releasing a page lock and thus writeback cannot
+	 * ever write the buffer.
+	 */
+	if (dirty)
+		clear_buffer_dirty(bh);
+	ret = ext3_journal_get_write_access(handle, bh);
+	if (!ret && dirty)
+		ret = ext3_journal_dirty_metadata(handle, bh);
+	return ret;
 }
 
 /*
-- 
1.6.4.2

Re: [PATCH 0/2] Fix dirtying of journalled buffers in data=journal mode

[Ted Ts'o]

On Thu, Aug 05, 2010 at 06:42:14PM +0200, Jan Kara wrote:
> 
>   this is a second attempt to fix "JBD: Spotted dirty metadata buffer"
> warnings from JBD/JBD2. This time I've found a much less hacky fix so
> I'm now satisfied. fsx-linux in data=journal mode now runs without warning
> for me. Ted, will you merge the ext4 patch, please? I'll merge the ext3 patch
> if noone objects...

Looks good to me; feel free to add an:

Reviewed-by: "Theodore Ts'o" <tytso@mit.edu>

to the ext3 patch.

The ext4 patch has survived my testing, and I've added it to the ext4
patch tree.

							- Ted

Re: [PATCH 2/2] ext3: Fix dirtying of journalled buffers in data=journal mode

[Andrew Morton]

On Thu,  5 Aug 2010 18:42:16 +0200
Jan Kara <jack@suse.cz> wrote:

> In data=journal mode, we still use block_write_begin() to prepare page for
> writing. This function can occasionally mark buffer dirty which violates
> journalling assumptions - when a buffer is part of a transaction, it should be
> dirty and a buffer can be already part of a forget list of some transaction
> when block_write_begin() gets called. This violation of journalling assumptions
> then results in "JBD: Spotted dirty metadata buffer..." warnings.
> 
> In fact, temporary dirtying the buffer while the page is still locked does not
> really cause problems to the journalling because we won't write the buffer
> until the page gets unlocked. So we just have to make sure to clear dirty bits
> before unlocking the page.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext3/inode.c |   18 +++++++++++++++++-
>  1 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
> index 735f019..1a84abb 100644
> --- a/fs/ext3/inode.c
> +++ b/fs/ext3/inode.c
> @@ -1149,9 +1149,25 @@ static int walk_page_buffers(	handle_t *handle,
>  static int do_journal_get_write_access(handle_t *handle,
>  					struct buffer_head *bh)
>  {
> +	int dirty = buffer_dirty(bh);
> +	int ret;
> +
>  	if (!buffer_mapped(bh) || buffer_freed(bh))
>  		return 0;
> -	return ext3_journal_get_write_access(handle, bh);
> +	/*
> +	 * __block_prepare_write() could have dirtied some buffers. Clean
> +	 * the dirty bit as jbd2_journal_get_write_access() could complain
> +	 * otherwise about fs integrity issues. Setting of the dirty bit
> +	 * by __block_prepare_write() isn't a real problem here as we clear
> +	 * the bit before releasing a page lock and thus writeback cannot
> +	 * ever write the buffer.
> +	 */
> +	if (dirty)
> +		clear_buffer_dirty(bh);

mark_buffer_dirty() can run set_page_dirty() which will set the page
dirty and increment dirty-page accounting.  If we then run
clear_buffer_dirty() we can end up with a dirty page which has clean
buffers and an off-by-one in dirty-page accounting.

Later, writeback will come along and will attempt to write the "dirty"
page.  It will discover the cleanness of the buffers, will mark the
page clean without doing any IO and will decrement the dirty-page
accounting.  So everything gets fixed up again.

So I don't see any problem here and this isn't the only place where
this sort of thing occurs.  It's just somethnig to be aware of and to
have a think about.


> +	ret = ext3_journal_get_write_access(handle, bh);
> +	if (!ret && dirty)
> +		ret = ext3_journal_dirty_metadata(handle, bh);
> +	return ret;
>  }
>  
>  /*
> -- 
> 1.6.4.2

Re: [PATCH 2/2] ext3: Fix dirtying of journalled buffers in data=journal mode

[Jan Kara]

On Thu 05-08-10 12:09:41, Andrew Morton wrote:
> On Thu,  5 Aug 2010 18:42:16 +0200
> Jan Kara <jack@suse.cz> wrote:
> 
> > In data=journal mode, we still use block_write_begin() to prepare page for
> > writing. This function can occasionally mark buffer dirty which violates
> > journalling assumptions - when a buffer is part of a transaction, it should be
> > dirty and a buffer can be already part of a forget list of some transaction
> > when block_write_begin() gets called. This violation of journalling assumptions
> > then results in "JBD: Spotted dirty metadata buffer..." warnings.
> > 
> > In fact, temporary dirtying the buffer while the page is still locked does not
> > really cause problems to the journalling because we won't write the buffer
> > until the page gets unlocked. So we just have to make sure to clear dirty bits
> > before unlocking the page.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/ext3/inode.c |   18 +++++++++++++++++-
> >  1 files changed, 17 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
> > index 735f019..1a84abb 100644
> > --- a/fs/ext3/inode.c
> > +++ b/fs/ext3/inode.c
> > @@ -1149,9 +1149,25 @@ static int walk_page_buffers(	handle_t *handle,
> >  static int do_journal_get_write_access(handle_t *handle,
> >  					struct buffer_head *bh)
> >  {
> > +	int dirty = buffer_dirty(bh);
> > +	int ret;
> > +
> >  	if (!buffer_mapped(bh) || buffer_freed(bh))
> >  		return 0;
> > -	return ext3_journal_get_write_access(handle, bh);
> > +	/*
> > +	 * __block_prepare_write() could have dirtied some buffers. Clean
> > +	 * the dirty bit as jbd2_journal_get_write_access() could complain
> > +	 * otherwise about fs integrity issues. Setting of the dirty bit
> > +	 * by __block_prepare_write() isn't a real problem here as we clear
> > +	 * the bit before releasing a page lock and thus writeback cannot
> > +	 * ever write the buffer.
> > +	 */
> > +	if (dirty)
> > +		clear_buffer_dirty(bh);
> 
> mark_buffer_dirty() can run set_page_dirty() which will set the page
> dirty and increment dirty-page accounting.  If we then run
> clear_buffer_dirty() we can end up with a dirty page which has clean
> buffers and an off-by-one in dirty-page accounting.
> 
> Later, writeback will come along and will attempt to write the "dirty"
> page.  It will discover the cleanness of the buffers, will mark the
> page clean without doing any IO and will decrement the dirty-page
> accounting.  So everything gets fixed up again.
> 
> So I don't see any problem here and this isn't the only place where
> this sort of thing occurs.  It's just somethnig to be aware of and to
> have a think about.
  Yes, JBD tends to do this a lot (with all the journaled buffers).
Thanks for review!

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