Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750889Ab0FNE1I (ORCPT ); Mon, 14 Jun 2010 00:27:08 -0400 Received: from bld-mail19.adl2.internode.on.net ([150.101.137.104]:58304 "EHLO mail.internode.on.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750744Ab0FNE1G (ORCPT ); Mon, 14 Jun 2010 00:27:06 -0400 Date: Mon, 14 Jun 2010 14:27:00 +1000 From: Dave Chinner To: Andi Kleen Cc: xfs@oss.sgi.com, akpm@linux-foundation.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] [16/23] XFS: Fix gcc 4.6 set but not read and unused statement warnings Message-ID: <20100614042700.GC6590@dastard> References: <20100610110.764742110@firstfloor.org> <20100610111052.3DDC5B1A2B@basil.firstfloor.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100610111052.3DDC5B1A2B@basil.firstfloor.org> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8351 Lines: 220 On Thu, Jun 10, 2010 at 01:10:52PM +0200, Andi Kleen wrote: > > 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.c while > modifying the superblock it would not be handled. See my comments about these below. > 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... > 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); > Cc: xfs@oss.sgi.com > > Signed-off-by: Andi Kleen > > --- > 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 > =================================================================== > --- linux-2.6.35-rc2-gcc.orig/fs/xfs/linux-2.6/xfs_sync.c > +++ linux-2.6.35-rc2-gcc/fs/xfs/linux-2.6/xfs_sync.c > @@ -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? > Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_da_btree.c > =================================================================== > --- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_da_btree.c > +++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_da_btree.c > @@ -581,10 +581,8 @@ xfs_da_node_add(xfs_da_state_t *state, x > xfs_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. > Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_dir2_block.c > =================================================================== > --- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_dir2_block.c > +++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_dir2_block.c > @@ -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); > /* Just remove the buf_len variable in this case. > Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_filestream.c > =================================================================== > --- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_filestream.c > +++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_filestream.c > @@ -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)); 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. > #define xfs_trans_unreserve_quota_nblks(tp, ip, nblks, ninos, flags) \ > Index: linux-2.6.35-rc2-gcc/fs/xfs/xfs_trans.c > =================================================================== > --- linux-2.6.35-rc2-gcc.orig/fs/xfs/xfs_trans.c > +++ linux-2.6.35-rc2-gcc/fs/xfs/xfs_trans.c > @@ -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. -- Dave Chinner david@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/