Re: writepage fs corruption fixes

[Andrea Arcangeli <andrea@suse.de>]

On Fri, Jul 09, 2004 at 10:56:34PM -0700, Andrew Morton wrote:
> Andrea Arcangeli <andrea@suse.de> wrote:
> >
> > +page_is_mapped:
> >  +
> >  +	end_index = i_size >> PAGE_CACHE_SHIFT;
> >   	if (page->index >= end_index) {
> >  -		unsigned offset = i_size_read(inode) & (PAGE_CACHE_SIZE - 1);
> >  +		unsigned offset = i_size & (PAGE_CACHE_SIZE - 1);
> >   		char *kaddr;
> >   
> >   		if (page->index > end_index || !offset)
> >  @@ -503,8 +506,6 @@ mpage_writepage(struct bio *bio, struct 
> >   		kunmap_atomic(kaddr, KM_USER0);
> >   	}
> >   
> >  -page_is_mapped:
> 
> What's the thinking behind moving the page_is_mapped label here?
> 
> We've established that we have found `first_unmapped' number of uptodate
> and dirty buffers at the "front" of the page, and we're about to stick
> (first_unmapped<<blkbits) bytes of this page into the BIO for writeout. 
> Hence everything which will go into the BIO is known to be uptodate and
> dirty.  So I'm wondering why this change was made.
> 
> 
> The change is correct, though.  It prevents us from writing non-zero data
> between i_size and the end of the final bh to the file. 
> block_write_full_page() does it too:

not sure to understand. The change is exactly to prevents us from
writing non-zero data between i_size and the end of the final bh. 

> (Note that this is a "best effort" thing - userspace could still write

sure I understand this is a best effort. I infact wanted to suggesting
that we could do it only once maybe (it's not guaranteed anyways, it's
up to userspace to do guarantee it, kernel cannot).

> But was this the reason for you making this change?

Just because block_write_full_page is doing it, so I think Chris was
correct making that change and moving the label. Either we remove it
from both, or we do it in both places. If we remove it we've just to be
careful on how to deal with the _first_ clearing. the prepare_write is
already ok, if the buffer is uptodate then we know we can write it. the
non-bh case needs to be checked and I belive that's the reason you were
doing the thing only for the non-bh case.

the only real fixes are the other two ones, the page_is_mapped move is
just because I agree with Chris it's more correct to do the same thing
as block_write_full_page to get the very same behaviour from both.