Messages in this thread | | | Date | Sat, 28 Aug 2004 15:54:47 +1000 | From | Nick Piggin <> | Subject | Re: data loss in 2.6.9-rc1-mm1 |
| |
Ram Pai wrote: > On Fri, 2004-08-27 at 21:35, Nick Piggin wrote: > >>Ram Pai wrote: >> >>>got it! Everything got changed to the new convention except that >>>the calculation of 'nr' just before the check "nr <= offset" . >>> >>>I have generated this patch which takes care of that and hence fixes the >>>data loss problem as well. I guess it is cleaner too. >>> >>>This patch is generated w.r.t 2.6.8.1. If everybody blesses this patch I >>>will forward it to Andrew. >> >>It looks like it should be OK... but at what point does it become >>simpler to use my patch which just moves the original calculation >>up, and does it again if we have to ->readpage()? >> >>(assuming you agree that it solves the problem) > > > I agree your patch also solves the problem. > > Either way is fine. Even Hugh's patch almost does the same thing as > yours.
Ahh, yep - Hugh just forgot to also move the "nr" calculation into the ->readpage path, so it hits twice on the fast path.
> The only advantage with my page is it does the calculation in > only one place and does not repeat it. Also I feel its more intuitive to
Well kind of - but you are having to jump through hoops to get there. Yours does the following checks:
/* fast path, read nr_pages from pagecache */ if (!isize) goto out; for (i = 0; i < nr_pages; i++) { if (index > end_index) goto out; if (index == end_index) { nr = ((isize - 1) & ~PAGE_CACHE_MASK) + 1; if (nr <= offset) { page_cache_release(page); goto out; } }
/* slowpath, ->readpage */ if (unlikely(!isize || index > end_index)) { page_cache_release(page); goto out; } }
Mine does: if (index > end_index) goto out; for (i = 0; i < pages_to_read; i++) { if (index == end_index) { nr = isize & ~PAGE_CACHE_MASK; if (nr <= offset) goto out; }
/* slowpath, ->readpage */ if (index > end_index) { page_cache_release(page); goto out; } if (index == end_index) { nr = isize & ~PAGE_CACHE_MASK; if (nr <= offset) { page_cache_release(page); goto out; } } }
So my fastpath is surely leaner, while the slowpath isn't a clear loser.
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...?
> assume that index 0 covers range 0 to 4095 i.e index n covers range > n*PAGE_SIZE to ((n+1)*PAGE_SIZE)-1. Currently the code assumes index 0 > covers range 1 to 4096 i.e index n covers range (n*PAGE_SIZE)+1 to > (n+1)*PAGE_SIZE. >
It is definitely a pretty ugly function all round. I like the 0-4095 thing better too, but my counter argument to that is that this is the minimal change, and similar to how it has previously worked.
> this is the 4th time we are trying to nail down the same thing. We > better get it right this time. So any correct patch is ok with me. >
I agree. We'll leave it to someone else to decide, then ;) - 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/
| |