Messages in this thread | | | Date | Sat, 10 Jul 2004 08:11:33 +0200 | From | Andrea Arcangeli <> | Subject | Re: writepage fs corruption fixes |
| |
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |