Messages in this thread | | | Subject | Re: RFC: [PATCH-2.6] Add helper function to lock multiple page cache pages. | From | Anton Altaparmakov <> | Date | Wed, 02 Feb 2005 15:56:38 +0000 |
| |
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 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/
| |