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

From: Dave Chinner <david@fromorbit.com>
To: Jens Axboe <axboe@fb.com>
Cc: axboe@kernel.dk, 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 13:40:03 +1100	[thread overview]
Message-ID: <20150325024003.GD31342@dastard> (raw)
In-Reply-To: <1427210823-5283-5-git-send-email-axboe@fb.com>

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.
-- 
Dave Chinner
david@fromorbit.com