[Lustre-devel] [PATCH] Fix task IO accounting on reads

[Lustre-devel] [PATCH] Fix task IO accounting on reads

[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).

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

[Lustre-devel] [PATCH] Fix task IO accounting on reads

[Andreas Dilger]

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.

[Lustre-devel] [PATCH] Fix task IO accounting on reads

[Mark Hills]

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

[Lustre-devel] [PATCH] Fix task IO accounting on reads

[Peter Kjellström]

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>

[Lustre-devel] [PATCH] Fix task IO accounting on reads

[Andreas Dilger]

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.

[Lustre-devel] [PATCH] Fix task IO accounting on reads

[Mark Hills]

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

[Lustre-devel] [PATCH] Fix task IO accounting on reads

[Andreas Dilger]

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.

[Lustre-devel] [PATCH] Fix task IO accounting on reads

[Peter Jones]

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).

[Lustre-devel] [PATCH] Fix task IO accounting on reads

[Mark Hills]

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

[Lustre-devel] [PATCH] Fix task IO accounting on reads

[Peter Jones]

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

[Lustre-devel] [PATCH] Fix task IO accounting on reads

[Richard Henwood]

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

[Lustre-devel] [PATCH] Fix task IO accounting on reads

[Mark Hills]

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

[Lustre-devel] [PATCH] Fix task IO accounting on reads

[Richard Henwood]

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