[PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[Evgeny Budilovsky]

Signed-off-by: Evgeny Budilovsky <budevg@gmail.com>
---
 drivers/staging/lustre/lustre/llite/lproc_llite.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/llite/lproc_llite.c b/drivers/staging/lustre/lustre/llite/lproc_llite.c
index 77f68b5..7e61468 100644
--- a/drivers/staging/lustre/lustre/llite/lproc_llite.c
+++ b/drivers/staging/lustre/lustre/llite/lproc_llite.c
@@ -910,7 +910,8 @@ void ll_stats_ops_tally(struct ll_sb_info *sbi, int op, int count)
 		 sbi->ll_stats_track_id == current->pid)
 		lprocfs_counter_add(sbi->ll_stats, op, count);
 	else if (sbi->ll_stats_track_type == STATS_TRACK_PPID &&
-		 sbi->ll_stats_track_id == current->real_parent->pid)
+		 sbi->ll_stats_track_id ==
+			rcu_dereference(current->real_parent)->pid)
 		lprocfs_counter_add(sbi->ll_stats, op, count);
 	else if (sbi->ll_stats_track_type == STATS_TRACK_GID &&
 		 sbi->ll_stats_track_id ==
-- 
1.8.1.2

Re: [PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[Greg Kroah-Hartman]

On Wed, Aug 06, 2014 at 09:22:43PM +0300, Evgeny Budilovsky wrote:
> 
> 
> Signed-off-by: Evgeny Budilovsky <budevg@gmail.com>

Why is this needed?  Is the current code a bug?  Where was the reference
added?  Is this causing a problem without this patch applied?  How far
back should it be backported, if at all?

I need lots more details here before I can take this patch, sorry.

greg k-h

Re: [PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[Evgeny Budilovsky]

On Thu, Aug 7, 2014 at 12:42 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Aug 06, 2014 at 09:22:43PM +0300, Evgeny Budilovsky wrote:
>>
>>
>> Signed-off-by: Evgeny Budilovsky <budevg@gmail.com>
>
> Why is this needed?  Is the current code a bug?  Where was the reference
> added?  Is this causing a problem without this patch applied?  How far
> back should it be backported, if at all?
>
> I need lots more details here before I can take this patch, sorry.

Sorry for the little information in the previous mail.

The motivation for this patch was to clean some of the warnings that
were generated
on drivers/staging by the sparse utility.

For this particular case the warning was
staging/lustre/lustre/llite/lproc_llite.c:913:51: warning: dereference
of noderef expression

And this is since current->real_parent is accessed directly and not
trough the rcu_dereference,
which is the common way to access it throughout the kernel.

This is not a critical bug and in the worst case the code here may
cause miss of statistics counter increase.
This is why I think it is not worth to backport the patch at all.

>
> greg k-h



-- 
Best Regards,
Evgeny

Re: [PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[Greg Kroah-Hartman]

On Thu, Aug 07, 2014 at 02:13:50PM +0300, Evgeny Budilovsky wrote:
> On Thu, Aug 7, 2014 at 12:42 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, Aug 06, 2014 at 09:22:43PM +0300, Evgeny Budilovsky wrote:
> >>
> >>
> >> Signed-off-by: Evgeny Budilovsky <budevg@gmail.com>
> >
> > Why is this needed?  Is the current code a bug?  Where was the reference
> > added?  Is this causing a problem without this patch applied?  How far
> > back should it be backported, if at all?
> >
> > I need lots more details here before I can take this patch, sorry.
> 
> Sorry for the little information in the previous mail.
> 
> The motivation for this patch was to clean some of the warnings that
> were generated
> on drivers/staging by the sparse utility.
> 
> For this particular case the warning was
> staging/lustre/lustre/llite/lproc_llite.c:913:51: warning: dereference
> of noderef expression
> 
> And this is since current->real_parent is accessed directly and not
> trough the rcu_dereference,
> which is the common way to access it throughout the kernel.
> 
> This is not a critical bug and in the worst case the code here may
> cause miss of statistics counter increase.
> This is why I think it is not worth to backport the patch at all.

You are right, and if this is just for some random "statistics" file,
can we just delete the whole function?

thanks,

greg k-h

Re: [PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[Oleg Drokin]

Hello!

On Aug 7, 2014, at 11:49 PM, Greg Kroah-Hartman wrote:
>> 
>> This is not a critical bug and in the worst case the code here may
>> cause miss of statistics counter increase.
>> This is why I think it is not worth to backport the patch at all.
> You are right, and if this is just for some random "statistics" file,
> can we just delete the whole function?

I hope not!
This is used all around the client to tally up various operations executed counts.

The statistic is then used by various userspace monitoring tools.

Bye,
    Oleg

Re: [PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[Greg Kroah-Hartman]

On Fri, Aug 08, 2014 at 12:03:20AM -0400, Oleg Drokin wrote:
> Hello!
> 
> On Aug 7, 2014, at 11:49 PM, Greg Kroah-Hartman wrote:
> >> 
> >> This is not a critical bug and in the worst case the code here may
> >> cause miss of statistics counter increase.
> >> This is why I think it is not worth to backport the patch at all.
> > You are right, and if this is just for some random "statistics" file,
> > can we just delete the whole function?
> 
> I hope not!
> This is used all around the client to tally up various operations executed counts.

Why would you do that?  Why would they care?

> The statistic is then used by various userspace monitoring tools.

Why not use the in-kernel monitoring tools instead of creating your own?
What does userspace do with that information?

thanks,

greg k-h

Re: [PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[Oleg Drokin]

On Aug 8, 2014, at 12:42 AM, Greg Kroah-Hartman wrote:

> On Fri, Aug 08, 2014 at 12:03:20AM -0400, Oleg Drokin wrote:
>> Hello!
>> 
>> On Aug 7, 2014, at 11:49 PM, Greg Kroah-Hartman wrote:
>>>> 
>>>> This is not a critical bug and in the worst case the code here may
>>>> cause miss of statistics counter increase.
>>>> This is why I think it is not worth to backport the patch at all.
>>> You are right, and if this is just for some random "statistics" file,
>>> can we just delete the whole function?
>> 
>> I hope not!
>> This is used all around the client to tally up various operations executed counts.
> Why would you do that?  Why would they care?

We would do that to provide information on the client operations performed.
They would care because they are interested in what particular clients might be doing.

>> The statistic is then used by various userspace monitoring tools.
> Why not use the in-kernel monitoring tools instead of creating your own?
> What does userspace do with that information?

We don't really control the userspace tools. People write tools to suit their needs
to monitor loads, see odd things the end users are doing or possibly for some
debugging even.
Correlating these numbers with what server sees also proves useful at times
(write combining for example).

Here's a sample of output of a recently mounted client that I poked on a bit (the lines starting with # are my comments):
# cat /proc/fs/lustre/llite/lustre-ffff88008dde27f0/stats
snapshot_time             1407473168.466102 secs.usecs
read_bytes                1 samples [bytes] 0 0 0
write_bytes               4 samples [bytes] 2 7 19
osc_write                 4 samples [bytes] 2 7 19
# The bytes counts show you minimum, maximum of writes seen and total number of bytes read-written.
# Lustre (and many other network filesystems) is very sensitive to small IO, esp. reads so it's good
# to know if you have a lot of it.
open                      6 samples [regs]
# The "regs" type just shows you how many of given type operations were performed since last statistic reset.
# Frequently that allows people to guess where does high load come from on a particular client when
# it's otherwise not obvious because not a lot of cpu is used.
# Some operations are heavier than others too.
close                     6 samples [regs]
readdir                   4 samples [regs]
setattr                   1 samples [regs]
truncate                  4 samples [regs]
getattr                   7 samples [regs]
create                    1 samples [regs]
alloc_inode               1 samples [regs]
getxattr                  8 samples [regs]
inode_permission          28 samples [regs]

As more operations types are seen the list grows.
Then there are also specific stats for readahead (data and metadata) so that interested people can make informed
decisions on the tuning there should they be unsatisfied with default settings.

I am not sure there's a similar mechanism in the kernel already that would allow us to get this sort of data easily
all in one place?

Bye,
    Oleg

Re: [PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[Greg Kroah-Hartman]

On Fri, Aug 08, 2014 at 01:06:15AM -0400, Oleg Drokin wrote:
> 
> On Aug 8, 2014, at 12:42 AM, Greg Kroah-Hartman wrote:
> 
> > On Fri, Aug 08, 2014 at 12:03:20AM -0400, Oleg Drokin wrote:
> >> Hello!
> >> 
> >> On Aug 7, 2014, at 11:49 PM, Greg Kroah-Hartman wrote:
> >>>> 
> >>>> This is not a critical bug and in the worst case the code here may
> >>>> cause miss of statistics counter increase.
> >>>> This is why I think it is not worth to backport the patch at all.
> >>> You are right, and if this is just for some random "statistics" file,
> >>> can we just delete the whole function?
> >> 
> >> I hope not!
> >> This is used all around the client to tally up various operations executed counts.
> > Why would you do that?  Why would they care?
> 
> We would do that to provide information on the client operations performed.
> They would care because they are interested in what particular clients might be doing.
> 
> >> The statistic is then used by various userspace monitoring tools.
> > Why not use the in-kernel monitoring tools instead of creating your own?
> > What does userspace do with that information?
> 
> We don't really control the userspace tools. People write tools to suit their needs
> to monitor loads, see odd things the end users are doing or possibly for some
> debugging even.
> Correlating these numbers with what server sees also proves useful at times
> (write combining for example).
> 
> Here's a sample of output of a recently mounted client that I poked on a bit (the lines starting with # are my comments):
> # cat /proc/fs/lustre/llite/lustre-ffff88008dde27f0/stats
> snapshot_time             1407473168.466102 secs.usecs
> read_bytes                1 samples [bytes] 0 0 0
> write_bytes               4 samples [bytes] 2 7 19
> osc_write                 4 samples [bytes] 2 7 19
> # The bytes counts show you minimum, maximum of writes seen and total number of bytes read-written.
> # Lustre (and many other network filesystems) is very sensitive to small IO, esp. reads so it's good
> # to know if you have a lot of it.
> open                      6 samples [regs]
> # The "regs" type just shows you how many of given type operations were performed since last statistic reset.
> # Frequently that allows people to guess where does high load come from on a particular client when
> # it's otherwise not obvious because not a lot of cpu is used.
> # Some operations are heavier than others too.
> close                     6 samples [regs]
> readdir                   4 samples [regs]
> setattr                   1 samples [regs]
> truncate                  4 samples [regs]
> getattr                   7 samples [regs]
> create                    1 samples [regs]
> alloc_inode               1 samples [regs]
> getxattr                  8 samples [regs]
> inode_permission          28 samples [regs]
> 
> As more operations types are seen the list grows.
> Then there are also specific stats for readahead (data and metadata) so that interested people can make informed
> decisions on the tuning there should they be unsatisfied with default settings.
> 
> I am not sure there's a similar mechanism in the kernel already that
> would allow us to get this sort of data easily all in one place?

perf should show you this, if not, please add the functionality there.
A filesystem is not the place to have performance monitoring code, this
needs to be removed before it can be moved out of staging.  Please work
with the trace/perf developers on this if there is something lacking
there.

thanks,

greg k-h
dG

> 
> Bye,
>     Oleg

Re: [PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[Peng Tao]

On Fri, Aug 8, 2014 at 1:32 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Aug 08, 2014 at 01:06:15AM -0400, Oleg Drokin wrote:
>>
>> On Aug 8, 2014, at 12:42 AM, Greg Kroah-Hartman wrote:
>>
>> > On Fri, Aug 08, 2014 at 12:03:20AM -0400, Oleg Drokin wrote:
>> >> Hello!
>> >>
>> >> On Aug 7, 2014, at 11:49 PM, Greg Kroah-Hartman wrote:
>> >>>>
>> >>>> This is not a critical bug and in the worst case the code here may
>> >>>> cause miss of statistics counter increase.
>> >>>> This is why I think it is not worth to backport the patch at all.
>> >>> You are right, and if this is just for some random "statistics" file,
>> >>> can we just delete the whole function?
>> >>
>> >> I hope not!
>> >> This is used all around the client to tally up various operations executed counts.
>> > Why would you do that?  Why would they care?
>>
>> We would do that to provide information on the client operations performed.
>> They would care because they are interested in what particular clients might be doing.
>>
>> >> The statistic is then used by various userspace monitoring tools.
>> > Why not use the in-kernel monitoring tools instead of creating your own?
>> > What does userspace do with that information?
>>
>> We don't really control the userspace tools. People write tools to suit their needs
>> to monitor loads, see odd things the end users are doing or possibly for some
>> debugging even.
>> Correlating these numbers with what server sees also proves useful at times
>> (write combining for example).
>>
>> Here's a sample of output of a recently mounted client that I poked on a bit (the lines starting with # are my comments):
>> # cat /proc/fs/lustre/llite/lustre-ffff88008dde27f0/stats
>> snapshot_time             1407473168.466102 secs.usecs
>> read_bytes                1 samples [bytes] 0 0 0
>> write_bytes               4 samples [bytes] 2 7 19
>> osc_write                 4 samples [bytes] 2 7 19
>> # The bytes counts show you minimum, maximum of writes seen and total number of bytes read-written.
>> # Lustre (and many other network filesystems) is very sensitive to small IO, esp. reads so it's good
>> # to know if you have a lot of it.
>> open                      6 samples [regs]
>> # The "regs" type just shows you how many of given type operations were performed since last statistic reset.
>> # Frequently that allows people to guess where does high load come from on a particular client when
>> # it's otherwise not obvious because not a lot of cpu is used.
>> # Some operations are heavier than others too.
>> close                     6 samples [regs]
>> readdir                   4 samples [regs]
>> setattr                   1 samples [regs]
>> truncate                  4 samples [regs]
>> getattr                   7 samples [regs]
>> create                    1 samples [regs]
>> alloc_inode               1 samples [regs]
>> getxattr                  8 samples [regs]
>> inode_permission          28 samples [regs]
>>
>> As more operations types are seen the list grows.
>> Then there are also specific stats for readahead (data and metadata) so that interested people can make informed
>> decisions on the tuning there should they be unsatisfied with default settings.
>>
>> I am not sure there's a similar mechanism in the kernel already that
>> would allow us to get this sort of data easily all in one place?
>
> perf should show you this, if not, please add the functionality there.
> A filesystem is not the place to have performance monitoring code, this
> needs to be removed before it can be moved out of staging.  Please work
> with the trace/perf developers on this if there is something lacking
> there.
>
nfs and nfsd track rpc ops statistics and export them via
/proc/self/mountstats, e.g.,

device 192.168.214.141:/d9691564-432b-11e2-8e5d-8b7acf882df3 mounted
on /mnt/pnfsd with fstype nfs4 statvers=1.1
        opts: rw,vers=4.1,rsize=262144,wsize=262144,namlen=255,acregmin=3,acregmax=60,acdirmin=30,acdirmax=60,hard,proto=tcp,port=0,timeo=600,retrans=2,sec=sys,clientaddr=192.168.214.128,local_lock=none
        age:    15426
        impl_id:        name='',domain='',date='0,0'
        caps:   caps=0x3ffff,wtmult=512,dtsize=32768,bsize=0,namlen=255
        nfsv4:  bm0=0xfdffbfff,bm1=0x40f9be3e,bm2=0x803,acl=0x3,sessions
        sec:    flavor=1,pseudoflavor=1
        events: 82474 12159573 9527 109202 7574 10119 16289648 3634869
10938 108551 2084272 182492 13646 7700 52594 60832 8829 48985 0 6564
1459053 66 0 0 0 289315 376376
        bytes:  11526471786 9942294760 3280371712 3278274560
14578366831 11710126268 2782400 2084272
        RPC iostats version: 1.0  p/v: 100003/4 (nfs)
        xprt:   tcp 859 0 2 0 12 408031 407999 29 2169734 0 32 2496 310753
        per-op statistics
                NULL: 0 0 0 0 0 0 0 0
                READ: 289327 289326 0 35877640 14615129136 63609 1800007 1893161
               WRITE: 376352 376360 0 11759732976 51184768 6698277
2246445 8978314
              COMMIT: 3076 3076 0 381424 393728 1827 15450 17329
                OPEN: 24926 24926 0 7329252 8968144 1373312 1794621 3169378
<snip...>

Why Lustre cannot do similar things?

Thanks,
Tao

> thanks,
>
> greg k-h
> dG
>
>>
>> Bye,
>>     Oleg

Re: [PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[Greg Kroah-Hartman]

On Sat, Aug 09, 2014 at 07:05:46PM +0800, Peng Tao wrote:
> On Fri, Aug 8, 2014 at 1:32 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Fri, Aug 08, 2014 at 01:06:15AM -0400, Oleg Drokin wrote:
> >>
> >> On Aug 8, 2014, at 12:42 AM, Greg Kroah-Hartman wrote:
> >>
> >> > On Fri, Aug 08, 2014 at 12:03:20AM -0400, Oleg Drokin wrote:
> >> >> Hello!
> >> >>
> >> >> On Aug 7, 2014, at 11:49 PM, Greg Kroah-Hartman wrote:
> >> >>>>
> >> >>>> This is not a critical bug and in the worst case the code here may
> >> >>>> cause miss of statistics counter increase.
> >> >>>> This is why I think it is not worth to backport the patch at all.
> >> >>> You are right, and if this is just for some random "statistics" file,
> >> >>> can we just delete the whole function?
> >> >>
> >> >> I hope not!
> >> >> This is used all around the client to tally up various operations executed counts.
> >> > Why would you do that?  Why would they care?
> >>
> >> We would do that to provide information on the client operations performed.
> >> They would care because they are interested in what particular clients might be doing.
> >>
> >> >> The statistic is then used by various userspace monitoring tools.
> >> > Why not use the in-kernel monitoring tools instead of creating your own?
> >> > What does userspace do with that information?
> >>
> >> We don't really control the userspace tools. People write tools to suit their needs
> >> to monitor loads, see odd things the end users are doing or possibly for some
> >> debugging even.
> >> Correlating these numbers with what server sees also proves useful at times
> >> (write combining for example).
> >>
> >> Here's a sample of output of a recently mounted client that I poked on a bit (the lines starting with # are my comments):
> >> # cat /proc/fs/lustre/llite/lustre-ffff88008dde27f0/stats
> >> snapshot_time             1407473168.466102 secs.usecs
> >> read_bytes                1 samples [bytes] 0 0 0
> >> write_bytes               4 samples [bytes] 2 7 19
> >> osc_write                 4 samples [bytes] 2 7 19
> >> # The bytes counts show you minimum, maximum of writes seen and total number of bytes read-written.
> >> # Lustre (and many other network filesystems) is very sensitive to small IO, esp. reads so it's good
> >> # to know if you have a lot of it.
> >> open                      6 samples [regs]
> >> # The "regs" type just shows you how many of given type operations were performed since last statistic reset.
> >> # Frequently that allows people to guess where does high load come from on a particular client when
> >> # it's otherwise not obvious because not a lot of cpu is used.
> >> # Some operations are heavier than others too.
> >> close                     6 samples [regs]
> >> readdir                   4 samples [regs]
> >> setattr                   1 samples [regs]
> >> truncate                  4 samples [regs]
> >> getattr                   7 samples [regs]
> >> create                    1 samples [regs]
> >> alloc_inode               1 samples [regs]
> >> getxattr                  8 samples [regs]
> >> inode_permission          28 samples [regs]
> >>
> >> As more operations types are seen the list grows.
> >> Then there are also specific stats for readahead (data and metadata) so that interested people can make informed
> >> decisions on the tuning there should they be unsatisfied with default settings.
> >>
> >> I am not sure there's a similar mechanism in the kernel already that
> >> would allow us to get this sort of data easily all in one place?
> >
> > perf should show you this, if not, please add the functionality there.
> > A filesystem is not the place to have performance monitoring code, this
> > needs to be removed before it can be moved out of staging.  Please work
> > with the trace/perf developers on this if there is something lacking
> > there.
> >
> nfs and nfsd track rpc ops statistics and export them via
> /proc/self/mountstats, e.g.,
> 
> device 192.168.214.141:/d9691564-432b-11e2-8e5d-8b7acf882df3 mounted
> on /mnt/pnfsd with fstype nfs4 statvers=1.1
>         opts: rw,vers=4.1,rsize=262144,wsize=262144,namlen=255,acregmin=3,acregmax=60,acdirmin=30,acdirmax=60,hard,proto=tcp,port=0,timeo=600,retrans=2,sec=sys,clientaddr=192.168.214.128,local_lock=none
>         age:    15426
>         impl_id:        name='',domain='',date='0,0'
>         caps:   caps=0x3ffff,wtmult=512,dtsize=32768,bsize=0,namlen=255
>         nfsv4:  bm0=0xfdffbfff,bm1=0x40f9be3e,bm2=0x803,acl=0x3,sessions
>         sec:    flavor=1,pseudoflavor=1
>         events: 82474 12159573 9527 109202 7574 10119 16289648 3634869
> 10938 108551 2084272 182492 13646 7700 52594 60832 8829 48985 0 6564
> 1459053 66 0 0 0 289315 376376
>         bytes:  11526471786 9942294760 3280371712 3278274560
> 14578366831 11710126268 2782400 2084272
>         RPC iostats version: 1.0  p/v: 100003/4 (nfs)
>         xprt:   tcp 859 0 2 0 12 408031 407999 29 2169734 0 32 2496 310753
>         per-op statistics
>                 NULL: 0 0 0 0 0 0 0 0
>                 READ: 289327 289326 0 35877640 14615129136 63609 1800007 1893161
>                WRITE: 376352 376360 0 11759732976 51184768 6698277
> 2246445 8978314
>               COMMIT: 3076 3076 0 381424 393728 1827 15450 17329
>                 OPEN: 24926 24926 0 7329252 8968144 1373312 1794621 3169378
> <snip...>
> 
> Why Lustre cannot do similar things?

Because maybe these stats preceed the introduction of perf and other
tracing/debug tools?  I don't know, it's really low down on the list of
reasons why lustre can't be merged out of staging at the moment, you all
have much bigger issues to address first.

thanks,

greg k-h

Re: [PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[Oleg Drokin]

> Because maybe these stats preceed the introduction of perf and other
> tracing/debug tools?  I don't know, it's really low down on the list of
> reasons why lustre can't be merged out of staging at the moment, you all
> have much bigger issues to address first.

I wonder what is the prioritized list you have in mind?

Bye,
    Oleg

Re: [PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[Greg Kroah-Hartman]

On Sat, Aug 09, 2014 at 10:34:36AM -0400, Oleg Drokin wrote:
> 
> > Because maybe these stats preceed the introduction of perf and other
> > tracing/debug tools?  I don't know, it's really low down on the list of
> > reasons why lustre can't be merged out of staging at the moment, you all
> > have much bigger issues to address first.
> 
> I wonder what is the prioritized list you have in mind?

Do I really have to spell out what is wrong with that codebase that
needs to be fixed up?  It took me 50+ patches for 3.17-rc1 to just fix
up a _portion_ of the include file mess that is in there.  Now is the
first time the code actually _builds_ properly in the kernel tree (i.e.
no magic header file include path modifications in Makefiles preventing
individual files from being built correctly.)

If you all don't know what needs to be done here, then I might as well
just delete the drivers/staging/lustre/ tree right now.

greg k-h

Re: [PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[Oleg Drokin]

On Aug 9, 2014, at 11:47 AM, Greg Kroah-Hartman wrote:

> On Sat, Aug 09, 2014 at 10:34:36AM -0400, Oleg Drokin wrote:
>> 
>>> Because maybe these stats preceed the introduction of perf and other
>>> tracing/debug tools?  I don't know, it's really low down on the list of
>>> reasons why lustre can't be merged out of staging at the moment, you all
>>> have much bigger issues to address first.
>> 
>> I wonder what is the prioritized list you have in mind?
> 
> Do I really have to spell out what is wrong with that codebase that
> needs to be fixed up?  It took me 50+ patches for 3.17-rc1 to just fix
> up a _portion_ of the include file mess that is in there.  Now is the
> first time the code actually _builds_ properly in the kernel tree (i.e.
> no magic header file include path modifications in Makefiles preventing
> individual files from being built correctly.)

Well, last time we discussed this topic, you said that moving most of lustre proc files
into sysfs and other suitable venues is what prevents moving lustre out of the
staging tree under fs/

There well might be include mess, but this is the first time I hear about it, and I have not seen
any build breakage or other obviously broken behavior.
I am sure there are more things too, that's why I am asking.
We are trying to deal with stuff we know about, but it's a bit harder to deal with the stuff we don't know about
that does not manifest itself in some clear way.

Bye,
    Oleg

Re: [PATCH] staging/lustre: use rcu_dereference to access rcu protected current->real_parent field

[Greg Kroah-Hartman]

On Mon, Aug 11, 2014 at 09:44:47PM -0400, Oleg Drokin wrote:
> 
> On Aug 9, 2014, at 11:47 AM, Greg Kroah-Hartman wrote:
> 
> > On Sat, Aug 09, 2014 at 10:34:36AM -0400, Oleg Drokin wrote:
> >> 
> >>> Because maybe these stats preceed the introduction of perf and other
> >>> tracing/debug tools?  I don't know, it's really low down on the list of
> >>> reasons why lustre can't be merged out of staging at the moment, you all
> >>> have much bigger issues to address first.
> >> 
> >> I wonder what is the prioritized list you have in mind?
> > 
> > Do I really have to spell out what is wrong with that codebase that
> > needs to be fixed up?  It took me 50+ patches for 3.17-rc1 to just fix
> > up a _portion_ of the include file mess that is in there.  Now is the
> > first time the code actually _builds_ properly in the kernel tree (i.e.
> > no magic header file include path modifications in Makefiles preventing
> > individual files from being built correctly.)
> 
> Well, last time we discussed this topic, you said that moving most of lustre proc files
> into sysfs and other suitable venues is what prevents moving lustre out of the
> staging tree under fs/

Yes, that's one of the big things, but the structure of the code itself
still needs lots of work.  You have wrapper functions and defines that
are not needed.  You have version-specific checks and still a quite
complex and unnecessary include directory structure.

> There well might be include mess, but this is the first time I hear about it, and I have not seen
> any build breakage or other obviously broken behavior.

The build breakage came about in a thread on this mailing list when we
had a tool that could build an individual .o file, which I found to
break on all of the lustre files due to the odd way you were redefining
the include search path of the .c files.  I unwound all of that to use
direct paths and now things build properly.  But even then, there are
way too many .h files and nested mess that is not needed and is the
result of trying to get this code to build on other platforms a long
time ago.

> I am sure there are more things too, that's why I am asking.
> We are trying to deal with stuff we know about, but it's a bit harder
> to deal with the stuff we don't know about that does not manifest
> itself in some clear way.

Try getting rid of the typedefs and wrapper functions and #defines for
function names and coding style issues.  That would be a great first
step and would then expose what really is left to do.  Right now, it's
just to hard to see the real issues.

thanks,

greg k-h