Re: [PATCH 4/6] Add stream ID support for buffered writeback

From: Jens Axboe <axboe@kernel.dk>
To: Dave Chinner <david@fromorbit.com>, Jens Axboe <axboe@fb.com>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	ming.l@ssi.samsung.com
Subject: Re: [PATCH 4/6] Add stream ID support for buffered writeback
Date: Wed, 25 Mar 2015 08:17:20 -0600	[thread overview]
Message-ID: <5512C370.4070501@kernel.dk> (raw)
In-Reply-To: <20150325024003.GD31342@dastard>

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