Hi Matthew,
On Wed, 2005-02-02 at 15:43 +0000, Matthew Wilcox wrote:
> On Wed, Feb 02, 2005 at 03:12:50PM +0000, Anton Altaparmakov wrote:
>
> I think the below loop would be clearer as a for loop ...
>
> err = 0;
> for (nr = 0; nr < nr_pages; nr++, start++) {
> if (start == lp_idx) {
> pages[nr] = locked_page;
> if (!nr)
> continue;
> lock_page(locked_page);
> if (!wbc)
> continue;
> if (wbc->for_reclaim) {
> up(&inode->i_sem);
> up_read(&inode->i_sb->s_umount);
> }
> /* Was the page truncated under us? */
> if (page_mapping(locked_page) != mapping) {
> err = -ESTALE;
> goto err_out_locked;
> }
> } else {
> pages[nr] = find_lock_page(mapping, start);
> if (pages[nr])
> continue;
> if (!cached_page) {
> cached_page = alloc_page(gfp_mask);
> if (unlikely(!cached_page))
> goto err_out;
> }
> err = add_to_page_cache_lru(cached_page,
> mapping, start, gfp_mask);
> if (unlikely(err)) {
> if (err == -EEXIST)
> continue;
> goto err_out;
> }
> pages[nr] = cached_page;
> cached_page = NULL;
> }
> }
>
> The above fixes two bugs in the below:
> - if (!unlikely(cached_page)) should be if (unlikely(!cached_page))
Ah, oops. Thanks! Well spotted! I did say it was only compile
tested... (-;
> - The -EEXIST case after add_to_page_cache_lru() would result in
> an infinite loop in the original as nr wasn't being incremented.
That was exactly what was meant to happen. It is not a bug. It is a
feature. This is why it is a while loop instead of a for loop. I need
to have @nr and @start incremented only if the code reaches the end of
the loop.
The -EEXIST case needs to repeat for the same @nr and @start. It
basically means that someone else allocated the page with index @start
and added it to the page cache in between us running find_lock_page()
and add_to_page_cache_lru(). So what we want to do is to run
find_lock_page() again which should then find and lock the page that the
other process created.
Of course what could happen is that between us getting the -EEXIST and
us repeating the find_lock_page() the page is freed again so the
find_lock_page() fails again. Perhaps this time we will succeed with
add_to_page_cache_lru() and if not we repeat again. Eventually either
find_lock_page() or add_to_page_cache_lru() will succeed so in practise
it will never be an endless loop.
If the while loop is changed to a for loop, the "continue;" on -EEXIST
would need to be changed to "goto repeat;" and a label "repeat:" would
need to be placed at the beginning of the loop. I considered this but
decided the while loop looks nicer. (-:
Thanks for the review!
> > + err = nr = 0;
> > + while (nr < nr_pages) {
> > + if (start == lp_idx) {
> > + pages[nr] = locked_page;
> > + if (nr) {
> > + lock_page(locked_page);
> > + if (wbc) {
> > + if (wbc->for_reclaim) {
> > + up(&inode->i_sem);
> > + up_read(&inode->i_sb->s_umount);
> > + }
> > + /* Was the page truncated under us? */
> > + if (page_mapping(locked_page) !=
> > + mapping) {
> > + err = -ESTALE;
> > + goto err_out_locked;
> > + }
> > + }
> > + }
> > + } else {
> > + pages[nr] = find_lock_page(mapping, start);
> > + if (!pages[nr]) {
> > + if (!cached_page) {
> > + cached_page = alloc_page(gfp_mask);
> > + if (!unlikely(cached_page))
> > + goto err_out;
> > + }
> > + err = add_to_page_cache_lru(cached_page,
> > + mapping, start, gfp_mask);
> > + if (unlikely(err)) {
> > + if (err == -EEXIST)
> > + continue;
> > + goto err_out;
> > + }
> > + pages[nr] = cached_page;
> > + cached_page = NULL;
> > + }
> > + }
> > + nr++;
> > + start++;
> > + }
Best regards,
Anton
--
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
