* [Lustre-devel] [PATCH] Fix task IO accounting on reads @ 2011-04-26 16:06 Mark Hills 2011-04-26 16:38 ` Andreas Dilger 2011-07-19 14:31 ` Richard Henwood 0 siblings, 2 replies; 13+ messages in thread From: Mark Hills @ 2011-04-26 16:06 UTC (permalink / raw) To: lustre-devel 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Lustre-devel] [PATCH] Fix task IO accounting on reads 2011-04-26 16:06 [Lustre-devel] [PATCH] Fix task IO accounting on reads Mark Hills @ 2011-04-26 16:38 ` Andreas Dilger 2011-04-27 11:23 ` Mark Hills 2011-07-19 14:31 ` Richard Henwood 1 sibling, 1 reply; 13+ messages in thread From: Andreas Dilger @ 2011-04-26 16:38 UTC (permalink / raw) To: lustre-devel 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-devel Cheers, Andreas -- Andreas Dilger Principal Engineer Whamcloud, Inc. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Lustre-devel] [PATCH] Fix task IO accounting on reads 2011-04-26 16:38 ` Andreas Dilger @ 2011-04-27 11:23 ` Mark Hills 2011-04-27 15:56 ` Peter Kjellström 2011-04-27 16:43 ` Andreas Dilger 0 siblings, 2 replies; 13+ messages in thread From: Mark Hills @ 2011-04-27 11:23 UTC (permalink / raw) To: lustre-devel 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. 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Lustre-devel] [PATCH] Fix task IO accounting on reads 2011-04-27 11:23 ` Mark Hills @ 2011-04-27 15:56 ` Peter Kjellström 2011-04-27 16:43 ` Andreas Dilger 1 sibling, 0 replies; 13+ messages in thread From: Peter Kjellström @ 2011-04-27 15:56 UTC (permalink / raw) To: lustre-devel 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-lustre.org/attachments/20110427/1e720af5/attachment.pgp> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Lustre-devel] [PATCH] Fix task IO accounting on reads 2011-04-27 11:23 ` Mark Hills 2011-04-27 15:56 ` Peter Kjellström @ 2011-04-27 16:43 ` Andreas Dilger 2011-04-27 17:14 ` Mark Hills 1 sibling, 1 reply; 13+ messages in thread From: Andreas Dilger @ 2011-04-27 16:43 UTC (permalink / raw) To: lustre-devel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Lustre-devel] [PATCH] Fix task IO accounting on reads 2011-04-27 16:43 ` Andreas Dilger @ 2011-04-27 17:14 ` Mark Hills 2011-04-27 21:15 ` Andreas Dilger 0 siblings, 1 reply; 13+ messages in thread From: Mark Hills @ 2011-04-27 17:14 UTC (permalink / raw) To: lustre-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Lustre-devel] [PATCH] Fix task IO accounting on reads 2011-04-27 17:14 ` Mark Hills @ 2011-04-27 21:15 ` Andreas Dilger 2011-04-27 21:19 ` Peter Jones 2011-07-19 10:41 ` Mark Hills 0 siblings, 2 replies; 13+ messages in thread From: Andreas Dilger @ 2011-04-27 21:15 UTC (permalink / raw) To: lustre-devel 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 >>> >> > > -- > Mark Cheers, Andreas -- Andreas Dilger Principal Engineer Whamcloud, Inc. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Lustre-devel] [PATCH] Fix task IO accounting on reads 2011-04-27 21:15 ` Andreas Dilger @ 2011-04-27 21:19 ` Peter Jones 2011-07-19 10:41 ` Mark Hills 1 sibling, 0 replies; 13+ messages in thread From: Peter Jones @ 2011-04-27 21:19 UTC (permalink / raw) To: lustre-devel 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). ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Lustre-devel] [PATCH] Fix task IO accounting on reads 2011-04-27 21:15 ` Andreas Dilger 2011-04-27 21:19 ` Peter Jones @ 2011-07-19 10:41 ` Mark Hills 2011-07-19 12:34 ` Peter Jones 1 sibling, 1 reply; 13+ messages in thread From: Mark Hills @ 2011-07-19 10:41 UTC (permalink / raw) To: lustre-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Lustre-devel] [PATCH] Fix task IO accounting on reads 2011-07-19 10:41 ` Mark Hills @ 2011-07-19 12:34 ` Peter Jones 0 siblings, 0 replies; 13+ messages in thread From: Peter Jones @ 2011-07-19 12:34 UTC (permalink / raw) To: lustre-devel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Lustre-devel] [PATCH] Fix task IO accounting on reads 2011-04-26 16:06 [Lustre-devel] [PATCH] Fix task IO accounting on reads Mark Hills 2011-04-26 16:38 ` Andreas Dilger @ 2011-07-19 14:31 ` Richard Henwood 2011-07-19 16:08 ` Mark Hills 1 sibling, 1 reply; 13+ messages in thread From: Richard Henwood @ 2011-07-19 14:31 UTC (permalink / raw) To: lustre-devel On Tue, Apr 26, 2011 at 11:06 AM, Mark Hills <Mark.Hills@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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Lustre-devel] [PATCH] Fix task IO accounting on reads 2011-07-19 14:31 ` Richard Henwood @ 2011-07-19 16:08 ` Mark Hills 2011-07-19 17:03 ` Richard Henwood 0 siblings, 1 reply; 13+ messages in thread From: Mark Hills @ 2011-07-19 16:08 UTC (permalink / raw) To: lustre-devel On Tue, 19 Jul 2011, Richard Henwood wrote: > On Tue, Apr 26, 2011 at 11:06 AM, Mark Hills <Mark.Hills@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 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. -- Mark ^ permalink raw reply [flat|nested] 13+ messages in thread
* [Lustre-devel] [PATCH] Fix task IO accounting on reads 2011-07-19 16:08 ` Mark Hills @ 2011-07-19 17:03 ` Richard Henwood 0 siblings, 0 replies; 13+ messages in thread From: Richard Henwood @ 2011-07-19 17:03 UTC (permalink / raw) To: lustre-devel On Tue, Jul 19, 2011 at 11:08 AM, Mark Hills <Mark.Hills@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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-07-19 17:03 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-04-26 16:06 [Lustre-devel] [PATCH] Fix task IO accounting on reads Mark Hills 2011-04-26 16:38 ` Andreas Dilger 2011-04-27 11:23 ` Mark Hills 2011-04-27 15:56 ` Peter Kjellström 2011-04-27 16:43 ` Andreas Dilger 2011-04-27 17:14 ` Mark Hills 2011-04-27 21:15 ` Andreas Dilger 2011-04-27 21:19 ` Peter Jones 2011-07-19 10:41 ` Mark Hills 2011-07-19 12:34 ` Peter Jones 2011-07-19 14:31 ` Richard Henwood 2011-07-19 16:08 ` Mark Hills 2011-07-19 17:03 ` Richard Henwood
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.