Re: RFC: [PATCH-2.6] Add helper function to lock multiple pagecache pages.

[Anton Altaparmakov]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/