lkml.org 
[lkml]   [2015]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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



\
 
 \ /
  Last update: 2015-03-25 15:41    [W:0.061 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site