[4/8] mm: write_cache_pages type overflow fix

On Fri, Oct 10, 2008 at 10:08:29AM -0400, Christoph Hellwig wrote:
> On Fri, Oct 10, 2008 at 10:05:35AM -0400, Theodore Tso wrote:
> > 3) A version which (optionally via a flag in the wbc structure)
> > instructs write_cache_pages() to not pursue those updates.  This has
> > not been written yet.
> 
> This one sounds best to me (although we'd have to actualy see it..)

something like  the below ?

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, 2008-10-10 at 21:24 +0530, Aneesh Kumar K.V wrote:
> On Fri, Oct 10, 2008 at 10:08:29AM -0400, Christoph Hellwig wrote:
> > On Fri, Oct 10, 2008 at 10:05:35AM -0400, Theodore Tso wrote:
> > > 3) A version which (optionally via a flag in the wbc structure)
> > > instructs write_cache_pages() to not pursue those updates.  This has
> > > not been written yet.
> > 
> > This one sounds best to me (although we'd have to actualy see it..)
> 
> something like  the below ?
>
> 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;
>  };

Doesn't seem in line with the bitflag method currently used in struct
writeback_control.

-chris


--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Oct 10, 2008 at 09:24:47PM +0530, Aneesh Kumar K.V wrote:
> On Fri, Oct 10, 2008 at 10:08:29AM -0400, Christoph Hellwig wrote:
> > On Fri, Oct 10, 2008 at 10:05:35AM -0400, Theodore Tso wrote:
> > > 3) A version which (optionally via a flag in the wbc structure)
> > > instructs write_cache_pages() to not pursue those updates.  This has
> > > not been written yet.
> > 
> > This one sounds best to me (although we'd have to actualy see it..)
> 
> something like  the below ?
> 
> 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;
>  };

I don't see a definition for WB_NO_NRWRITE_UPDATE and
WB_NO_INDEX_UPDATE in your patch?

Given the structure seems to be using bitfields for all of the other
fields, why not do this instead?

	unsigned no_nrwrite_update:1;
	unsigned no_index_update:1;

Personally, I'm old school, and prefer using an int flag field and
using #define's for flags, but the rest of the structure is using
bitfields for flags, and it's probably better to be consistent...
	

							- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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?

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html