Re: [patch 4/8] mm: write_cache_pages type overflow fix

[Christoph Hellwig <hch@infradead.org>]

On Fri, Oct 10, 2008 at 09:24:47PM +0530, Aneesh Kumar K.V wrote:
> diff --git a/include/linux/writeback.h b/include/linux/writeback.h
> index bd91987..7599af2 100644
> --- a/include/linux/writeback.h
> +++ b/include/linux/writeback.h
> @@ -63,6 +63,8 @@ struct writeback_control {
>  	unsigned for_writepages:1;	/* This is a writepages() call */
>  	unsigned range_cyclic:1;	/* range_start is cyclic */
>  	unsigned more_io:1;		/* more io to be dispatched */
> +	/* flags which control the write_cache_pages behaviour */
> +	int writeback_flags;

As Ted already said please follow the bitfields style already used.

> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -876,11 +876,18 @@ int write_cache_pages(struct address_space *mapping,
>  	pgoff_t end;		/* Inclusive */
>  	int scanned = 0;
>  	int range_whole = 0;
> +	int flags = wbc->writeback_flags;
> +	long *nr_to_write, count;
>  
>  	if (wbc->nonblocking && bdi_write_congested(bdi)) {
>  		wbc->encountered_congestion = 1;
>  		return 0;
>  	}
> +	if (flags & WB_NO_NRWRITE_UPDATE) {
> +		count  = wbc->nr_to_write;
> +		nr_to_write = &count;
> +	} else
> +		nr_to_write = &wbc->nr_to_write;

I think we'd be better off always using a local variable and updating
wbc->nr_to_write again before the exit for the !WB_NO_NRWRITE_UPDATE
case.

> -	if (wbc->range_cyclic || (range_whole && wbc->nr_to_write > 0))
> +	if ((wbc->range_cyclic ||
> +			(range_whole && wbc->nr_to_write > 0)) && 
> +			(flags & ~WB_NO_INDEX_UPDATE)) {
>  		mapping->writeback_index = index;

The conditional looks rather odd, what about:

	if (!wbc->no_index_update &&
	    (wbc->range_cyclic || (range_whole && wbc->nr_to_write > 0))

Also I wonder what this is for.  Do you want what Chris did in his
original patch in ext4 code, or is there another reason to not update
the writeback_index sometimes?