On Tue, Jun 15, 2010 at 06:53:41AM -0400, Christoph Hellwig wrote:
> > + /*
> > + * XXX: This is the Holy Hand Grenade of PotentiallyInvalidMapping. As
> > + * the page lock has been dropped by ->writepage, that mapping could
> > + * be anything
> > + */
>
> Why is this an XXX comment?
>
With the page lock released, the mapping may be no longer valid. Nick
posted a patch in relation to it that I need to look at. The comment was
because Andrew highlight that this was buggy and I wanted to make sure I
didn't forget about it.
> > + /*
> > + * Wait on writeback if requested to. This happens when
> > + * direct reclaiming a large contiguous area and the
> > + * first attempt to free a range of pages fails.
> > + */
> > + if (PageWriteback(page) && sync_writeback == PAGEOUT_IO_SYNC)
> > + wait_on_page_writeback(page);
> > +
> > + if (!PageWriteback(page)) {
> > + /* synchronous write or broken a_ops? */
> > + ClearPageReclaim(page);
> > + }
>
> how about:
>
> if (PageWriteback(page) {
> if (sync_writeback == PAGEOUT_IO_SYNC)
> wait_on_page_writeback(page);
> } else {
> /* synchronous write or broken a_ops? */
> ClearPageReclaim(page);
> }
>
Sure, that's tidier.
> > if (!may_write_to_queue(mapping->backing_dev_info))
> > return PAGE_KEEP;
> >
> > /*
> > + * Clean a list of pages. It is expected that all the pages on page_list have been
> > + * locked as part of isolation from the LRU.
>
> A rather pointless line of 80 chars. I see the point for long string
> literals, but here's it's just a pain.
>
I'll trim it.
> > + *
> > + * XXX: Is there a problem with holding multiple page locks like this?
>
> I think there is. There's quite a few places that do hold multiple
> pages locked, but they always lock pages in increasing page->inxex order.
> Given that this locks basically in random order it could cause problems
> for those places.
>
Hmm, ok. In that case, I'll have to release the locks on the list and
reacquire them. It was something I would have preferred to avoid. Thanks
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
