Re: [PATCH 11/12] vmscan: Write out dirty pages in batch

[Christoph Hellwig]

> +	/*
> +	 * 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?

> +	/*
> +	 * 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);
	}

>  	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.

> + *
> + * 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.

--
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/