Messages in this thread | | | Date | Wed, 25 Mar 2015 08:17:20 -0600 | From | Jens Axboe <> | Subject | Re: [PATCH 4/6] Add stream ID support for buffered writeback |
| |
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.
-- Jens Axboe
| |