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