* [PATCH] XFS: remove pointless conditional testing 'nmp' vs NULL in fs/xfs/xfs_rtalloc.c::xfs_growfs_rt() @ 2006-08-12 22:16 Jesper Juhl 2006-08-14 1:09 ` Nathan Scott 0 siblings, 1 reply; 5+ messages in thread From: Jesper Juhl @ 2006-08-12 22:16 UTC (permalink / raw) To: linux-kernel; +Cc: xfs-masters, xfs, nathans, Jesper Juhl In fs/xfs/xfs_rtalloc.c::xfs_growfs_rt() there's a completely useless conditional at the error_exit label. The 'if (nmp)' check is pointless and might as well be removed for two reasons. 1) if 'nmp' is NULL then kmem_free() will end up calling kfree() with a NULL argument - which in turn will just cause a return from kfree(). No harm done. 2) At the beginning of the function there's an assignment; '*nmp = *mp;' so if 'nmp' was NULL we'd already have blown up due to dereferencing a NULL pointer. This patch gets rid of the pointless check. Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com> --- fs/xfs/xfs_rtalloc.c | 3 +-- 1 files changed, 1 insertion(+), 2 deletions(-) --- linux-2.6.18-rc4-orig/fs/xfs/xfs_rtalloc.c 2006-08-11 00:11:13.000000000 +0200 +++ linux-2.6.18-rc4/fs/xfs/xfs_rtalloc.c 2006-08-13 00:07:43.000000000 +0200 @@ -2107,8 +2107,7 @@ xfs_growfs_rt( * Error paths come here. */ error_exit: - if (nmp) - kmem_free(nmp, sizeof(*nmp)); + kmem_free(nmp, sizeof(*nmp)); xfs_trans_cancel(tp, cancelflags); return error; } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] XFS: remove pointless conditional testing 'nmp' vs NULL in fs/xfs/xfs_rtalloc.c::xfs_growfs_rt() 2006-08-12 22:16 [PATCH] XFS: remove pointless conditional testing 'nmp' vs NULL in fs/xfs/xfs_rtalloc.c::xfs_growfs_rt() Jesper Juhl @ 2006-08-14 1:09 ` Nathan Scott 2006-08-14 7:25 ` Jesper Juhl 0 siblings, 1 reply; 5+ messages in thread From: Nathan Scott @ 2006-08-14 1:09 UTC (permalink / raw) To: Jesper Juhl; +Cc: linux-kernel, xfs-masters, xfs On Sun, Aug 13, 2006 at 12:16:50AM +0200, Jesper Juhl wrote: > In fs/xfs/xfs_rtalloc.c::xfs_growfs_rt() there's a completely useless > conditional at the error_exit label. > The 'if (nmp)' check is pointless and might as well be removed for two > reasons. > > 1) if 'nmp' is NULL then kmem_free() will end up calling kfree() with a NULL > argument - which in turn will just cause a return from kfree(). No harm > done. Thats valid. > 2) At the beginning of the function there's an assignment; '*nmp = *mp;' so Thats not. Theres no assignment at the start of the function; theres one inside the main body of the loop 20+ lines into it, and right after a mem alloc with flags requiring no failure. Later that local variable is freed then set to NULL inside the loop, before continuing the next iteration... Really this code would be better if reworked slightly to just allocate nmp once before entering the loop, and then free it once at the end... we wouldn't need a goto, just a few breaks in the loop and a conditional transaction cancel. > This patch gets rid of the pointless check. Hmm, seems like code churn that makes the code slightly less obvious, but thats just me... I'd prefer a tested patch that implements the above suggestion, to be honest. :) cheers. -- Nathan ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] XFS: remove pointless conditional testing 'nmp' vs NULL in fs/xfs/xfs_rtalloc.c::xfs_growfs_rt() 2006-08-14 1:09 ` Nathan Scott @ 2006-08-14 7:25 ` Jesper Juhl 2006-08-16 20:44 ` Jesper Juhl 0 siblings, 1 reply; 5+ messages in thread From: Jesper Juhl @ 2006-08-14 7:25 UTC (permalink / raw) To: Nathan Scott; +Cc: linux-kernel, xfs-masters, xfs On 14/08/06, Nathan Scott <nathans@sgi.com> wrote: > On Sun, Aug 13, 2006 at 12:16:50AM +0200, Jesper Juhl wrote: > > In fs/xfs/xfs_rtalloc.c::xfs_growfs_rt() there's a completely useless > > conditional at the error_exit label. > > The 'if (nmp)' check is pointless and might as well be removed for two > > reasons. > > > > 1) if 'nmp' is NULL then kmem_free() will end up calling kfree() with a NULL > > argument - which in turn will just cause a return from kfree(). No harm > > done. > > Thats valid. > > > 2) At the beginning of the function there's an assignment; '*nmp = *mp;' so > > Thats not. Theres no assignment at the start of the function; > theres one inside the main body of the loop 20+ lines into it, > and right after a mem alloc with flags requiring no failure. > Later that local variable is freed then set to NULL inside the > loop, before continuing the next iteration... > > Really this code would be better if reworked slightly to just > allocate nmp once before entering the loop, and then free it > once at the end... we wouldn't need a goto, just a few breaks > in the loop and a conditional transaction cancel. > > > This patch gets rid of the pointless check. > > Hmm, seems like code churn that makes the code slightly less > obvious, but thats just me... I'd prefer a tested patch that > implements the above suggestion, to be honest. :) > Ok, I'll see what I can come up with. -- Jesper Juhl <jesper.juhl@gmail.com> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please http://www.expita.com/nomime.html ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] XFS: remove pointless conditional testing 'nmp' vs NULL in fs/xfs/xfs_rtalloc.c::xfs_growfs_rt() 2006-08-14 7:25 ` Jesper Juhl @ 2006-08-16 20:44 ` Jesper Juhl 2006-08-17 6:31 ` Nathan Scott 0 siblings, 1 reply; 5+ messages in thread From: Jesper Juhl @ 2006-08-16 20:44 UTC (permalink / raw) To: Nathan Scott; +Cc: linux-kernel, xfs-masters, xfs On Monday 14 August 2006 09:25, Jesper Juhl wrote: > On 14/08/06, Nathan Scott <nathans@sgi.com> wrote: > > On Sun, Aug 13, 2006 at 12:16:50AM +0200, Jesper Juhl wrote: > > > > Really this code would be better if reworked slightly to just > > allocate nmp once before entering the loop, and then free it > > once at the end... we wouldn't need a goto, just a few breaks > > in the loop and a conditional transaction cancel. > > > > > This patch gets rid of the pointless check. > > > > Hmm, seems like code churn that makes the code slightly less > > obvious, but thats just me... I'd prefer a tested patch that > > implements the above suggestion, to be honest. :) > > > Ok, I'll see what I can come up with. > How this? Compile tested only since I'm at home and don't have any XFS filesystems to play with atm. Rework fs/xfs/xfs_rtalloc.c::xfs_growfs_rt() to allocate and free 'nmp' just once and make the error handling a bit clearer. Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com> --- fs/xfs/xfs_rtalloc.c | 37 +++++++++++++++---------------------- 1 files changed, 15 insertions(+), 22 deletions(-) --- linux-2.6.18-rc4-orig/fs/xfs/xfs_rtalloc.c 2006-08-11 00:11:13.000000000 +0200 +++ linux-2.6.18-rc4/fs/xfs/xfs_rtalloc.c 2006-08-16 22:36:03.000000000 +0200 @@ -1976,7 +1976,11 @@ xfs_growfs_rt( if ((error = xfs_growfs_rt_alloc(mp, rsumblocks, nrsumblocks, mp->m_sb.sb_rsumino))) return error; - nmp = NULL; + + /* + * Allocate a new (fake) mount/sb. + */ + nmp = kmem_alloc(sizeof(*nmp), KM_SLEEP); /* * Loop over the bitmap blocks. * We will do everything one bitmap block at a time. @@ -1987,10 +1991,6 @@ xfs_growfs_rt( ((sbp->sb_rextents & ((1 << mp->m_blkbit_log) - 1)) != 0); bmbno < nrbmblocks; bmbno++) { - /* - * Allocate a new (fake) mount/sb. - */ - nmp = kmem_alloc(sizeof(*nmp), KM_SLEEP); *nmp = *mp; nsbp = &nmp->m_sb; /* @@ -2018,13 +2018,13 @@ xfs_growfs_rt( cancelflags = 0; if ((error = xfs_trans_reserve(tp, 0, XFS_GROWRTFREE_LOG_RES(nmp), 0, 0, 0))) - goto error_exit; + break; /* * Lock out other callers by grabbing the bitmap inode lock. */ if ((error = xfs_trans_iget(mp, tp, mp->m_sb.sb_rbmino, 0, XFS_ILOCK_EXCL, &ip))) - goto error_exit; + break; ASSERT(ip == mp->m_rbmip); /* * Update the bitmap inode's size. @@ -2038,7 +2038,7 @@ xfs_growfs_rt( */ if ((error = xfs_trans_iget(mp, tp, mp->m_sb.sb_rsumino, 0, XFS_ILOCK_EXCL, &ip))) - goto error_exit; + break; ASSERT(ip == mp->m_rsumip); /* * Update the summary inode's size. @@ -2053,7 +2053,7 @@ xfs_growfs_rt( mp->m_rsumlevels != nmp->m_rsumlevels) { error = xfs_rtcopy_summary(mp, nmp, tp); if (error) - goto error_exit; + break; } /* * Update superblock fields. @@ -2080,18 +2080,13 @@ xfs_growfs_rt( error = xfs_rtfree_range(nmp, tp, sbp->sb_rextents, nsbp->sb_rextents - sbp->sb_rextents, &bp, &sumbno); if (error) - goto error_exit; + break; /* * Mark more blocks free in the superblock. */ xfs_trans_mod_sb(tp, XFS_TRANS_SB_FREXTENTS, nsbp->sb_rextents - sbp->sb_rextents); /* - * Free the fake mp structure. - */ - kmem_free(nmp, sizeof(*nmp)); - nmp = NULL; - /* * Update mp values into the real mp structure. */ mp->m_rsumlevels = nrsumlevels; @@ -2101,15 +2096,13 @@ xfs_growfs_rt( */ xfs_trans_commit(tp, 0, NULL); } - return 0; - + if (error) + xfs_trans_cancel(tp, cancelflags); /* - * Error paths come here. + * Free the fake mp structure. */ -error_exit: - if (nmp) - kmem_free(nmp, sizeof(*nmp)); - xfs_trans_cancel(tp, cancelflags); + kmem_free(nmp, sizeof(*nmp)); + return error; } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] XFS: remove pointless conditional testing 'nmp' vs NULL in fs/xfs/xfs_rtalloc.c::xfs_growfs_rt() 2006-08-16 20:44 ` Jesper Juhl @ 2006-08-17 6:31 ` Nathan Scott 0 siblings, 0 replies; 5+ messages in thread From: Nathan Scott @ 2006-08-17 6:31 UTC (permalink / raw) To: Jesper Juhl; +Cc: linux-kernel, xfs-masters, xfs On Wed, Aug 16, 2006 at 10:44:19PM +0200, Jesper Juhl wrote: > On Monday 14 August 2006 09:25, Jesper Juhl wrote: > > On 14/08/06, Nathan Scott <nathans@sgi.com> wrote: > > > On Sun, Aug 13, 2006 at 12:16:50AM +0200, Jesper Juhl wrote: > > > > > > > This patch gets rid of the pointless check. > > > > > > Hmm, seems like code churn that makes the code slightly less > > > obvious, but thats just me... I'd prefer a tested patch that > > > implements the above suggestion, to be honest. :) > > > > > Ok, I'll see what I can come up with. > > > > How this? > > Compile tested only since I'm at home and don't have any XFS filesystems to > play with atm. It looks good. I've tested it, and it works fine - I'll merge it into my tree shortly. Thanks for following up on this. cheers. -- Nathan ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2006-08-17 6:31 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-08-12 22:16 [PATCH] XFS: remove pointless conditional testing 'nmp' vs NULL in fs/xfs/xfs_rtalloc.c::xfs_growfs_rt() Jesper Juhl 2006-08-14 1:09 ` Nathan Scott 2006-08-14 7:25 ` Jesper Juhl 2006-08-16 20:44 ` Jesper Juhl 2006-08-17 6:31 ` Nathan Scott
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox