Re: [PATCH 1/2] ext3: Fix buffer dirtying in data=journal mode (fwd)

Re: [PATCH 1/2] ext3: Fix buffer dirtying in data=journal mode (fwd)

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

On Tue 03-08-10 15:58:42, Andrew Morton wrote:
> On Wed, 4 Aug 2010 00:34:18 +0200
> Jan Kara <jack@suse.cz> wrote:
> 
> > On Tue 03-08-10 12:06:44, Andrew Morton wrote:
> > > On Tue, 3 Aug 2010 15:57:48 +0200
> > > Jan Kara <jack@suse.cz> wrote:
> > > 
> > > > > I don't know if that's correct.  write() will zero out the pagecache
> > > > > for the whole length if copy_from_user() fails.  Then it will go and
> > > > > mark the page dirty regardless of whether the copy_from_user() failed. 
> > > > > I think.
> > > >   Yeah, you are right. page_zero_new_buffers() and/or __block_commit_write
> > > > take care of this. So code-wise it would be cleanest to just remove that
> > > > problematic hunk from __block_prepare_write. But that means auditing all the
> > > > filesystems that get to __block_prepare_write that they also end up calling
> > > > block_commit_write so that the buffer is dirtied anyway. Sigh... OK, I'll
> > > > do that.
> > > It'd be worth going back into the git (actually the bitkeeper) archive,
> > > see if we can find out why that code was added.
> >   OK, the history is interesting. The code has been added by commit
> > 637aff46 and at that time was correct to avoid zeroing valid data filled
> > via mmap. But shortly after that came afddba49, which introduced
> > page_zero_new_buffers() which handles this problem on it's own. So the hunk
> > is obsolete. But OTOH it makes it easier to see that we cannot zero-out
> > uptodate data in a new buffer. Speaking of which just removing the hunk
> > won't fix my problem as page_zero_new_buffers() called from
> > __block_prepare_write dirties the buffer anyway. So in the end I'm not sure
> > whether we should remove it or not...
> 
> hm.  Well if we're going to leave the code there then the least we can
> do is add a big comment explaining all of this?
  In the end, I think removing the code *and* adding a big comment should
be fine. Attached is a patch which does this. I've added Nick and fsdevel
to CC.

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

[-- Attachment #2: 0001-fs-Remove-obsolete-buffer-handling.patch --]
[-- Type: text/x-patch, Size: 1589 bytes --]

>From 40310ae771ba1d625c7917891a5d4f84f5b458ac Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Wed, 4 Aug 2010 17:10:56 +0200
Subject: [PATCH] fs: Remove obsolete buffer handling

When we freshly allocate a new block under an uptodate page, we have
to make sure it doesn't get zeroed and gets marked dirty. Commit
637aff46 added a code to handle this case properly. Later, commit
afddba49 added page_zero_new_buffers() function which takes care of
this case properly as well and all filesystems use it either directly
or indirectly. So let's remove the obsolete code so that we handle
this special case only in one place.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/buffer.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index d54812b..a2947c2 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1876,9 +1876,17 @@ static int __block_prepare_write(struct inode *inode, struct page *page,
 				unmap_underlying_metadata(bh->b_bdev,
 							bh->b_blocknr);
 				if (PageUptodate(page)) {
-					clear_buffer_new(bh);
+					/*
+					 * New buffer underlying already
+					 * uptodate data. This can happen when
+					 * data was already written via mmap.
+					 * We must not zero-out already valid
+					 * data and must mark buffer dirty.
+					 * page_zero_new_buffers() takes care
+					 * of this (or block_commit_write() if
+					 * data gets overwritten).
+					 */
 					set_buffer_uptodate(bh);
-					mark_buffer_dirty(bh);
 					continue;
 				}
 				if (block_end > to || block_start < from)
-- 
1.6.4.2

Re: [PATCH 1/2] ext3: Fix buffer dirtying in data=journal mode (fwd)

[Nick Piggin]

On Wed, Aug 04, 2010 at 05:35:22PM +0200, Jan Kara wrote:
> On Tue 03-08-10 15:58:42, Andrew Morton wrote:
> >From 40310ae771ba1d625c7917891a5d4f84f5b458ac Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@suse.cz>
> Date: Wed, 4 Aug 2010 17:10:56 +0200
> Subject: [PATCH] fs: Remove obsolete buffer handling
> 
> When we freshly allocate a new block under an uptodate page, we have
> to make sure it doesn't get zeroed and gets marked dirty. Commit
> 637aff46 added a code to handle this case properly. Later, commit
> afddba49 added page_zero_new_buffers() function which takes care of
> this case properly as well and all filesystems use it either directly
> or indirectly. So let's remove the obsolete code so that we handle
> this special case only in one place.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

I don't see a problem, but it is a special case. Because the data
which is causing the buffer to be dirtied is already in the pagecache
-- not the data we're about to write in there.

But with the comment it is clear enough.

Acked-by: Nick Piggin <npiggin@suse.de>


> ---
>  fs/buffer.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index d54812b..a2947c2 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1876,9 +1876,17 @@ static int __block_prepare_write(struct inode *inode, struct page *page,
>  				unmap_underlying_metadata(bh->b_bdev,
>  							bh->b_blocknr);
>  				if (PageUptodate(page)) {
> -					clear_buffer_new(bh);
> +					/*
> +					 * New buffer underlying already
> +					 * uptodate data. This can happen when
> +					 * data was already written via mmap.
> +					 * We must not zero-out already valid
> +					 * data and must mark buffer dirty.
> +					 * page_zero_new_buffers() takes care
> +					 * of this (or block_commit_write() if
> +					 * data gets overwritten).
> +					 */
>  					set_buffer_uptodate(bh);
> -					mark_buffer_dirty(bh);
>  					continue;
>  				}
>  				if (block_end > to || block_start < from)
> --