Fix gcc 4.6 set but unused variable warnings
Subsystems (29)
cover Fix gcc 4.6 set but unused variable warnings
gcc 4.6 -Wall gained a new set but unused variable warning.
First it was incredibly noisy, but when looking at it in detail it actually found some bugs.
I haven't tried to fix all occurrences, but
at least the major ones for my configuration.
If someone is interested more work would be possible on this.
There are still some warnings left, but no flood
and there's really usually some code improvement.
In a few cases I simply shut up the compiler, but
in many other cases dead code is gone or a real
problem has been fixed.
Some of the changes need more review (especially marked and with suitable cc), but most are straight forward.
I did not fix all bugs -- those that were too hard are just commented with the warning left in. I also marked areas that need more attention in the individual patches.
Andrew, something for your tree?
-Andi
PATCH 1/23 x86: percpu: Avoid warnings of unused variables in per cpu
Avoid hundreds of warnings with a gcc 4.6 -Wall build
--- arch/x86/include/asm/percpu.h | 2 ++ 1 file changed, 2 insertions(+)
arch/x86/include/asm/percpu.h +2 -0
@@ -77,6 +77,7 @@ do { \if (0 ) { \pto_T__ pto_tmp__; \ pto_tmp__ = (val); \ + (void )pto_tmp__; \ } \switch (sizeof (var)) { \case 1 : \
@@ -115,6 +116,7 @@ do { \if (0 ) { \pao_T__ pao_tmp__; \ pao_tmp__ = (val); \ + (void )pao_tmp__; \ } \switch (sizeof (var)) { \case 1 : \
Re: [PATCH] [1/23] x86: percpu: Avoid warnings of unused variables in per cpu
Quoted 6 lines · Andi Kleen
Avoid hundreds of warnings with a gcc 4.6 -Wall build
Signed-off-by Andi Kleen <[email protected]>
Can x86 or -mm please take this patch? There isn't any pending percpu patch currently and it seems a bit silly to start a branch for this one.
Thanks.
Signature · 1 line
tejun
Re: [PATCH] [1/23] x86: percpu: Avoid warnings of unused variables in per cpu
Quoted 6 lines · Tejun Heo
Quoted 5 lines · Andi Kleen
Avoid hundreds of warnings with a gcc 4.6 -Wall build
Quoted 7 lines
Quoted 1 line
Signed-off-by Andi Kleen <[email protected]>Acked-by Tejun Heo <[email protected]>Can x86 or -mm please take this patch? There isn't any pending percpu patch currently and it seems a bit silly to start a branch for this one.
Sure, queued it up.
Thanks,
Ingo
Re: [PATCH] [1/23] x86: percpu: Avoid warnings of unused variables in per cpu
Quoted 19 lines · Ingo Molnar
Quoted 6 lines · Tejun Heo
Quoted 5 lines · Andi Kleen
Avoid hundreds of warnings with a gcc 4.6 -Wall build
Quoted 7 lines
Quoted 1 line
Signed-off-by Andi Kleen<[email protected]>Acked-by Tejun Heo<[email protected]>Can x86 or -mm please take this patch? There isn't any pending percpu patch currently and it seems a bit silly to start a branch for this one.
Sure, queued it up.
Quoted 9 lines
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Thanks for this.. I applied it over here.. and now don't have all of that spam from percpu while compiling the kernel..
only one I see now is something about
warning: variable 'gfp' set but not used
if there's another I can test it if you would like.
Justin P. Mattock
Re: [PATCH] [1/23] x86: percpu: Avoid warnings of unused variables in per cpu
Quoted 4 lines
Thanks for this.. I applied it over here.. and now don't have all of that spam from percpu while compiling the kernel..
Which gcc version are you using?
Quoted 3 lines
only one I see now is something about
warning: variable 'gfp' set but not used
That's fixed in 'IRQ: Move alloc_desk_mask variables inside ifdef'
The whole series fixes a lot more warnings, however there are still quite some left (in my config)
-Andi
Signature · 1 line
[email protected] -- Speaking for myself only.
Re: [PATCH] [1/23] x86: percpu: Avoid warnings of unused variables in per cpu
Quoted 6 lines · Andi Kleen
Quoted 4 lines
Thanks for this.. I applied it over here.. and now don't have all of that spam from percpu while compiling the kernel..
Which gcc version are you using?
gcc version 4.6.0 20100416
Quoted 10 lines
Quoted 3 lines
only one I see now is something about
warning: variable 'gfp' set but not usedThat's fixed in 'IRQ: Move
alloc_desk_maskvariables inside ifdef'The whole series fixes a lot more warnings, however there are still quite some left (in my config)
-Andi
let me go and add all of these patches, to see
if I see any more warnings..
Justin P. Mattock
Re: [PATCH] [1/23] x86: percpu: Avoid warnings of unused variables in per cpu
In reply to Andi KleenQuoted 10 lines · Andi Kleen
Quoted 4 lines
Thanks for this.. I applied it over here.. and now don't have all of that spam from percpu while compiling the kernel..
Which gcc version are you using?
Quoted 3 lines
only one I see now is something about
warning: variable 'gfp' set but not used
Quoted 6 lines
That's fixed in 'IRQ: Move
alloc_desk_maskvariables inside ifdef'The whole series fixes a lot more warnings, however there are still quite some left (in my config)
-Andi
o.k. I applied your patches, except
patch #10 x86: percpu: Avoid warnings of unused variables
will try later on and see.
as for the warnings, I can send a seperate post for all to see, still quit a bit but lots nicer after your patch(s)
Justin P. Mattock
[tip:x86/urgent] percpu, x86: Avoid warnings of unused variables in per cpu
In reply to Tejun HeoCommit-ID: 157a9df3b0f8042f1a6a4fe9e0e494431aaca1ae Gitweb: http://git.kernel.org/tip/157a9df3b0f8042f1a6a4fe9e0e494431aaca1ae Author: Andi Kleen <[email protected]> AuthorDate: Thu, 10 Jun 2010 13:10:36 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 10 Jun 2010 14:09:30 +0200
percpu, x86: Avoid warnings of unused variables in per cpu
Avoid hundreds of warnings with a gcc 4.6 -Wall build.
LKML-Reference: <[email protected]>
--- arch/x86/include/asm/percpu.h | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
arch/x86/include/asm/percpu.h +2 -0
@@ -77,6 +77,7 @@ do { \if (0 ) { \pto_T__ pto_tmp__; \ pto_tmp__ = (val); \ + (void )pto_tmp__; \ } \switch (sizeof (var)) { \case 1 : \
@@ -115,6 +116,7 @@ do { \if (0 ) { \pao_T__ pao_tmp__; \ pao_tmp__ = (val); \ + (void )pao_tmp__; \ } \switch (sizeof (var)) { \case 1 : \
PATCH 2/23 IRQ: Move alloc_desk_mask variables inside ifdef
In reply to Andi KleenThey are only useful with CONFIG_CPUMASK_OFFSTACK
Avoids hundreds of warnings with a gcc 4.6 -Wall build.
--- include/linux/irq.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/irq.h +1 -1
@@ -439,12 +439,12 @@ extern int set_irq_msi(unsigned int irq,static inline bool alloc_desc_masks (struct irq_desc *desc,int node,bool boot) { +#ifdef CONFIG_CPUMASK_OFFSTACK gfp_t gfp =GFP_ATOMIC ;if (boot) gfp =GFP_NOWAIT ; -#ifdef CONFIG_CPUMASK_OFFSTACK if (!alloc_cpumask_var_node (&desc->affinity, gfp, node))return false;
PATCH 3/23 x86: Avoid unused by set variables in rdmsr
In reply to Andi KleenAvoids quite a lot of warnings with a gcc 4.6 -Wall build because this happens in a commonly used header file (apic.h)
--- arch/x86/include/asm/msr.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/include/asm/msr.h +2 -2
@@ -148,8 +148,8 @@ static inline unsigned long long native_#define rdmsr (msr, val1, val2) \ do { \ u64 __val = native_read_msr((msr)); \ - (val1) = (u32)__val; \ - (val2) = (u32)(__val >> 32); \ + (void)((val1) = (u32)__val); \ + (void)((val2) = (u32)(__val >> 32)); \ } while (0)static inline void wrmsr (unsigned msr,unsigned low,unsigned high)
PATCH 4/23 pagemap: Avoid unused-but-set variable
In reply to Andi KleenAvoid quite a lot of warnings in header files in a gcc 4.6 -Wall builds
--- include/linux/pagemap.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
include/linux/pagemap.h +3 -1
@@ -423,8 +423,10 @@ static inline int fault_in_pages_readablconst char __user *end = uaddr + size -1 ;if (((unsigned long )uaddr &PAGE_MASK ) != - ((unsigned long )end &PAGE_MASK )) + ((unsigned long )end &PAGE_MASK )) { ret =__get_user (c, end); + (void )c; + } }return ret; }
Andrew Morton
maintainerRe: [PATCH] [4/23] pagemap: Avoid unused-but-set variable
Quoted 15 lines · On Thu, 10 Jun 2010 13:10:39 +0200 (CEST) Andi Kleen <[email protected]>…
Avoid quite a lot of warnings in header files in a gcc 4.6 -Wall builds
Signed-off-by Andi Kleen <[email protected]>--- include/linux/pagemap.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)Index:
linux-2.6.35-rc2-gcc/include/linux/pagemap.h
===================================================================include/linux/pagemap.h +0
-0@@ -423,8 +423,10 @@ static inline int fault_in_pages_readablconst char __user *end = uaddr + size -1 ;
include/linux/pagemap.h static inline int fault_in_pages_readabl · line 432
Quoted 10 lines
include/linux/pagemap.h +3
-1if (((unsigned long )uaddr &PAGE_MASK ) != - ((unsigned long )end &PAGE_MASK )) + ((unsigned long )end &PAGE_MASK )) { ret =__get_user (c, end); + (void )c; + } }return ret; }
urgh. In fact I'd urgh the whole patchset.
Problem is, anyone who looks at all these random (void) casts is going to have a hard time working out why they're there. This is worsened by the long-standing practice wherein some people put unneeded (void) casts all over the place due to being traumatised by lint 15 years ago (I think).
Wouldn't it be better to make this stuff self-documenting, so anyone who reads the code can immediately see what it's doing, rather than scratching their heads over random, seemingly-unneeded casts?
#define gcc_46_is_a_pita(expr) ((void)(expr))
?
Re: [PATCH] [4/23] pagemap: Avoid unused-but-set variable
Quoted 3 lines
urgh. In fact I'd urgh the whole patchset.
Problem is, anyone who looks at all these random (void) casts is going
There are actually not that many in the patch kit if you read it completely. I think that's nearly the only one outside a macro.
Quoted 3 lines
to have a hard time working out why they're there. This is worsened by the long-standing practice wherein some people put unneeded (void) casts all over the place due to being traumatised by lint 15 years ago (I think).
It's really the same reason why they used this with lints.
When I tried it first I started complaining about gcc too (I even filled a gcc bug), then disabled it in my build,
but after some contemplation I found it really finds real bugs.
Quoted 6 lines
Wouldn't it be better to make this stuff self-documenting, so anyone who reads the code can immediately see what it's doing, rather than scratching their heads over random, seemingly-unneeded casts?
#define
gcc_46_is_a_pita(expr)((void)(expr))
The warning is really useful and we'll get all used to it.
So I went with the plain cast. Maybe some more comments would be useful.
-Andi
Signature · 1 line
[email protected] -- Speaking for myself only.
PATCH 5/23 x86 boot: Set ax register in boot vga query
In reply to Andi KleenI think the ax register should be input to the BIOS call, not be unused, but not fully sure. hpa?
Found by gcc 4.6's new warnings.
--- arch/x86/boot/video-vga.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
arch/x86/boot/video-vga.c +1 -2
@@ -41,13 +41,12 @@ static __videocard video_vga;static u8 vga_set_basic_mode (void ) {struct biosregs ireg, oreg; -u16 ax;u8 mode;initregs (&ireg);/* Query current mode */ - ax =0x0f00 ; + ireg.ax =0x0f00 ;intcall (0x10 , &ireg, &oreg); mode = oreg.al;
H. Peter Anvin
reviewerRe: [PATCH] [5/23] x86 boot: Set ax register in boot vga query
Quoted 2 lines · Andi Kleen
I think the ax register should be input to the BIOS call, not be unused, but not fully sure. hpa?
Quite correct. A typo during the conversion to the "glove box" interface.
Thanks, will apply!
-hpa
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
[tip:x86/urgent] x86, setup: Set ax register in boot vga query
In reply to Andi KleenCommit-ID: cf3bdc29fcbf2cb4cfa238591021d41ea8f8431f Gitweb: http://git.kernel.org/tip/cf3bdc29fcbf2cb4cfa238591021d41ea8f8431f Author: Andi Kleen <[email protected]> AuthorDate: Thu, 10 Jun 2010 13:10:40 +0200 Committer: H. Peter Anvin <[email protected]> CommitDate: Thu, 10 Jun 2010 15:24:29 -0700
x86, setup: Set ax register in boot vga query
Catch missing conversion to the register structure "glove box" scheme.
Found by gcc 4.6's new warnings.
LKML-Reference: <[email protected]>
--- arch/x86/boot/video-vga.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
arch/x86/boot/video-vga.c +1 -2
@@ -41,13 +41,12 @@ static __videocard video_vga;static u8 vga_set_basic_mode (void ) {struct biosregs ireg, oreg; -u16 ax;u8 mode;initregs (&ireg);/* Query current mode */ - ax =0x0f00 ; + ireg.ax =0x0f00 ;intcall (0x10 , &ireg, &oreg); mode = oreg.al;
PATCH 6/23 perf: Fix set but unused variables in perf
In reply to Andi KleenJust dead code I believe
--- arch/x86/kernel/cpu/perf_event.c | 5 ----- 1 file changed, 5 deletions(-)
arch/x86/kernel/cpu/perf_event.c +0 -5
@@ -953,12 +953,9 @@ static void x86_pmu_enable_event(structstatic int x86_pmu_enable (struct perf_event *event) {struct cpu_hw_events *cpuc = &__get_cpu_var (cpu_hw_events); -struct hw_perf_event *hwc;int assign[X86_PMC_IDX_MAX ];int n, n0, ret; - hwc = &event->hw; - n0 = cpuc->n_events; n =collect_events (cpuc, event, false);if (n <0 )
@@ -1122,7 +1119,6 @@ static int x86_pmu_handle_irq(struct pt_struct perf_sample_data data;struct cpu_hw_events *cpuc;struct perf_event *event; -struct hw_perf_event *hwc;int idx, handled =0 ;u64 val;
@@ -1135,7 +1131,6 @@ static int x86_pmu_handle_irq(struct pt_continue ; event = cpuc->events[idx]; - hwc = &event->hw; val =x86_perf_event_update (event);if (val & (1ULL << (x86_pmu.cntval_bits -1 )))
PATCH 7/23 x86: fix set but not read variables
In reply to Andi KleenJust some dead code, no real bugs.
Found by gcc 4.6 -Wall
--- arch/x86/kernel/aperture_64.c | 4 ++-- arch/x86/kernel/cpu/mtrr/generic.c | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-)
arch/x86/kernel/cpu/mtrr/generic.c +1 -2
@@ -433,13 +433,12 @@ static void generic_get_mtrr(unsigned in {unsigned int mask_lo, mask_hi, base_lo, base_hi;unsigned int tmp, hi; -int cpu;/* * get_mtrr doesn't need to update mtrr_state, also it could be called * from any cpu, so try to print it out directly. */ - cpu =get_cpu (); +get_cpu ();rdmsr (MTRRphysMask_MSR (reg), mask_lo, mask_hi);
arch/x86/kernel/aperture_64.c +2 -2
@@ -280,7 +280,7 @@ void __init early_gart_iommu_check(void)* or BIOS forget to put that in reserved. * try to update e820 to make that region as reserved. */ -u32 agp_aper_base =0 , agp_aper_order =0 ; +u32 agp_aper_order =0 ;int i, fix, slot, valid_agp =0 ;u32 ctl;u32 aper_size =0 , aper_order =0 , last_aper_order =0 ;
@@ -291,7 +291,7 @@ void __init early_gart_iommu_check(void)return ;/* This is mostly duplicate of iommu_hole_init */ - agp_aper_base =search_agp_bridge (&agp_aper_order, &valid_agp); +search_agp_bridge (&agp_aper_order, &valid_agp); fix =0 ;for (i =0 ; i <ARRAY_SIZE (bus_dev_ranges); i++) {
PATCH 8/23 KGDB: Remove set but unused newPC
In reply to Andi KleenI'm not fully sure this is the correct fix, maybe this was a bug and newPC should really have a side effect. Jason?
Found by gcc 4.6's new warnings
--- arch/x86/kernel/kgdb.c | 3 --- 1 file changed, 3 deletions(-)
arch/x86/kernel/kgdb.c +0 -3
@@ -458,7 +458,6 @@ int kgdb_arch_handle_exception(int e_vec {unsigned long addr;char *ptr; -int newPC;switch (remcomInBuffer[0 ]) {case 'c' :
@@ -469,8 +468,6 @@ int kgdb_arch_handle_exception(int e_vec linux_regs->ip = addr;case 'D' :case 'k' : - newPC = linux_regs->ip; -/* clear the trace bit */ linux_regs->flags &= ~X86_EFLAGS_TF ;atomic_set (&kgdb_cpu_doing_single_step,-1 );
Jason Wessel
maintainerRe: [PATCH] [8/23] KGDB: Remove set but unused newPC
Quoted 5 lines · Andi Kleen
I'm not fully sure this is the correct fix, maybe this was a bug and newPC should really have a side effect. Jason?
Andi,
This looks fine to me. The newPC got there from the merge of the 32/64
kgdb specific implementation. Definitely don't need it anymore.
I'll queue it for 2.6.36.
Thanks,
Jason.
Quoted 32 lines
Found by gcc 4.6's new warnings
Signed-off-by Andi Kleen <[email protected]>--- arch/x86/kernel/kgdb.c | 3 --- 1 file changed, 3 deletions(-)Index:
linux-2.6.35-rc2-gcc/arch/x86/kernel/kgdb.c
===================================================================arch/x86/kernel/kgdb.c +0
-3@@ -458,7 +458,6 @@ int kgdb_arch_handle_exception(int e_vec {unsigned long addr;char *ptr; -int newPC;switch (remcomInBuffer[0 ]) {case 'c' :@@ -469,8 +468,6 @@ int kgdb_arch_handle_exception(int e_vec linux_regs->ip = addr;case 'D' :case 'k' : - newPC = linux_regs->ip; -/* clear the trace bit */ linux_regs->flags &= ~X86_EFLAGS_TF ;atomic_set (&kgdb_cpu_doing_single_step,-1 );
PATCH 9/23 PRINTK: Use stable variable to dump kmsg buffer
In reply to Andi Kleenkmsg_dump takes care to sample the global variables inside a spinlock, but then goes on to use the same variables outside the spinlock region too.
Use the correct variable. This will make the race
window smaller.
Found by gcc 4.6's new warnings.
--- kernel/printk.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
kernel/printk.c +5 -5
@@ -1520,9 +1520,9 @@ void kmsg_dump(enum kmsg_dump_reason rea chars = logged_chars;spin_unlock_irqrestore (&logbuf_lock, flags); -if (logged_chars > end) { - s1 = log_buf + log_buf_len - logged_chars + end; - l1 = logged_chars - end; +if (chars > end) { + s1 = log_buf + log_buf_len - chars + end; + l1 = chars - end; s2 = log_buf; l2 = end;
@@ -1530,8 +1530,8 @@ void kmsg_dump(enum kmsg_dump_reason rea s1 ="" ; l1 =0 ; - s2 = log_buf + end - logged_chars; - l2 = logged_chars; + s2 = log_buf + end - chars; + l2 = chars; }if (!spin_trylock_irqsave (&dump_list_lock, flags)) {
PATCH 10/23 SCHED: Only allocate per cpu cpu mask buffer with offstack cpumasks
In reply to Andi KleenThis will save a few bytes in the non offstack cpumask case.
Found by gcc 4.6's new warnings.
--- kernel/sched.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
kernel/sched.c +4 -1
@@ -7482,7 +7482,7 @@ static void init_tg_rt_entry(struct taskvoid __init sched_init (void ) {int i, j; -unsigned long alloc_size =0 , ptr; +unsigned long alloc_size =0 ;#ifdef CONFIG_FAIR_GROUP_SCHED alloc_size +=2 * nr_cpu_ids *sizeof (void **);
@@ -7494,7 +7494,10 @@ void __init sched_init(void) alloc_size +=num_possible_cpus () *cpumask_size ();#endif if (alloc_size) { +#ifdef CONFIG_CPUMASK_OFFSTACK +unsigned long ptr; ptr = (unsigned long )kzalloc (alloc_size,GFP_NOWAIT ); +#endif #ifdef CONFIG_FAIR_GROUP_SCHED init_task_group.se = (struct sched_entity **)ptr;
Peter Zijlstra
maintainerRe: [PATCH] [10/23] SCHED: Only allocate per cpu cpu mask buffer with offstack cpumasks
Quoted 26 lines · Andi Kleen
This will save a few bytes in the non offstack cpumask case.
Found by gcc 4.6's new warnings.
--- kernel/sched.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)Index:
linux-2.6.35-rc2-gcc/kernel/sched.c
===================================================================kernel/sched.c +1
-1@@ -7482,7 +7482,7 @@ static void init_tg_rt_entry(struct taskvoid __init sched_init (void ) {int i, j; -unsigned long alloc_size =0 , ptr; +unsigned long alloc_size =0 ;#ifdef CONFIG_FAIR_GROUP_SCHED alloc_size +=2 * nr_cpu_ids *sizeof (void **);
kernel/sched.c void __init sched_init(void) · line 7503
Quoted 10 lines
kernel/sched.c +3
-0@@ -7494,7 +7494,10 @@ void __init sched_init(void) alloc_size +=num_possible_cpus () *cpumask_size ();#endif if (alloc_size) { +#ifdef CONFIG_CPUMASK_OFFSTACK +unsigned long ptr; ptr = (unsigned long )kzalloc (alloc_size,GFP_NOWAIT ); +#endif #ifdef CONFIG_FAIR_GROUP_SCHED init_task_group.se = (struct sched_entity **)ptr;
This patch will actually break things.. please read the code.
I guess we could move the unsigned long into that block, but I really don't see the point.
Re: [PATCH] [10/23] SCHED: Only allocate per cpu cpu mask buffer with offstack cpumasks
Quoted 4 lines
This patch will actually break things.. please read the code.
I guess we could move the unsigned long into that block, but I really don't see the point.
How does it break things?
ptr is not used for anything unless that define is set. gcc doesn't lie on this.
-Andi
Signature · 1 line
[email protected] -- Speaking for myself only.
Peter Zijlstra
maintainerRe: [PATCH] [10/23] SCHED: Only allocate per cpu cpu mask buffer with offstack cpumasks
Quoted 9 lines · Andi Kleen
Quoted 4 lines
This patch will actually break things.. please read the code.
I guess we could move the unsigned long into that block, but I really don't see the point.
How does it break things?
ptr is not used for anything unless that define is set. gcc doesn't lie on this.
if (alloc_size) {
ptr = (unsigned long)kzalloc(alloc_size, GFP_NOWAIT);#ifdef CONFIG_FAIR_GROUP_SCHED
init_task_group.se = (struct sched_entity **)ptr;
ptr += nr_cpu_ids * sizeof(void **);
init_task_group.cfs_rq = (struct cfs_rq **)ptr;
ptr += nr_cpu_ids * sizeof(void **);#endif /* CONFIG_FAIR_GROUP_SCHED */
#ifdef CONFIG_RT_GROUP_SCHED
init_task_group.rt_se = (struct sched_rt_entity **)ptr;
ptr += nr_cpu_ids * sizeof(void **);
init_task_group.rt_rq = (struct rt_rq **)ptr;
ptr += nr_cpu_ids * sizeof(void **);#endif /* CONFIG_RT_GROUP_SCHED */
#ifdef CONFIG_CPUMASK_OFFSTACK
for_each_possible_cpu(i) {
per_cpu(load_balance_tmpmask, i) = (void *)ptr;
ptr += cpumask_size();
}
#endif /* CONFIG_CPUMASK_OFFSTACK */
}Re: [PATCH] [10/23] SCHED: Only allocate per cpu cpu mask buffer with offstack cpumasks
Quoted 10 lines · Peter Zijlstra
Quoted 9 lines · Andi Kleen
Quoted 4 lines
This patch will actually break things.. please read the code.
I guess we could move the unsigned long into that block, but I really don't see the point.
How does it break things?
ptr is not used for anything unless that define is set. gcc doesn't lie on this.
Ok.
For the !FAIR_GROUP_SCHED || !CPUMASK_OFFSTACK case it's still wasted memory because it is not used for anything. If you enable the kmemleak tracer you should also get a warning about this
I updated the patch with a better ifdef to have CONFIG_UNFAIR_GROUP_SCHED too.
-Andi
---
SCHED: Only allocate per cpu cpu mask buffer with offstack cpumasks v2
This will save a few bytes in the non offstack cpumask case.
Found by gcc 4.6's new warnings.
--- kernel/sched.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
kernel/sched.c +4 -1
@@ -7482,7 +7482,7 @@ static void init_tg_rt_entry(struct taskvoid __init sched_init (void ) {int i, j; -unsigned long alloc_size =0 , ptr; +unsigned long alloc_size =0 ;#ifdef CONFIG_FAIR_GROUP_SCHED alloc_size +=2 * nr_cpu_ids *sizeof (void **);
@@ -7494,7 +7494,10 @@ void __init sched_init(void) alloc_size +=num_possible_cpus () *cpumask_size ();#endif if (alloc_size) { +#if defined(CONFIG_CPUMASK_OFFSTACK ) || defined(CONFIG_FAIR_GROUP_SCHED ) +unsigned long ptr; ptr = (unsigned long )kzalloc (alloc_size,GFP_NOWAIT ); +#endif #ifdef CONFIG_FAIR_GROUP_SCHED init_task_group.se = (struct sched_entity **)ptr;
Signature · 1 line
[email protected] -- Speaking for myself only.
Peter Zijlstra
maintainerRe: [PATCH] [10/23] SCHED: Only allocate per cpu cpu mask buffer with offstack cpumasks
Quoted 1 line · Andi Kleen
+#if
defined(CONFIG_CPUMASK_OFFSTACK)||defined(CONFIG_FAIR_GROUP_SCHED)
Still wrong.. something like the below might work, but I really think its not worth the trouble.
--- kernel/sched.c | 11 ++++++----- 1 files changed, 6 insertions(+), 5 deletions(-)
kernel/sched.c +6 -5
@@ -7689,7 +7689,9 @@ static void init_tg_rt_entry(struct task_group *tg, struct rt_rq *rt_rq,void __init sched_init (void ) {int i, j; -unsigned long alloc_size =0 , ptr; + +#if definedCONFIG_FAIR_GROUP_SCHED || definedCONFIG_RT_GROUP_SCHED || definedCONFIG_CPUMASK_OFFSTACK +unsigned long alloc_size =0 ;#ifdef CONFIG_FAIR_GROUP_SCHED alloc_size +=2 * nr_cpu_ids *sizeof (void **);
@@ -7701,6 +7703,8 @@ void __init sched_init(void) alloc_size +=num_possible_cpus () *cpumask_size ();#endif if (alloc_size) { +unsigned long ptr; + ptr = (unsigned long )kzalloc (alloc_size,GFP_NOWAIT );#ifdef CONFIG_FAIR_GROUP_SCHED
@@ -7726,6 +7730,7 @@ void __init sched_init(void) }#endif /* CONFIG_CPUMASK_OFFSTACK */ } +#endif /* {FAIR,RT}_GROUP,OFFSTACK */ #ifdef CONFIG_SMP init_defrootdomain ();
@@ -7762,7 +7767,6 @@ void __init sched_init(void)#ifdef CONFIG_FAIR_GROUP_SCHED init_task_group.shares = init_task_group_load;INIT_LIST_HEAD (&rq->leaf_cfs_rq_list); -#ifdef CONFIG_CGROUP_SCHED /* * How much cpu bandwidth does init_task_group get? *
@@ -7783,16 +7787,13 @@ void __init sched_init(void)* directly in rq->cfs (i.e init_task_group->se[] = NULL). */ init_tg_cfs_entry (&init_task_group, &rq->cfs,NULL , i,1 ,NULL ); -#endif #endif /* CONFIG_FAIR_GROUP_SCHED */ rq->rt.rt_runtime = def_rt_bandwidth.rt_runtime;#ifdef CONFIG_RT_GROUP_SCHED INIT_LIST_HEAD (&rq->leaf_rt_rq_list); -#ifdef CONFIG_CGROUP_SCHED init_tg_rt_entry (&init_task_group, &rq->rt,NULL , i,1 ,NULL );#endif -#endif for (j =0 ; j <CPU_LOAD_IDX_MAX ; j++) rq->cpu_load[j] =0 ;
Re: [PATCH] [10/23] SCHED: Only allocate per cpu cpu mask buffer with offstack cpumasks
Quoted 5 lines · Peter Zijlstra
Quoted 1 line · Andi Kleen
+#if
defined(CONFIG_CPUMASK_OFFSTACK)||defined(CONFIG_FAIR_GROUP_SCHED)Still wrong.. something like the below might work, but I really think its not worth the trouble.
Yes it seems the ifdef maze in this function is too much for me.
Anyways I retract the patch, but you'll be reminded of it once you update to gcc 4.6
-Andi
PATCH 11/23 KVM: Fix KVM_SET_SIGNAL_MASK
In reply to Andi KleenReal bug fix.
When the user passed in a NULL mask pass this on from the ioctl handler.
Found by gcc 4.6's new warnings.
--- virt/kvm/kvm_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
virt/kvm/kvm_main.c +1 -1
@@ -1520,7 +1520,7 @@ out_free2: goto out; p = &sigset; } - r =kvm_vcpu_ioctl_set_sigmask (vcpu, &sigset); + r =kvm_vcpu_ioctl_set_sigmask (vcpu, p);break ; }case KVM_GET_FPU : {
Avi Kivity
AppliedRe: [PATCH] [11/23] KVM: Fix KVM_SET_SIGNAL_MASK
Quoted 7 lines · Andi Kleen
Real bug fix.
When the user passed in a NULL mask pass this on from the ioctl handler.
Found by gcc 4.6's new warnings.
Applied, thanks.
Signature · 1 line
error compiling committee.c: too many arguments to function
PATCH 12/23 BTRFS: Clean up unused variables -- bugs
In reply to Andi KleenThese are all the cases where a variable is set, but not read which are really bugs.
- Couple of incorrect error handling fixed.
- One incorrect use of a allocation policy
- Some other things
Still needs more review.
Found by gcc 4.6's new warnings
--- fs/btrfs/dir-item.c | 2 +- fs/btrfs/extent-tree.c | 3 +-- fs/btrfs/extent_io.c | 2 ++ fs/btrfs/inode.c | 6 +++--- fs/btrfs/relocation.c | 4 +++- fs/btrfs/tree-log.c | 2 +- 6 files changed, 11 insertions(+), 8 deletions(-)
fs/btrfs/extent-tree.c +1 -2
@@ -3337,8 +3337,7 @@ struct btrfs_block_rsv *btrfs_alloc_blocbtrfs_init_block_rsv (block_rsv); alloc_target =btrfs_get_alloc_profile (root,0 ); - block_rsv->space_info =__find_space_info (fs_info, -BTRFS_BLOCK_GROUP_METADATA ); + block_rsv->space_info =__find_space_info (fs_info, alloc_target);return block_rsv; }
fs/btrfs/dir-item.c +1 -1
@@ -427,5 +427,5 @@ int btrfs_delete_one_dir_name(struct btr ret =btrfs_truncate_item (trans, root, path, item_len - sub_item_len,1 ); } -return 0 ; +return ret; }
fs/btrfs/extent_io.c +2 -0
@@ -2825,6 +2825,8 @@ int extent_prepare_write(struct extent_iNULL ,1 , end_bio_extent_preparewrite,0 , 0, 0); +if (ret && !err) + err = ret; iocount++; block_start = block_start + iosize; }else {
fs/btrfs/inode.c +3 -3
@@ -1372,7 +1372,7 @@ int btrfs_merge_bio_hook(struct page *paif (map_length < length + size)return 1 ; -return 0 ; +return ret; }/*
@@ -2672,7 +2672,7 @@ static int check_path_shared(struct btrf {struct extent_buffer *eb;int level; -int ret; +int ret =0 ;u64 refs;for (level =0 ; level <BTRFS_MAX_LEVEL ; level++) {
@@ -2686,7 +2686,7 @@ static int check_path_shared(struct btrfif (refs >1 )return 1 ; } -return 0 ; +return ret;/* XXX callers? */ }/*
fs/btrfs/tree-log.c +1 -1
@@ -2273,7 +2273,7 @@ fail: }btrfs_end_log_trans (root); -return 0 ; +return err; }/* see comments for btrfs_del_dir_entries_in_log */
fs/btrfs/relocation.c +3 -1
@@ -3098,6 +3098,8 @@ static int add_tree_block(struct reloc_cBUG_ON (item_size !=sizeof (struct btrfs_extent_item_v0 )); ret =get_ref_objectid_v0 (rc, path, extent_key, &ref_owner,NULL ); +if (ret <0 ) +return ret;BUG_ON (ref_owner >=BTRFS_MAX_LEVEL ); level = (int )ref_owner;/* FIXME: get real generation */
@@ -4142,7 +4144,7 @@ int btrfs_reloc_clone_csums(struct inodebtrfs_add_ordered_sum (inode, ordered, sums); }btrfs_put_ordered_extent (ordered); -return 0 ; +return ret; } void btrfs_reloc_cow_block(struct btrfs_trans_handle *trans,
PATCH 13/23 BTRFS: Clean up unused variables -- nonbugs
In reply to Andi KleenThese are all the cases where a variable is set, but not read which are not bugs as far as I can see, but simply leftovers.
Still needs more review.
Found by gcc 4.6's new warnings
fs/btrfs/compression.c | 2 -- fs/btrfs/ctree.c | 20 ++------------------ fs/btrfs/dir-item.c | 2 +- fs/btrfs/disk-io.c | 10 ---------- fs/btrfs/extent-tree.c | 2 -- fs/btrfs/extent_io.c | 10 ++-------- fs/btrfs/inode.c | 19 +++---------------- fs/btrfs/ioctl.c | 2 -- fs/btrfs/ordered-data.c | 2 -- fs/btrfs/relocation.c | 2 ++ fs/btrfs/root-tree.c | 2 -- fs/btrfs/super.c | 1 + fs/btrfs/tree-defrag.c | 2 -- fs/btrfs/tree-log.c | 15 +-------------- fs/btrfs/volumes.c | 4 ---- fs/btrfs/xattr.c | 2 -- fs/btrfs/zlib.c | 5 -----
--- fs/btrfs/compression.c | 2 -- fs/btrfs/ctree.c | 20 ++------------------ fs/btrfs/disk-io.c | 11 ----------- fs/btrfs/extent-tree.c | 2 -- fs/btrfs/extent_io.c | 9 --------- fs/btrfs/inode.c | 14 -------------- fs/btrfs/ioctl.c | 2 -- fs/btrfs/ordered-data.c | 2 -- fs/btrfs/root-tree.c | 2 -- fs/btrfs/super.c | 6 ++---- fs/btrfs/tree-defrag.c | 2 -- fs/btrfs/tree-log.c | 15 --------------- fs/btrfs/volumes.c | 4 ---- fs/btrfs/xattr.c | 2 -- fs/btrfs/zlib.c | 5 ----- 15 files changed, 4 insertions(+), 94 deletions(-)
fs/btrfs/ctree.c +2 -18
@@ -200,7 +200,6 @@ int btrfs_copy_root(struct btrfs_trans_h struct extent_buffer **cow_ret, u64 new_root_objectid) {struct extent_buffer *cow; -u32 nritems;int ret =0 ;int level;struct btrfs_disk_key disk_key;
@@ -210,7 +209,6 @@ int btrfs_copy_root(struct btrfs_trans_hWARN_ON (root->ref_cows && trans->transid != root->last_trans); level =btrfs_header_level (buf); - nritems =btrfs_header_nritems (buf);if (level ==0 )btrfs_item_key (buf, &disk_key,0 );else
@@ -1008,7 +1006,6 @@ static noinline int balance_level(structint wret;int pslot;int orig_slot = path->slots[level]; -int err_on_enospc =0 ;u64 orig_ptr;if (level ==0 )
@@ -1071,8 +1068,7 @@ static noinline int balance_level(struct BTRFS_NODEPTRS_PER_BLOCK(root) / 4)return 0 ; -if (btrfs_header_nritems (mid) <2 ) - err_on_enospc =1 ; +btrfs_header_nritems (mid); left =read_node_slot (root, parent, pslot -1 );if (left) {
@@ -1103,8 +1099,7 @@ static noinline int balance_level(struct wret =push_node_left (trans, root, left, mid,1 );if (wret <0 ) ret = wret; -if (btrfs_header_nritems (mid) <2 ) - err_on_enospc =1 ; +btrfs_header_nritems (mid); }/*
@@ -1224,14 +1219,12 @@ static noinline int push_nodes_for_inserint wret;int pslot;int orig_slot = path->slots[level]; -u64 orig_ptr;if (level ==0 )return 1 ; mid = path->nodes[level];WARN_ON (btrfs_header_generation (mid) != trans->transid); - orig_ptr =btrfs_node_blockptr (mid, orig_slot);if (level <BTRFS_MAX_LEVEL -1 ) parent = path->nodes[level +1 ];
@@ -2534,7 +2527,6 @@ static noinline int __push_leaf_left(str {struct btrfs_disk_key disk_key;struct extent_buffer *right = path->nodes[0 ]; -int slot;int i;int push_space =0 ;int push_items =0 ;
@@ -2546,8 +2538,6 @@ static noinline int __push_leaf_left(stru32 this_item_size;u32 old_left_item_size; - slot = path->slots[1 ]; -if (empty) nr = right_nritems;else
@@ -3239,7 +3229,6 @@ int btrfs_truncate_item(struct btrfs_tra {int ret =0 ;int slot; -int slot_orig;struct extent_buffer *leaf;struct btrfs_item *item;u32 nritems;
@@ -3249,7 +3238,6 @@ int btrfs_truncate_item(struct btrfs_traunsigned int size_diff;int i; - slot_orig = path->slots[0 ]; leaf = path->nodes[0 ]; slot = path->slots[0 ];
@@ -3354,7 +3342,6 @@ int btrfs_extend_item(struct btrfs_trans {int ret =0 ;int slot; -int slot_orig;struct extent_buffer *leaf;struct btrfs_item *item;u32 nritems;
@@ -3363,7 +3350,6 @@ int btrfs_extend_item(struct btrfs_transunsigned int old_size;int i; - slot_orig = path->slots[0 ]; leaf = path->nodes[0 ]; nritems =btrfs_header_nritems (leaf);
@@ -3696,7 +3682,6 @@ int btrfs_insert_empty_items(struct btrf struct btrfs_key *cpu_key, u32 *data_size, int nr) { - struct extent_buffer *leaf; int ret = 0;int slot;int i;
@@ -3713,7 +3698,6 @@ int btrfs_insert_empty_items(struct btrfif (ret <0 ) goto out; - leaf = path->nodes[0 ]; slot = path->slots[0 ];BUG_ON (slot <0 );
fs/btrfs/super.c +2 -4
@@ -61,6 +61,8 @@ static void btrfs_put_super(struct super ret =close_ctree (root); sb->s_fs_info =NULL ; + + (void )ret;/* FIXME: need to fix VFS to return error? */ }enum {
@@ -434,7 +436,6 @@ static int btrfs_fill_super(struct super {struct inode *inode;struct dentry *root_dentry; -struct btrfs_super_block *disk_super;struct btrfs_root *tree_root;struct btrfs_key key;int err;
@@ -456,7 +457,6 @@ static int btrfs_fill_super(struct superreturn PTR_ERR (tree_root); } sb->s_fs_info = tree_root; - disk_super = &tree_root->fs_info->super_copy; key.objectid =BTRFS_FIRST_FREE_OBJECTID ; key.type =BTRFS_INODE_ITEM_KEY ;
@@ -569,7 +569,6 @@ static int btrfs_get_sb(struct file_systchar *subvol_name =NULL ;u64 subvol_objectid =0 ;int error =0 ; -int found =0 ;if (!(flags &MS_RDONLY )) mode |=FMODE_WRITE ;
@@ -605,7 +604,6 @@ static int btrfs_get_sb(struct file_syst goto error_close_devices; } - found =1 ;btrfs_close_devices (fs_devices); }else {char b[BDEVNAME_SIZE ];
fs/btrfs/disk-io.c +0 -11
@@ -338,7 +338,6 @@ static int csum_dirty_buffer(struct btrfstruct extent_io_tree *tree;u64 start = (u64 )page->index <<PAGE_CACHE_SHIFT ;u64 found_start; -int found_level;unsigned long len;struct extent_buffer *eb;int ret;
@@ -369,8 +368,6 @@ static int csum_dirty_buffer(struct btrfWARN_ON (1 ); goto err; } - found_level =btrfs_header_level (eb); -csum_tree_block (root, eb,0 ); err:free_extent_buffer (eb);
@@ -533,11 +530,9 @@ int btrfs_congested_async(struct btrfs_fstatic void run_one_async_start (struct btrfs_work *work) { -struct btrfs_fs_info *fs_info;struct async_submit_bio *async; async = container_of(work, struct async_submit_bio, work); - fs_info =BTRFS_I (async->inode)->root->fs_info; async->submit_bio_start (async->inode, async->rw, async->bio, async->mirror_num, async->bio_flags, async->bio_offset);
@@ -850,12 +845,8 @@ struct extent_buffer *read_tree_block(st u32 blocksize, u64 parent_transid) {struct extent_buffer *buf =NULL ; -struct inode *btree_inode = root->fs_info->btree_inode; -struct extent_io_tree *io_tree;int ret; - io_tree = &BTRFS_I (btree_inode)->io_tree; - buf =btrfs_find_create_tree_block (root, bytenr, blocksize);if (!buf)return NULL ;
@@ -1377,7 +1368,6 @@ static int bio_ready_for_csum(struct biou64 start =0 ;struct page *page;struct extent_io_tree *io_tree =NULL ; -struct btrfs_fs_info *info =NULL ;struct bio_vec *bvec;int i;int ret;
@@ -1396,7 +1386,6 @@ static int bio_ready_for_csum(struct bio buf_len = page->private >>2 ; start =page_offset (page) + bvec->bv_offset; io_tree = &BTRFS_I (page->mapping->host)->io_tree; - info =BTRFS_I (page->mapping->host)->root->fs_info; }/* are we fully contained in this bio? */ if (buf_len <= length)
fs/btrfs/extent-tree.c +0 -2
@@ -5420,7 +5420,6 @@ static noinline void reada_walk_down(stru64 generation;u64 refs;u64 flags; -u64 last =0 ;u32 nritems;u32 blocksize;struct btrfs_key key;
@@ -5488,7 +5487,6 @@ reada: generation);if (ret)break ; - last = bytenr + blocksize; nread++; } wc->reada_slot = slot;
fs/btrfs/root-tree.c +0 -2
@@ -181,7 +181,6 @@ int btrfs_insert_root(struct btrfs_transint btrfs_find_dead_roots (struct btrfs_root *root,u64 objectid) {struct btrfs_root *dead_root; -struct btrfs_item *item;struct btrfs_root_item *ri;struct btrfs_key key;struct btrfs_key found_key;
@@ -214,7 +213,6 @@ again: nritems =btrfs_header_nritems (leaf); slot = path->slots[0 ]; } - item =btrfs_item_nr (leaf, slot);btrfs_item_key_to_cpu (leaf, &key, slot);if (btrfs_key_type (&key) !=BTRFS_ROOT_ITEM_KEY ) goto next;
fs/btrfs/compression.c +0 -2
@@ -163,7 +163,6 @@ fail:*/ static void end_compressed_bio_read (struct bio *bio,int err) { -struct extent_io_tree *tree;struct compressed_bio *cb = bio->bi_private;struct inode *inode;struct page *page;
@@ -187,7 +186,6 @@ static void end_compressed_bio_read(stru/* ok, we're the last bio for this extent, lets start * the decompression. */ - tree = &BTRFS_I (inode)->io_tree; ret = btrfs_zlib_decompress_biovec(cb->compressed_pages, cb->start, cb->orig_bio->bi_io_vec,
fs/btrfs/extent_io.c +0 -9
@@ -1901,10 +1901,8 @@ static int submit_one_bio(int rw, structstruct page *page = bvec->bv_page;struct extent_io_tree *tree = bio->bi_private;u64 start; -u64 end; start = ((u64 )page->index <<PAGE_CACHE_SHIFT ) + bvec->bv_offset; - end = start + bvec->bv_len -1 ; bio->bi_private =NULL ;
@@ -2204,7 +2202,6 @@ static int __extent_writepage(struct pagu64 last_byte =i_size_read (inode);u64 block_start;u64 iosize; -u64 unlock_start;sector_t sector;struct extent_state *cached_state =NULL ;struct extent_map *em;
@@ -2329,7 +2326,6 @@ static int __extent_writepage(struct pagif (tree->ops && tree->ops->writepage_end_io_hook) tree->ops->writepage_end_io_hook (page, start, page_end,NULL ,1 ); - unlock_start = page_end +1 ; goto done; }
@@ -2340,7 +2336,6 @@ static int __extent_writepage(struct pagif (tree->ops && tree->ops->writepage_end_io_hook) tree->ops->writepage_end_io_hook (page, cur, page_end,NULL ,1 ); - unlock_start = page_end +1 ;break ; } em = epd->get_extent(inode, page, pg_offset, cur,
@@ -2387,7 +2382,6 @@ static int __extent_writepage(struct pag cur += iosize; pg_offset += iosize; - unlock_start = cur;continue ; }/* leave this out until we have a page_mkwrite call */
@@ -2473,7 +2467,6 @@ static int extent_write_cache_pages(strupgoff_t index;pgoff_t end;/* Inclusive */ int scanned =0 ; -int range_whole =0 ;pagevec_init (&pvec,0 );if (wbc->range_cyclic) {
@@ -2482,8 +2475,6 @@ static int extent_write_cache_pages(stru }else { index = wbc->range_start >>PAGE_CACHE_SHIFT ; end = wbc->range_end >>PAGE_CACHE_SHIFT ; -if (wbc->range_start ==0 && wbc->range_end ==LLONG_MAX ) - range_whole =1 ; scanned =1 ; } retry:
fs/btrfs/inode.c +0 -14
@@ -319,8 +319,6 @@ static noinline int compress_file_range(struct btrfs_root *root =BTRFS_I (inode)->root;struct btrfs_trans_handle *trans;u64 num_bytes; -u64 orig_start; -u64 disk_num_bytes;u64 blocksize = root->sectorsize;u64 actual_end;u64 isize =i_size_read (inode);
@@ -335,8 +333,6 @@ static noinline int compress_file_range(int i;int will_compress; - orig_start = start; - actual_end =min_t (u64, isize, end +1 ); again: will_compress =0 ;
@@ -371,7 +367,6 @@ again: total_compressed =min (total_compressed, max_uncompressed); num_bytes = (end - start + blocksize) & ~(blocksize -1 ); num_bytes =max (blocksize, num_bytes); - disk_num_bytes = num_bytes; total_in =0 ; ret =0 ;
@@ -467,7 +462,6 @@ again:if (total_compressed >= total_in) { will_compress =0 ; }else { - disk_num_bytes = total_compressed; num_bytes = total_in; } }
@@ -757,8 +751,6 @@ static noinline int cow_file_range(strucu64 disk_num_bytes;u64 cur_alloc_size;u64 blocksize = root->sectorsize; -u64 actual_end; -u64 isize =i_size_read (inode);struct btrfs_key ins;struct extent_map *em;struct extent_map_tree *em_tree = &BTRFS_I (inode)->extent_tree;
@@ -769,8 +761,6 @@ static noinline int cow_file_range(strucbtrfs_set_trans_block_group (trans, inode); trans->block_rsv = &root->fs_info->delalloc_block_rsv; - actual_end =min_t (u64, isize, end +1 ); - num_bytes = (end - start + blocksize) & ~(blocksize -1 ); num_bytes =max (blocksize, num_bytes); disk_num_bytes = num_bytes;
@@ -2237,7 +2227,6 @@ void btrfs_orphan_cleanup(struct btrfs_r {struct btrfs_path *path;struct extent_buffer *leaf; -struct btrfs_item *item;struct btrfs_key key, found_key;struct btrfs_trans_handle *trans;struct inode *inode;
@@ -2275,7 +2264,6 @@ void btrfs_orphan_cleanup(struct btrfs_r/* pull out the item */ leaf = path->nodes[0 ]; - item =btrfs_item_nr (leaf, path->slots[0 ]);btrfs_item_key_to_cpu (leaf, &found_key, path->slots[0 ]);/* make sure the item matches what we want */
@@ -5640,7 +5628,6 @@ static void btrfs_submit_direct(int rw,struct btrfs_root *root =BTRFS_I (inode)->root;struct btrfs_dio_private *dip;struct bio_vec *bvec = bio->bi_io_vec; -u64 start;int skip_sum;int write = rw & (1 <<BIO_RW );int ret =0 ;
@@ -5666,7 +5653,6 @@ static void btrfs_submit_direct(int rw, dip->inode = inode; dip->logical_offset = file_offset; - start = dip->logical_offset; dip->bytes =0 ; do { dip->bytes += bvec->bv_len;
fs/btrfs/ioctl.c +0 -2
@@ -708,7 +708,6 @@ static noinline int btrfs_ioctl_resize(schar *sizestr;char *devstr =NULL ;int ret =0 ; -int namelen;int mod =0 ;if (root->fs_info->sb->s_flags &MS_RDONLY )
@@ -722,7 +721,6 @@ static noinline int btrfs_ioctl_resize(sreturn PTR_ERR (vol_args); vol_args->name[BTRFS_PATH_NAME_MAX ] ='\0' ; - namelen =strlen (vol_args->name);mutex_lock (&root->fs_info->volume_mutex); sizestr = vol_args->name;
fs/btrfs/ordered-data.c +0 -2
@@ -526,7 +526,6 @@ int btrfs_wait_ordered_range(struct inod {u64 end;u64 orig_end; -u64 wait_end;struct btrfs_ordered_extent *ordered;int found;
@@ -537,7 +536,6 @@ int btrfs_wait_ordered_range(struct inodif (orig_end >INT_LIMIT (loff_t)) orig_end =INT_LIMIT (loff_t); } - wait_end = orig_end; again:/* start IO across the range first to instantiate any delalloc * extents
fs/btrfs/tree-defrag.c +0 -2
@@ -36,7 +36,6 @@ int btrfs_defrag_leaves(struct btrfs_traint ret =0 ;int wret;int level; -int orig_level;int is_extent =0 ;int next_key_ret =0 ;u64 last_ret =0 ;
@@ -64,7 +63,6 @@ int btrfs_defrag_leaves(struct btrfs_trareturn -ENOMEM ; level =btrfs_header_level (root->node); - orig_level = level;if (level ==0 ) goto out;
fs/btrfs/tree-log.c +0 -15
@@ -786,7 +786,6 @@ static noinline int add_inode_ref(struct {struct inode *dir;int ret; -struct btrfs_key location;struct btrfs_inode_ref *ref;struct btrfs_dir_item *di;struct inode *inode;
@@ -795,10 +794,6 @@ static noinline int add_inode_ref(structunsigned long ref_ptr;unsigned long ref_end; - location.objectid = key->objectid; - location.type =BTRFS_INODE_ITEM_KEY ; - location.offset =0 ; -/* * it is possible that we didn't log all the parent directories * for a given inode. If we don't find the dir, just don't
@@ -1583,7 +1578,6 @@ static int replay_one_buffer(struct btrfstruct btrfs_path *path;struct btrfs_root *root = wc->replay_dest;struct btrfs_key key; -u32 item_size;int level;int i;int ret;
@@ -1601,7 +1595,6 @@ static int replay_one_buffer(struct btrf nritems =btrfs_header_nritems (eb);for (i =0 ; i < nritems; i++) {btrfs_item_key_to_cpu (eb, &key, i); - item_size =btrfs_item_size_nr (eb, i);/* inode keys are done during the first stage */ if (key.type == BTRFS_INODE_ITEM_KEY &&
@@ -1668,7 +1661,6 @@ static noinline int walk_down_log_tree(s struct walk_control *wc) {u64 root_owner; -u64 root_gen;u64 bytenr;u64 ptr_gen;struct extent_buffer *next;
@@ -1698,7 +1690,6 @@ static noinline int walk_down_log_tree(s parent = path->nodes[*level]; root_owner =btrfs_header_owner (parent); - root_gen =btrfs_header_generation (parent); next =btrfs_find_create_tree_block (root, bytenr, blocksize);
@@ -1749,7 +1740,6 @@ static noinline int walk_up_log_tree(str struct walk_control *wc) {u64 root_owner; -u64 root_gen;int i;int slot;int ret;
@@ -1757,8 +1747,6 @@ static noinline int walk_up_log_tree(strfor (i = *level; i <BTRFS_MAX_LEVEL -1 && path->nodes[i]; i++) { slot = path->slots[i];if (slot +1 <btrfs_header_nritems (path->nodes[i])) { -struct extent_buffer *node; - node = path->nodes[i]; path->slots[i]++; *level = i;WARN_ON (*level ==0 );
@@ -1771,7 +1759,6 @@ static noinline int walk_up_log_tree(str parent = path->nodes[*level +1 ]; root_owner =btrfs_header_owner (parent); - root_gen =btrfs_header_generation (parent); wc->process_func (root, path->nodes[*level], wc,btrfs_header_generation (path->nodes[*level]));if (wc->free) {
@@ -2729,7 +2716,6 @@ static int btrfs_log_inode(struct btrfs_struct btrfs_key max_key;struct btrfs_root *log = root->log_root;struct extent_buffer *src =NULL ; -u32 size;int err =0 ;int ret;int nritems;
@@ -2793,7 +2779,6 @@ again:break ; src = path->nodes[0 ]; - size =btrfs_item_size_nr (src, path->slots[0 ]);if (ins_nr && ins_start_slot + ins_nr == path->slots[0 ]) { ins_nr++; goto next_slot;
fs/btrfs/volumes.c +0 -4
@@ -1901,7 +1901,6 @@ int btrfs_balance(struct btrfs_root *devu64 size_to_free;struct btrfs_path *path;struct btrfs_key key; -struct btrfs_chunk *chunk;struct btrfs_root *chunk_root = dev_root->fs_info->chunk_root;struct btrfs_trans_handle *trans;struct btrfs_key found_key;
@@ -1965,9 +1964,6 @@ int btrfs_balance(struct btrfs_root *devif (found_key.objectid != key.objectid)break ; - chunk =btrfs_item_ptr (path->nodes[0 ], - path->slots[0 ], - struct btrfs_chunk);/* chunk zero is special */ if (found_key.offset ==0 )break ;
fs/btrfs/xattr.c +0 -2
@@ -178,7 +178,6 @@ ssize_t btrfs_listxattr(struct dentry *dstruct inode *inode = dentry->d_inode;struct btrfs_root *root =BTRFS_I (inode)->root;struct btrfs_path *path; -struct btrfs_item *item;struct extent_buffer *leaf;struct btrfs_dir_item *di;int ret =0 , slot, advance;
@@ -234,7 +233,6 @@ ssize_t btrfs_listxattr(struct dentry *d } advance =1 ; - item =btrfs_item_nr (leaf, slot);btrfs_item_key_to_cpu (leaf, &found_key, slot);/* check to make sure this item is what we want */
fs/btrfs/zlib.c +0 -5
@@ -199,8 +199,6 @@ int btrfs_zlib_compress_pages(struct addint nr_pages =0 ;struct page *in_page =NULL ;struct page *out_page =NULL ; -int out_written =0 ; -int in_read =0 ;unsigned long bytes_left; *out_pages =0 ;
@@ -233,9 +231,6 @@ int btrfs_zlib_compress_pages(struct add workspace->def_strm.avail_out =PAGE_CACHE_SIZE ; workspace->def_strm.avail_in =min (len,PAGE_CACHE_SIZE ); - out_written =0 ; - in_read =0 ; -while (workspace->def_strm.total_in < len) { ret =zlib_deflate (&workspace->def_strm,Z_SYNC_FLUSH );if (ret !=Z_OK ) {
PATCH 14/23 NFSD: Fix initialized but not read warnings
In reply to Andi KleenFixes at least one real minor bug: the nfs4 recovery dir sysctl would not return its status properly.
Also I finished Al's 1e41568d7378d ("Take ima_path_check() in nfsd past dentry_open() in nfsd_open()") commit, it moved the IMA code, but left the old path initializer in there.
The rest is just dead code removed I think, although I was not fully sure about the "is_borc" stuff. Some more review would be still good.
Found by gcc 4.6's new warnings.
--- fs/nfsd/nfs4state.c | 2 -- fs/nfsd/nfsctl.c | 2 ++ fs/nfsd/nfsproc.c | 2 -- fs/nfsd/vfs.c | 11 +---------- 4 files changed, 3 insertions(+), 14 deletions(-)
fs/nfsd/nfs4state.c +0 -2
@@ -3346,11 +3346,9 @@ static inline void nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny) { struct nfs4_stateowner *sop; -unsigned int hval;if (fl->fl_lmops == &nfsd_posix_mng_ops) { sop = (struct nfs4_stateowner *) fl->fl_owner; - hval =lockownerid_hashval (sop->so_id);kref_get (&sop->so_ref); deny->ld_sop = sop; deny->ld_clientid = sop->so_client->cl_clientid;
fs/nfsd/nfsctl.c +2 -0
@@ -1310,6 +1310,8 @@ static ssize_t __write_recoverydir(strucreturn -EINVAL ; status =nfs4_reset_recoverydir (recdir); +if (status) +return status; } return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%s\n",
fs/nfsd/nfsproc.c +0 -2
@@ -290,7 +290,6 @@ nfsd_proc_create(struct svc_rqst *rqstp,* gospel of sun micro */ if (type !=S_IFREG ) { -int is_borc =0 ;if (type !=S_IFBLK && type !=S_IFCHR ) { rdev =0 ; }else if (type ==S_IFCHR && !(attr->ia_valid &ATTR_SIZE )) {
@@ -298,7 +297,6 @@ nfsd_proc_create(struct svc_rqst *rqstp, type =S_IFIFO ; }else {/* Okay, char or block special */ - is_borc =1 ;if (!rdev) rdev = wanted; }
fs/nfsd/vfs.c +1 -10
@@ -1632,7 +1632,7 @@ nfsd_link(struct svc_rqst *rqstp, struct char *name, int len, struct svc_fh *tfhp) { struct dentry *ddir, *dnew, *dold; -struct inode *dirp, *dest; +struct inode *dirp;__be32 err;int host_err;
@@ -1660,7 +1660,6 @@ nfsd_link(struct svc_rqst *rqstp, struct goto out_nfserr; dold = tfhp->fh_dentry; - dest = dold->d_inode; host_err =mnt_want_write (tfhp->fh_export->ex_path.mnt);if (host_err) {
@@ -2112,15 +2111,7 @@ nfsd_permission(struct svc_rqst *rqstp,if (err == -EACCES &&S_ISREG (inode->i_mode) && acc == (NFSD_MAY_READ |NFSD_MAY_OWNER_OVERRIDE )) err =inode_permission (inode,MAY_EXEC ); -if (err) - goto nfsd_out; -/* Do integrity (permission) checking now, but defer incrementing -* IMA counts to the actual file open. -*/ - path.mnt = exp->ex_path.mnt; - path.dentry = dentry; -nfsd_out:return err?nfserrno (err) :0 ; }
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH] [14/23] NFSD: Fix initialized but not read warnings
Quoted 21 lines · Neil Brown
Hi Andi,
NFSD stuff should really go to Bruce Fields and [email protected]Thanks,
NeilBrownQuoted 11 lines · On Thu, 10 Jun 2010 13:10:50 +0200 (CEST) Andi Kleen <[email protected]>…
Fixes at least one real minor bug: the nfs4 recovery dir sysctl would not return its status properly.
Also I finished Al's
1e41568d7378d("Takeima_path_check()in nfsd pastdentry_open()innfsd_open()") commit, it moved the IMA code, but left the old path initializer in there.The rest is just dead code removed I think, although I was not fully sure about the "
is_borc" stuff. Some more review would be still good.
Thanks, all look good to me (though I might like to figure out how we eneded up in this situation in a couple of cases...).
Absent any objection, I'll apply these to the nfsd tree for 2.6.36.
--b.
Quoted 107 lines
Quoted 106 lines
Found by gcc 4.6's new warnings.
Signed-off-by Andi Kleen <[email protected]>--- fs/nfsd/nfs4state.c | 2 -- fs/nfsd/nfsctl.c | 2 ++ fs/nfsd/nfsproc.c | 2 -- fs/nfsd/vfs.c | 11 +---------- 4 files changed, 3 insertions(+), 14 deletions(-)Index:
linux-2.6.35-rc2-gcc/fs/nfsd/nfs4state.c
===================================================================fs/nfsd/nfs4state.c +0
-2@@ -3346,11 +3346,9 @@ static inline void nfs4_set_lock_denied(struct file_lock *fl, struct nfsd4_lock_denied *deny) { struct nfs4_stateowner *sop; -unsigned int hval;if (fl->fl_lmops == &nfsd_posix_mng_ops) { sop = (struct nfs4_stateowner *) fl->fl_owner; - hval =lockownerid_hashval (sop->so_id);kref_get (&sop->so_ref); deny->ld_sop = sop; deny->ld_clientid = sop->so_client->cl_clientid;Index:
linux-2.6.35-rc2-gcc/fs/nfsd/nfsctl.c
===================================================================fs/nfsd/nfsctl.c +2
-0@@ -1310,6 +1310,8 @@ static ssize_t __write_recoverydir(strucreturn -EINVAL ; status =nfs4_reset_recoverydir (recdir); +if (status) +return status; } return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%s\n",Index:
linux-2.6.35-rc2-gcc/fs/nfsd/nfsproc.c
===================================================================fs/nfsd/nfsproc.c +0
-2@@ -290,7 +290,6 @@ nfsd_proc_create(struct svc_rqst *rqstp,* gospel of sun micro */ if (type !=S_IFREG ) { -int is_borc =0 ;if (type !=S_IFBLK && type !=S_IFCHR ) { rdev =0 ; }else if (type ==S_IFCHR && !(attr->ia_valid &ATTR_SIZE )) {@@ -298,7 +297,6 @@ nfsd_proc_create(struct svc_rqst *rqstp, type =S_IFIFO ; }else {/* Okay, char or block special */ - is_borc =1 ;if (!rdev) rdev = wanted; }Index:
linux-2.6.35-rc2-gcc/fs/nfsd/vfs.c
===================================================================fs/nfsd/vfs.c +1
-11@@ -1632,7 +1632,7 @@ nfsd_link(struct svc_rqst *rqstp, struct char *name, int len, struct svc_fh *tfhp) { struct dentry *ddir, *dnew, *dold; -struct inode *dirp, *dest; +struct inode *dirp;__be32 err;int host_err;@@ -1660,7 +1660,6 @@ nfsd_link(struct svc_rqst *rqstp, struct goto out_nfserr; dold = tfhp->fh_dentry; - dest = dold->d_inode; host_err =mnt_want_write (tfhp->fh_export->ex_path.mnt);if (host_err) {@@ -2112,15 +2111,7 @@ nfsd_permission(struct svc_rqst *rqstp,if (err == -EACCES &&S_ISREG (inode->i_mode) && acc == (NFSD_MAY_READ |NFSD_MAY_OWNER_OVERRIDE )) err =inode_permission (inode,MAY_EXEC ); -if (err) - goto nfsd_out; -/* Do integrity (permission) checking now, but defer incrementing -* IMA counts to the actual file open. -*/ - path.mnt = exp->ex_path.mnt; - path.dentry = dentry; -nfsd_out:return err?nfserrno (err) :0 ; } --To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
PATCH 15/23 EXT4: Fix initialized but not read variables
In reply to Andi KleenNo real bugs found, just various dead code removed I believe. Some review would be good.
Found by gcc 4.6's new warnings
--- fs/ext4/balloc.c | 2 -- fs/ext4/extents.c | 8 ++------ fs/ext4/inode.c | 5 ----- fs/ext4/mballoc.c | 23 ++++++++--------------- fs/ext4/namei.c | 2 -- fs/ext4/resize.c | 2 -- fs/jbd2/journal.c | 12 ------------ fs/jbd2/recovery.c | 7 ------- 8 files changed, 10 insertions(+), 51 deletions(-)
fs/ext4/balloc.c +0 -2
@@ -377,14 +377,12 @@ void ext4_add_groupblocks(handle_t *handext4_grpblk_t bit;unsigned int i;struct ext4_group_desc *desc; -struct ext4_super_block *es;struct ext4_sb_info *sbi;int err =0 , ret, blk_free_count;ext4_grpblk_t blocks_freed;struct ext4_group_info *grp; sbi =EXT4_SB (sb); - es = sbi->s_es;ext4_debug ("Adding block(s) %llu-%llu\n" , block, block + count -1 );ext4_get_group_no_and_offset (sb, block, &block_group, &bit);
fs/ext4/extents.c +2 -6
@@ -1083,7 +1083,6 @@ static int ext4_ext_grow_indepth(handle_ {struct ext4_ext_path *curp = path;struct ext4_extent_header *neh; -struct ext4_extent_idx *fidx;struct buffer_head *bh;ext4_fsblk_t newblock;int err =0 ;
@@ -1144,10 +1143,10 @@ static int ext4_ext_grow_indepth(handle_ext4_idx_store_pblock (curp->p_idx, newblock); neh =ext_inode_hdr (inode); - fidx =EXT_FIRST_INDEX (neh);ext_debug ("new root: num %d(%d), lblock %d, ptr %llu\n" ,le16_to_cpu (neh->eh_entries),le16_to_cpu (neh->eh_max), -le32_to_cpu (fidx->ei_block),idx_pblock (fidx)); +le32_to_cpu (EXT_FIRST_INDEX (neh)->ei_block), +idx_pblock (fidx)); neh->eh_depth =cpu_to_le16 (path->p_depth +1 ); err =ext4_ext_dirty (handle, inode, curp);
@@ -2954,7 +2953,6 @@ static int ext4_split_unwritten_extents(struct ext4_extent *ex1 =NULL ;struct ext4_extent *ex2 =NULL ;struct ext4_extent *ex3 =NULL ; -struct ext4_extent_header *eh;ext4_lblk_t ee_block, eof_block;unsigned int allocated, ee_len, depth;ext4_fsblk_t newblock;
@@ -2971,7 +2969,6 @@ static int ext4_split_unwritten_extents( eof_block = map->m_lblk + map->m_len; depth =ext_depth (inode); - eh = path[depth].p_hdr; ex = path[depth].p_ext; ee_block =le32_to_cpu (ex->ee_block); ee_len =ext4_ext_get_actual_len (ex);
@@ -3058,7 +3055,6 @@ static int ext4_split_unwritten_extents( err =PTR_ERR (path); goto out; } - eh = path[depth].p_hdr; ex = path[depth].p_ext;if (ex2 != &newex) ex2 = ex;
fs/ext4/inode.c +0 -5
@@ -3146,13 +3146,10 @@ static int ext4_da_write_begin(struct fiint ret, retries =0 ;struct page *page;pgoff_t index; -unsigned from, to;struct inode *inode = mapping->host;handle_t *handle; index = pos >>PAGE_CACHE_SHIFT ; - from = pos & (PAGE_CACHE_SIZE -1 ); - to = from + len;if (ext4_nonda_switch (inode->i_sb)) { *fsdata = (void *)FALL_BACK_TO_NONDELALLOC ;
@@ -5754,7 +5751,6 @@ static int ext4_expand_extra_isize(struc {struct ext4_inode *raw_inode;struct ext4_xattr_ibody_header *header; -struct ext4_xattr_entry *entry;if (EXT4_I (inode)->i_extra_isize >= new_extra_isize)return 0 ;
@@ -5762,7 +5758,6 @@ static int ext4_expand_extra_isize(struc raw_inode =ext4_raw_inode (&iloc); header =IHDR (inode, raw_inode); - entry =IFIRST (header);/* No extended attributes present */ if (!ext4_test_inode_state(inode, EXT4_STATE_XATTR) ||
fs/ext4/mballoc.c +8 -15
@@ -1999,7 +1999,6 @@ ext4_mb_regular_allocator(struct ext4_alext4_group_t ngroups, group, i;int cr;int err =0 ; -int bsbits;struct ext4_sb_info *sbi;struct super_block *sb;struct ext4_buddy e4b;
@@ -2041,8 +2040,6 @@ ext4_mb_regular_allocator(struct ext4_al ac->ac_2order = i -1 ; } - bsbits = ac->ac_sb->s_blocksize_bits; -/* if stream allocation is enabled, use global goal */ if (ac->ac_flags &EXT4_MB_STREAM_ALLOC ) {/* TBD: may be hot point */
@@ -2712,7 +2709,6 @@ ext4_mb_mark_diskspace_used(struct ext4_ handle_t *handle, unsigned int reserv_blks) { struct buffer_head *bitmap_bh = NULL; -struct ext4_super_block *es;struct ext4_group_desc *gdp;struct buffer_head *gdp_bh;struct ext4_sb_info *sbi;
@@ -2725,8 +2721,6 @@ ext4_mb_mark_diskspace_used(struct ext4_ sb = ac->ac_sb; sbi =EXT4_SB (sb); - es = sbi->s_es; - err = -EIO ; bitmap_bh =ext4_read_block_bitmap (sb, ac->ac_b_ex.fe_group);
@@ -2849,8 +2843,8 @@ ext4_mb_normalize_request(struct ext4_al {int bsbits, max;ext4_lblk_t end; -loff_t size, orig_size, start_off; -ext4_lblk_t start, orig_start; +loff_t size, start_off; +ext4_lblk_t start;struct ext4_inode_info *ei =EXT4_I (ac->ac_inode);struct ext4_prealloc_space *pa;
@@ -2922,8 +2916,9 @@ ext4_mb_normalize_request(struct ext4_al start_off = (loff_t )ac->ac_o_ex.fe_logical << bsbits; size = ac->ac_o_ex.fe_len << bsbits; } - orig_size = size = size >> bsbits; - orig_start = start = start_off >> bsbits; + +/* FIXME: start_off is not used for anything. bug? */ +/* FIXME: start may be uninitialized? */ /* don't cover already allocated blocks in selected range */ if (ar->pleft && start <= ar->lleft) {
@@ -3547,7 +3542,6 @@ ext4_mb_release_inode_pa(struct ext4_budext4_group_t group;ext4_grpblk_t bit;unsigned long long grp_blk_start; -sector_t start;int err =0 ;int free =0 ;
@@ -3567,9 +3561,10 @@ ext4_mb_release_inode_pa(struct ext4_budif (bit >= end)break ; next =mb_find_next_bit (bitmap_bh->b_data, end, bit); - start =ext4_group_first_block_no (sb, group) + bit;mb_debug (1 ," free preallocated %u/%u in group %u\n" , - (unsigned ) start, (unsigned ) next - bit, + (unsigned ) +ext4_group_first_block_no (sb, group) + bit, + (unsigned ) next - bit, (unsigned ) group); free += next - bit;
@@ -4494,7 +4489,6 @@ void ext4_free_blocks(handle_t *handle,struct super_block *sb = inode->i_sb;struct ext4_allocation_context *ac =NULL ;struct ext4_group_desc *gdp; -struct ext4_super_block *es;unsigned long freed =0 ;unsigned int overflow;ext4_grpblk_t bit;
@@ -4513,7 +4507,6 @@ void ext4_free_blocks(handle_t *handle, } sbi =EXT4_SB (sb); - es =EXT4_SB (sb)->s_es;if (!(flags &EXT4_FREE_BLOCKS_VALIDATED ) && !ext4_data_block_valid (sbi, block, count)) { ext4_error(sb, "Freeing blocks not in datazone - "
fs/ext4/namei.c +0 -2
@@ -1088,7 +1088,6 @@ static struct dentry *ext4_lookup(structstruct dentry *ext4_get_parent (struct dentry *child) {__u32 ino; -struct inode *inode;static const struct qstr dotdot = { .name =".." , .len =2 ,
@@ -1097,7 +1096,6 @@ struct dentry *ext4_get_parent(struct destruct buffer_head *bh; bh =ext4_find_entry (child->d_inode, &dotdot, &de); - inode =NULL ;if (!bh)return ERR_PTR (-ENOENT ); ino =le32_to_cpu (de->inode);
fs/ext4/resize.c +0 -2
@@ -953,7 +953,6 @@ int ext4_group_extend(struct super_block ext4_fsblk_t n_blocks_count) {ext4_fsblk_t o_blocks_count; -ext4_group_t o_groups_count;ext4_grpblk_t last;ext4_grpblk_t add;struct buffer_head *bh;
@@ -965,7 +964,6 @@ int ext4_group_extend(struct super_block* yet: we're going to revalidate es->s_blocks_count after * taking the s_resize_lock below. */ o_blocks_count =ext4_blocks_count (es); - o_groups_count =EXT4_SB (sb)->s_groups_count;if (test_opt (sb,DEBUG )) printk(KERN_DEBUG "EXT4-fs: extending last group from %llu uto %llu blocks\n",
fs/jbd2/journal.c +0 -12
@@ -1392,13 +1392,9 @@ int jbd2_journal_check_used_features (joint jbd2_journal_check_available_features (journal_t *journal,unsigned long compat,unsigned long ro,unsigned long incompat) { -journal_superblock_t *sb; -if (!compat && !ro && !incompat)return 1 ; - sb = journal->j_superblock; -/* We can support any known requested features iff the * superblock is in version 2. Otherwise we fail to support any * extended sb features. */
@@ -1618,7 +1614,6 @@ int jbd2_journal_flush(journal_t *journaint jbd2_journal_wipe (journal_t *journal,int write) { -journal_superblock_t *sb;int err =0 ;J_ASSERT (!(journal->j_flags &JBD2_LOADED ));
@@ -1627,8 +1622,6 @@ int jbd2_journal_wipe(journal_t *journalif (err)return err; - sb = journal->j_superblock; -if (!journal->j_tail) goto no_recovery;
@@ -2202,8 +2195,6 @@ void jbd2_journal_init_jbd_inode(structvoid jbd2_journal_release_jbd_inode (journal_t *journal,struct jbd2_inode *jinode) { -int writeout =0 ; -if (!journal)return ; restart:
@@ -2220,9 +2211,6 @@ restart: goto restart; } -/* Do we need to wait for data writeback? */ -if (journal->j_committing_transaction == jinode->i_transaction) - writeout =1 ;if (jinode->i_transaction) {list_del (&jinode->i_list); jinode->i_transaction =NULL ;
fs/jbd2/recovery.c +0 -6
@@ -285,12 +285,10 @@ int jbd2_journal_recover(journal_t *jourint jbd2_journal_skip_recovery (journal_t *journal) {int err; -journal_superblock_t * sb;struct recovery_info info;memset (&info,0 ,sizeof (info)); - sb = journal->j_superblock; err =do_one_pass (journal, &info,PASS_SCAN );
@@ -365,11 +363,6 @@ static int do_one_pass(journal_t *journaint tag_bytes =journal_tag_bytes (journal);__u32 crc32_sum = ~0 ;/* Transactional Checksums */ -/* Precompute the maximum metadata descriptors in a descriptor block */ -int MAX_BLOCKS_PER_DESC ; -MAX_BLOCKS_PER_DESC = ((journal->j_blocksize-sizeof (journal_header_t)) - / tag_bytes);
[email protected]
maintainerRe: [PATCH] [15/23] EXT4: Fix initialized but not read variables
Quoted 10 lines · Andi Kleen
No real bugs found, just various dead code removed I believe. Some review would be good.
Found by gcc 4.6's new warnings
Signed-off-by Andi Kleen <[email protected]>
I've reviewed this and pulled a fixed version into the ext4 patch queue. You deleted the initializers for size and start in mballoc.c, which introduced a bug (ironically you commented about start not getting inititialized in a FIXME, when the patch deleted said initialization right above the introduced FIXME comment):
fs/ext4/mballoc.c ext4_mb_normalize_request(struct ext4_al · line 2924
Quoted 12 lines
fs/ext4/mballoc.c +3
-2@@ -2922,8 +2916,9 @@ ext4_mb_normalize_request(struct ext4_al start_off = (loff_t )ac->ac_o_ex.fe_logical << bsbits; size = ac->ac_o_ex.fe_len << bsbits; } - orig_size = size = size >> bsbits; - orig_start = start = start_off >> bsbits; + +/* FIXME: start_off is not used for anything. bug? */ +/* FIXME: start may be uninitialized? */ /* don't cover already allocated blocks in selected range */ if (ar->pleft && start <= ar->lleft) {
Also you only fixed one of two uses of the variable 'fidx' which you removed here:
fs/ext4/extents.c static int ext4_ext_grow_indepth(handle_ · line 1152
Quoted 13 lines
fs/ext4/extents.c +2
-2@@ -1144,10 +1143,10 @@ static int ext4_ext_grow_indepth(handle_ext4_idx_store_pblock (curp->p_idx, newblock); neh =ext_inode_hdr (inode); - fidx =EXT_FIRST_INDEX (neh);ext_debug ("new root: num %d(%d), lblock %d, ptr %llu\n" ,le16_to_cpu (neh->eh_entries),le16_to_cpu (neh->eh_max), -le32_to_cpu (fidx->ei_block),idx_pblock (fidx)); +le32_to_cpu (EXT_FIRST_INDEX (neh)->ei_block), +idx_pblock (fidx)); neh->eh_depth =cpu_to_le16 (path->p_depth +1 ); err =ext4_ext_dirty (handle, inode, curp);
Other than that the patch was fine. I've fixed these up and I'll carry the patch in the ext4 tree.
Thanks,
- Ted
PATCH 16/23 XFS: Fix gcc 4.6 set but not read and unused statement warnings
In reply to Andi KleenFor my configuration, that is without quota or RT.
Mostly dead code removed I think (but needs additional review)
That is there were one or two bad error handling cases, but they were not easily fixable, with comments and I left the warnings in for those for you to remember.
e.g. if there is a ENOSPC down in xfs_trans.c while modifying the superblock it would not be handled.
Unused statements were mostly related to stub macros for disabled features like QUOTA or RT ALLOC. I replace those with inlines.
There were also some problems with variables used in ASSERT() I partly moved those into the ASSERT itself and partly used a new QASSERT that always evaluates.
--- fs/xfs/linux-2.6/xfs_sync.c | 3 +++ fs/xfs/support/debug.h | 4 ++++ fs/xfs/xfs_alloc.c | 10 +++------- fs/xfs/xfs_da_btree.c | 15 +++++---------- fs/xfs/xfs_dir2_block.c | 6 +++--- fs/xfs/xfs_filestream.c | 10 ++-------- fs/xfs/xfs_iget.c | 3 --- fs/xfs/xfs_inode.c | 4 ---- fs/xfs/xfs_inode_item.c | 8 ++------ fs/xfs/xfs_log.c | 2 -- fs/xfs/xfs_quota.h | 14 ++++++++++---- fs/xfs/xfs_trans.c | 1 + 12 files changed, 33 insertions(+), 47 deletions(-)
fs/xfs/linux-2.6/xfs_sync.c +3 -0
@@ -554,6 +554,9 @@ xfs_sync_worker(xfs_log_force (mp,0 );xfs_reclaim_inodes (mp,0 );/* dgc: errors ignored here */ +/* ak: yes and you'll get a warning for it now when you +* upgrade compilers. +*/ error =xfs_qm_sync (mp,SYNC_TRYLOCK );if (xfs_log_need_covered (mp)) error =xfs_commit_dummy_trans (mp,0 );
fs/xfs/xfs_da_btree.c +5 -10
@@ -581,10 +581,8 @@ xfs_da_node_add(xfs_da_state_t *state, xxfs_da_intnode_t *node;xfs_da_node_entry_t *btree;int tmp; -xfs_mount_t *mp; node = oldblk->bp->data; - mp = state->mp;ASSERT (be16_to_cpu (node->hdr.info.magic) ==XFS_DA_NODE_MAGIC );ASSERT ((oldblk->index >=0 ) && (oldblk->index <=be16_to_cpu (node->hdr.count)));ASSERT (newblk->blkno !=0 );
@@ -710,8 +708,6 @@ STATIC int xfs_da_root_join(xfs_da_state_t *state, xfs_da_state_blk_t *root_blk) {xfs_da_intnode_t *oldroot; -/* REFERENCED */ -xfs_da_blkinfo_t *blkinfo;xfs_da_args_t *args;xfs_dablk_t child;xfs_dabuf_t *bp;
@@ -742,15 +738,14 @@ xfs_da_root_join(xfs_da_state_t *state,if (error)return (error);ASSERT (bp !=NULL ); - blkinfo = bp->data;if (be16_to_cpu (oldroot->hdr.level) ==1 ) { -ASSERT (be16_to_cpu (blkinfo->magic) ==XFS_DIR2_LEAFN_MAGIC || -be16_to_cpu (blkinfo->magic) ==XFS_ATTR_LEAF_MAGIC ); +ASSERT (be16_to_cpu (bp->data->magic) ==XFS_DIR2_LEAFN_MAGIC || +be16_to_cpu (bp->data->magic) ==XFS_ATTR_LEAF_MAGIC ); }else { -ASSERT (be16_to_cpu (blkinfo->magic) ==XFS_DA_NODE_MAGIC ); +ASSERT (be16_to_cpu (bp->data->magic) ==XFS_DA_NODE_MAGIC ); } -ASSERT (!blkinfo->forw); -ASSERT (!blkinfo->back); +ASSERT (!bp->data->forw); +ASSERT (!bp->data->back);memcpy (root_blk->bp->data, bp->data, state->blocksize);xfs_da_log_buf (args->trans, root_blk->bp,0 , state->blocksize -1 ); error =xfs_da_shrink_inode (args, child, bp);
fs/xfs/xfs_dir2_block.c +3 -3
@@ -1073,10 +1073,10 @@ xfs_dir2_sf_to_block(*/ buf_len = dp->i_df.if_bytes; - buf =kmem_alloc (dp->i_df.if_bytes,KM_SLEEP ); + buf =kmem_alloc (buf_len,KM_SLEEP ); -memcpy (buf, sfp, dp->i_df.if_bytes); -xfs_idata_realloc (dp, -dp->i_df.if_bytes,XFS_DATA_FORK ); +memcpy (buf, sfp, buf_len); +xfs_idata_realloc (dp, -buf_len,XFS_DATA_FORK ); dp->i_d.di_size =0 ;xfs_trans_log_inode (tp, dp,XFS_ILOG_CORE );/*
fs/xfs/xfs_filestream.c +2 -8
@@ -140,9 +140,8 @@ _xfs_filestream_pick_ag(int flags, xfs_extlen_t minlen) { - int streams, max_streams; int err, trylock, nscan; -xfs_extlen_t longest, free, minfree, maxfree =0 ; +xfs_extlen_t longest, minfree, maxfree =0 ;xfs_agnumber_t ag, max_ag =NULLAGNUMBER ;struct xfs_perag *pag;
@@ -174,7 +173,6 @@ _xfs_filestream_pick_ag(/* Keep track of the AG with the most free blocks. */ if (pag->pagf_freeblks > maxfree) { maxfree = pag->pagf_freeblks; - max_streams =atomic_read (&pag->pagf_fstrms); max_ag = ag; }
@@ -196,8 +194,6 @@ _xfs_filestream_pick_ag( (flags & XFS_PICK_LOWSPACE))) {/* Break out, retaining the reference on the AG. */ - free = pag->pagf_freeblks; - streams =atomic_read (&pag->pagf_fstrms);xfs_perag_put (pag); *agp = ag;break ;
@@ -234,8 +230,6 @@ next_ag:if (max_ag !=NULLAGNUMBER ) {xfs_filestream_get_ag (mp, max_ag);TRACE_AG_PICK1 (mp, max_ag, maxfree); - streams = max_streams; - free = maxfree; *agp = max_ag;break ; }
@@ -364,7 +358,7 @@ xfs_fstrm_free_func(/* Drop the reference taken on the AG when the item was added. */ ref =xfs_filestream_put_ag (ip->i_mount, item->ag); -ASSERT (ref >=0 ); +QASSERT (ref >=0 );TRACE_FREE (ip->i_mount, ip, item->pip, item->ag,xfs_filestream_peek_ag (ip->i_mount, item->ag));
fs/xfs/xfs_iget.c +0 -3
@@ -265,7 +265,6 @@ xfs_iget_cache_miss( {struct xfs_inode *ip;int error; -unsigned long first_index, mask;xfs_agino_t agino =XFS_INO_TO_AGINO (mp, ino); ip =xfs_inode_alloc (mp, ino);
@@ -302,8 +301,6 @@ xfs_iget_cache_miss(BUG (); } - mask = ~(((XFS_INODE_CLUSTER_SIZE (mp) >> mp->m_sb.sb_inodelog)) -1 ); - first_index = agino & mask;write_lock (&pag->pag_ici_lock);/* insert the new inode */
fs/xfs/xfs_inode.c +0 -4
@@ -925,7 +925,6 @@ xfs_iread_extents(int error;xfs_ifork_t *ifp;xfs_extnum_t nextents; -size_t size;if (unlikely (XFS_IFORK_FORMAT (ip, whichfork) !=XFS_DINODE_FMT_BTREE )) { XFS_ERROR_REPORT("xfs_iread_extents", XFS_ERRLEVEL_LOW,
@@ -933,7 +932,6 @@ xfs_iread_extents(return XFS_ERROR (EFSCORRUPTED ); } nextents =XFS_IFORK_NEXTENTS (ip, whichfork); - size = nextents *sizeof (xfs_bmbt_rec_t); ifp =XFS_IFORK_PTR (ip, whichfork);/*
@@ -3517,13 +3515,11 @@ xfs_iext_remove_indirect(xfs_extnum_t ext_diff;/* extents to remove in current list */ xfs_extnum_t nex1;/* number of extents before idx */ xfs_extnum_t nex2;/* extents after idx + count */ -int nlists;/* entries in indirection array */ int page_idx = idx;/* index in target extent list */ ASSERT (ifp->if_flags &XFS_IFEXTIREC ); erp =xfs_iext_idx_to_irec (ifp, &page_idx, &erp_idx,0 );ASSERT (erp !=NULL ); - nlists = ifp->if_real_bytes /XFS_IEXT_BUFSZ ; nex1 = page_idx; ext_cnt = count;while (ext_cnt) {
fs/xfs/xfs_inode_item.c +2 -6
@@ -220,7 +220,6 @@ xfs_inode_item_format(xfs_inode_t *ip;size_t data_bytes;xfs_bmbt_rec_t *ext_buffer; -int nrecs;xfs_mount_t *mp; ip = iip->ili_inode;
@@ -323,9 +322,8 @@ xfs_inode_item_format(ASSERT (ip->i_df.if_u1.if_extents !=NULL );ASSERT (ip->i_d.di_nextents >0 );ASSERT (iip->ili_extents_buf ==NULL ); - nrecs = ip->i_df.if_bytes / - (uint )sizeof (xfs_bmbt_rec_t); -ASSERT (nrecs >0 ); +ASSERT ((ip->i_df.if_bytes / + (uint )sizeof (xfs_bmbt_rec_t)) >0 );#ifdef XFS_NATIVE_HOST if (nrecs == ip->i_d.di_nextents) {/*
@@ -957,10 +955,8 @@ xfs_iflush_abort( xfs_inode_t *ip) {xfs_inode_log_item_t *iip = ip->i_itemp; -xfs_mount_t *mp; iip = ip->i_itemp; - mp = ip->i_mount;if (iip) {struct xfs_ail *ailp = iip->ili_item.li_ailp;if (iip->ili_item.li_flags &XFS_LI_IN_AIL ) {
fs/xfs/xfs_log.c +0 -2
@@ -1047,7 +1047,6 @@ xlog_alloc_log(xfs_mount_t *mp,xlog_in_core_t *iclog, *prev_iclog=NULL ;xfs_buf_t *bp;int i; -int iclogsize;int error =ENOMEM ;uint log2_size =0 ;
@@ -1127,7 +1126,6 @@ xlog_alloc_log(xfs_mount_t *mp,* with different amounts of memory. See the definition of * xlog_in_core_t in xfs_log_priv.h for details. */ - iclogsize = log->l_iclog_size;ASSERT (log->l_iclog_size >=4096 );for (i=0 ; i < log->l_iclog_bufs; i++) { *iclogp =kmem_zalloc (sizeof (xlog_in_core_t),KM_MAYFAIL );
fs/xfs/xfs_quota.h +10 -4
@@ -346,7 +346,13 @@ xfs_qm_vop_dqalloc(struct xfs_inode *ip,#define xfs_trans_mod_dquot_byino (tp, ip, fields, delta)#define xfs_trans_apply_dquot_deltas (tp)#define xfs_trans_unreserve_and_mod_dquots (tp) -#define xfs_trans_reserve_quota_nblks (tp, ip, nblks, ninos, flags) (0) + +static inline int xfs_trans_reserve_quota_nblks (struct xfs_trans *t, +struct xfs_inode *i,long a,long b,uint c) +{ +return 0 ; +} +#define xfs_trans_reserve_quota_bydquots (tp, mp, u, g, nb, ni, fl) (0)#define xfs_qm_vop_create_dqattach (tp, ip, u, g)#define xfs_qm_vop_rename_dqattach (it) (0)
@@ -355,13 +361,13 @@ xfs_qm_vop_dqalloc(struct xfs_inode *ip,#define xfs_qm_dqattach (ip, fl) (0)#define xfs_qm_dqattach_locked (ip, fl) (0)#define xfs_qm_dqdetach (ip) -#define xfs_qm_dqrele (d) +static inline void xfs_qm_dqrele (struct xfs_dquot *d) {}#define xfs_qm_statvfs (ip, s) -#define xfs_qm_sync (mp, fl) (0) +static inline int xfs_qm_sync (struct xfs_mount *m,int i) {return 0 ; }#define xfs_qm_newmount (mp, a, b) (0)#define xfs_qm_mount_quotas (mp)#define xfs_qm_unmount (mp) -#define xfs_qm_unmount_quotas (mp) (0) +static inline void xfs_qm_unmount_quotas (struct xfs_mount *m) {}#endif /* CONFIG_XFS_QUOTA */ #define xfs_trans_unreserve_quota_nblks (tp, ip, nblks, ninos, flags) \
fs/xfs/xfs_trans.c +1 -0
@@ -1120,6 +1120,7 @@ xfs_trans_unreserve_and_mod_sb( error =xfs_mod_incore_sb_batch (tp->t_mountp, msb, (uint)(msbp - msb), rsvd);ASSERT (error ==0 ); +/* FIXME: need real error handling here, error can be ENOSPC */ } }
fs/xfs/xfs_alloc.c +3 -7
@@ -688,8 +688,6 @@ xfs_alloc_ag_vextent_near(xfs_agblock_t ltbno;/* start bno of left side entry */ xfs_agblock_t ltbnoa;/* aligned ... */ xfs_extlen_t ltdiff;/* difference to left side entry */ -/*REFERENCED*/ -xfs_agblock_t ltend;/* end bno of left side entry */ xfs_extlen_t ltlen;/* length of left side entry */ xfs_extlen_t ltlena;/* aligned ... */ xfs_agblock_t ltnew;/* useful start bno of left side */
@@ -814,8 +812,7 @@ xfs_alloc_ag_vextent_near(if ((error =xfs_alloc_get_rec (cnt_cur, <bno, <len, &i))) goto error0;XFS_WANT_CORRUPTED_GOTO (i ==1 , error0); - ltend = ltbno + ltlen; -ASSERT (ltend <=be32_to_cpu (XFS_BUF_TO_AGF (args->agbp)->agf_length)); +ASSERT (ltbno + ltlen <=be32_to_cpu (XFS_BUF_TO_AGF (args->agbp)->agf_length)); args->len = blen;if (!xfs_alloc_fix_minleft (args)) {xfs_btree_del_cursor (cnt_cur,XFS_BTREE_NOERROR );
@@ -828,7 +825,7 @@ xfs_alloc_ag_vextent_near(*/ args->agbno = bnew;ASSERT (bnew >= ltbno); -ASSERT (bnew + blen <= ltend); +ASSERT (bnew + blen <= ltbno + ltlen);/* * Set up a cursor for the by-bno tree. */
@@ -1157,7 +1154,6 @@ xfs_alloc_ag_vextent_near(/* * Fix up the length and compute the useful address. */ - ltend = ltbno + ltlen; args->len =XFS_EXTLEN_MIN (ltlena, args->maxlen);xfs_alloc_fix_len (args);if (!xfs_alloc_fix_minleft (args)) {
@@ -1170,7 +1166,7 @@ xfs_alloc_ag_vextent_near( (void )xfs_alloc_compute_diff (args->agbno, rlen, args->alignment, ltbno, ltlen, <new);ASSERT (ltnew >= ltbno); -ASSERT (ltnew + rlen <= ltend); +ASSERT (ltnew + rlen <= ltbno + ltlen);ASSERT (ltnew + rlen <=be32_to_cpu (XFS_BUF_TO_AGF (args->agbp)->agf_length)); args->agbno = ltnew; if ((error = xfs_alloc_fixup_trees(cnt_cur, bno_cur_lt, ltbno, ltlen,
fs/xfs/support/debug.h +4 -0
@@ -37,6 +37,9 @@ extern void assfail(char *expr, char *f,#ifndef DEBUG #define ASSERT (expr) ((void)0) +/* Assert that always evaluates its input to avoid warnings */ +#define QASSERT (expr) ((void)(expr)) +#ifndef STATIC # define STATIC static noinline#endif
@@ -45,6 +48,7 @@ extern void assfail(char *expr, char *f,#define ASSERT (expr) \ (unlikely(expr) ? (void)0 : assfail(#expr, __FILE__, __LINE__)) +#define QASSERT (expr) ASSERT(expr)#ifndef STATIC # define STATIC noinline
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings
Most of this looks good to me, but I really don't like the QASSERT macro. If you're fine with it I'll split this up into smaller patches and resubmit the unmodified parts under your name and redo those bits I don't like.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings
Quoted 2 lines · Christoph Hellwig
Most of this looks good to me, but I really don't like the QASSERT macro. If you're fine with it I'll split this up into smaller patches
Why do you not like it?
The alternative will be likely uglier.
Quoted 2 lines
and resubmit the unmodified parts under your name and redo those bits I don't like.
Fine for me.
-Andi
Signature · 6 lines
[email protected] -- Speaking for myself only. _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings
In reply to Andi KleenQuoted 11 lines · Andi Kleen
For my configuration, that is without quota or RT.
Mostly dead code removed I think (but needs additional review)
That is there were one or two bad error handling cases, but they were not easily fixable, with comments and I left the warnings in for those for you to remember.
e.g. if there is a ENOSPC down in
xfs_trans.cwhile modifying the superblock it would not be handled.
See my comments about these below.
Quoted 3 lines
Unused statements were mostly related to stub macros for disabled features like QUOTA or RT ALLOC. I replace those with inlines.
Did you compile with/without XFS_DEBUG (I don't think so)? when changing code that affects ASSERT statements, CONFIG_XFS_DEBUG needs to be selected to test that this code compiles. Most XFs developers build with XFS_CONFIG_DEBUG for everything other than performance testing, so ensuring this builds is definitely required. ;)
I'd also be interested if any fixes are needed with all options enabled. Seems silly just to fix a few warnings in just one particular configuration rather than all of them...
Quoted 3 lines
There were also some problems with variables used in
ASSERT()I partly moved those into the ASSERT itself and partly used a new QASSERT that always evaluates.
That's a pretty ugly hack for a single occurrence of a warning. Re-arrnaging the code is a much better idea than introducing a new ASSERT type. e.g:
- ASSERT(ref >= 0);
+ if (ref < 0)
+ ASSERT(0);
Quoted 24 lines
Signed-off-by Andi Kleen <[email protected]>--- fs/xfs/linux-2.6/xfs_sync.c | 3 +++ fs/xfs/support/debug.h | 4 ++++ fs/xfs/xfs_alloc.c | 10 +++------- fs/xfs/xfs_da_btree.c | 15 +++++---------- fs/xfs/xfs_dir2_block.c | 6 +++--- fs/xfs/xfs_filestream.c | 10 ++-------- fs/xfs/xfs_iget.c | 3 --- fs/xfs/xfs_inode.c | 4 ---- fs/xfs/xfs_inode_item.c | 8 ++------ fs/xfs/xfs_log.c | 2 -- fs/xfs/xfs_quota.h | 14 ++++++++++---- fs/xfs/xfs_trans.c | 1 + 12 files changed, 33 insertions(+), 47 deletions(-)Index:
linux-2.6.35-rc2-gcc/fs/xfs/linux-2.6/xfs_sync.c
===================================================================
fs/xfs/linux-2.6/xfs_sync.c xfs_sync_worker( · line 562
Quoted 9 lines
fs/xfs/linux-2.6/xfs_sync.c +3
-0@@ -554,6 +554,9 @@ xfs_sync_worker(xfs_log_force (mp,0 );xfs_reclaim_inodes (mp,0 );/* dgc: errors ignored here */ +/* ak: yes and you'll get a warning for it now when you +* upgrade compilers. +*/ error =xfs_qm_sync (mp,SYNC_TRYLOCK );if (xfs_log_need_covered (mp)) error =xfs_commit_dummy_trans (mp,0 );
I don't think the coment is necessary - the compiler will remind us that we are ignoring errors.
FWIW, we've now got a situation where external static code checkers will tell us that we are not handling an error case (which is where this code and comment came from), and now the compiler will warn us we are assigning but not using the return value. It's a bit of a Catch-22 situation. Hence my question is this - how are we supposed to "safely" ignore a return value seeing as the compiler is now making the current method rather noisy?
Quoted 5 lines
Index:
linux-2.6.35-rc2-gcc/fs/xfs/xfs_da_btree.c
===================================================================
fs/xfs/xfs_da_btree.c xfs_da_node_add(xfs_da_state_t *state, x · line 588
Quoted 10 lines
fs/xfs/xfs_da_btree.c +0
-2@@ -581,10 +581,8 @@ xfs_da_node_add(xfs_da_state_t *state, xxfs_da_intnode_t *node;xfs_da_node_entry_t *btree;int tmp; -xfs_mount_t *mp; node = oldblk->bp->data; - mp = state->mp;ASSERT (be16_to_cpu (node->hdr.info.magic) ==XFS_DA_NODE_MAGIC );ASSERT ((oldblk->index >=0 ) && (oldblk->index <=be16_to_cpu (node->hdr.count)));ASSERT (newblk->blkno !=0 );
That'll break a CONFIG_XFS_DEBUG build as the next statement:
if (state->args->whichfork == XFS_DATA_FORK)
ASSERT(newblk->blkno >= mp->m_dirleafblk &&
newblk->blkno < mp->m_dirfreeblk);uses mp inside ASSERT statements.
Quoted 8 lines
Index:
linux-2.6.35-rc2-gcc/fs/xfs/xfs_dir2_block.c
===================================================================fs/xfs/xfs_dir2_block.c +0
-0@@ -1073,10 +1073,10 @@ xfs_dir2_sf_to_block(*/ buf_len = dp->i_df.if_bytes;
fs/xfs/xfs_dir2_block.c xfs_dir2_sf_to_block( · line 1082
Quoted 10 lines
fs/xfs/xfs_dir2_block.c +3
-3- buf =kmem_alloc (dp->i_df.if_bytes,KM_SLEEP ); + buf =kmem_alloc (buf_len,KM_SLEEP ); -memcpy (buf, sfp, dp->i_df.if_bytes); -xfs_idata_realloc (dp, -dp->i_df.if_bytes,XFS_DATA_FORK ); +memcpy (buf, sfp, buf_len); +xfs_idata_realloc (dp, -buf_len,XFS_DATA_FORK ); dp->i_d.di_size =0 ;xfs_trans_log_inode (tp, dp,XFS_ILOG_CORE );/*
Just remove the buf_len variable in this case.
Quoted 42 lines
Index:
linux-2.6.35-rc2-gcc/fs/xfs/xfs_filestream.c
===================================================================fs/xfs/xfs_filestream.c +1
-7@@ -140,9 +140,8 @@ _xfs_filestream_pick_ag(int flags, xfs_extlen_t minlen) { - int streams, max_streams; int err, trylock, nscan; -xfs_extlen_t longest, free, minfree, maxfree =0 ; +xfs_extlen_t longest, minfree, maxfree =0 ;xfs_agnumber_t ag, max_ag =NULLAGNUMBER ;struct xfs_perag *pag;@@ -174,7 +173,6 @@ _xfs_filestream_pick_ag(/* Keep track of the AG with the most free blocks. */ if (pag->pagf_freeblks > maxfree) { maxfree = pag->pagf_freeblks; - max_streams =atomic_read (&pag->pagf_fstrms); max_ag = ag; }@@ -196,8 +194,6 @@ _xfs_filestream_pick_ag( (flags & XFS_PICK_LOWSPACE))) {/* Break out, retaining the reference on the AG. */ - free = pag->pagf_freeblks; - streams =atomic_read (&pag->pagf_fstrms);xfs_perag_put (pag); *agp = ag;break ;@@ -234,8 +230,6 @@ next_ag:if (max_ag !=NULLAGNUMBER ) {xfs_filestream_get_ag (mp, max_ag);TRACE_AG_PICK1 (mp, max_ag, maxfree); - streams = max_streams; - free = maxfree; *agp = max_ag;break ; }
fs/xfs/xfs_filestream.c xfs_fstrm_free_func( · line 363
Quoted 7 lines
fs/xfs/xfs_filestream.c +1
-1@@ -364,7 +358,7 @@ xfs_fstrm_free_func(/* Drop the reference taken on the AG when the item was added. */ ref =xfs_filestream_put_ag (ip->i_mount, item->ag); -ASSERT (ref >=0 ); +QASSERT (ref >=0 );TRACE_FREE (ip->i_mount, ip, item->pip, item->ag,xfs_filestream_peek_ag (ip->i_mount, item->ag));
These are all "unused" because they are used in debug code only. i.e. in XFS_FILESTREAMS_TRACE configs. This is manual debug code that needs to be converted to the trace infrastructure - the compiler may say it is unused, but it is not dead code, so shoul dnot be removed.
See also my comment about QASSERT() above.
fs/xfs/xfs_trans.c xfs_trans_unreserve_and_mod_sb( · line 1123
Quoted 10 lines
fs/xfs/xfs_filestream.c +0
-0#define xfs_trans_unreserve_quota_nblks (tp, ip, nblks, ninos, flags) \Index:
linux-2.6.35-rc2-gcc/fs/xfs/xfs_trans.c
===================================================================fs/xfs/xfs_trans.c +1
-0@@ -1120,6 +1120,7 @@ xfs_trans_unreserve_and_mod_sb( error =xfs_mod_incore_sb_batch (tp->t_mountp, msb, (uint)(msbp - msb), rsvd);ASSERT (error ==0 ); +/* FIXME: need real error handling here, error can be ENOSPC */
That comment is wrong and hence is not needed. By design, we should never, ever get an error here because we've already reserved all the space we need and this is just an accounting call. That's what the ASSERT(error == 0) is documenting. It's ben placed there to catch any re-occurrence of the race condition that is documented in the function head comment during development. Anyway, if we do get an error here, we cannot handle it anyway - it's too late to do anything short of a complete shutdown as we've already written the transaction to the log.
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings
Quoted 5 lines · Dave Chinner
Quoted 3 lines
Unused statements were mostly related to stub macros for disabled features like QUOTA or RT ALLOC. I replace those with inlines.
Did you compile with/without
XFS_DEBUG(I don't think so)? when
No.
I merely made my own config work with relatively little warnings.
Quoted 4 lines
changing code that affects ASSERT statements,
CONFIG_XFS_DEBUGneeds to be selected to test that this code compiles. Most XFs developers build withXFS_CONFIG_DEBUGfor everything other than performance testing, so ensuring this builds is definitely required. ;)
Ok fair enough.
Quoted 4 lines
I'd also be interested if any fixes are needed with all options enabled. Seems silly just to fix a few warnings in just one particular configuration rather than all of them...
There are tons more warnings with allyesconfig I'm sure, not only in xfs. They will need time to work out.
This will happen over time as people eventually move to gcc 4.6 (after it's released)
Some of the warnings are also related
to not enabling everything (e.g. no quota)
Quoted 8 lines
Quoted 4 lines
There were also some problems with variables used in
ASSERT()I partly moved those into the ASSERT itself and partly used a new QASSERT that always evaluates.That's a pretty ugly hack for a single occurrence of a warning. Re-arrnaging the code is a much better idea than introducing a new ASSERT type. e.g:
I originally planned to use it for more, but then ended up changing other code in other ways.
I still don't think it's a ugly hack, it's a relatively simple way to handle this. But I can change this single occurrence.
Quoted 7 lines
FWIW, we've now got a situation where external static code checkers will tell us that we are not handling an error case (which is where this code and comment came from), and now the compiler will warn us we are assigning but not using the return value. It's a bit of a Catch-22 situation. Hence my question is this - how are we supposed to "safely" ignore a return value seeing as the compiler is now making the current method rather noisy?
Fix the problem?
Otherwise you can use a (void) cast, but that's a bad way if there's a real problem.
Quoted 5 lines
fs/xfs/xfs_dir2_block.c xfs_dir2_sf_to_block( · line 1082
Quoted 3 lines
fs/xfs/xfs_dir2_block.c +0
-0dp->i_d.di_size =0 ;xfs_trans_log_inode (tp, dp,XFS_ILOG_CORE );/* Just remove the
buf_lenvariable in this case.
I think the code looks cleaner when using buf_len
Quoted 3 lines
These are all "unused" because they are used in debug code only. i.e. in
XFS_FILESTREAMS_TRACEconfigs. This is manual debug code that needs to be converted to the trace infrastructure - the compiler may
I have no plans to do that.
Quoted 1 line
say it is unused, but it is not dead code, so shoul dnot be removed.
I did not remove anything.
Quoted 8 lines
fs/xfs/xfs_trans.c xfs_trans_unreserve_and_mod_sb( · line 1123
Quoted 3 lines
fs/xfs/xfs_trans.c +1
-0(uint)(msbp - msb), rsvd);ASSERT (error ==0 ); +/* FIXME: need real error handling here, error can be ENOSPC */ That comment is wrong and hence is not needed. By design, we should never, ever get an error here because we've already reserved all the space we need and this is just an accounting call. That's what the ASSERT(error == 0) is documenting. It's ben placed there to catch
Ok. But I must say ASSERT() is not really a good way to document things like that. Whoever wrote this gets what he deserves now ...
Quoted 4 lines
function head comment during development. Anyway, if we do get an error here, we cannot handle it anyway - it's too late to do anything short of a complete shutdown as we've already written the transaction to the log.
Well I guess it should be unconditional BUG_ON then.
-Andi
Signature · 6 lines
[email protected] -- Speaking for myself only. _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings
Quoted 23 lines · Andi Kleen
Quoted 7 lines · Dave Chinner
Quoted 3 lines
There were also some problems with variables used in
ASSERT()I partly moved those into the ASSERT itself and partly used a new QASSERT that always evaluates.That's a pretty ugly hack for a single occurrence of a warning. Re-arrnaging the code is a much better idea than introducing a new ASSERT type. e.g:
I originally planned to use it for more, but then ended up changing other code in other ways.
I still don't think it's a ugly hack, it's a relatively simple way to handle this. But I can change this single occurrence.
Quoted 7 lines
FWIW, we've now got a situation where external static code checkers will tell us that we are not handling an error case (which is where this code and comment came from), and now the compiler will warn us we are assigning but not using the return value. It's a bit of a Catch-22 situation. Hence my question is this - how are we supposed to "safely" ignore a return value seeing as the compiler is now making the current method rather noisy?
Quoted 1 line
Fix the problem?
There is no problem. The end of the error reporting line is the main loop of a background kernel thread - anything important is already stashed for later reporting to a real context - so all that is left to do is throw away the error once it propagated to the top of the call chain....
Quoted 2 lines
Otherwise you can use a (void) cast, but that's a bad way if there's a real problem.
Right, and that's exactly my point - we removed all the (void) casts because the error checker flagged them as dangerous. Now the compiler is complaining about not using the error that is returned. So my question still stands....
Quoted 9 lines
Quoted 8 lines
fs/xfs/xfs_trans.c xfs_trans_unreserve_and_mod_sb( · line 1123
Quoted 3 lines
fs/xfs/xfs_trans.c +1
-0(uint)(msbp - msb), rsvd);ASSERT (error ==0 ); +/* FIXME: need real error handling here, error can be ENOSPC */ That comment is wrong and hence is not needed. By design, we should never, ever get an error here because we've already reserved all the space we need and this is just an accounting call. That's what the ASSERT(error == 0) is documenting. It's ben placed there to catch
Quoted 3 lines
Ok. But I must say
ASSERT()is not really a good way to document things like that. Whoever wrote this gets what he deserves now ...
We have historically documented code assumptions and bounds with ASSERT() calls rather than in comments because it means they are checked at runtime. It means we find out really quickly when we've made some change that has had an unintended side effect, rather than it going unnoticed until some user trips over it.
This one in specific has been there for at least 5 years - goes back to before git was used and has proven to be useful for finding at least one subtle race in new code introduced back in 2007 (45c34141126a89da07197d5b89c04c6847f1171a "[XFS] Apply transaction delta counts atomically to incore counters").
FWIW, there's around 2000 asserts in the XFS code - that's about 2% of the code - which means assumptions in the XFS code are pretty well documented compared to other Linux filesystems...
Quoted 6 lines
Quoted 4 lines
function head comment during development. Anyway, if we do get an error here, we cannot handle it anyway - it's too late to do anything short of a complete shutdown as we've already written the transaction to the log.
Well I guess it should be unconditional
BUG_ONthen.
Don't be silly. A filesystem shutdown is all that is necessary, especially as the XFS shutdown procedure has hooks to turn corruption events into system panics if the admin wants to configure it that way (generally useful for triggering crash dumps on corruption events for offline triage).
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
function head comment during development. Anyway, if we do get an error here, we cannot handle it anyway - it's too late to do anything short of a complete shutdown as we've already written the transaction to the log.
Well I guess it should be unconditional
BUG_ONthen.Don't be silly. A filesystem shutdown is all that is necessary,
Without BUG_ON it will not end up in kerneloops.org and you will never know about it. That's standard Linux kernel development practice.
Maybe XFS should catch up on that.
Ok in principle you could make the shutdown a WARN()
Anyways I'm out of this.
-Andi
Signature · 6 lines
[email protected] -- Speaking for myself only. _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings
Quoted 11 lines · Andi Kleen
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
function head comment during development. Anyway, if we do get an error here, we cannot handle it anyway - it's too late to do anything short of a complete shutdown as we've already written the transaction to the log.
Well I guess it should be unconditional
BUG_ONthen.Don't be silly. A filesystem shutdown is all that is necessary,
Without
BUG_ONit will not end up in kerneloops.org and you will never know about it.
We find out about corrupted filesystems all the time from users sending mail to the list. Even if we did panic by default on corruption events, kerneloops.org is *useless* for reporting them because finding out about a corruption is only the very first step of what is usually a long and involved process that requires user interaction to gather information necessary to find the cause of the corruption.
Besides, if we _really_ want the machine to panic on corruption, then we configure the machine specifically for it via setting the relevant corruption type bit in /proc/sys/fs/xfs/panic_mask. This is generally only used when a developer asks a user to set it to get kernel crash dumps triggered when a corruption event occurs so we can do remote, offline analysis of the failure.
Quoted 2 lines
That's standard Linux kernel development
practice. Maybe XFS should catch up on that.
I find this really amusing because linux filesystems have, over the last few years, implemented a simpler version of XFS's way of dealing with corruption events(*). Perhaps you should catch up with the state of the art before throwing rocks, Andi....
Cheers,
Dave.
(*) extN, fat, hpfs, jfs, nilfs2, ntfs, ocfs2 and logfs all have configurable corruption event behaviour that default to remount-ro and can be configured to panic the machine.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings
Quoted 7 lines
We find out about corrupted filesystems all the time from users sending mail to the list. Even if we did panic by default on corruption events, kerneloops.org is *useless* for reporting them because finding out about a corruption is only the very first step of what is usually a long and involved process that requires user interaction to gather information necessary to find the cause of the corruption.
The idea behind kerneloops.org
is normally that any single report can be always a flake (broken memory, hardware, flipped bit whatever).
An error becomes important and interesting when there are multiple occurrences of it in the field.
Quoted 2 lines
Besides, if we _really_ want the machine to panic on corruption,
BUG_ON is not panic normally.
Quoted 5 lines
then we configure the machine specifically for it via setting the relevant corruption type bit in
/proc/sys/fs/xfs/panic_mask. This is generally only used when a developer asks a user to set it to get kernel crash dumps triggered when a corruption event occurs so we can do remote, offline analysis of the failure.
Especially when you're talking about desktop class systems without ECC memory that will mean you'll spend at least some time on errors which are simply bit flips.
Quoted 4 lines
Quoted 2 lines
That's standard Linux kernel development
practice. Maybe XFS should catch up on that.I find this really amusing because linux filesystems have, over the
This has really nothing to do with file systems, it's general practice for everything (well except XFS)
Quoted 3 lines
last few years, implemented a simpler version of XFS's way of dealing with corruption
events(*). Perhaps you should catch up with the state of the art before throwing rocks, Andi....
I suspect you miss quite a lot of valuable information from your user base by not supporting kerneloops.org. On the other hand it would likely also save you from spending time on flakes.
That said you don't need BUG_ON to support it (WARN etc. work too), it's just the easiest way.
-Andi
Signature · 6 lines
[email protected] -- Speaking for myself only. _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings
Quoted 7 lines · Andi Kleen
I suspect you miss quite a lot of valuable information from your user base by not supporting kerneloops.org. On the other hand it would likely also save you from spending time on flakes.
That said you don't need
BUG_ONto support it (WARN etc. work too), it's just the easiest way.
Note that a XFS filesystem shutdown already gives a stack trace. But picking up every filesystem shutdown on kerneloops.org seems to be quite a bit too much. It's usually due to IO errors from the underlying device.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings
Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · Andi Kleen
I suspect you miss quite a lot of valuable information from your user base by not supporting kerneloops.org. On the other hand it would likely also save you from spending time on flakes.
Quoted 7 lines
Quoted 2 lines
That said you don't need
BUG_ONto support it (WARN etc. work too), it's just the easiest way.Note that a XFS filesystem shutdown already gives a stack trace. But picking up every filesystem shutdown on kerneloops.org seems to be quite a bit too much. It's usually due to IO errors from the underlying device.
Yes, but known race check asserts should be probably there, right? Maybe you need a special kind of ASSERT (or shutdown) for those?
-Andi
Signature · 6 lines
[email protected] -- Speaking for myself only. _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 17/23 EXT3: Fix set but unused variables
In reply to Andi KleenI think it's mostly dead code, but needs more review. I was not fully sure about the jbd case.
--- fs/ext3/namei.c | 3 +-- fs/ext3/resize.c | 2 -- fs/jbd/journal.c | 7 ------- fs/jbd/recovery.c | 7 ------- 4 files changed, 1 insertion(+), 18 deletions(-)
fs/ext3/namei.c +1 -2
@@ -1447,7 +1447,6 @@ static int ext3_add_entry (handle_t *han struct inode *inode) {struct inode *dir = dentry->d_parent->d_inode; -unsigned long offset;struct buffer_head * bh;struct ext3_dir_entry_2 *de;struct super_block * sb;
@@ -1469,7 +1468,7 @@ static int ext3_add_entry (handle_t *hanext3_mark_inode_dirty (handle, dir); } blocks = dir->i_size >> sb->s_blocksize_bits; -for (block =0 , offset =0 ; block < blocks; block++) { +for (block =0 ; block < blocks; block++) { bh =ext3_bread (handle, dir, block,0 , &retval);if (!bh)return retval;
fs/ext3/resize.c +0 -2
@@ -964,7 +964,6 @@ int ext3_group_extend(struct super_block ext3_fsblk_t n_blocks_count) {ext3_fsblk_t o_blocks_count; -unsigned long o_groups_count;ext3_grpblk_t last;ext3_grpblk_t add;struct buffer_head * bh;
@@ -976,7 +975,6 @@ int ext3_group_extend(struct super_block* yet: we're going to revalidate es->s_blocks_count after * taking the s_resize_lock below. */ o_blocks_count =le32_to_cpu (es->s_blocks_count); - o_groups_count =EXT3_SB (sb)->s_groups_count;if (test_opt (sb,DEBUG )) printk(KERN_DEBUG "EXT3-fs: extending last group from "E3FSBLK" uto "E3FSBLK" blocks\n",
fs/jbd/journal.c +0 -7
@@ -1281,13 +1281,9 @@ int journal_check_used_features (journalint journal_check_available_features (journal_t *journal,unsigned long compat,unsigned long ro,unsigned long incompat) { -journal_superblock_t *sb; -if (!compat && !ro && !incompat)return 1 ; - sb = journal->j_superblock; -/* We can support any known requested features iff the * superblock is in version 2. Otherwise we fail to support any * extended sb features. */
@@ -1481,7 +1477,6 @@ int journal_flush(journal_t *journal)int journal_wipe (journal_t *journal,int write) { -journal_superblock_t *sb;int err =0 ;J_ASSERT (!(journal->j_flags &JFS_LOADED ));
@@ -1490,8 +1485,6 @@ int journal_wipe(journal_t *journal, intif (err)return err; - sb = journal->j_superblock; -if (!journal->j_tail) goto no_recovery;
fs/jbd/recovery.c +0 -7
@@ -283,12 +283,10 @@ int journal_recover(journal_t *journal)int journal_skip_recovery (journal_t *journal) {int err; -journal_superblock_t * sb;struct recovery_info info;memset (&info,0 ,sizeof (info)); - sb = journal->j_superblock; err =do_one_pass (journal, &info,PASS_SCAN );
@@ -321,11 +319,6 @@ static int do_one_pass(journal_t *journaunsigned int sequence;int blocktype; -/* Precompute the maximum metadata descriptors in a descriptor block */ -int MAX_BLOCKS_PER_DESC ; -MAX_BLOCKS_PER_DESC = ((journal->j_blocksize-sizeof (journal_header_t)) - /sizeof (journal_block_tag_t)); -/* * First thing is to establish what we expect to find in the log * (in terms of transaction IDs), and where (in terms of log
Re: [PATCH] [17/23] EXT3: Fix set but unused variables
Quoted 7 lines · Andi Kleen
I think it's mostly dead code, but needs more review. I was not fully sure about the jbd case.
- Ted
[email protected]
maintainerRe: [PATCH] [17/23] EXT3: Fix set but unused variables
In reply to Andi KleenQuoted 5 lines · Andi Kleen
Index:
linux-2.6.35-rc2-gcc/fs/jbd/recovery.c
===================================================================
fs/jbd/recovery.c int journal_recover(journal_t *journal) · line 291
Quoted 9 lines
fs/jbd/recovery.c +0
-2@@ -283,12 +283,10 @@ int journal_recover(journal_t *journal)int journal_skip_recovery (journal_t *journal) {int err; -journal_superblock_t * sb;struct recovery_info info;memset (&info,0 ,sizeof (info)); - sb = journal->j_superblock;
Oops, spoke too soon. This will cause a compile error if CONFIG_JBD_DEBUG is defined.
The following pesudo-patch is required:
#ifdef CONFIG_JBD_DEBUG
- int dropped = info.end_transaction - be32_to_cpu(sb->s_sequence);
+ int dropped = info.end_transaction -
+ be32_to_cpu(journal->j_superblock->s_sequence);
#endif
- Ted
Jan Kara
maintainerRe: [PATCH] [17/23] EXT3: Fix set but unused variables
Quoted 22 lines · Andi Kleen
Quoted 5 lines
Index:
linux-2.6.35-rc2-gcc/fs/jbd/recovery.c
===================================================================fs/jbd/recovery.c int journal_recover(journal_t *journal) · line 291
Quoted 9 lines
fs/jbd/recovery.c +0
-2@@ -283,12 +283,10 @@ int journal_recover(journal_t *journal)int journal_skip_recovery (journal_t *journal) {int err; -journal_superblock_t * sb;struct recovery_info info;memset (&info,0 ,sizeof (info)); - sb = journal->j_superblock;Oops, spoke too soon. This will cause a compile error if
CONFIG_JBD_DEBUGis defined.The following pesudo-patch is required:
#ifdef
CONFIG_JBD_DEBUG
Review
Quoted 4 lines
(quoted diff) +2
-1- int dropped = info.end_transaction - be32_to_cpu(sb->s_sequence); + int dropped = info.end_transaction - + be32_to_cpu(journal->j_superblock->s_sequence); #endif
I have merged the patch with your fix to my tree.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SuSE CR Labs
PATCH 18/23 ACPI: Fix unused but set variables in ACPI
In reply to Andi KleenSome minor improvements in error handling, but overall it was mostly dead code.
--- drivers/acpi/acpica/utmutex.c | 5 +---- drivers/acpi/numa.c | 4 +--- drivers/acpi/osl.c | 7 +++---- drivers/acpi/power.c | 6 +----- drivers/acpi/processor_idle.c | 3 +-- drivers/acpi/processor_thermal.c | 2 ++ drivers/acpi/video.c | 10 +++++----- drivers/ata/libata-acpi.c | 6 ------ 8 files changed, 14 insertions(+), 29 deletions(-)
drivers/acpi/processor_thermal.c +2 -0
@@ -506,6 +506,8 @@ static ssize_t acpi_processor_write_limi } result =acpi_processor_apply_limit (pr); +if (result <0 ) +return result;return count; }
drivers/acpi/acpica/utmutex.c +1 -4
@@ -279,13 +279,10 @@ acpi_status acpi_ut_acquire_mutex(acpi_macpi_status acpi_ut_release_mutex (acpi_mutex_handle mutex_id) { -acpi_thread_id this_thread_id; -ACPI_FUNCTION_NAME (ut_release_mutex); - this_thread_id =acpi_os_get_thread_id ();ACPI_DEBUG_PRINT ((ACPI_DB_MUTEX ,"Thread %p releasing Mutex [%s]\n" , -ACPI_CAST_PTR (void, this_thread_id), +ACPI_CAST_PTR (void,acpi_os_get_thread_id ()),acpi_ut_get_mutex_name (mutex_id)));if (mutex_id >ACPI_MAX_MUTEX ) {
drivers/acpi/numa.c +1 -3
@@ -255,12 +255,10 @@ acpi_parse_memory_affinity(struct acpi_sstatic int __init acpi_parse_srat (struct acpi_table_header *table) { -struct acpi_table_srat *srat; -if (!table)return -EINVAL ; - srat = (struct acpi_table_srat *)table; +/* Real work done in acpi_table_parse_srat below. */ return 0 ; }
drivers/acpi/osl.c +3 -4
@@ -141,15 +141,14 @@ static struct osi_linux {static void __init acpi_request_region (struct acpi_generic_address *addr,unsigned int length,char *desc) { -struct resource *res; -if (!addr->address || !length)return ; +/* Resources are never freed */ if (addr->space_id ==ACPI_ADR_SPACE_SYSTEM_IO ) - res =request_region (addr->address, length, desc); +request_region (addr->address, length, desc);else if (addr->space_id ==ACPI_ADR_SPACE_SYSTEM_MEMORY ) - res =request_mem_region (addr->address, length, desc); +request_mem_region (addr->address, length, desc); }static int __init acpi_reserve_resources (void )
drivers/acpi/processor_idle.c +1 -2
@@ -795,13 +795,12 @@ static inline void acpi_idle_do_entry(st }else if (cx->entry_method ==ACPI_CSTATE_HALT ) {acpi_safe_halt (); }else { -int unused;/* IO port based C-state */ inb (cx->address);/* Dummy wait op - must do something useless after P_LVL2 read because chipsets cannot guarantee that STPCLK# signal gets asserted in time to freeze execution properly. */ - unused =inl (acpi_gbl_FADT.xpm_timer_block.address); +inl (acpi_gbl_FADT.xpm_timer_block.address); }start_critical_timings (); }
drivers/acpi/video.c +5 -5
@@ -2140,7 +2140,7 @@ acpi_video_bus_get_devices(struct acpi_v status =acpi_video_bus_get_one_device (dev, video);if (ACPI_FAILURE (status)) { printk(KERN_WARNING PREFIX - "Cant attach device"); + "Cant attach device\n");continue ; } }
@@ -2150,19 +2150,19 @@ acpi_video_bus_get_devices(struct acpi_vstatic int acpi_video_bus_put_one_device (struct acpi_video_device *device) {acpi_status status; -struct acpi_video_bus *video; -if (!device || !device->video)return -ENOENT ; - video = device->video; -acpi_video_device_remove_fs (device->dev); status =acpi_remove_notify_handler (device->dev->handle,ACPI_DEVICE_NOTIFY , acpi_video_device_notify); +if (ACPI_FAILURE (status)) { + printk(KERN_WARNING PREFIX + "Cant remove video notify handler\n"); + }if (device->backlight) {sysfs_remove_link (&device->backlight->dev.kobj,"device" );backlight_device_unregister (device->backlight);
drivers/ata/libata-acpi.c +0 -6
@@ -145,12 +145,6 @@ static void ata_acpi_handle_hotplug(strustruct ata_eh_info *ehi = &ap->link.eh_info;int wait =0 ;unsigned long flags; -acpi_handle handle; - -if (dev) - handle = dev->acpi_handle; -else - handle = ap->acpi_handle;spin_lock_irqsave (ap->lock, flags);/*
drivers/acpi/power.c +1 -5
@@ -553,8 +553,6 @@ static int acpi_power_seq_show(struct seint result =0 , state;struct acpi_power_resource *resource =NULL ;struct list_head *node, *next; -struct acpi_power_reference *ref; - resource = seq->private;
@@ -579,10 +577,8 @@ static int acpi_power_seq_show(struct se }mutex_lock (&resource->resource_lock); -list_for_each_safe (node, next, &resource->reference) { - ref = container_of(node, struct acpi_power_reference, node); +list_for_each_safe (node, next, &resource->reference) count++; - }mutex_unlock (&resource->resource_lock); seq_printf(seq, "system level: S%d\n"
PATCH 19/23 KVM: Fix unused but set warnings
In reply to Andi KleenNo real bugs in this one, the real bug I found is in a separate patch.
--- arch/x86/kvm/paging_tmpl.h | 1 + arch/x86/kvm/vmx.c | 3 +-- virt/kvm/assigned-dev.c | 2 -- 3 files changed, 2 insertions(+), 4 deletions(-)
arch/x86/kvm/paging_tmpl.h +1 -0
@@ -442,6 +442,7 @@ static int FNAME(page_fault)(struct kvm_kvm_mmu_free_some_pages (vcpu); sptep =FNAME (fetch)(vcpu, addr, &walker, user_fault, write_fault, level, &write_pt, pfn); + (void )sptep;pgprintk ("%s: shadow pte %p %llx ptwrite %d\n" , __func__, sptep, *sptep, write_pt);
arch/x86/kvm/vmx.c +1 -2
@@ -1624,10 +1624,9 @@ static void enter_pmode(struct kvm_vcpustatic gva_t rmode_tss_base (struct kvm *kvm) {if (!kvm->arch.tss_addr) { -struct kvm_memslots *slots;gfn_t base_gfn; - slots =kvm_memslots (kvm); +kvm_memslots (kvm); base_gfn = kvm->memslots->memslots[0 ].base_gfn + kvm->memslots->memslots[0 ].npages -3 ;return base_gfn <<PAGE_SHIFT ;
virt/kvm/assigned-dev.c +0 -2
@@ -58,12 +58,10 @@ static int find_index_from_host_irq(strustatic void kvm_assigned_dev_interrupt_work_handler (struct work_struct *work) {struct kvm_assigned_dev_kernel *assigned_dev; -struct kvm *kvm;int i; assigned_dev = container_of(work, struct kvm_assigned_dev_kernel, interrupt_work); - kvm = assigned_dev->kvm;spin_lock_irq (&assigned_dev->assigned_dev_lock);if (assigned_dev->irq_requested_type &KVM_DEV_IRQ_HOST_MSIX ) {
Re: [PATCH] [19/23] KVM: Fix unused but set warnings
Quoted 11 lines · Andi Kleen
No real bugs in this one, the real bug I found is in a separate patch.
Index:
linux-2.6.35-rc2-gcc/arch/x86/kvm/vmx.c
===================================================================arch/x86/kvm/vmx.c +0
-0@@ -1624,10 +1624,9 @@ static void enter_pmode(struct kvm_vcpustatic gva_t rmode_tss_base (struct kvm *kvm)
arch/x86/kvm/vmx.c static void enter_pmode(struct kvm_vcpu · line 1632
Quoted 10 lines
arch/x86/kvm/vmx.c +1
-2{if (!kvm->arch.tss_addr) { -struct kvm_memslots *slots;gfn_t base_gfn; - slots =kvm_memslots (kvm); +kvm_memslots (kvm); base_gfn = kvm->memslots->memslots[0 ].base_gfn + kvm->memslots->memslots[0 ].npages -3 ;return base_gfn<<PAGE_SHIFT ;
I think the base_gfn assignment below needs to use slots to get the rcu dereference correct. I'll apply the patch without this hunk and fix it independently.
Signature · 1 line
error compiling committee.c: too many arguments to function
PATCH 20/23 MM: Fix unused but set warnings
In reply to Andi KleenNo real bugs, just some dead code and some fixups.
--- arch/x86/include/asm/pgtable_64.h | 4 ++-- include/linux/highmem.h | 6 +++++- include/linux/mmdebug.h | 2 +- mm/filemap.c | 2 -- mm/memory.c | 2 -- mm/slab.c | 2 +- 6 files changed, 9 insertions(+), 9 deletions(-)
mm/filemap.c +0 -2
@@ -2238,14 +2238,12 @@ static ssize_t generic_perform_write(str do { struct page *page; - pgoff_t index; /* Pagecache index for current page */ unsigned long offset; /* Offset into pagecache page */ unsigned long bytes; /* Bytes to write to page */ size_t copied; /* Bytes copied from user */ void *fsdata; offset = (pos & (PAGE_CACHE_SIZE - 1)); - index = pos >> PAGE_CACHE_SHIFT; bytes = min_t(unsigned long, PAGE_CACHE_SIZE - offset, iov_iter_count(i));
mm/memory.c +0 -2
@@ -307,7 +307,6 @@ void free_pgd_range(struct mmu_gather *t {pgd_t *pgd;unsigned long next; -unsigned long start;/* * The next few lines have given us lots of grief...
@@ -351,7 +350,6 @@ void free_pgd_range(struct mmu_gather *tif (addr > end -1 )return ; - start = addr; pgd =pgd_offset (tlb->mm, addr); do { next = pgd_addr_end(addr, end);
arch/x86/include/asm/pgtable_64.h +2 -2
@@ -126,8 +126,8 @@ static inline int pgd_large(pgd_t pgd) {/* x86-64 always has all page tables mapped. */ #define pte_offset_map (dir, address) pte_offset_kernel((dir), (address))#define pte_offset_map_nested (dir, address) pte_offset_kernel((dir), (address)) -#define pte_unmap (pte)/* NOP */ -#define pte_unmap_nested (pte)/* NOP */ +#define pte_unmap (pte) ((void)(pte))/* NOP */ +#define pte_unmap_nested (pte) ((void)(pte))/* NOP */ #define update_mmu_cache (vma, address, ptep) do { } while (0)
include/linux/mmdebug.h +1 -1
@@ -4,7 +4,7 @@#ifdef CONFIG_DEBUG_VM #define VM_BUG_ON (cond) BUG_ON(cond)#else -#define VM_BUG_ON (cond) do { } while (0) +#define VM_BUG_ON (cond) do { (void)(cond); } while (0)#endif #ifdef CONFIG_DEBUG_VIRTUAL
mm/slab.c +1 -1
@@ -395,7 +395,7 @@ static void kmem_list3_init(struct kmem_#define STATS_DEC_ACTIVE (x) do { } while (0)#define STATS_INC_ALLOCED (x) do { } while (0)#define STATS_INC_GROWN (x) do { } while (0) -#define STATS_ADD_REAPED (x,y) do { } while (0) +#define STATS_ADD_REAPED (x,y) do { (void)(y); } while (0)#define STATS_SET_HIGH (x) do { } while (0)#define STATS_INC_ERR (x) do { } while (0)#define STATS_INC_NODEALLOCS (x) do { } while (0)
include/linux/highmem.h +5 -1
@@ -72,7 +72,11 @@ static inline void *kmap_atomic(struct p }#define kmap_atomic_prot (page, idx, prot) kmap_atomic(page, idx) -#define kunmap_atomic (addr, idx) do { pagefault_enable(); } while (0) +static inline void kunmap_atomic (void *addr,enum km_type idx) +{ +pagefault_enable (); +} +#define kmap_atomic_pfn (pfn, idx) kmap_atomic(pfn_to_page(pfn), (idx))#define kmap_atomic_to_page (ptr) virt_to_page(ptr)
PATCH 21/23 kernel/*: Fix unused but set warnings
In reply to Andi KleenNo real bugs I believe, just some dead code.
--- kernel/debug/kdb/kdb_bp.c | 2 -- kernel/hrtimer.c | 3 +-- kernel/sched_fair.c | 3 +-- kernel/sysctl.c | 5 +---- kernel/trace/ring_buffer.c | 2 -- 5 files changed, 3 insertions(+), 12 deletions(-)
kernel/sched_fair.c +1 -2
@@ -1311,7 +1311,7 @@ static struct sched_group * find_idlest_group(struct sched_domain *sd, struct task_struct *p, int this_cpu, int load_idx) { - struct sched_group *idlest = NULL, *this = NULL, *group = sd->groups; + struct sched_group *idlest = NULL, *group = sd->groups;unsigned long min_load =ULONG_MAX , this_load =0 ;int imbalance =100 + (sd->imbalance_pct-100 )/2 ;
@@ -1346,7 +1346,6 @@ find_idlest_group(struct sched_domain *sif (local_group) { this_load = avg_load; - this = group; }else if (avg_load < min_load) { min_load = avg_load; idlest = group;
kernel/sysctl.c +1 -4
@@ -1711,10 +1711,7 @@ static __init int sysctl_init(void) {sysctl_set_parent (NULL , root_table);#ifdef CONFIG_SYSCTL_SYSCALL_CHECK - { -int err; - err =sysctl_check_table (current->nsproxy, root_table); - } +sysctl_check_table (current->nsproxy, root_table);#endif return 0 ; }
kernel/hrtimer.c +1 -2
@@ -1096,11 +1096,10 @@ EXPORT_SYMBOL_GPL(hrtimer_cancel);*/ ktime_t hrtimer_get_remaining (const struct hrtimer *timer) { -struct hrtimer_clock_base *base;unsigned long flags;ktime_t rem; - base =lock_hrtimer_base (timer, &flags); +lock_hrtimer_base (timer, &flags); rem =hrtimer_expires_remaining (timer);unlock_hrtimer_base (timer, &flags);
kernel/debug/kdb/kdb_bp.c +0 -2
@@ -274,7 +274,6 @@ static int kdb_bp(int argc, const char *int i, bpno;kdb_bp_t *bp, *bp_check;int diag; -int free;char *symname =NULL ;long offset =0ul ;int nextarg;
@@ -305,7 +304,6 @@ static int kdb_bp(int argc, const char */* * Find an empty bp structure to allocate */ - free =KDB_MAXBPT ;for (bpno =0 , bp = kdb_breakpoints; bpno <KDB_MAXBPT ; bpno++, bp++) {if (bp->bp_free)break ;
kernel/trace/ring_buffer.c +0 -2
@@ -3007,13 +3007,11 @@ static void rb_advance_reader(struct rinstatic void rb_advance_iter (struct ring_buffer_iter *iter) { -struct ring_buffer *buffer;struct ring_buffer_per_cpu *cpu_buffer;struct ring_buffer_event *event;unsigned length; cpu_buffer = iter->cpu_buffer; - buffer = cpu_buffer->buffer;/* * Check if we are at the end of the buffer.
PATCH 22/23 BLOCK: Fix unused but set variables in blk-merge
In reply to Andi KleenJust some dead code.
--- block/blk-merge.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
block/blk-merge.c +1 -2
@@ -12,7 +12,6 @@static unsigned int __blk_recalc_rq_segments (struct request_queue *q,struct bio *bio) { -unsigned int phys_size;struct bio_vec *bv, *bvprv =NULL ;int cluster, i, high, highprv =1 ;unsigned int seg_size, nr_phys_segs;
@@ -24,7 +23,7 @@ static unsigned int __blk_recalc_rq_segm fbio = bio; cluster =test_bit (QUEUE_FLAG_CLUSTER , &q->queue_flags); seg_size =0 ; - phys_size = nr_phys_segs =0 ; + nr_phys_segs =0 ;for_each_bio (bio) {bio_for_each_segment (bv, bio, i) {/*
PATCH 23/23 FS: Fix unused but set warnings
In reply to Andi KleenNo real bugs I believe, just some dead code, and some shut up code.
fs/splice.c +0 -2
@@ -597,7 +597,6 @@ ssize_t default_file_splice_read(structstruct page *pages[PIPE_DEF_BUFFERS ];struct partial_page partial[PIPE_DEF_BUFFERS ];struct iovec *vec, __vec[PIPE_DEF_BUFFERS ]; -pgoff_t index;ssize_t res;size_t this_len;int error;
@@ -621,7 +620,6 @@ ssize_t default_file_splice_read(struct goto shrink_ret; } - index = *ppos >>PAGE_CACHE_SHIFT ; offset = *ppos & ~PAGE_CACHE_MASK ; nr_pages = (len + offset +PAGE_CACHE_SIZE -1 ) >>PAGE_CACHE_SHIFT ;
include/linux/audit.h +1 -1
@@ -544,7 +544,7 @@ extern int audit_signals;#define audit_putname (n) do { ; } while (0)#define __audit_inode (n,d) do { ; } while (0)#define __audit_inode_child (i,p) do { ; } while (0) -#define audit_inode (n,d) do { ; } while (0) +#define audit_inode (n,d) do { (void)(d); } while (0)#define audit_inode_child (i,p) do { ; } while (0)#define audit_core_dumps (i) do { ; } while (0)#define auditsc_get_stamp (c,t,s) (0)
fs/notify/inotify/inotify.c +0 -4
@@ -570,7 +570,6 @@ void inotify_destroy(struct inotify_handwhile (1 ) {struct inotify_watch *watch;struct list_head *watches; -struct super_block *sb;struct inode *inode;mutex_lock (&ih->mutex);
@@ -580,7 +579,6 @@ void inotify_destroy(struct inotify_handbreak ; } watch = list_first_entry(watches, struct inotify_watch, h_list); - sb = watch->inode->i_sb;if (!pin_to_kill (ih, watch))continue ;
@@ -797,7 +795,6 @@ void inotify_evict_watch(struct inotify_int inotify_rm_wd (struct inotify_handle *ih,u32 wd) {struct inotify_watch *watch; -struct super_block *sb;struct inode *inode;mutex_lock (&ih->mutex);
@@ -806,7 +803,6 @@ int inotify_rm_wd(struct inotify_handlemutex_unlock (&ih->mutex);return -EINVAL ; } - sb = watch->inode->i_sb;if (!pin_to_kill (ih, watch))return 0 ;