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

[Jens Axboe]

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@xxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/