Fix task IO accounting on reads
PATCH 1/1 Fix task IO accounting on reads
We've often found it inconvenient that reads from Lustre mounts do not correctly increment IO counts (/proc/<pid>/io).
Below is a patch which aims to fix this functionality.
The symptom is that writes are accounted for, but not reads. It seems that the accounting it normally done in the kernels page writeback and readahead functionality. Therefore as Lustre implements its own readahead, it must also maintain its own accounting on reads (but not writes).
Is anyone able to confirm this rationale for the location of this particular call, and consider the following patch? Thanks.
Signature · 5 lines
Mark --- lustre/llite/rw.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
lustre/llite/rw.c +3 -0
@@ -58,6 +58,7 @@#include <linux/mm.h> #include <linux/pagemap.h> #include <linux/smp_lock.h> +#include <linux/task_io_accounting_ops.h> #define DEBUG_SUBSYSTEM S_LLITE
@@ -1311,6 +1312,8 @@ static int ll_issue_page_read(struct obd_export *exp,if (rc) {LL_CDEBUG_PAGE (D_ERROR , page,"read queue failed: rc %d\n" , rc);page_cache_release (page); + }else { +task_io_account_read (CFS_PAGE_SIZE ); }RETURN (rc); }
Signature · 1 line
1.7.1.1
PATCH 1/1 Fix task IO accounting on reads
Quoted 5 lines · Mark Hills
We've often found it inconvenient that reads from Lustre mounts do not correctly increment IO counts (/proc/<pid>/io).
Below is a patch which aims to fix this functionality.
Quoted 7 lines
The symptom is that writes are accounted for, but not reads. It seems that the accounting it normally done in the kernels page writeback and readahead functionality. Therefore as Lustre implements its own readahead, it must also maintain its own accounting on reads (but not writes).
Is anyone able to confirm this rationale for the location of this particular call, and consider the following patch? Thanks.
Mark,
It looks like this functionality was added to the kernel in Git commit hash 7c3ab7381e79dfc7db14a67c6f4f3285664e1ec2, which was between 2.6.19 and 2.6.20. This call needs to be made conditional so that it doesn't break the client build on older kernels (it looks like it would break RHEL5 and SLES10 clients, which don't have this functionality AFAICS).
It looks like it may be possible to create a compatibility macro in lustre/include/linux/lustre_compat25.h so that it doesn't break for clients running on older kernels:
#ifdef CONFIG_TASK_IO_ACCOUNTING
#include <linux/task_io_accounting_ops.h> #else
#define task_io_accounting_read(bytes) do {} while (0) #endif
Quoted 31 lines
lustre/llite/rw.c| 3 +++1 files changed, 3 insertions(+), 0 deletions(-)lustre/llite/rw.c +3
-0@@ -58,6 +58,7 @@#include <linux/mm.h> #include <linux/pagemap.h> #include <linux/smp_lock.h> +#include <linux/task_io_accounting_ops.h> #define DEBUG_SUBSYSTEM S_LLITE@@ -1311,6 +1312,8 @@ static int ll_issue_page_read(struct obd_export *exp,if (rc) {LL_CDEBUG_PAGE (D_ERROR , page,"read queue failed: rc %d\n" , rc);page_cache_release (page); + }else { +task_io_account_read (CFS_PAGE_SIZE ); }RETURN (rc); }Signature · 6 lines
1.7.1.1 _______________________________________________ Lustre-devel mailing list Lustre-devel at lists.lustre.org http://lists.lustre.org/mailman/listinfo/lustre-devel
Cheers, Andreas
--
Andreas Dilger
Principal Engineer
Whamcloud, Inc.
PATCH 1/1 Fix task IO accounting on reads
Quoted 25 lines · Andreas Dilger
Quoted 5 lines · Mark Hills
We've often found it inconvenient that reads from Lustre mounts do not correctly increment IO counts (/proc/<pid>/io).
Below is a patch which aims to fix this functionality.
Quoted 7 lines
The symptom is that writes are accounted for, but not reads. It seems that the accounting it normally done in the kernels page writeback and readahead functionality. Therefore as Lustre implements its own readahead, it must also maintain its own accounting on reads (but not writes).
Is anyone able to confirm this rationale for the location of this particular call, and consider the following patch? Thanks.
Mark,
It looks like this functionality was added to the kernel in Git commit hash7c3ab7381e79dfc7db14a67c6f4f3285664e1ec2, which was between 2.6.19 and 2.6.20. This call needs to be made conditional so that it doesn't break the client build on older kernels (it looks like it would break RHEL5 and SLES10 clients, which don't have this functionality AFAICS).It looks like it may be possible to create a compatibility macro in
lustre/include/linux/lustre_compat25.hso that it doesn't break for clients running on older kernels:
Quoted 5 lines
#ifdef
CONFIG_TASK_IO_ACCOUNTING
#include <linux/task_io_accounting_ops.h> #else
#definetask_io_accounting_read(bytes)do {} while (0) #endif
Newer kernels always define task_io_account_read() as a function, even if it's a no-op. So to avoid redefining it it should check the kernel version, not the CONFIG_ flag.
A revised patch is below, which I tested with kernel 2.6.32.28. Unfortunately I odn't have a pre-2.6.20 platform for testing.
Thanks Andreas,
Signature · 6 lines
Mark --- lustre/include/linux/lustre_compat25.h | 6 ++++++ lustre/llite/rw.c | 2 ++ 2 files changed, 8 insertions(+), 0 deletions(-)
lustre/include/linux/lustre_compat25.h +6 -0
@@ -143,6 +143,12 @@ void groups_free(struct group_info *ginfo);#endif /* LINUX_VERSION_CODE < KERNEL_VERSION(2,6,4) */ +#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,6 ,20 ) +#define task_io_account_read (bytes) do {} while (0) +#else +#include <linux/task_io_accounting_ops.h> +#endif +#ifndef page_private#define page_private (page) ((page)->private)#define set_page_private (page, v) ((page)->private = (v))
lustre/llite/rw.c +2 -0
@@ -1311,6 +1311,8 @@ static int ll_issue_page_read(struct obd_export *exp,if (rc) {LL_CDEBUG_PAGE (D_ERROR , page,"read queue failed: rc %d\n" , rc);page_cache_release (page); + }else { +task_io_account_read (CFS_PAGE_SIZE ); }RETURN (rc); }
Signature · 1 line
1.7.1.1
PATCH 1/1 Fix task IO accounting on reads
Quoted 1 line · Mark Hills
On Tue, 26 Apr 2011, Andreas Dilger wrote:
...
Quoted 17 lines
Quoted 11 lines
Mark,
It looks like this functionality was added to the kernel in Git commit hash7c3ab7381e79dfc7db14a67c6f4f3285664e1ec2, which was between 2.6.19 and 2.6.20. This call needs to be made conditional so that it doesn't break the client build on older kernels (it looks like it would break RHEL5 and SLES10 clients, which don't have this functionality AFAICS).It looks like it may be possible to create a compatibility macro in
lustre/include/linux/lustre_compat25.hso that it doesn't break for clients running on older kernels:Quoted 5 lines
#ifdef
CONFIG_TASK_IO_ACCOUNTING
#include <linux/task_io_accounting_ops.h> #else
#definetask_io_accounting_read(bytes)do {} while (0) #endif
Quoted 6 lines
Newer kernels always define
task_io_account_read()as a function, even if it's a no-op. So to avoid redefining it it should check the kernel version, not theCONFIG_flag.A revised patch is below, which I tested with kernel 2.6.32.28. Unfortunately I odn't have a pre-2.6.20 platform for testing.
I suspect a real world complication that you'd have to handle is that the RHEL-5u6 kernel (2.6.18-238) has task_io_accounting_ops.h/task_io_accounting_read (yet another Redhat backport...)
/Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20110427/1e720af5/attachment.pgp>
PATCH 1/1 Fix task IO accounting on reads
In reply to Mark HillsQuoted 32 lines · Mark Hills
Quoted 25 lines · Andreas Dilger
Quoted 5 lines · Mark Hills
We've often found it inconvenient that reads from Lustre mounts do not correctly increment IO counts (/proc/<pid>/io).
Below is a patch which aims to fix this functionality.
Quoted 7 lines
The symptom is that writes are accounted for, but not reads. It seems that the accounting it normally done in the kernels page writeback and readahead functionality. Therefore as Lustre implements its own readahead, it must also maintain its own accounting on reads (but not writes).
Is anyone able to confirm this rationale for the location of this particular call, and consider the following patch? Thanks.
Mark,
It looks like this functionality was added to the kernel in Git commit hash7c3ab7381e79dfc7db14a67c6f4f3285664e1ec2, which was between 2.6.19 and 2.6.20. This call needs to be made conditional so that it doesn't break the client build on older kernels (it looks like it would break RHEL5 and SLES10 clients, which don't have this functionality AFAICS).It looks like it may be possible to create a compatibility macro in
lustre/include/linux/lustre_compat25.hso that it doesn't break for clients running on older kernels:Quoted 5 lines
#ifdef
CONFIG_TASK_IO_ACCOUNTING
#include <linux/task_io_accounting_ops.h> #else
#definetask_io_accounting_read(bytes)do {} while (0) #endif
Quoted 3 lines
Newer kernels always define
task_io_account_read()as a function, even if it's a no-op. So to avoid redefining it it should check the kernel version, not theCONFIG_flag.
I don't think this is a real problem, because if CONFIG_TASK_IO_ACCOUNTING is not defined, then the compatibility functions in the task_io_accounting_ops.h file will not be included and there shouldn't be any problem. I don't see task_io_accounting_ops.h included by any other headers in the kernel, only by fs/buffer.c and fs/direct-io.c, so my original proposal should be safe.
Quoted 46 lines
A revised patch is below, which I tested with kernel 2.6.32.28. Unfortunately I odn't have a pre-2.6.20 platform for testing.
Thanks Andreas,
Signature · 6 lines
Mark --- lustre/include/linux/lustre_compat25.h | 6 ++++++ lustre/llite/rw.c | 2 ++ 2 files changed, 8 insertions(+), 0 deletions(-)lustre/include/linux/lustre_compat25.h +6
-0@@ -143,6 +143,12 @@ void groups_free(struct group_info *ginfo);#endif /* LINUX_VERSION_CODE < KERNEL_VERSION(2,6,4) */ +#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,6 ,20 ) +#define task_io_account_read (bytes) do {} while (0) +#else +#include <linux/task_io_accounting_ops.h> +#endif +#ifndef page_private#define page_private (page) ((page)->private)#define set_page_private (page, v) ((page)->private = (v))lustre/llite/rw.c +2
-0@@ -1311,6 +1311,8 @@ static int ll_issue_page_read(struct obd_export *exp,if (rc) {LL_CDEBUG_PAGE (D_ERROR , page,"read queue failed: rc %d\n" , rc);page_cache_release (page); + }else { +task_io_account_read (CFS_PAGE_SIZE ); }RETURN (rc); }Signature · 1 line
1.7.1.1
Cheers, Andreas
--
Andreas Dilger
Principal Engineer
Whamcloud, Inc.
PATCH 1/1 Fix task IO accounting on reads
Quoted 37 lines · Andreas Dilger
Quoted 32 lines · Mark Hills
Quoted 25 lines · Andreas Dilger
Quoted 5 lines · Mark Hills
We've often found it inconvenient that reads from Lustre mounts do not correctly increment IO counts (/proc/<pid>/io).
Below is a patch which aims to fix this functionality.
Quoted 7 lines
The symptom is that writes are accounted for, but not reads. It seems that the accounting it normally done in the kernels page writeback and readahead functionality. Therefore as Lustre implements its own readahead, it must also maintain its own accounting on reads (but not writes).
Is anyone able to confirm this rationale for the location of this particular call, and consider the following patch? Thanks.
Mark,
It looks like this functionality was added to the kernel in Git commit hash7c3ab7381e79dfc7db14a67c6f4f3285664e1ec2, which was between 2.6.19 and 2.6.20. This call needs to be made conditional so that it doesn't break the client build on older kernels (it looks like it would break RHEL5 and SLES10 clients, which don't have this functionality AFAICS).It looks like it may be possible to create a compatibility macro in
lustre/include/linux/lustre_compat25.hso that it doesn't break for clients running on older kernels:Quoted 5 lines
#ifdef
CONFIG_TASK_IO_ACCOUNTING
#include <linux/task_io_accounting_ops.h> #else
#definetask_io_accounting_read(bytes)do {} while (0) #endifQuoted 3 lines
Newer kernels always define
task_io_account_read()as a function, even if it's a no-op. So to avoid redefining it it should check the kernel version, not theCONFIG_flag.
Quoted 6 lines
I don't think this is a real problem, because if
CONFIG_TASK_IO_ACCOUNTINGis not defined, then the compatibility functions in thetask_io_accounting_ops.hfile will not be included and there shouldn't be any problem. I don't seetask_io_accounting_ops.hincluded by any other headers in the kernel, only byfs/buffer.candfs/direct-io.c, so my original proposal should be safe.
Ah yes, of course you are correct.
If you're happy the _ops.h header won't get included (indirectly or otherwise) in future.
I guess I don't need to re-send with that modification, but let me know if you'd prefer that.
Thanks for your help.
[...]
Quoted 39 lines
Quoted 38 lines
---
lustre/include/linux/lustre_compat25.h | 6 ++++++ lustre/llite/rw.c | 2 ++ 2 files changed, 8 insertions(+), 0 deletions(-)lustre/include/linux/lustre_compat25.h +6
-0@@ -143,6 +143,12 @@ void groups_free(struct group_info *ginfo);#endif /* LINUX_VERSION_CODE < KERNEL_VERSION(2,6,4) */ +#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,6 ,20 ) +#define task_io_account_read (bytes) do {} while (0) +#else +#include <linux/task_io_accounting_ops.h> +#endif +#ifndef page_private#define page_private (page) ((page)->private)#define set_page_private (page, v) ((page)->private = (v))lustre/llite/rw.c +2
-0@@ -1311,6 +1311,8 @@ static int ll_issue_page_read(struct obd_export *exp,if (rc) {LL_CDEBUG_PAGE (D_ERROR , page,"read queue failed: rc %d\n" , rc);page_cache_release (page); + }else { +task_io_account_read (CFS_PAGE_SIZE ); }RETURN (rc); }Signature · 1 line
1.7.1.1
Signature · 1 line
Mark
PATCH 1/1 Fix task IO accounting on reads
Quoted 32 lines · Mark Hills
Quoted 22 lines · Andreas Dilger
Quoted 17 lines · Mark Hills
Quoted 10 lines · Andreas Dilger
It looks like this functionality was added to the kernel in Git commit hash
7c3ab7381e79dfc7db14a67c6f4f3285664e1ec2, which was between 2.6.19 and 2.6.20. This call needs to be made conditional so that it doesn't break the client build on older kernels (it looks like it would break RHEL5 and SLES10 clients, which don't have this functionality AFAICS).It looks like it may be possible to create a compatibility macro in
lustre/include/linux/lustre_compat25.hso that it doesn't break for clients running on older kernels:Quoted 5 lines
#ifdef
CONFIG_TASK_IO_ACCOUNTING
#include <linux/task_io_accounting_ops.h> #else
#definetask_io_accounting_read(bytes)do {} while (0) #endifQuoted 3 lines
Newer kernels always define
task_io_account_read()as a function, even if it's a no-op. So to avoid redefining it it should check the kernel version, not theCONFIG_flag.Quoted 6 lines
I don't think this is a real problem, because if
CONFIG_TASK_IO_ACCOUNTINGis not defined, then the compatibility functions in thetask_io_accounting_ops.hfile will not be included and there shouldn't be any problem. I don't seetask_io_accounting_ops.hincluded by any other headers in the kernel, only byfs/buffer.candfs/direct-io.c, so my original proposal should be safe.Ah yes, of course you are correct.
Quoted 7 lines
If you're happy the
_ops.hheader won't get included (indirectly or otherwise) in future.I guess I don't need to re-send with that modification, but let me know if you'd prefer that.
Thanks for your help.
Mark,
for inclusion into the next 1.8 release the updated patch should be attached to a new bug at bugzilla.lustre.org, and a review requested on the patch (I can be one of the reviewers). For inclusion into the 2.1 release, please create a new bug at jira.whamcloud.com with the updated patch. The patch should include the standard kernel "Signed-off-by: {your name} <your email>" line.
Sorry for the complexity in submitting patches while development transitions away from Oracle for newer releases.
Quoted 32 lines
[...]
Quoted 31 lines
Quoted 31 lines
---
lustre/include/linux/lustre_compat25.h | 6 ++++++ lustre/llite/rw.c | 2 ++ 2 files changed, 8 insertions(+), 0 deletions(-)lustre/include/linux/lustre_compat25.h +6
-0@@ -143,6 +143,12 @@ void groups_free(struct group_info *ginfo);#endif /* LINUX_VERSION_CODE < KERNEL_VERSION(2,6,4) */ +#if LINUX_VERSION_CODE <KERNEL_VERSION (2 ,6 ,20 ) +#define task_io_account_read (bytes) do {} while (0) +#else +#include <linux/task_io_accounting_ops.h> +#endif +#ifndef page_private#define page_private (page) ((page)->private)#define set_page_private (page, v) ((page)->private = (v))lustre/llite/rw.c +1
-0@@ -1311,6 +1311,8 @@ static int ll_issue_page_read(struct obd_export *exp,if (rc) {LL_CDEBUG_PAGE (D_ERROR , page,"read queue failed: rc %d\n" , rc);page_cache_release (page); + }else {
Quoted 11 lines
Quoted 8 lines
Quoted 7 lines
lustre/llite/rw.c +1
-0+task_io_account_read (CFS_PAGE_SIZE ); }RETURN (rc); }Signature · 1 line
1.7.1.1Signature · 1 line
Mark
Cheers, Andreas
--
Andreas Dilger
Principal Engineer
Whamcloud, Inc.
PATCH 1/1 Fix task IO accounting on reads
Oracle will also require that a contributor agreement is submitted - see
http://wiki.lustre.org/index.php/Contribution_Policy
Quoted 2 lines · Andreas Dilger
<snip>
for inclusion into the next 1.8 release the updated patch should be attached to a new bug at bugzilla.lustre.org, and a review requested on the patch (I can be one of the reviewers).
PATCH 1/1 Fix task IO accounting on reads
In reply to Andreas DilgerOn Wed, 27 Apr 2011, Andreas Dilger wrote:
[...]
Quoted 9 lines
for inclusion into the next 1.8 release the updated patch should be attached to a new bug at bugzilla.lustre.org, and a review requested on the patch (I can be one of the reviewers). For inclusion into the 2.1 release, please create a new bug at jira.whamcloud.com with the updated patch. The patch should include the standard kernel "Signed-off-by:
{your name} <your email>" line.Sorry for the complexity in submitting patches while development transitions away from Oracle for newer releases.
Hi Andreas, I created an entry in Bugzilla with the revised patch attached to it:
https://bugzilla.lustre.org/show_bug.cgi?id=24536
I wasn't able to add you as reviewer, because I could not clearly see a field to do this.
As for the 2.1 release, I took a quick look and it seems the code has changed substantially and the patch does not apply.
I don't even have a 2.1 setup to verify the bug exists. And as I'm unfamiliar with the code, I don't think I can reasonably make an equivalent patch to 2.1 right now.
Thanks
Signature · 1 line
Mark
PATCH 1/1 Fix task IO accounting on reads
Mark
Sorry if Andreas was not clear enough about contributing patches. Hopefully the more detailed information at
http://wiki.whamcloud.com/display/PUB/Submitting+Changes will help. If you are only able to work with 1.8.x then by all means upload the patch into gerrit and another engineer could handle porting it to master (subject to other priorities of course). IIRC Framestore did not sign a Sun\Oracle contributor agreement so I suspect Oracle will not land your patch.
Regards
Peter
Quoted 29 lines · Mark Hills
On Wed, 27 Apr 2011, Andreas Dilger wrote:
[...]
Quoted 9 lines
for inclusion into the next 1.8 release the updated patch should be attached to a new bug at bugzilla.lustre.org, and a review requested on the patch (I can be one of the reviewers). For inclusion into the 2.1 release, please create a new bug at jira.whamcloud.com with the updated patch. The patch should include the standard kernel "Signed-off-by:
{your name}<your email>" line.Sorry for the complexity in submitting patches while development transitions away from Oracle for newer releases.
Hi Andreas, I created an entry in Bugzilla with the revised patch attached to it:
https://bugzilla.lustre.org/show_bug.cgi?id=24536I wasn't able to add you as reviewer, because I could not clearly see a field to do this.
As for the 2.1 release, I took a quick look and it seems the code has changed substantially and the patch does not apply.
I don't even have a 2.1 setup to verify the bug exists. And as I'm unfamiliar with the code, I don't think I can reasonably make an equivalent patch to 2.1 right now.
Thanks
Signature · 3 lines
Peter Jones Whamcloud, Inc. www.whamcloud.com
PATCH 1/1 Fix task IO accounting on reads
In reply to Mark HillsQuoted 10 lines · Mark Hills
We've often found it inconvenient that reads from Lustre mounts do not correctly increment IO counts (/proc/<pid>/io).
Below is a patch which aims to fix this functionality.
The symptom is that writes are accounted for, but not reads. It seems that the accounting it normally done in the kernels page writeback and readahead functionality. Therefore as Lustre implements its own readahead, it must also maintain its own accounting on reads (but not writes).
Hi Mark;
Are you aware of the (soon to land) patch:
Lustre client procfs stats: read_bytes does not record the number of bytes transfered from the fs:
http://jira.whamcloud.com/browse/LU-333
richard,
Signature · 3 lines
Richard.Henwood at whamcloud.com Whamcloud Inc. tel: +1 512 410 9612
PATCH 1/1 Fix task IO accounting on reads
Quoted 12 lines · Richard Henwood
Quoted 10 lines · Mark Hills
We've often found it inconvenient that reads from Lustre mounts do not correctly increment IO counts (/proc/<pid>/io).
Below is a patch which aims to fix this functionality.
The symptom is that writes are accounted for, but not reads. It seems that the accounting it normally done in the kernels page writeback and readahead functionality. Therefore as Lustre implements its own readahead, it must also maintain its own accounting on reads (but not writes).
Quoted 6 lines
Hi Mark;
Are you aware of the (soon to land) patch:
Lustre client procfs stats:read_bytesdoes not record the number of bytes transfered from the fs:
http://jira.whamcloud.com/browse/LU-333
I was aware of these stats (and this patch), but I concluded that they did not track memory mapped access.
So I think our patches address two separate parts of the process. But I'm happy to be corrected -- it was some time ago that I did this.
Signature · 1 line
Mark
PATCH 1/1 Fix task IO accounting on reads
On Tue, Jul 19, 2011 at 11:08 AM, Mark Hills <[email protected]> wrote:
<snip>
Quoted 7 lines
I was aware of these stats (and this patch), but I concluded that they did not track memory mapped access.
So I think our patches address two separate parts of the process. But I'm happy to be corrected -- it was some time ago that I did this.
It was indeed a while ago, and I'm sorry I missed this conversation when it was current!
It seems that both patches have the same motivation for reliable read stats, but the patches are different with complementary approaches.
richard,
Signature · 3 lines
Richard.Henwood at whamcloud.com Whamcloud Inc. tel: +1 512 410 9612