Hi Jens,
This RFC DRAFT patch is on top of your "[PATCH v2] Support for write stream IDs"
I throw it out early to get comments if it's the way to go.
Quote LWN(http://lwn.net/Articles/638722):
"There would be clear value in a closer association between stream IDs
and specific buffered-write operations. Getting there would require storing
the stream ID with each dirtied page, though; that, in turn, almost certainly
implies shoehorning the stream ID into the associated page structure.
That would not be an easy task; it is not surprising that it is not a part of
this patch set. Should the lack of per-buffered-write stream IDs prove to be
a serious constraint in the future, somebody will certainly be motivated to
try to find a place to store another eight bits in struct page."
This draft patch stores stream_id in buffer head instead of page.
---
fs/buffer.c | 4 ++--
fs/ext4/page-io.c | 2 +-
fs/mpage.c | 5 ++++-
include/linux/buffer_head.h | 1 +
mm/filemap.c | 17 +++++++++++++++++
5 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/fs/buffer.c b/fs/buffer.c
index 5191523..274fcc5 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, streamid_to_flags(inode_streamid(inode)));
+ _submit_bh(write_op, bh, streamid_to_flags(bh->b_streamid));
nr_underway++;
}
bh = next;
@@ -1828,7 +1828,7 @@ recover:
struct buffer_head *next = bh->b_this_page;
if (buffer_async_write(bh)) {
clear_buffer_dirty(bh);
- _submit_bh(write_op, bh, streamid_to_flags(inode_streamid(inode)));
+ _submit_bh(write_op, bh, streamid_to_flags(bh->b_streamid));
nr_underway++;
}
bh = next;
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index 980c5a9..c912129 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -406,7 +406,7 @@ submit_and_retry:
ret = io_submit_init_bio(io, bh);
if (ret)
return ret;
- bio_set_streamid(io->io_bio, inode_streamid(inode));
+ bio_set_streamid(io->io_bio, bh->b_streamid);
}
ret = bio_add_page(io->io_bio, page, bh->b_size, bh_offset(bh));
if (ret != bh->b_size)
diff --git a/fs/mpage.c b/fs/mpage.c
index fba13f4..1c5ae1c 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -481,12 +481,15 @@ static int __mpage_writepage(struct page *page, struct writeback_control *wbc,
int length;
struct buffer_head map_bh;
loff_t i_size = i_size_read(inode);
+ int streamid = 0;
int ret = 0;
if (page_has_buffers(page)) {
struct buffer_head *head = page_buffers(page);
struct buffer_head *bh = head;
+ streamid = bh->b_streamid;
+
/* If they're all mapped and dirty, do it */
page_block = 0;
do {
@@ -605,7 +608,7 @@ alloc_new:
bio_get_nr_vecs(bdev), GFP_NOFS|__GFP_HIGH);
if (bio == NULL)
goto confused;
- bio_set_streamid(bio, inode_streamid(inode));
+ bio_set_streamid(bio, streamid);
}
/*
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 73b4522..5dbb460 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -75,6 +75,7 @@ struct buffer_head {
struct address_space *b_assoc_map; /* mapping this buffer is
associated with */
atomic_t b_count; /* users using this buffer_head */
+ unsigned int b_streamid;
};
/*
diff --git a/mm/filemap.c b/mm/filemap.c
index 6bf5e42..3a31b95 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2419,6 +2419,21 @@ struct page *grab_cache_page_write_begin(struct address_space *mapping,
}
EXPORT_SYMBOL(grab_cache_page_write_begin);
+static void buffer_head_set_streamid(struct page *page, struct file *file)
+{
+ struct buffer_head *bh, *head;
+
+ if (!page_has_buffers(page))
+ return;
+
+ head = page_buffers(page);
+ bh = head;
+ do {
+ bh->b_streamid = file->f_streamid;
+ bh = bh->b_this_page;
+ } while (bh != head);
+}
+
ssize_t generic_perform_write(struct file *file,
struct iov_iter *i, loff_t pos)
{
@@ -2466,6 +2481,8 @@ again:
if (unlikely(status < 0))
break;
+ buffer_head_set_streamid(page, file);
+
if (mapping_writably_mapped(mapping))
flush_dcache_page(page);
--
1.9.1
On 04/21/2015 11:09 AM, Ming Lin wrote:
> Hi Jens,
>
> This RFC DRAFT patch is on top of your "[PATCH v2] Support for write stream IDs"
> I throw it out early to get comments if it's the way to go.
>
> Quote LWN(http://lwn.net/Articles/638722):
>
> "There would be clear value in a closer association between stream IDs
> and specific buffered-write operations. Getting there would require storing
> the stream ID with each dirtied page, though; that, in turn, almost certainly
> implies shoehorning the stream ID into the associated page structure.
> That would not be an easy task; it is not surprising that it is not a part of
> this patch set. Should the lack of per-buffered-write stream IDs prove to be
> a serious constraint in the future, somebody will certainly be motivated to
> try to find a place to store another eight bits in struct page."
>
> This draft patch stores stream_id in buffer head instead of page.
This is pointless. You need to store it in the page, if the whole point
is that you want this to be trackable. And adding it to struct page
would be a no-go, we can't increase the size of that. See various other
discussions around, for instance, IO priorities for buffered writeback
and tracking that state on the side.
--
Jens Axboe
On Tue, Apr 21, 2015 at 10:12 AM, Jens Axboe <[email protected]> wrote:
> On 04/21/2015 11:09 AM, Ming Lin wrote:
>>
>> Hi Jens,
>>
>> This RFC DRAFT patch is on top of your "[PATCH v2] Support for write
>> stream IDs"
>> I throw it out early to get comments if it's the way to go.
>>
>> Quote LWN(http://lwn.net/Articles/638722):
>>
>> "There would be clear value in a closer association between stream IDs
>> and specific buffered-write operations. Getting there would require
>> storing
>> the stream ID with each dirtied page, though; that, in turn, almost
>> certainly
>> implies shoehorning the stream ID into the associated page structure.
>> That would not be an easy task; it is not surprising that it is not a part
>> of
>> this patch set. Should the lack of per-buffered-write stream IDs prove to
>> be
>> a serious constraint in the future, somebody will certainly be motivated
>> to
>> try to find a place to store another eight bits in struct page."
>>
>> This draft patch stores stream_id in buffer head instead of page.
>
>
> This is pointless. You need to store it in the page, if the whole point is
> that you want this to be trackable. And adding it to struct page would be a
> no-go, we can't increase the size of that. See various other discussions
> around, for instance, IO priorities for buffered writeback and tracking that
> state on the side.
I googled, but didn't find related discussions.
Could you please point me a link?
Thanks.
On 04/21/2015 12:34 PM, Ming Lin wrote:
> On Tue, Apr 21, 2015 at 10:12 AM, Jens Axboe <[email protected]> wrote:
>> On 04/21/2015 11:09 AM, Ming Lin wrote:
>>>
>>> Hi Jens,
>>>
>>> This RFC DRAFT patch is on top of your "[PATCH v2] Support for write
>>> stream IDs"
>>> I throw it out early to get comments if it's the way to go.
>>>
>>> Quote LWN(http://lwn.net/Articles/638722):
>>>
>>> "There would be clear value in a closer association between stream IDs
>>> and specific buffered-write operations. Getting there would require
>>> storing
>>> the stream ID with each dirtied page, though; that, in turn, almost
>>> certainly
>>> implies shoehorning the stream ID into the associated page structure.
>>> That would not be an easy task; it is not surprising that it is not a part
>>> of
>>> this patch set. Should the lack of per-buffered-write stream IDs prove to
>>> be
>>> a serious constraint in the future, somebody will certainly be motivated
>>> to
>>> try to find a place to store another eight bits in struct page."
>>>
>>> This draft patch stores stream_id in buffer head instead of page.
>>
>>
>> This is pointless. You need to store it in the page, if the whole point is
>> that you want this to be trackable. And adding it to struct page would be a
>> no-go, we can't increase the size of that. See various other discussions
>> around, for instance, IO priorities for buffered writeback and tracking that
>> state on the side.
>
> I googled, but didn't find related discussions.
> Could you please point me a link?
This is the most recent effort:
https://lwn.net/Articles/628631/
My point is that adding it to the buffer_head accomplishes nothing. You
need to track from when the page was dirtied.
--
Jens Axboe
On Tue 21-04-15 13:05:30, Jens Axboe wrote:
> On 04/21/2015 12:34 PM, Ming Lin wrote:
> >On Tue, Apr 21, 2015 at 10:12 AM, Jens Axboe <[email protected]> wrote:
> >>On 04/21/2015 11:09 AM, Ming Lin wrote:
> >>>
> >>>Hi Jens,
> >>>
> >>>This RFC DRAFT patch is on top of your "[PATCH v2] Support for write
> >>>stream IDs"
> >>>I throw it out early to get comments if it's the way to go.
> >>>
> >>>Quote LWN(http://lwn.net/Articles/638722):
> >>>
> >>>"There would be clear value in a closer association between stream IDs
> >>>and specific buffered-write operations. Getting there would require
> >>>storing
> >>>the stream ID with each dirtied page, though; that, in turn, almost
> >>>certainly
> >>>implies shoehorning the stream ID into the associated page structure.
> >>>That would not be an easy task; it is not surprising that it is not a part
> >>>of
> >>>this patch set. Should the lack of per-buffered-write stream IDs prove to
> >>>be
> >>>a serious constraint in the future, somebody will certainly be motivated
> >>>to
> >>>try to find a place to store another eight bits in struct page."
> >>>
> >>>This draft patch stores stream_id in buffer head instead of page.
> >>
> >>
> >>This is pointless. You need to store it in the page, if the whole point is
> >>that you want this to be trackable. And adding it to struct page would be a
> >>no-go, we can't increase the size of that. See various other discussions
> >>around, for instance, IO priorities for buffered writeback and tracking that
> >>state on the side.
> >
> >I googled, but didn't find related discussions.
> >Could you please point me a link?
>
> This is the most recent effort:
>
> https://lwn.net/Articles/628631/
>
> My point is that adding it to the buffer_head accomplishes nothing.
> You need to track from when the page was dirtied.
Technically, you could have a radix tree in an inode that would store
stream id (or other info you need to track) for each dirty page. I'm not
convinced it's worth the complexity and the overhead but it would be doable
and it won't bloat struct page...
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On 04/22/2015 07:57 AM, Jan Kara wrote:
> On Tue 21-04-15 13:05:30, Jens Axboe wrote:
>> On 04/21/2015 12:34 PM, Ming Lin wrote:
>>> On Tue, Apr 21, 2015 at 10:12 AM, Jens Axboe <[email protected]> wrote:
>>>> On 04/21/2015 11:09 AM, Ming Lin wrote:
>>>>>
>>>>> Hi Jens,
>>>>>
>>>>> This RFC DRAFT patch is on top of your "[PATCH v2] Support for write
>>>>> stream IDs"
>>>>> I throw it out early to get comments if it's the way to go.
>>>>>
>>>>> Quote LWN(https://urldefense.proofpoint.com/v1/url?u=http://lwn.net/Articles/638722%29:&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3JMVyziIyZtZ5cv9eWNLwQ%3D%3D%0A&m=eufWIgHAAF0gB7U%2FEOk2iHyObFrgcrM5NnNLDnRvmMc%3D%0A&s=5f6fea75e6421dfb626f57041781e733cb276872d2f9465187cc05bbcc3d7b0f
>>>>>
>>>>> "There would be clear value in a closer association between stream IDs
>>>>> and specific buffered-write operations. Getting there would require
>>>>> storing
>>>>> the stream ID with each dirtied page, though; that, in turn, almost
>>>>> certainly
>>>>> implies shoehorning the stream ID into the associated page structure.
>>>>> That would not be an easy task; it is not surprising that it is not a part
>>>>> of
>>>>> this patch set. Should the lack of per-buffered-write stream IDs prove to
>>>>> be
>>>>> a serious constraint in the future, somebody will certainly be motivated
>>>>> to
>>>>> try to find a place to store another eight bits in struct page."
>>>>>
>>>>> This draft patch stores stream_id in buffer head instead of page.
>>>>
>>>>
>>>> This is pointless. You need to store it in the page, if the whole point is
>>>> that you want this to be trackable. And adding it to struct page would be a
>>>> no-go, we can't increase the size of that. See various other discussions
>>>> around, for instance, IO priorities for buffered writeback and tracking that
>>>> state on the side.
>>>
>>> I googled, but didn't find related discussions.
>>> Could you please point me a link?
>>
>> This is the most recent effort:
>>
>> https://urldefense.proofpoint.com/v1/url?u=https://lwn.net/Articles/628631/&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3JMVyziIyZtZ5cv9eWNLwQ%3D%3D%0A&m=eufWIgHAAF0gB7U%2FEOk2iHyObFrgcrM5NnNLDnRvmMc%3D%0A&s=34699d56963c665d00d15e88c0f146122c693f9979a7fe7be7751f91a6372452
>>
>> My point is that adding it to the buffer_head accomplishes nothing.
>> You need to track from when the page was dirtied.
> Technically, you could have a radix tree in an inode that would store
> stream id (or other info you need to track) for each dirty page. I'm not
> convinced it's worth the complexity and the overhead but it would be doable
> and it won't bloat struct page...
Yeah, that was my point, you'd need to track it on the side. But that's
really just for the case of buffered IO where you can't rely on just the
inode streamid. So I'm with you there, it's not a strict requirement by
any stretch. It's something that can always be revisited, if we need to.
--
Jens Axboe