lkml.org 
[lkml]   [2010]   [Jun]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 11/12] vmscan: Write out dirty pages in batch
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


\
 
 \ /
  Last update: 2010-06-15 13:13    [from the cache]
©2003-2011 Jasper Spaans