Message ID | 20081010155447.GA14628@skywalker |
---|---|
State | Superseded, archived |
Headers | show |
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
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; }; /* diff --git a/mm/page-writeback.c b/mm/page-writeback.c index 718efa6..c198ead 100644 --- 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; pagevec_init(&pvec, 0); if (wbc->range_cyclic) { @@ -939,7 +946,7 @@ int write_cache_pages(struct address_space *mapping, unlock_page(page); ret = 0; } - if (ret || (--(wbc->nr_to_write) <= 0)) + if (ret || (--(*nr_to_write) <= 0)) done = 1; if (wbc->nonblocking && bdi_write_congested(bdi)) { wbc->encountered_congestion = 1; @@ -958,8 +965,11 @@ int write_cache_pages(struct address_space *mapping, index = 0; goto retry; } - 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; + } return ret; }