staging/lustre: use rcu_dereference to access rcu protected current->real_parent field
PATCH 1/1 staging/lustre: use rcu_dereference to access rcu protected current->real_parent field
--- drivers/staging/lustre/lustre/llite/lproc_llite.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/staging/lustre/lustre/llite/lproc_llite.c +2 -1
@@ -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 ==
Signature · 1 line
1.8.1.2
Greg Kroah-Hartman
maintainerQuoted 3 lines · Evgeny Budilovsky
Signed-off-by Evgeny Budilovsky <[email protected]>
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
Quoted 10 lines · Greg Kroah-Hartman
Quoted 3 lines · Evgeny Budilovsky
Signed-off-by Evgeny Budilovsky <[email protected]>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.
Quoted 2 lines
greg k-h
Signature · 2 lines
Best Regards, Evgeny
Greg Kroah-Hartman
maintainerQuoted 23 lines · Evgeny Budilovsky
Quoted 10 lines · Greg Kroah-Hartman
Quoted 3 lines · Evgeny Budilovsky
Signed-off-by Evgeny Budilovsky <[email protected]>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
ondrivers/stagingby the sparse utility.For this particular case the warning was
staging/lustre/lustre/llite/lproc_llite.c:913:51: warning: dereference of noderef expression
Quoted 7 lines
And this is since
current->real_parentis accessed directly and not trough thercu_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
Hello!
Quoted 6 lines · Greg Kroah-Hartman
Quoted 4 lines
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
Greg Kroah-Hartman
maintainerQuoted 4 lines · Oleg Drokin
Hello!
Quoted 1 line · Greg Kroah-Hartman
Quoted 1 line
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
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?
Quoted 1 line
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
Quoted 8 lines · Greg Kroah-Hartman
Quoted 7 lines · Oleg Drokin
Hello!
Quoted 4 lines · Greg Kroah-Hartman
Quoted 4 lines
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.
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 3 lines
Quoted 1 line
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
Greg Kroah-Hartman
maintainerQuoted 55 lines · Oleg Drokin
Quoted 8 lines · Greg Kroah-Hartman
Quoted 7 lines · Oleg Drokin
Hello!
Quoted 4 lines · Greg Kroah-Hartman
Quoted 4 lines
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.Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 3 lines
Quoted 1 line
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]
Quoted 6 lines
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
Quoted 3 lines
Bye,
Oleg
Quoted 60 lines · Greg Kroah-Hartman
Quoted 59 lines · Oleg Drokin
Quoted 8 lines · Greg Kroah-Hartman
Quoted 7 lines · Oleg Drokin
Hello!
Quoted 4 lines · Greg Kroah-Hartman
Quoted 4 lines
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.Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 3 lines
Quoted 1 line
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.
Quoted 9 lines
Quoted 2 lines
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
Quoted 8 lines
thanks,
greg k-h
dGQuoted 3 lines
Bye,
Oleg
Greg Kroah-Hartman
maintainerQuoted 96 lines · Peng Tao
Quoted 60 lines · Greg Kroah-Hartman
Quoted 59 lines · Oleg Drokin
Quoted 8 lines · Greg Kroah-Hartman
Quoted 7 lines · Oleg Drokin
Hello!
Quoted 4 lines · Greg Kroah-Hartman
Quoted 4 lines
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.Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 3 lines
Quoted 1 line
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.Quoted 9 lines
Quoted 2 lines
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/pnfsdwith fstype nfs4 statvers=1.1opts: 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
Quoted 3 lines
<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
Quoted 5 lines
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
Greg Kroah-Hartman
maintainerQuoted 7 lines · Oleg Drokin
Quoted 5 lines
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
Quoted 7 lines · Greg Kroah-Hartman
Quoted 6 lines · Oleg Drokin
Quoted 5 lines
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.
Quoted 8 lines
Quoted 1 line
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
Greg Kroah-Hartman
maintainerQuoted 19 lines · Oleg Drokin
Quoted 7 lines · Greg Kroah-Hartman
Quoted 6 lines · Oleg Drokin
Quoted 5 lines
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.
Quoted 8 lines
Quoted 1 line
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.)
Quoted 3 lines
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.
Quoted 2 lines
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.
Quoted 4 lines
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