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.
--
Mark
---
lustre/llite/rw.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/lustre/llite/rw.c b/lustre/llite/rw.c
index ccd6a42..c7a84b7 100644
--- a/lustre/llite/rw.c
+++ b/lustre/llite/rw.c
@@ -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);
}
--
1.7.1.1
Andreas Dilger
2011-Apr-26 16:38 UTC
[Lustre-devel] [PATCH] Fix task IO accounting on reads
On 2011-04-26, at 10:06 AM, Mark Hills wrote:> 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.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> lustre/llite/rw.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/lustre/llite/rw.c b/lustre/llite/rw.c > index ccd6a42..c7a84b7 100644 > --- a/lustre/llite/rw.c > +++ b/lustre/llite/rw.c > @@ -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); > } > -- > 1.7.1.1 > > _______________________________________________ > Lustre-devel mailing list > Lustre-devel at lists.lustre.org > http://lists.lustre.org/mailman/listinfo/lustre-develCheers, Andreas -- Andreas Dilger Principal Engineer Whamcloud, Inc.
On Tue, 26 Apr 2011, Andreas Dilger wrote:> On 2011-04-26, at 10:06 AM, Mark Hills wrote: > > 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. > > 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) > #endifNewer 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, -- Mark --- lustre/include/linux/lustre_compat25.h | 6 ++++++ lustre/llite/rw.c | 2 ++ 2 files changed, 8 insertions(+), 0 deletions(-) diff --git a/lustre/include/linux/lustre_compat25.h b/lustre/include/linux/lustre_compat25.h index 81865d0..666374b 100644 --- a/lustre/include/linux/lustre_compat25.h +++ b/lustre/include/linux/lustre_compat25.h @@ -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)) diff --git a/lustre/llite/rw.c b/lustre/llite/rw.c index ccd6a42..1fddd5b 100644 --- a/lustre/llite/rw.c +++ b/lustre/llite/rw.c @@ -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); } -- 1.7.1.1
Peter Kjellström
2011-Apr-27 15:56 UTC
[Lustre-devel] [PATCH] Fix task IO accounting on reads
On Wednesday, April 27, 2011 01:23:57 PM Mark Hills wrote:> On Tue, 26 Apr 2011, Andreas Dilger wrote:...> > 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 > > 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.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/attachments/20110427/1e720af5/attachment.bin
Andreas Dilger
2011-Apr-27 16:43 UTC
[Lustre-devel] [PATCH] Fix task IO accounting on reads
On 2011-04-27, at 5:23 AM, Mark Hills wrote:> On Tue, 26 Apr 2011, Andreas Dilger wrote: >> On 2011-04-26, at 10:06 AM, Mark Hills wrote: >>> 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. >> >> 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 > > 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.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.> 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, > > -- > Mark > > --- > lustre/include/linux/lustre_compat25.h | 6 ++++++ > lustre/llite/rw.c | 2 ++ > 2 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/lustre/include/linux/lustre_compat25.h b/lustre/include/linux/lustre_compat25.h > index 81865d0..666374b 100644 > --- a/lustre/include/linux/lustre_compat25.h > +++ b/lustre/include/linux/lustre_compat25.h > @@ -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)) > diff --git a/lustre/llite/rw.c b/lustre/llite/rw.c > index ccd6a42..1fddd5b 100644 > --- a/lustre/llite/rw.c > +++ b/lustre/llite/rw.c > @@ -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); > } > -- > 1.7.1.1 >Cheers, Andreas -- Andreas Dilger Principal Engineer Whamcloud, Inc.
On Wed, 27 Apr 2011, Andreas Dilger wrote:> On 2011-04-27, at 5:23 AM, Mark Hills wrote: > > On Tue, 26 Apr 2011, Andreas Dilger wrote: > >> On 2011-04-26, at 10:06 AM, Mark Hills wrote: > >>> 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. > >> > >> 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 > > > > 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. > > 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.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. [...]> > --- > > lustre/include/linux/lustre_compat25.h | 6 ++++++ > > lustre/llite/rw.c | 2 ++ > > 2 files changed, 8 insertions(+), 0 deletions(-) > > > > diff --git a/lustre/include/linux/lustre_compat25.h b/lustre/include/linux/lustre_compat25.h > > index 81865d0..666374b 100644 > > --- a/lustre/include/linux/lustre_compat25.h > > +++ b/lustre/include/linux/lustre_compat25.h > > @@ -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)) > > diff --git a/lustre/llite/rw.c b/lustre/llite/rw.c > > index ccd6a42..1fddd5b 100644 > > --- a/lustre/llite/rw.c > > +++ b/lustre/llite/rw.c > > @@ -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); > > } > > -- > > 1.7.1.1 > > >-- Mark
Andreas Dilger
2011-Apr-27 21:15 UTC
[Lustre-devel] [PATCH] Fix task IO accounting on reads
On 2011-04-27, at 11:14 AM, Mark Hills wrote:> On Wed, 27 Apr 2011, Andreas Dilger wrote: >> On 2011-04-27, at 5:23 AM, Mark Hills wrote: >>> On Tue, 26 Apr 2011, Andreas Dilger wrote: >>>> 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 >>> >>> 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. >> >> 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. > > 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.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.> [...] >>> --- >>> lustre/include/linux/lustre_compat25.h | 6 ++++++ >>> lustre/llite/rw.c | 2 ++ >>> 2 files changed, 8 insertions(+), 0 deletions(-) >>> >>> diff --git a/lustre/include/linux/lustre_compat25.h b/lustre/include/linux/lustre_compat25.h >>> index 81865d0..666374b 100644 >>> --- a/lustre/include/linux/lustre_compat25.h >>> +++ b/lustre/include/linux/lustre_compat25.h >>> @@ -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)) >>> diff --git a/lustre/llite/rw.c b/lustre/llite/rw.c >>> index ccd6a42..1fddd5b 100644 >>> --- a/lustre/llite/rw.c >>> +++ b/lustre/llite/rw.c >>> @@ -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); >>> } >>> -- >>> 1.7.1.1 >>> >> > > -- > MarkCheers, Andreas -- Andreas Dilger Principal Engineer Whamcloud, Inc.
Oracle will also require that a contributor agreement is submitted - see http://wiki.lustre.org/index.php/Contribution_Policy On 11-04-27 2:15 PM, Andreas Dilger wrote:> <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).
On Wed, 27 Apr 2011, Andreas Dilger wrote: [...]> 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 -- Mark
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 On 11-07-19 3:41 AM, Mark Hills wrote:> On Wed, 27 Apr 2011, Andreas Dilger wrote: > > [...] >> 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 >-- Peter Jones Whamcloud, Inc. www.whamcloud.com
Richard Henwood
2011-Jul-19 14:31 UTC
[Lustre-devel] [PATCH] Fix task IO accounting on reads
On Tue, Apr 26, 2011 at 11:06 AM, Mark Hills <Mark.Hills at framestore.com> wrote:> 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, -- Richard.Henwood at whamcloud.com Whamcloud Inc. tel: +1 512 410 9612
On Tue, 19 Jul 2011, Richard Henwood wrote:> On Tue, Apr 26, 2011 at 11:06 AM, Mark Hills <Mark.Hills at framestore.com> wrote: > > 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-333I 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. -- Mark
Richard Henwood
2011-Jul-19 17:03 UTC
[Lustre-devel] [PATCH] Fix task IO accounting on reads
On Tue, Jul 19, 2011 at 11:08 AM, Mark Hills <Mark.Hills at framestore.com> wrote: <snip>> > 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, -- Richard.Henwood at whamcloud.com Whamcloud Inc. tel: +1 512 410 9612