Provide cgroup isolation for buffered writes.

This patchset adds tracking to the page_cgroup structure for which cgroup has dirtied a page, and uses that information to provide isolation between cgroups performing writeback.

I know that there is some discussion to remove request descriptor limits entirely, but I included a patch to introduce per-cgroup limits to enable this functionality. Without it, we didn't see much isolation improvement.

I think most of this material has been discussed on lkml previously, this is just another attempt to make a patchset that handles buffered writes for CFQ.

There was a lot of previous discussion at:
http://thread.gmane.org/gmane.linux.kernel/1007922

Thanks to Andrea Righi, Kamezawa Hiroyuki, Munehiro Ikeda, Nauman Rafique, and Vivek Goyal for work on previous versions of these patches.

 Documentation/block/biodoc.txt |   10 +
 block/blk-cgroup.c             |  204 +++++++++++++++++++++-
 block/blk-cgroup.h             |    9 +-
 block/blk-core.c               |  216 +++++++++++++++--------
 block/blk-settings.c           |    2 +-
 block/blk-sysfs.c              |   60 ++++---
 block/cfq-iosched.c            |  390 +++++++++++++++++++++++++++++++---------
 block/cfq.h                    |    6 +-
 block/elevator.c               |   11 +-
 fs/buffer.c                    |    2 +
 fs/direct-io.c                 |    2 +
 include/linux/blk_types.h      |    2 +
 include/linux/blkdev.h         |   81 ++++++++-
 include/linux/blkio-track.h    |   89 +++++++++
 include/linux/elevator.h       |   14 ++-
 include/linux/iocontext.h      |    1 +
 include/linux/memcontrol.h     |    6 +
 include/linux/mmzone.h         |    4 +-
 include/linux/page_cgroup.h    |   12 +-
 init/Kconfig                   |   16 ++
 mm/Makefile                    |    3 +-
 mm/bounce.c                    |    2 +
 mm/filemap.c                   |    2 +
 mm/memcontrol.c                |    6 +
 mm/memory.c                    |    6 +
 mm/page-writeback.c            |   14 ++-
 mm/page_cgroup.c               |   29 ++-
 mm/swap_state.c                |    2 +
 28 files changed, 985 insertions(+), 216 deletions(-)

[PATCH 1/6] Add IO cgroup tracking for buffered writes. [PATCH 2/6] Make async queues per cgroup.
[PATCH 3/6] Modify CFQ to use IO tracking information. [PATCH 4/6] With per-cgroup async, don't special case queues. [PATCH 5/6] Add stat for per cgroup writeout done by flusher. [PATCH 6/6] Per cgroup request descriptor counts

Quoted 4 lines · Justin TerAvest

This patchset adds tracking to the page_cgroup structure for which cgroup has dirtied a page, and uses that information to provide isolation between cgroups performing writeback.

Justin,

So if somebody is trying to isolate a workload which does bunch of READS and lots of buffered WRITES, this patchset should help in the sense that all the heavy WRITES can be put into a separate cgroup of low weight?

Other application which are primarily doing READS, direct WRITES or little bit of buffered WRITES should still get good latencies if heavy writer is isolated in a separate group?

If yes, then this piece standalone can make sense. And once the other piece/patches of memory cgroup dirty ratio and cgroup aware buffered writeout come in, then one will be able to differentiate buffered writes of different groups.

Thanks
Vivek

Quoted 50 lines

I know that there is some discussion to remove request descriptor limits entirely, but I included a patch to introduce per-cgroup limits to enable this functionality. Without it, we didn't see much isolation improvement.

I think most of this material has been discussed on lkml previously, this is just another attempt to make a patchset that handles buffered writes for CFQ.

There was a lot of previous discussion at:
http://thread.gmane.org/gmane.linux.kernel/1007922

Thanks to Andrea Righi, Kamezawa Hiroyuki, Munehiro Ikeda, Nauman Rafique, and Vivek Goyal for work on previous versions of these patches.

 Documentation/block/biodoc.txt |   10 +
 block/blk-cgroup.c             |  204 +++++++++++++++++++++-
 block/blk-cgroup.h             |    9 +-
 block/blk-core.c               |  216 +++++++++++++++--------
 block/blk-settings.c           |    2 +-
 block/blk-sysfs.c              |   60 ++++---
 block/cfq-iosched.c            |  390 +++++++++++++++++++++++++++++++---------
 block/cfq.h                    |    6 +-
 block/elevator.c               |   11 +-
 fs/buffer.c                    |    2 +
 fs/direct-io.c                 |    2 +
 include/linux/blk_types.h      |    2 +
 include/linux/blkdev.h         |   81 ++++++++-
 include/linux/blkio-track.h    |   89 +++++++++
 include/linux/elevator.h       |   14 ++-
 include/linux/iocontext.h      |    1 +
 include/linux/memcontrol.h     |    6 +
 include/linux/mmzone.h         |    4 +-
 include/linux/page_cgroup.h    |   12 +-
 init/Kconfig                   |   16 ++
 mm/Makefile                    |    3 +-
 mm/bounce.c                    |    2 +
 mm/filemap.c                   |    2 +
 mm/memcontrol.c                |    6 +
 mm/memory.c                    |    6 +
 mm/page-writeback.c            |   14 ++-
 mm/page_cgroup.c               |   29 ++-
 mm/swap_state.c                |    2 +
 28 files changed, 985 insertions(+), 216 deletions(-)

[PATCH 1/6] Add IO cgroup tracking for buffered writes. [PATCH 2/6] Make async queues per cgroup.
[PATCH 3/6] Modify CFQ to use IO tracking information. [PATCH 4/6] With per-cgroup async, don't special case queues. [PATCH 5/6] Add stat for per cgroup writeout done by flusher. [PATCH 6/6] Per cgroup request descriptor counts

Quoted 16 lines · Vivek Goyal

Quoted 4 lines · Justin TerAvest

This patchset adds tracking to the page_cgroup structure for which cgroup has dirtied a page, and uses that information to provide isolation between cgroups performing writeback.

Justin,

So if somebody is trying to isolate a workload which does bunch of READS and lots of buffered WRITES, this patchset should help in the sense that all the heavy WRITES can be put into a separate cgroup of low weight?

Other application which are primarily doing READS, direct WRITES or little bit of buffered WRITES should still get good latencies if heavy writer is isolated in a separate group?

Quoted 4 lines

If yes, then this piece standalone can make sense. And once the other piece/patches of memory cgroup dirty ratio and cgroup aware buffered writeout come in, then one will be able to differentiate buffered writes of different groups.

Thinking more about it, currently anyway SYNC preempts the ASYNC. So the question would be will it help me enable get better isolation latencies of READS agains buffered WRITES?

Thanks
Vivek

Quoted 54 lines

Thanks
Vivek

Quoted 50 lines

I know that there is some discussion to remove request descriptor limits entirely, but I included a patch to introduce per-cgroup limits to enable this functionality. Without it, we didn't see much isolation improvement.

I think most of this material has been discussed on lkml previously, this is just another attempt to make a patchset that handles buffered writes for CFQ.

There was a lot of previous discussion at:
  http://thread.gmane.org/gmane.linux.kernel/1007922

Thanks to Andrea Righi, Kamezawa Hiroyuki, Munehiro Ikeda, Nauman Rafique, and Vivek Goyal for work on previous versions of these patches.

 Documentation/block/biodoc.txt |   10 +
 block/blk-cgroup.c             |  204 +++++++++++++++++++++- block/blk-cgroup.h             |    9 +-
 block/blk-core.c               |  216 +++++++++++++++-------- block/blk-settings.c           |    2 +-
 block/blk-sysfs.c              |   60 ++++---
 block/cfq-iosched.c            |  390 +++++++++++++++++++++++++++++++--------- block/cfq.h                    |    6 +-
 block/elevator.c               |   11 +-
 fs/buffer.c                    |    2 +
 fs/direct-io.c                 |    2 +
 include/linux/blk_types.h      |    2 +
 include/linux/blkdev.h         |   81 ++++++++- include/linux/blkio-track.h    |   89 +++++++++ include/linux/elevator.h       |   14 ++- include/linux/iocontext.h      |    1 + include/linux/memcontrol.h     |    6 +
 include/linux/mmzone.h         |    4 +- include/linux/page_cgroup.h    |   12 +-
 init/Kconfig                   |   16 ++
 mm/Makefile                    |    3 +-
 mm/bounce.c                    |    2 +
 mm/filemap.c                   |    2 +
 mm/memcontrol.c                |    6 +
 mm/memory.c                    |    6 +
 mm/page-writeback.c            |   14 ++-
 mm/page_cgroup.c               |   29 ++-
 mm/swap_state.c                |    2 +

 28 files changed, 985 insertions(+), 216 deletions(-)

[PATCH 1/6] Add IO cgroup tracking for buffered writes. [PATCH 2/6] Make async queues per cgroup.
[PATCH 3/6] Modify CFQ to use IO tracking information. [PATCH 4/6] With per-cgroup async, don't special case queues. [PATCH 5/6] Add stat for per cgroup writeout done by flusher. [PATCH 6/6] Per cgroup request descriptor counts

Quoted 22 lines · Vivek Goyal

Quoted 20 lines · Vivek Goyal

Quoted 4 lines · Justin TerAvest

This patchset adds tracking to the page_cgroup structure for which cgroup has dirtied a page, and uses that information to provide isolation between cgroups performing writeback.

Justin,

So if somebody is trying to isolate a workload which does bunch of READS and lots of buffered WRITES, this patchset should help in the sense that all the heavy WRITES can be put into a separate cgroup of low weight?

Other application which are primarily doing READS, direct WRITES or little bit of buffered WRITES should still get good latencies if heavy writer is isolated in a separate group?

If yes, then this piece standalone can make sense. And once the other piece/patches of memory cgroup dirty ratio and cgroup aware buffered writeout come in, then one will be able to differentiate buffered writes of different groups.

Quoted 3 lines

Thinking more about it, currently anyway SYNC preempts the ASYNC. So the question would be will it help me enable get better isolation latencies of READS agains buffered WRITES?

Ah! Sorry, I left out a patch that disables cross-group preemption. I'll add that to the patchset and email out v2 soon.

Quoted 27 lines · Justin TerAvest

Quoted 25 lines · Vivek Goyal

Quoted 12 lines · Vivek Goyal

Quoted 4 lines · Justin TerAvest

This patchset adds tracking to the page_cgroup structure for which cgroup has dirtied a page, and uses that information to provide isolation between cgroups performing writeback.

Justin,

So if somebody is trying to isolate a workload which does bunch of READS and lots of buffered WRITES, this patchset should help in the sense that all the heavy WRITES can be put into a separate cgroup of low weight?

Quoted 8 lines

Other application which are primarily doing READS, direct WRITES or little bit of buffered WRITES should still get good latencies if heavy writer is isolated in a separate group?

If yes, then this piece standalone can make sense. And once the other piece/patches of memory cgroup dirty ratio and cgroup aware buffered writeout come in, then one will be able to differentiate buffered writes of different groups.

Thinking more about it, currently anyway SYNC preempts the ASYNC. So the question would be will it help me enable get better isolation latencies of READS agains buffered WRITES?

Quoted 2 lines

Ah! Sorry, I left out a patch that disables cross-group preemption. I'll add that to the patchset and email out v2 soon.

Well, what I was referring to that even in current code sync preempts all async in CFQ. So it looks like this patchset will not help get better latencies in presence of WRITES?

The only place it can help is that one is looking for service differentation between two or more buffered write streams. For that we need to fix upper layers first.

Vivek

Quoted 28 lines · Vivek Goyal

Quoted 27 lines · Justin TerAvest

Quoted 25 lines · Vivek Goyal

Quoted 12 lines · Vivek Goyal

Quoted 4 lines · Justin TerAvest

This patchset adds tracking to the page_cgroup structure for which cgroup has dirtied a page, and uses that information to provide isolation between cgroups performing writeback.

Justin,

So if somebody is trying to isolate a workload which does bunch of READS and lots of buffered WRITES, this patchset should help in the sense that all the heavy WRITES can be put into a separate cgroup of low weight?

Quoted 8 lines

Other application which are primarily doing READS, direct WRITES or little bit of buffered WRITES should still get good latencies if heavy writer is isolated in a separate group?

If yes, then this piece standalone can make sense. And once the other piece/patches of memory cgroup dirty ratio and cgroup aware buffered writeout come in, then one will be able to differentiate buffered writes of different groups.

Thinking more about it, currently anyway SYNC preempts the ASYNC. So the question would be will it help me enable get better isolation latencies of READS agains buffered WRITES?

Quoted 6 lines

Quoted 2 lines

Ah! Sorry, I left out a patch that disables cross-group preemption. I'll add that to the patchset and email out v2 soon.

Well, what I was referring to that even in current code sync preempts all async in CFQ. So it looks like this patchset will not help get better latencies in presence of WRITES?

Hi Vivek,

I should have been more clear. I forgot to include a patch that changes the behavior of that preemption. I haven't mailed out v2 yet because I was also writing a change to put the css_id in pc->flags instead of its own field.

The preemption change would look like:

Previously, a sync queue in can preempt an async queue in another cgroup. This changes that behavior to disallow such preemption.

block/cfq-iosched.c +3 -2

index ab7a216..0494c0c 100644
@@ -3390,6 +3390,9 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue
        if (!cfqq)
                return false;
 
+       if (new_cfqq->cfqg != cfqq->cfqg)
+               return false;
+
        if (cfq_class_idle(new_cfqq))
                return false;
 

@@ -3409,9 +3412,6 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue
        if (rq_is_sync(rq) && !cfq_cfqq_sync(cfqq))
                return true;
 
-       if (new_cfqq->cfqg != cfqq->cfqg)
-               return false;
 

I will include the test results that show that isolation is also improved between a reader and a buffered writer.

Quoted 69 lines · Justin TerAvest

Quoted 28 lines · Vivek Goyal

Quoted 27 lines · Justin TerAvest

Quoted 25 lines · Vivek Goyal

Quoted 12 lines · Vivek Goyal

Quoted 4 lines · Justin TerAvest

This patchset adds tracking to the page_cgroup structure for which cgroup has dirtied a page, and uses that information to provide isolation between cgroups performing writeback.

Justin,

So if somebody is trying to isolate a workload which does bunch of READS and lots of buffered WRITES, this patchset should help in the sense that all the heavy WRITES can be put into a separate cgroup of low weight?

Quoted 8 lines

Other application which are primarily doing READS, direct WRITES or little bit of buffered WRITES should still get good latencies if heavy writer is isolated in a separate group?

If yes, then this piece standalone can make sense. And once the other piece/patches of memory cgroup dirty ratio and cgroup aware buffered writeout come in, then one will be able to differentiate buffered writes of different groups.

Thinking more about it, currently anyway SYNC preempts the ASYNC. So the question would be will it help me enable get better isolation latencies of READS agains buffered WRITES?

Quoted 6 lines

Quoted 2 lines

Ah! Sorry, I left out a patch that disables cross-group preemption. I'll add that to the patchset and email out v2 soon.

Well, what I was referring to that even in current code sync preempts all async in CFQ. So it looks like this patchset will not help get better latencies in presence of WRITES?

Hi Vivek,

I should have been more clear. I forgot to include a patch that changes the behavior of that preemption. I haven't mailed out v2 yet because I was also writing a change to put the css_id in pc->flags instead of its own field.

The preemption change would look like:

    Previously, a sync queue in can preempt an async queue in another cgroup. This changes that behavior to disallow such preemption.

block/cfq-iosched.c +3 -2

index ab7a216..0494c0c 100644
@@ -3390,6 +3390,9 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue
         if (!cfqq)
                 return false;
 
+       if (new_cfqq->cfqg != cfqq->cfqg)
+               return false;
+
         if (cfq_class_idle(new_cfqq))
                 return false;
 

@@ -3409,9 +3412,6 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue
         if (rq_is_sync(rq) && !cfq_cfqq_sync(cfqq))
                 return true;
 
-       if (new_cfqq->cfqg != cfqq->cfqg)
-               return false;
 

Quoted 3 lines

I will include the test results that show that isolation is also improved between a reader and a buffered writer.

Justin,

Right now all the async queues go in one cgroup and that is root cgroup. So it is perfectly fine to let sync preempt async.

I will be interested in seeing the results. But do you have a theoritical explanation also that why with this patch set we will get better isolation between READS and WRITES?

Especially after following patch from Shaohua Li.

commit f8ae6e3eb8251be32c6e913393d9f8d9e0609489
Author: Shaohua Li <[email protected]>
Date:   Fri Jan 14 08:41:02 2011 +0100

   block cfq: make queue preempt work for queues from different workload

This patch will make sure that sync always preempt async. So I am not able to understand that how this patch series provides better latencies for READS in presence of WRITES.

Thanks
Vivek

Quoted 76 lines · Vivek Goyal

Quoted 69 lines · Justin TerAvest

Quoted 28 lines · Vivek Goyal

Quoted 27 lines · Justin TerAvest

Quoted 25 lines · Vivek Goyal

Quoted 12 lines · Vivek Goyal

Quoted 4 lines · Justin TerAvest

This patchset adds tracking to the page_cgroup structure for which cgroup has dirtied a page, and uses that information to provide isolation between cgroups performing writeback.

Justin,

So if somebody is trying to isolate a workload which does bunch of READS and lots of buffered WRITES, this patchset should help in the sense that all the heavy WRITES can be put into a separate cgroup of low weight?

Quoted 8 lines

Other application which are primarily doing READS, direct WRITES or little bit of buffered WRITES should still get good latencies if heavy writer is isolated in a separate group?

If yes, then this piece standalone can make sense. And once the other piece/patches of memory cgroup dirty ratio and cgroup aware buffered writeout come in, then one will be able to differentiate buffered writes of different groups.

Thinking more about it, currently anyway SYNC preempts the ASYNC. So the question would be will it help me enable get better isolation latencies of READS agains buffered WRITES?

Quoted 6 lines

Quoted 2 lines

Ah! Sorry, I left out a patch that disables cross-group preemption. I'll add that to the patchset and email out v2 soon.

Well, what I was referring to that even in current code sync preempts all async in CFQ. So it looks like this patchset will not help get better latencies in presence of WRITES?

Hi Vivek,

I should have been more clear. I forgot to include a patch that changes the behavior of that preemption. I haven't mailed out v2 yet because I was also writing a change to put the css_id in pc->flags instead of its own field.

The preemption change would look like:

    Previously, a sync queue in can preempt an async queue in another cgroup. This changes that behavior to disallow such preemption.

block/cfq-iosched.c +3 -2

index ab7a216..0494c0c 100644
@@ -3390,6 +3390,9 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue
         if (!cfqq)
                 return false;
 
+       if (new_cfqq->cfqg != cfqq->cfqg)
+               return false;
+
         if (cfq_class_idle(new_cfqq))
                 return false;
 

@@ -3409,9 +3412,6 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue
         if (rq_is_sync(rq) && !cfq_cfqq_sync(cfqq))
                 return true;
 
-       if (new_cfqq->cfqg != cfqq->cfqg)
-               return false;
 

Quoted 3 lines

I will include the test results that show that isolation is also improved between a reader and a buffered writer.

Justin,

Quoted 6 lines

Right now all the async queues go in one cgroup and that is root cgroup. So it is perfectly fine to let sync preempt async.

I will be interested in seeing the results. But do you have a theoritical explanation also that why with this patch set we will get better isolation between READS and WRITES?

Hi Vivek,

Some of the change is probably because the patch set also changes preemption semantics. Because now a group will be charged for the buffered WRITES it is causing, it will give less time than before to a task that is doing lots of buffered writes. I feel like I am still not understanding something you are saying, but it will probably make more sense once I have supporting data.

Quoted 10 lines

Especially after following patch from Shaohua Li.

commit f8ae6e3eb8251be32c6e913393d9f8d9e0609489
Author: Shaohua Li <[email protected]>
Date:   Fri Jan 14 08:41:02 2011 +0100

   block cfq: make queue preempt work for queues from different workload

Quoted 3 lines

This patch will make sure that sync always preempt async. So I am not able to understand that how this patch series provides better latencies for READS in presence of WRITES.

I have primarily been measuring isolation between READS and WRITES. Do you have any tools you'd recommend for getting data on when latencies are better?

This patchset adds tracking to the page_cgroup structure for which cgroup has dirtied a page, and uses that information to provide isolation between cgroups performing writeback.

I know that there is some discussion to remove request descriptor limits entirely, but I included a patch to introduce per-cgroup limits to enable this functionality. Without it, we didn't see much isolation improvement.

I think most of this material has been discussed on lkml previously, this is just another attempt to make a patchset that handles buffered writes for CFQ.

There was a lot of previous discussion at:
http://thread.gmane.org/gmane.linux.kernel/1007922

Thanks to Andrea Righi, Kamezawa Hiroyuki, Munehiro Ikeda, Nauman Rafique, and Vivek Goyal for work on previous versions of these patches.

 Documentation/block/biodoc.txt |   10 +
 block/blk-cgroup.c             |  204 +++++++++++++++++++++-
 block/blk-cgroup.h             |    9 +-
 block/blk-core.c               |  216 +++++++++++++++--------
 block/blk-settings.c           |    2 +-
 block/blk-sysfs.c              |   60 ++++---
 block/cfq-iosched.c            |  390 +++++++++++++++++++++++++++++++---------
 block/cfq.h                    |    6 +-
 block/elevator.c               |   11 +-
 fs/buffer.c                    |    2 +
 fs/direct-io.c                 |    2 +
 include/linux/blk_types.h      |    2 +
 include/linux/blkdev.h         |   81 ++++++++-
 include/linux/blkio-track.h    |   89 +++++++++
 include/linux/elevator.h       |   14 ++-
 include/linux/iocontext.h      |    1 +
 include/linux/memcontrol.h     |    6 +
 include/linux/mmzone.h         |    4 +-
 include/linux/page_cgroup.h    |   12 +-
 init/Kconfig                   |   16 ++
 mm/Makefile                    |    3 +-
 mm/bounce.c                    |    2 +
 mm/filemap.c                   |    2 +
 mm/memcontrol.c                |    6 +
 mm/memory.c                    |    6 +
 mm/page-writeback.c            |   14 ++-
 mm/page_cgroup.c               |   29 ++-
 mm/swap_state.c                |    2 +
 28 files changed, 985 insertions(+), 216 deletions(-)

[PATCH 1/6] Add IO cgroup tracking for buffered writes. [PATCH 2/6] Make async queues per cgroup.
[PATCH 3/6] Modify CFQ to use IO tracking information. [PATCH 4/6] With per-cgroup async, don't special case queues. [PATCH 5/6] Add stat for per cgroup writeout done by flusher. [PATCH 6/6] Per cgroup request descriptor counts

B) I wonder why the information cannot be recorded in page->private. When page has buffers, you can record the information to buffer struct. About swapio (if you take care of), you can record information to bio.

Anyway, thank you for the work. And please join the discussion and explain "Without it, we didn't see much isolation improvement." with real data.

Quoted 8 lines · On Tue,  8 Mar 2011 13:20:50 -0800 Justin TerAvest <[email protected]> wrote:

This patchset adds tracking to the page_cgroup structure for which cgroup has dirtied a page, and uses that information to provide isolation between cgroups performing writeback.

I know that there is some discussion to remove request descriptor limits entirely, but I included a patch to introduce per-cgroup limits to enable this functionality. Without it, we didn't see much isolation improvement.

Quoted 9 lines

I think most of this material has been discussed on lkml previously, this is just another attempt to make a patchset that handles buffered writes for CFQ.

There was a lot of previous discussion at:
  http://thread.gmane.org/gmane.linux.kernel/1007922

Thanks to Andrea Righi, Kamezawa Hiroyuki, Munehiro Ikeda, Nauman Rafique, and Vivek Goyal for work on previous versions of these patches.

2 points from me.

1. If you know there is a thread, please join even if it's a bit little thread.
2. In these days, I got many comments as 'page_cgroup is toooo big', so I don't feel fun when I see a patch which increases size of page_cgroup.

I don't like to increase size of page_cgroup but I think you can record information without increasing size of page_cgroup.

A) As Andrea did, encode it to pc->flags.
  But I'm afraid that there is a racy case because memory cgroup uses some test_and_set() bits.
B) I wonder why the information cannot be recorded in page->private. When page has buffers, you can record the information to buffer struct. About swapio (if you take care of), you can record information to bio.

Anyway, thank you for the work. And please join the discussion and explain "Without it, we didn't see much isolation improvement." with real data.

Regards,
-Kame

Quoted 38 lines

 Documentation/block/biodoc.txt |   10 +
 block/blk-cgroup.c             |  204 +++++++++++++++++++++- block/blk-cgroup.h             |    9 +-
 block/blk-core.c               |  216 +++++++++++++++-------- block/blk-settings.c           |    2 +-
 block/blk-sysfs.c              |   60 ++++---
 block/cfq-iosched.c            |  390 +++++++++++++++++++++++++++++++--------- block/cfq.h                    |    6 +-
 block/elevator.c               |   11 +-
 fs/buffer.c                    |    2 +
 fs/direct-io.c                 |    2 +
 include/linux/blk_types.h      |    2 +
 include/linux/blkdev.h         |   81 ++++++++- include/linux/blkio-track.h    |   89 +++++++++ include/linux/elevator.h       |   14 ++- include/linux/iocontext.h      |    1 + include/linux/memcontrol.h     |    6 +
 include/linux/mmzone.h         |    4 +- include/linux/page_cgroup.h    |   12 +-
 init/Kconfig                   |   16 ++
 mm/Makefile                    |    3 +-
 mm/bounce.c                    |    2 +
 mm/filemap.c                   |    2 +
 mm/memcontrol.c                |    6 +
 mm/memory.c                    |    6 +
 mm/page-writeback.c            |   14 ++-
 mm/page_cgroup.c               |   29 ++-
 mm/swap_state.c                |    2 +

 28 files changed, 985 insertions(+), 216 deletions(-)

[PATCH 1/6] Add IO cgroup tracking for buffered writes. [PATCH 2/6] Make async queues per cgroup.
[PATCH 3/6] Modify CFQ to use IO tracking information. [PATCH 4/6] With per-cgroup async, don't special case queues. [PATCH 5/6] Add stat for per cgroup writeout done by flusher. [PATCH 6/6] Per cgroup request descriptor counts

Quoted 8 lines · On Tue,  8 Mar 2011 13:20:50 -0800 Justin TerAvest <[email protected]> wrote:

This patchset adds tracking to the page_cgroup structure for which cgroup has dirtied a page, and uses that information to provide isolation between cgroups performing writeback.

I know that there is some discussion to remove request descriptor limits entirely, but I included a patch to introduce per-cgroup limits to enable this functionality. Without it, we didn't see much isolation improvement.

Quoted 9 lines

I think most of this material has been discussed on lkml previously, this is just another attempt to make a patchset that handles buffered writes for CFQ.

There was a lot of previous discussion at:
  http://thread.gmane.org/gmane.linux.kernel/1007922

Thanks to Andrea Righi, Kamezawa Hiroyuki, Munehiro Ikeda, Nauman Rafique, and Vivek Goyal for work on previous versions of these patches.

2 points from me.

1. If you know there is a thread, please join even if it's a bit little thread.
2. In these days, I got many comments as 'page_cgroup is toooo big', so I don't feel fun when I see a patch which increases size of page_cgroup.

I don't like to increase size of page_cgroup but I think you can record information without increasing size of page_cgroup.

A) As Andrea did, encode it to pc->flags.
  But I'm afraid that there is a racy case because memory cgroup uses some test_and_set() bits.
B) I wonder why the information cannot be recorded in page->private. When page has buffers, you can record the information to buffer struct. About swapio (if you take care of), you can record information to bio.

Anyway, thank you for the work. And please join the discussion and explain "Without it, we didn't see much isolation improvement." with real data.

Regards,
-Kame

Quoted 38 lines

 Documentation/block/biodoc.txt |   10 +
 block/blk-cgroup.c             |  204 +++++++++++++++++++++- block/blk-cgroup.h             |    9 +-
 block/blk-core.c               |  216 +++++++++++++++-------- block/blk-settings.c           |    2 +-
 block/blk-sysfs.c              |   60 ++++---
 block/cfq-iosched.c            |  390 +++++++++++++++++++++++++++++++--------- block/cfq.h                    |    6 +-
 block/elevator.c               |   11 +-
 fs/buffer.c                    |    2 +
 fs/direct-io.c                 |    2 +
 include/linux/blk_types.h      |    2 +
 include/linux/blkdev.h         |   81 ++++++++- include/linux/blkio-track.h    |   89 +++++++++ include/linux/elevator.h       |   14 ++- include/linux/iocontext.h      |    1 + include/linux/memcontrol.h     |    6 +
 include/linux/mmzone.h         |    4 +- include/linux/page_cgroup.h    |   12 +-
 init/Kconfig                   |   16 ++
 mm/Makefile                    |    3 +-
 mm/bounce.c                    |    2 +
 mm/filemap.c                   |    2 +
 mm/memcontrol.c                |    6 +
 mm/memory.c                    |    6 +
 mm/page-writeback.c            |   14 ++-
 mm/page_cgroup.c               |   29 ++-
 mm/swap_state.c                |    2 +

 28 files changed, 985 insertions(+), 216 deletions(-)

[PATCH 1/6] Add IO cgroup tracking for buffered writes. [PATCH 2/6] Make async queues per cgroup.
[PATCH 3/6] Modify CFQ to use IO tracking information. [PATCH 4/6] With per-cgroup async, don't special case queues. [PATCH 5/6] Add stat for per cgroup writeout done by flusher. [PATCH 6/6] Per cgroup request descriptor counts

Quoted 9 lines

I don't like to increase size of page_cgroup but I think you can record information without increasing size of page_cgroup.

A) As Andrea did, encode it to pc->flags.
  But I'm afraid that there is a racy case because memory cgroup uses some test_and_set() bits.
B) I wonder why the information cannot be recorded in page->private. When page has buffers, you can record the information to buffer struct. About swapio (if you take care of), you can record information to bio.

Hi Kame,

I'm concerned that by using something like buffer_heads stored in page->private, we will only be supported on some filesystems and not others. In addition, I'm not sure if all filesystems attach buffer heads at the same time; if page->private is modified in the flusher thread, we might not be able to determine the thread that dirtied the page in the first place.

On Thu, Mar 10, 2011 at 10:08:03AM -0800, Justin TerAvest wrote:

[..]

Quoted 10 lines

Quoted 9 lines

I don't like to increase size of page_cgroup but I think you can record information without increasing size of page_cgroup.

A) As Andrea did, encode it to pc->flags.
  But I'm afraid that there is a racy case because memory cgroup uses some test_and_set() bits.
B) I wonder why the information cannot be recorded in page->private. When page has buffers, you can record the information to buffer struct. About swapio (if you take care of), you can record information to bio.

Quoted 8 lines

Hi Kame,

I'm concerned that by using something like buffer_heads stored in page->private, we will only be supported on some filesystems and not others. In addition, I'm not sure if all filesystems attach buffer heads at the same time; if page->private is modified in the flusher thread, we might not be able to determine the thread that dirtied the page in the first place.

I think the person who dirtied the page can store the information in page->private (assuming buffer heads were not generated) and if flusher thread later ends up generating buffer heads and ends up modifying page->private, this can be copied in buffer heads?

Thanks
Vivek

Quoted 22 lines · Vivek Goyal

On Thu, Mar 10, 2011 at 10:08:03AM -0800, Justin TerAvest wrote:

[..]

Quoted 10 lines

Quoted 9 lines

I don't like to increase size of page_cgroup but I think you can record information without increasing size of page_cgroup.

A) As Andrea did, encode it to pc->flags.
  But I'm afraid that there is a racy case because memory cgroup uses some test_and_set() bits.
B) I wonder why the information cannot be recorded in page->private. When page has buffers, you can record the information to buffer struct. About swapio (if you take care of), you can record information to bio.

Quoted 8 lines

Hi Kame,

I'm concerned that by using something like buffer_heads stored in page->private, we will only be supported on some filesystems and not others. In addition, I'm not sure if all filesystems attach buffer heads at the same time; if page->private is modified in the flusher thread, we might not be able to determine the thread that dirtied the page in the first place.

Quoted 4 lines

I think the person who dirtied the page can store the information in page->private (assuming buffer heads were not generated) and if flusher thread later ends up generating buffer heads and ends up modifying page->private, this can be copied in buffer heads?

This scares me a bit.

As I understand it, fs/ code expects total ownership of page->private. This adds a responsibility for every user to copy the data through and store it in the buffer head (or anything else). btrfs seems to do something entirely different in some cases and store a different kind of value.

I don't know that it's right to add the burden to copy the original value to everything that wants to use page->private.

Quoted 28 lines · Justin TerAvest

Quoted 22 lines · Vivek Goyal

On Thu, Mar 10, 2011 at 10:08:03AM -0800, Justin TerAvest wrote:

[..]

Quoted 10 lines

Quoted 9 lines

I don't like to increase size of page_cgroup but I think you can record information without increasing size of page_cgroup.

A) As Andrea did, encode it to pc->flags.
  But I'm afraid that there is a racy case because memory cgroup uses some test_and_set() bits.
B) I wonder why the information cannot be recorded in page->private. When page has buffers, you can record the information to buffer struct. About swapio (if you take care of), you can record information to bio.

Quoted 8 lines

Hi Kame,

I'm concerned that by using something like buffer_heads stored in page->private, we will only be supported on some filesystems and not others. In addition, I'm not sure if all filesystems attach buffer heads at the same time; if page->private is modified in the flusher thread, we might not be able to determine the thread that dirtied the page in the first place.

Quoted 4 lines

I think the person who dirtied the page can store the information in page->private (assuming buffer heads were not generated) and if flusher thread later ends up generating buffer heads and ends up modifying page->private, this can be copied in buffer heads?

Quoted 7 lines

This scares me a bit.

As I understand it, fs/ code expects total ownership of page->private. This adds a responsibility for every user to copy the data through and store it in the buffer head (or anything else). btrfs seems to do something entirely different in some cases and store a different kind of value.

If filesystems are using page->private for some other purpose also, then I guess we have issues.

I am ccing linux-fsdevel to have some feedback on the idea of trying to store cgroup id of page dirtying thread in page->private and/or buffer head for tracking which group originally dirtied the page in IO controller during writeback.

Quoted 37 lines · Justin TerAvest

Quoted 28 lines · Vivek Goyal

Quoted 22 lines · Justin TerAvest

[..]

Quoted 10 lines

Quoted 9 lines

I don't like to increase size of page_cgroup but I think you can record information without increasing size of page_cgroup.

A) As Andrea did, encode it to pc->flags.
  But I'm afraid that there is a racy case because memory cgroup uses some   test_and_set() bits.
B) I wonder why the information cannot be recorded in page->private.   When page has buffers, you can record the information to buffer struct.   About swapio (if you take care of), you can record information to bio.

Quoted 8 lines

Hi Kame,

I'm concerned that by using something like buffer_heads stored in page->private, we will only be supported on some filesystems and not others. In addition, I'm not sure if all filesystems attach buffer heads at the same time; if page->private is modified in the flusher thread, we might not be able to determine the thread that dirtied the page in the first place.

Quoted 4 lines

I think the person who dirtied the page can store the information in page->private (assuming buffer heads were not generated) and if flusher thread later ends up generating buffer heads and ends up modifying page->private, this can be copied in buffer heads?

Quoted 7 lines

This scares me a bit.

As I understand it, fs/ code expects total ownership of page->private. This adds a responsibility for every user to copy the data through and store it in the buffer head (or anything else). btrfs seems to do something entirely different in some cases and store a different kind of value.

Quoted 7 lines

If filesystems are using page->private for some other purpose also, then I guess we have issues.

I am ccing linux-fsdevel to have some feedback on the idea of trying to store cgroup id of page dirtying thread in page->private and/or buffer head for tracking which group originally dirtied the page in IO controller during writeback.

A quick "grep" showed that btrfs, ceph and logfs are using page->private for other purposes also.

I was under the impression that either page->private is null or it points to buffer heads for the writeback case. So storing the info directly in either buffer head directly or first in page->private and then transferring it to buffer heads would have helped.

Excerpts from Vivek Goyal's message of 2011-03-10 14:41:06 -0500:

Quoted 25 lines · Vivek Goyal

Quoted 13 lines

Quoted 5 lines

Quoted 4 lines

I think the person who dirtied the page can store the information in page->private (assuming buffer heads were not generated) and if flusher thread later ends up generating buffer heads and ends up modifying page->private, this can be copied in buffer heads?

Quoted 7 lines

This scares me a bit.

As I understand it, fs/ code expects total ownership of page->private. This adds a responsibility for every user to copy the data through and store it in the buffer head (or anything else). btrfs seems to do something entirely different in some cases and store a different kind of value.

Quoted 7 lines

If filesystems are using page->private for some other purpose also, then I guess we have issues.

I am ccing linux-fsdevel to have some feedback on the idea of trying to store cgroup id of page dirtying thread in page->private and/or buffer head for tracking which group originally dirtied the page in IO controller during writeback.

A quick "grep" showed that btrfs, ceph and logfs are using page->private for other purposes also.

Quoted 4 lines

I was under the impression that either page->private is null or it points to buffer heads for the writeback case. So storing the info directly in either buffer head directly or first in page->private and then transferring it to buffer heads would have helped.

Right, btrfs has its own uses for page->private, and we expect to own it. With a proper callback, the FS could store the extra information you need in out own structs.

Quoted 31 lines · Chris Mason

Excerpts from Vivek Goyal's message of 2011-03-10 14:41:06 -0500:

Quoted 29 lines · Vivek Goyal

Quoted 13 lines

Quoted 5 lines

Quoted 4 lines

I think the person who dirtied the page can store the information in page->private (assuming buffer heads were not generated) and if flusher thread later ends up generating buffer heads and ends up modifying page->private, this can be copied in buffer heads?

Quoted 7 lines

This scares me a bit.

As I understand it, fs/ code expects total ownership of page->private. This adds a responsibility for every user to copy the data through and store it in the buffer head (or anything else). btrfs seems to do something entirely different in some cases and store a different kind of value.

Quoted 7 lines

If filesystems are using page->private for some other purpose also, then I guess we have issues.

I am ccing linux-fsdevel to have some feedback on the idea of trying to store cgroup id of page dirtying thread in page->private and/or buffer head for tracking which group originally dirtied the page in IO controller during writeback.

A quick "grep" showed that btrfs, ceph and logfs are using page->private for other purposes also.

I was under the impression that either page->private is null or it points to buffer heads for the writeback case. So storing the info directly in either buffer head directly or first in page->private and then transferring it to buffer heads would have helped.

Quoted 3 lines

Right, btrfs has its own uses for page->private, and we expect to own it. With a proper callback, the FS could store the extra information you need in out own structs.

There is no requirement that page->private ever points to a buffer_head, and Lustre clients use it for its own tracking structure (never touching buffer_heads at all). Any assumption about what a filesystem is storing in page->private in other parts of the code is just broken.

Quoted 36 lines · Chris Mason

Quoted 34 lines

Excerpts from Vivek Goyal's message of 2011-03-10 14:41:06 -0500:

Quoted 22 lines · Vivek Goyal

Quoted 13 lines

Quoted 5 lines

Quoted 4 lines

I think the person who dirtied the page can store the information in page->private (assuming buffer heads were not generated) and if flusher thread later ends up generating buffer heads and ends up modifying page->private, this can be copied in buffer heads?

Quoted 7 lines

This scares me a bit.

As I understand it, fs/ code expects total ownership of page->private. This adds a responsibility for every user to copy the data through and store it in the buffer head (or anything else). btrfs seems to do something entirely different in some cases and store a different kind of value.

Quoted 7 lines

If filesystems are using page->private for some other purpose also, then I guess we have issues.

I am ccing linux-fsdevel to have some feedback on the idea of trying to store cgroup id of page dirtying thread in page->private and/or buffer head for tracking which group originally dirtied the page in IO controller during writeback.

Quoted 7 lines

A quick "grep" showed that btrfs, ceph and logfs are using page->private for other purposes also.

I was under the impression that either page->private is null or it points to buffer heads for the writeback case. So storing the info directly in either buffer head directly or first in page->private and then transferring it to buffer heads would have helped.

Right, btrfs has its own uses for page->private, and we expect to own it. With a proper callback, the FS could store the extra information you need in out own structs.

Quoted 1 line

There is no requirement that page->private ever points to a buffer_head, and Lustre clients use it for its own tracking structure (never touching buffer_heads at all). Any assumption about what a filesystem is storing in page->private in other parts of the code is just broken.

Andreas,

As Chris mentioned, will providing callbacks so that filesystem can save/restore page->private be reasonable?

Excerpts from Vivek Goyal's message of 2011-03-10 16:38:32 -0500:

Quoted 41 lines · Andreas Dilger

Quoted 32 lines · Chris Mason

Quoted 31 lines

Excerpts from Vivek Goyal's message of 2011-03-10 14:41:06 -0500:

Quoted 29 lines · Vivek Goyal

Quoted 13 lines

Quoted 5 lines

Quoted 4 lines

I think the person who dirtied the page can store the information in page->private (assuming buffer heads were not generated) and if flusher thread later ends up generating buffer heads and ends up modifying page->private, this can be copied in buffer heads?

Quoted 7 lines

This scares me a bit.

As I understand it, fs/ code expects total ownership of page->private. This adds a responsibility for every user to copy the data through and store it in the buffer head (or anything else). btrfs seems to do something entirely different in some cases and store a different kind of value.

Quoted 7 lines

If filesystems are using page->private for some other purpose also, then I guess we have issues.

I am ccing linux-fsdevel to have some feedback on the idea of trying to store cgroup id of page dirtying thread in page->private and/or buffer head for tracking which group originally dirtied the page in IO controller during writeback.

A quick "grep" showed that btrfs, ceph and logfs are using page->private for other purposes also.

I was under the impression that either page->private is null or it points to buffer heads for the writeback case. So storing the info directly in either buffer head directly or first in page->private and then transferring it to buffer heads would have helped.

Quoted 5 lines

Quoted 3 lines

Right, btrfs has its own uses for page->private, and we expect to own it. With a proper callback, the FS could store the extra information you need in out own structs.

There is no requirement that page->private ever points to a buffer_head, and Lustre clients use it for its own tracking structure (never touching buffer_heads at all). Any assumption about what a filesystem is storing in page->private in other parts of the code is just broken.

Andreas,

Quoted 2 lines

As Chris mentioned, will providing callbacks so that filesystem can save/restore page->private be reasonable?

Just to clarify, I think saving/restoring page->private is going to be hard. I'd rather just have a call back that says here's a page, storage this for the block io controller please, and another one that returns any previously stored info.

Excerpts from Vivek Goyal's message of 2011-03-10 16:38:32 -0500:

Quoted 45 lines · Andreas Dilger

Quoted 36 lines · Chris Mason

Quoted 26 lines

Excerpts from Vivek Goyal's message of 2011-03-10 14:41:06 -0500:

Quoted 25 lines · Vivek Goyal

Quoted 13 lines

Quoted 5 lines

Quoted 4 lines

I think the person who dirtied the page can store the information in page->private (assuming buffer heads were not generated) and if flusher thread later ends up generating buffer heads and ends up modifying page->private, this can be copied in buffer heads?

Quoted 7 lines

This scares me a bit.

As I understand it, fs/ code expects total ownership of page->private. This adds a responsibility for every user to copy the data through and store it in the buffer head (or anything else). btrfs seems to do something entirely different in some cases and store a different kind of value.

Quoted 7 lines

If filesystems are using page->private for some other purpose also, then I guess we have issues.

I am ccing linux-fsdevel to have some feedback on the idea of trying to store cgroup id of page dirtying thread in page->private and/or buffer head for tracking which group originally dirtied the page in IO controller during writeback.

A quick "grep" showed that btrfs, ceph and logfs are using page->private for other purposes also.

Quoted 8 lines

Quoted 4 lines

I was under the impression that either page->private is null or it points to buffer heads for the writeback case. So storing the info directly in either buffer head directly or first in page->private and then transferring it to buffer heads would have helped.

Right, btrfs has its own uses for page->private, and we expect to own it. With a proper callback, the FS could store the extra information you need in out own structs.

Quoted 5 lines

There is no requirement that page->private ever points to a buffer_head, and Lustre clients use it for its own tracking structure (never touching buffer_heads at all). Any assumption about what a filesystem is storing in page->private in other parts of the code is just broken.

Andreas,

Quoted 2 lines

As Chris mentioned, will providing callbacks so that filesystem can save/restore page->private be reasonable?

Just to clarify, I think saving/restoring page->private is going to be hard. I'd rather just have a call back that says here's a page, storage this for the block io controller please, and another one that returns any previously stored info.

Quoted 46 lines · Chris Mason

Excerpts from Vivek Goyal's message of 2011-03-10 16:38:32 -0500:

Quoted 45 lines · Andreas Dilger

Quoted 36 lines · Chris Mason

Quoted 26 lines

Excerpts from Vivek Goyal's message of 2011-03-10 14:41:06 -0500:

Quoted 25 lines · Vivek Goyal

Quoted 13 lines

Quoted 5 lines

Quoted 4 lines

I think the person who dirtied the page can store the information in page->private (assuming buffer heads were not generated) and if flusher thread later ends up generating buffer heads and ends up modifying page->private, this can be copied in buffer heads?

Quoted 7 lines

This scares me a bit.

As I understand it, fs/ code expects total ownership of page->private. This adds a responsibility for every user to copy the data through and store it in the buffer head (or anything else). btrfs seems to do something entirely different in some cases and store a different kind of value.

Quoted 7 lines

If filesystems are using page->private for some other purpose also, then I guess we have issues.

I am ccing linux-fsdevel to have some feedback on the idea of trying to store cgroup id of page dirtying thread in page->private and/or buffer head for tracking which group originally dirtied the page in IO controller during writeback.

A quick "grep" showed that btrfs, ceph and logfs are using page->private for other purposes also.

Quoted 8 lines

Quoted 4 lines

I was under the impression that either page->private is null or it points to buffer heads for the writeback case. So storing the info directly in either buffer head directly or first in page->private and then transferring it to buffer heads would have helped.

Right, btrfs has its own uses for page->private, and we expect to own it. With a proper callback, the FS could store the extra information you need in out own structs.

Quoted 5 lines

There is no requirement that page->private ever points to a buffer_head, and Lustre clients use it for its own tracking structure (never touching buffer_heads at all). Any assumption about what a filesystem is storing in page->private in other parts of the code is just broken.

Andreas,

Quoted 7 lines

Quoted 2 lines

As Chris mentioned, will providing callbacks so that filesystem can save/restore page->private be reasonable?

Just to clarify, I think saving/restoring page->private is going to be hard. I'd rather just have a call back that says here's a page, storage this for the block io controller please, and another one that returns any previously stored info.

Agreed - there is absolutely no guarantee that some other thread doesn't grab the page while it is under writeback and dereference page->private expecting there to be buffer heads or some filesystem specific structure to be there. Hence swapping out the expected structure with something different is problematic.

However, I think there's bigger issues. e.g. page->private might point to multiple bufferheads that map to non-contiguous disk blocks that were written by different threads - what happens if we get two concurrent IOs to the one page, perhaps with different cgroup IDs?

Further, page->private might not even point to a per-page specific structure - it might point to a structure shared by multiple pages (e.g. an extent map). Adding a callback like this requires filesystems to be able to store per-page or per-block information for external users. Indeed, one of the areas of development in XFS right now is to move away from storing internal per-block/per-page information because of the memory overhead it causes.

IMO, if you really need some per-page information, then just put it in the struct page - you can't hide the memory overhead just by having the filesystem to store it for you. That just adds unnecessary complexity...

Quoted 4 lines

IMO, if you really need some per-page information, then just put it in the struct page - you can't hide the memory overhead just by having the filesystem to store it for you. That just adds unnecessary complexity...

Ok. I guess adding anything to struct page is going to be hard and we might have to fall back to looking into using page_cgroup for tracking page state. I was trying to explore the ways so that we don't have to instantiate whole page_cgroup structure just for trying to figure out who dirtied the page.

Quoted 11 lines · On Thu, 10 Mar 2011 21:15:31 -0500 Vivek Goyal <[email protected]> wrote:

Quoted 4 lines

IMO, if you really need some per-page information, then just put it in the struct page - you can't hide the memory overhead just by having the filesystem to store it for you. That just adds unnecessary complexity...

Ok. I guess adding anything to struct page is going to be hard and we might have to fall back to looking into using page_cgroup for tracking page state. I was trying to explore the ways so that we don't have to instantiate whole page_cgroup structure just for trying to figure out who dirtied the page.

Is this bad ?
==

At handling ASYNC I/O in blkio layer, it's unknown that who dirtied the page. This lack of information makes impossible to throttole Async I/O per cgroup in blkio queue layer.

This patch records the information into radix-tree and preserve the information. There is no 'clear' operation because all I/O starts when the page is marked as DIRTY.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

---
 include/linux/radix-tree.h |    3 +++
 lib/radix-tree.c           |   42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

Index: mmotm-Mar10/include/linux/radix-tree.h
===================================================================

include/linux/radix-tree.h +3 -0

@@ -58,12 +58,14 @@ struct radix_tree_root {
 	unsigned int		height;
 	gfp_t			gfp_mask;
 	struct radix_tree_node	__rcu *rnode;
+	int			iohint;
 };
 
 #define RADIX_TREE_INIT(mask)	{					\
 	.height = 0,							\
 	.gfp_mask = (mask),						\
 	.rnode = NULL,							\
+	.iohint = 0,							\
 }
 
 #define RADIX_TREE(name, mask) \

@@ -74,6 +76,7 @@ do {									\
 	(root)->height = 0;						\
 	(root)->gfp_mask = (mask);					\
 	(root)->rnode = NULL;						\
+	(root)->iohint = 0;						\
 } while (0)
 
 /**

Index: mmotm-Mar10/lib/radix-tree.c
===================================================================

lib/radix-tree.c +41 -0

@@ -31,6 +31,7 @@
 #include <linux/string.h>
 #include <linux/bitops.h>
 #include <linux/rcupdate.h>
+#include <linux/blkdev.h>
 
 
 #ifdef __KERNEL__

@@ -51,6 +52,9 @@ struct radix_tree_node {
 	struct rcu_head	rcu_head;
 	void __rcu	*slots[RADIX_TREE_MAP_SIZE];
 	unsigned long	tags[RADIX_TREE_MAX_TAGS][RADIX_TREE_TAG_LONGS];
+#ifdef CONFIG_BLK_CGROUP
+	unsigned short  iohint[RADIX_TREE_MAP_SIZE];
+#endif
 };
 
 struct radix_tree_path {

@@ -473,6 +477,8 @@ void *radix_tree_tag_set(struct radix_tr
 		offset = (index >> shift) & RADIX_TREE_MAP_MASK;
 		if (!tag_get(slot, tag, offset))
 			tag_set(slot, tag, offset);
+		if (height == 1 && slot && tag == PAGECACHE_TAG_DIRTY)
+			blkio_record_hint(&slot->iohint[offset]);
 		slot = slot->slots[offset];
 		BUG_ON(slot == NULL);
 		shift -= RADIX_TREE_MAP_SHIFT;

@@ -1418,3 +1425,38 @@ void __init radix_tree_init(void)
 	radix_tree_init_maxindex();
 	hotcpu_notifier(radix_tree_callback, 0);
 }
+
+#ifdef CONFIG_BLK_CGROUP
+
+unsigned short radix_tree_lookup_iohint(struct radix_tree_root *root,
+				int index)
+{
+	unsigned int height, shift;
+	struct radix_tree_node *node;
+
+	node = rcu_redereference(root->rnode);
+	if (node == NULL)
+		return 0;
+	if (!radix_tree_is_indirect_ptr(node))
+		return root->iohint;
+	node = radxi_tree_indirect_to_ptr(node);
+
+	height = node->height;
+	if (index > radix_tree_maxindex(height))
+		return 0;
+	shift = (height - 1) * RADIX_TREE_MAP_SHIFT;
+	for ( ; ; ) {
+		int offset;
+
+		if (node == NULL)
+			return 0;
+		offset = (index >> shift) & RADIX_TREE_MAP_MASK;
+		if (height == 1)
+			return node->iohint[offset];
+		node = rcu_rereference(node->slots[offset]);
+		shift -= RADIX_TREE_MAP_SHIFT;
+		height--;
+	}
+}
+EXPORT_SYMBOL(radix_tree_lookup_iohint);
+#endif

Quoted 15 lines · KAMEZAWA Hiroyuki

Quoted 11 lines · On Thu, 10 Mar 2011 21:15:31 -0500 Vivek Goyal <[email protected]> wrote:

Quoted 4 lines

IMO, if you really need some per-page information, then just put it in the struct page - you can't hide the memory overhead just by having the filesystem to store it for you. That just adds unnecessary complexity...

Ok. I guess adding anything to struct page is going to be hard and we might have to fall back to looking into using page_cgroup for tracking page state. I was trying to explore the ways so that we don't have to instantiate whole page_cgroup structure just for trying to figure out who dirtied the page.

Quoted 2 lines

Is this bad ?
==

Sounds like an interesting idea. I am primarily concered about the radix tree node size increase. Not sure how big a concern this is.

Also tracking is useful for two things.

- Proportinal IO
- IO throttling

For proportional IO, anyway we have to use it with memory controller to control per cgroup dirty ratio so storing info in page_cgroup should not hurt.

The only other case where dependence on page_cgroup hurts is IO throttling where IO controller does not really need memory cgroup controller (I hope so). But we are still not sure if throttling IO at device level is a good idea and how to resolve issues related to priority inversion.