| Message ID | 1427210823-5283-5-git-send-email-axboe@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 24, 2015 at 09:27:01AM -0600, Jens Axboe wrote: > Add a streamid field to the writeback_control structure, and use > it for the various parts of buffered writeback. > > Signed-off-by: Jens Axboe <axboe@fb.com> > --- > fs/buffer.c | 4 ++-- > fs/fs-writeback.c | 1 + > fs/mpage.c | 1 + > include/linux/writeback.h | 2 ++ > mm/filemap.c | 1 + > mm/migrate.c | 1 + > mm/page-writeback.c | 1 + > mm/vmscan.c | 1 + > 8 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/fs/buffer.c b/fs/buffer.c > index 20805db2c987..1ae99868f6fb 100644 > --- a/fs/buffer.c > +++ b/fs/buffer.c > @@ -1774,7 +1774,7 @@ static int __block_write_full_page(struct inode *inode, struct page *page, > do { > struct buffer_head *next = bh->b_this_page; > if (buffer_async_write(bh)) { > - submit_bh(write_op, bh); > + _submit_bh(write_op, bh, streamid_to_flags(wbc->streamid)); Urk, the is the second patchset in a couple of days to add some random parameter to submit_bh like this (control group thottling patch from Tejun was the other). This doesn't scale.... > diff --git a/include/linux/writeback.h b/include/linux/writeback.h > index 00048339c23e..3ac2ce545dac 100644 > --- a/include/linux/writeback.h > +++ b/include/linux/writeback.h > @@ -78,6 +78,8 @@ struct writeback_control { > > enum writeback_sync_modes sync_mode; > > + unsigned int streamid; > + > unsigned for_kupdate:1; /* A kupdate writeback */ > unsigned for_background:1; /* A background writeback */ > unsigned tagged_writepages:1; /* tag-and-write to avoid livelock */ > diff --git a/mm/filemap.c b/mm/filemap.c > index ad7242043bdb..85aa2cc77d67 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -276,6 +276,7 @@ int __filemap_fdatawrite_range(struct address_space *mapping, loff_t start, > struct writeback_control wbc = { > .sync_mode = sync_mode, > .nr_to_write = LONG_MAX, > + .streamid = inode_streamid(mapping->host), > .range_start = start, > .range_end = end, > }; I don't think this is the right layer to be specifying the stream id. When we do buffered writeback, the filesystem's .writepage implementation gets called and it has access to the inode and hence can grab the stream id there. the struct wbc is used for writeback across more than one inode, and hence there's going to be nothing but confusion when this gets set and we iterate writeback across multiple inodes. i.e. the stream id should be set by the code the packs the pages into the bio/bh that is being submitted where there is a direct relationship between the inode and the IO being built, rather than at a high layer where the stream id has ambiguous meaning.... Cheers, Dave.
On 03/24/2015 08:40 PM, Dave Chinner wrote: > On Tue, Mar 24, 2015 at 09:27:01AM -0600, Jens Axboe wrote: >> Add a streamid field to the writeback_control structure, and use >> it for the various parts of buffered writeback. >> >> Signed-off-by: Jens Axboe <axboe@fb.com> >> --- >> fs/buffer.c | 4 ++-- >> fs/fs-writeback.c | 1 + >> fs/mpage.c | 1 + >> include/linux/writeback.h | 2 ++ >> mm/filemap.c | 1 + >> mm/migrate.c | 1 + >> mm/page-writeback.c | 1 + >> mm/vmscan.c | 1 + >> 8 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/fs/buffer.c b/fs/buffer.c >> index 20805db2c987..1ae99868f6fb 100644 >> --- a/fs/buffer.c >> +++ b/fs/buffer.c >> @@ -1774,7 +1774,7 @@ static int __block_write_full_page(struct inode *inode, struct page *page, >> do { >> struct buffer_head *next = bh->b_this_page; >> if (buffer_async_write(bh)) { >> - submit_bh(write_op, bh); >> + _submit_bh(write_op, bh, streamid_to_flags(wbc->streamid)); > > Urk, the is the second patchset in a couple of days to add some > random parameter to submit_bh like this (control group thottling > patch from Tejun was the other). > > This doesn't scale.... It's not adding a parameter, it's just calling _submit_bh() instead of submit_bh(). >> diff --git a/include/linux/writeback.h b/include/linux/writeback.h >> index 00048339c23e..3ac2ce545dac 100644 >> --- a/include/linux/writeback.h >> +++ b/include/linux/writeback.h >> @@ -78,6 +78,8 @@ struct writeback_control { >> >> enum writeback_sync_modes sync_mode; >> >> + unsigned int streamid; >> + >> unsigned for_kupdate:1; /* A kupdate writeback */ >> unsigned for_background:1; /* A background writeback */ >> unsigned tagged_writepages:1; /* tag-and-write to avoid livelock */ >> diff --git a/mm/filemap.c b/mm/filemap.c >> index ad7242043bdb..85aa2cc77d67 100644 >> --- a/mm/filemap.c >> +++ b/mm/filemap.c >> @@ -276,6 +276,7 @@ int __filemap_fdatawrite_range(struct address_space *mapping, loff_t start, >> struct writeback_control wbc = { >> .sync_mode = sync_mode, >> .nr_to_write = LONG_MAX, >> + .streamid = inode_streamid(mapping->host), >> .range_start = start, >> .range_end = end, >> }; > > I don't think this is the right layer to be specifying the stream > id. When we do buffered writeback, the filesystem's .writepage > implementation gets called and it has access to the inode and hence > can grab the stream id there. the struct wbc is used for writeback > across more than one inode, and hence there's going to be nothing > but confusion when this gets set and we iterate writeback across > multiple inodes. > > i.e. the stream id should be set by the code the packs the pages > into the bio/bh that is being submitted where there is a direct > relationship between the inode and the IO being built, rather than > at a high layer where the stream id has ambiguous meaning.... Yeah that's a good point, it was a bit of a lazy mans solution to not have to touch all the ->writepage(s) hooks. But you are right, I'll do that instead.
diff --git a/fs/buffer.c b/fs/buffer.c index 20805db2c987..1ae99868f6fb 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -1774,7 +1774,7 @@ static int __block_write_full_page(struct inode *inode, struct page *page, do { struct buffer_head *next = bh->b_this_page; if (buffer_async_write(bh)) { - submit_bh(write_op, bh); + _submit_bh(write_op, bh, streamid_to_flags(wbc->streamid)); nr_underway++; } bh = next; @@ -1828,7 +1828,7 @@ recover: struct buffer_head *next = bh->b_this_page; if (buffer_async_write(bh)) { clear_buffer_dirty(bh); - submit_bh(write_op, bh); + _submit_bh(write_op, bh, streamid_to_flags(wbc->streamid)); nr_underway++; } bh = next; diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index e907052eeadb..0d0a9f6268f5 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -733,6 +733,7 @@ static long writeback_sb_inodes(struct super_block *sb, * We use I_SYNC to pin the inode in memory. While it is set * evict_inode() will wait so the inode cannot be freed. */ + wbc.streamid = inode->i_streamid; __writeback_single_inode(inode, &wbc); work->nr_pages -= write_chunk - wbc.nr_to_write; diff --git a/fs/mpage.c b/fs/mpage.c index 3e79220babac..ae19d35bdd96 100644 --- a/fs/mpage.c +++ b/fs/mpage.c @@ -605,6 +605,7 @@ alloc_new: bio_get_nr_vecs(bdev), GFP_NOFS|__GFP_HIGH); if (bio == NULL) goto confused; + bio_set_streamid(bio, wbc->streamid); } /* diff --git a/include/linux/writeback.h b/include/linux/writeback.h index 00048339c23e..3ac2ce545dac 100644 --- a/include/linux/writeback.h +++ b/include/linux/writeback.h @@ -78,6 +78,8 @@ struct writeback_control { enum writeback_sync_modes sync_mode; + unsigned int streamid; + unsigned for_kupdate:1; /* A kupdate writeback */ unsigned for_background:1; /* A background writeback */ unsigned tagged_writepages:1; /* tag-and-write to avoid livelock */ diff --git a/mm/filemap.c b/mm/filemap.c index ad7242043bdb..85aa2cc77d67 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -276,6 +276,7 @@ int __filemap_fdatawrite_range(struct address_space *mapping, loff_t start, struct writeback_control wbc = { .sync_mode = sync_mode, .nr_to_write = LONG_MAX, + .streamid = inode_streamid(mapping->host), .range_start = start, .range_end = end, }; diff --git a/mm/migrate.c b/mm/migrate.c index 85e042686031..614407dceb55 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -645,6 +645,7 @@ static int writeout(struct address_space *mapping, struct page *page) struct writeback_control wbc = { .sync_mode = WB_SYNC_NONE, .nr_to_write = 1, + .streamid = inode_streamid(mapping->host), .range_start = 0, .range_end = LLONG_MAX, .for_reclaim = 1 diff --git a/mm/page-writeback.c b/mm/page-writeback.c index 45e187b2d971..47120abe8afb 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -2051,6 +2051,7 @@ int write_one_page(struct page *page, int wait) int ret = 0; struct writeback_control wbc = { .sync_mode = WB_SYNC_ALL, + .streamid = inode_streamid(mapping->host), .nr_to_write = 1, }; diff --git a/mm/vmscan.c b/mm/vmscan.c index 5e8eadd71bac..67fcdd082c17 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -546,6 +546,7 @@ static pageout_t pageout(struct page *page, struct address_space *mapping, struct writeback_control wbc = { .sync_mode = WB_SYNC_NONE, .nr_to_write = SWAP_CLUSTER_MAX, + .streamid = inode_streamid(mapping->host), .range_start = 0, .range_end = LLONG_MAX, .for_reclaim = 1,
Add a streamid field to the writeback_control structure, and use it for the various parts of buffered writeback. Signed-off-by: Jens Axboe <axboe@fb.com> --- fs/buffer.c | 4 ++-- fs/fs-writeback.c | 1 + fs/mpage.c | 1 + include/linux/writeback.h | 2 ++ mm/filemap.c | 1 + mm/migrate.c | 1 + mm/page-writeback.c | 1 + mm/vmscan.c | 1 + 8 files changed, 10 insertions(+), 2 deletions(-)