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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 25 Mar 2015 08:17:20 -0600
From:	Jens Axboe <axboe@...nel.dk>
To:	Dave Chinner <david@...morbit.com>, Jens Axboe <axboe@...com>
CC:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	ming.l@....samsung.com
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@...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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/