Messages in this thread | | | Date | Sat, 28 Aug 2004 15:52:41 +0100 (BST) | From | Hugh Dickins <> | Subject | Re: data loss in 2.6.9-rc1-mm1 |
| |
On Sat, 28 Aug 2004, Nick Piggin wrote: > > Ahh, yep - Hugh just forgot to also move the "nr" calculation > into the ->readpage path, so it hits twice on the fast path.
Yes, your patch is better than mine.
> What's more, it looks like mine handles the corner case of reading off the > end of a non-PAGE_SIZE file (but within the same page). I think yours will > drop through and do the ->readpage, while mine doesn't...?
It's a common case, not a corner case: a short read to end of file, then app does another read which returns 0 for the end of file: that wouldn't normally fall through to readpage in Ram's case, but would do unnecessary page_cache_readahead (won't do much) and find_get_page.
> I agree. We'll leave it to someone else to decide, then ;)
I vote for Nick's patch.
I do have one reservation on do_generic_mapping_read, common to all these versions, unrelated to the current issue.
I'm surprised to notice that you're careful to re-i_size_read after readpage, but otherwise rely on the initial i_size_read. We could go around this loop many many times, faulting user pages in actor, rescheduling away: the old (e.g. 2.4 or 2.6.0) code was deficient after readpage, but at least it reassessed i_size each time around the loop. I guess if the file is truncated meanwhile, the common case would be for a find_get_page to fail, and then the situation be corrected after readpage; perhaps it's more likely to show up as read returning too little on a large file being steadily appended. Maybe you already ruled these cases out as not worth the overhead of handling, but it does surprise me that the old code was safer in this respect.
Hugh
- 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/
| |