* [PATCH] ext4: remove unneeded variable. @ 2011-11-11 15:02 Namjae Jeon 2011-11-17 9:45 ` Amit Sahrawat 2011-11-19 20:11 ` Srivatsa S. Bhat 0 siblings, 2 replies; 5+ messages in thread From: Namjae Jeon @ 2011-11-11 15:02 UTC (permalink / raw) To: tytso; +Cc: linux-kernel, linux-ext4, Namjae Jeon, Amit Sahrawat ret2 is not needed in ext4_flush_completed_IO(). Signed-off-by: Namjae Jeon <linkinjeon@gmail.com> Signed-off-by: Amit Sahrawat <amit.sahrawat83@gmail.com> --- fs/ext4/fsync.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-) diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c index 00a2cb7..40397ac 100644 --- a/fs/ext4/fsync.c +++ b/fs/ext4/fsync.c @@ -81,7 +81,6 @@ int ext4_flush_completed_IO(struct inode *inode) struct ext4_inode_info *ei = EXT4_I(inode); unsigned long flags; int ret = 0; - int ret2 = 0; dump_completed_IO(inode); spin_lock_irqsave(&ei->i_completed_io_lock, flags); @@ -105,12 +104,10 @@ int ext4_flush_completed_IO(struct inode *inode) */ spin_unlock_irqrestore(&ei->i_completed_io_lock, flags); ret = ext4_end_io_nolock(io); - if (ret < 0) - ret2 = ret; spin_lock_irqsave(&ei->i_completed_io_lock, flags); } spin_unlock_irqrestore(&ei->i_completed_io_lock, flags); - return (ret2 < 0) ? ret2 : 0; + return (ret < 0) ? ret : 0; } /* -- 1.7.4.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ext4: remove unneeded variable. 2011-11-11 15:02 [PATCH] ext4: remove unneeded variable Namjae Jeon @ 2011-11-17 9:45 ` Amit Sahrawat 2011-11-19 20:11 ` Srivatsa S. Bhat 1 sibling, 0 replies; 5+ messages in thread From: Amit Sahrawat @ 2011-11-17 9:45 UTC (permalink / raw) To: Namjae Jeon; +Cc: tytso, linux-kernel, linux-ext4 Any feedback for this patch? We surely don't need 'ret2' variable in this code. Thanks & Regards, Amit Sahrawat On Fri, Nov 11, 2011 at 8:32 PM, Namjae Jeon <linkinjeon@gmail.com> wrote: > ret2 is not needed in ext4_flush_completed_IO(). > > Signed-off-by: Namjae Jeon <linkinjeon@gmail.com> > Signed-off-by: Amit Sahrawat <amit.sahrawat83@gmail.com> > --- > fs/ext4/fsync.c | 5 +---- > 1 files changed, 1 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c > index 00a2cb7..40397ac 100644 > --- a/fs/ext4/fsync.c > +++ b/fs/ext4/fsync.c > @@ -81,7 +81,6 @@ int ext4_flush_completed_IO(struct inode *inode) > struct ext4_inode_info *ei = EXT4_I(inode); > unsigned long flags; > int ret = 0; > - int ret2 = 0; > > dump_completed_IO(inode); > spin_lock_irqsave(&ei->i_completed_io_lock, flags); > @@ -105,12 +104,10 @@ int ext4_flush_completed_IO(struct inode *inode) > */ > spin_unlock_irqrestore(&ei->i_completed_io_lock, flags); > ret = ext4_end_io_nolock(io); > - if (ret < 0) > - ret2 = ret; > spin_lock_irqsave(&ei->i_completed_io_lock, flags); > } > spin_unlock_irqrestore(&ei->i_completed_io_lock, flags); > - return (ret2 < 0) ? ret2 : 0; > + return (ret < 0) ? ret : 0; > } > > /* > -- > 1.7.4.4 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ext4: remove unneeded variable. 2011-11-11 15:02 [PATCH] ext4: remove unneeded variable Namjae Jeon 2011-11-17 9:45 ` Amit Sahrawat @ 2011-11-19 20:11 ` Srivatsa S. Bhat 2011-11-19 21:09 ` Srivatsa S. Bhat 1 sibling, 1 reply; 5+ messages in thread From: Srivatsa S. Bhat @ 2011-11-19 20:11 UTC (permalink / raw) To: Namjae Jeon; +Cc: tytso, linux-kernel, linux-ext4, Amit Sahrawat On 11/11/2011 08:32 PM, Namjae Jeon wrote: > ret2 is not needed in ext4_flush_completed_IO(). > Not needed? I went through the code briefly, and I don't agree. > Signed-off-by: Namjae Jeon <linkinjeon@gmail.com> > Signed-off-by: Amit Sahrawat <amit.sahrawat83@gmail.com> > --- > fs/ext4/fsync.c | 5 +---- > 1 files changed, 1 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c > index 00a2cb7..40397ac 100644 > --- a/fs/ext4/fsync.c > +++ b/fs/ext4/fsync.c > @@ -81,7 +81,6 @@ int ext4_flush_completed_IO(struct inode *inode) > struct ext4_inode_info *ei = EXT4_I(inode); > unsigned long flags; > int ret = 0; > - int ret2 = 0; > > dump_completed_IO(inode); > spin_lock_irqsave(&ei->i_completed_io_lock, flags); > @@ -105,12 +104,10 @@ int ext4_flush_completed_IO(struct inode *inode) > */ > spin_unlock_irqrestore(&ei->i_completed_io_lock, flags); > ret = ext4_end_io_nolock(io); > - if (ret < 0) > - ret2 = ret; > spin_lock_irqsave(&ei->i_completed_io_lock, flags); > } > spin_unlock_irqrestore(&ei->i_completed_io_lock, flags); > - return (ret2 < 0) ? ret2 : 0; > + return (ret < 0) ? ret : 0; > } Please note that there is a while loop involved here. Which means, that ret2 is used to store the last negative value of ret. And due to the loop, ret can be over-written in the next loop iteration, which we can afford, because we have already stored what we need to save, in ret2. And this ret2 value is used to return appropriate value to the caller. Thanks, Srivatsa S. Bhat ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ext4: remove unneeded variable. 2011-11-19 20:11 ` Srivatsa S. Bhat @ 2011-11-19 21:09 ` Srivatsa S. Bhat 2011-11-19 21:53 ` NamJae Jeon 0 siblings, 1 reply; 5+ messages in thread From: Srivatsa S. Bhat @ 2011-11-19 21:09 UTC (permalink / raw) To: Namjae Jeon; +Cc: tytso, linux-kernel, linux-ext4, Amit Sahrawat On 11/20/2011 01:41 AM, Srivatsa S. Bhat wrote: > On 11/11/2011 08:32 PM, Namjae Jeon wrote: >> ret2 is not needed in ext4_flush_completed_IO(). >> > > Not needed? I went through the code briefly, and I don't agree. > >> Signed-off-by: Namjae Jeon <linkinjeon@gmail.com> >> Signed-off-by: Amit Sahrawat <amit.sahrawat83@gmail.com> >> --- >> fs/ext4/fsync.c | 5 +---- >> 1 files changed, 1 insertions(+), 4 deletions(-) >> >> diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c >> index 00a2cb7..40397ac 100644 >> --- a/fs/ext4/fsync.c >> +++ b/fs/ext4/fsync.c >> @@ -81,7 +81,6 @@ int ext4_flush_completed_IO(struct inode *inode) >> struct ext4_inode_info *ei = EXT4_I(inode); >> unsigned long flags; >> int ret = 0; >> - int ret2 = 0; >> >> dump_completed_IO(inode); >> spin_lock_irqsave(&ei->i_completed_io_lock, flags); >> @@ -105,12 +104,10 @@ int ext4_flush_completed_IO(struct inode *inode) >> */ >> spin_unlock_irqrestore(&ei->i_completed_io_lock, flags); >> ret = ext4_end_io_nolock(io); >> - if (ret < 0) >> - ret2 = ret; >> spin_lock_irqsave(&ei->i_completed_io_lock, flags); >> } >> spin_unlock_irqrestore(&ei->i_completed_io_lock, flags); >> - return (ret2 < 0) ? ret2 : 0; >> + return (ret < 0) ? ret : 0; >> } > > Please note that there is a while loop involved here. Which means, that ret2 > is used to store the last negative value of ret. And due to the loop, ret can > be over-written in the next loop iteration, which we can afford, because we > have already stored what we need to save, in ret2. And this ret2 value is used > to return appropriate value to the caller. > Actually, what I really meant was, removing ret2 as merely "unneeded" might not be the right thing to do because once you apply your patch, you end up altering the value returned by this function! If the return value is indeed wrong in the current code, you should rather be saying that this is a bug fix, with appropriate justification IMO. Thanks, Srivatsa S. Bhat ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ext4: remove unneeded variable. 2011-11-19 21:09 ` Srivatsa S. Bhat @ 2011-11-19 21:53 ` NamJae Jeon 0 siblings, 0 replies; 5+ messages in thread From: NamJae Jeon @ 2011-11-19 21:53 UTC (permalink / raw) To: Srivatsa S. Bhat; +Cc: tytso, linux-kernel, linux-ext4, Amit Sahrawat 2011/11/20 Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>: > On 11/20/2011 01:41 AM, Srivatsa S. Bhat wrote: >> On 11/11/2011 08:32 PM, Namjae Jeon wrote: >>> ret2 is not needed in ext4_flush_completed_IO(). >>> >> >> Not needed? I went through the code briefly, and I don't agree. >> >>> Signed-off-by: Namjae Jeon <linkinjeon@gmail.com> >>> Signed-off-by: Amit Sahrawat <amit.sahrawat83@gmail.com> >>> --- >>> fs/ext4/fsync.c | 5 +---- >>> 1 files changed, 1 insertions(+), 4 deletions(-) >>> >>> diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c >>> index 00a2cb7..40397ac 100644 >>> --- a/fs/ext4/fsync.c >>> +++ b/fs/ext4/fsync.c >>> @@ -81,7 +81,6 @@ int ext4_flush_completed_IO(struct inode *inode) >>> struct ext4_inode_info *ei = EXT4_I(inode); >>> unsigned long flags; >>> int ret = 0; >>> - int ret2 = 0; >>> >>> dump_completed_IO(inode); >>> spin_lock_irqsave(&ei->i_completed_io_lock, flags); >>> @@ -105,12 +104,10 @@ int ext4_flush_completed_IO(struct inode *inode) >>> */ >>> spin_unlock_irqrestore(&ei->i_completed_io_lock, flags); >>> ret = ext4_end_io_nolock(io); >>> - if (ret < 0) >>> - ret2 = ret; >>> spin_lock_irqsave(&ei->i_completed_io_lock, flags); >>> } >>> spin_unlock_irqrestore(&ei->i_completed_io_lock, flags); >>> - return (ret2 < 0) ? ret2 : 0; >>> + return (ret < 0) ? ret : 0; >>> } >> >> Please note that there is a while loop involved here. Which means, that ret2 >> is used to store the last negative value of ret. And due to the loop, ret can >> be over-written in the next loop iteration, which we can afford, because we >> have already stored what we need to save, in ret2. And this ret2 value is used >> to return appropriate value to the caller. >> > > Actually, what I really meant was, removing ret2 as merely "unneeded" might not > be the right thing to do because once you apply your patch, you end up altering > the value returned by this function! > > If the return value is indeed wrong in the current code, you should rather > be saying that this is a bug fix, with appropriate justification IMO. > Thanks for your review. My thinking was reaching for to while loop(). I will remember your advice. Thanks Srivatsa. > Thanks, > Srivatsa S. Bhat > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-11-19 21:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-11-11 15:02 [PATCH] ext4: remove unneeded variable Namjae Jeon 2011-11-17 9:45 ` Amit Sahrawat 2011-11-19 20:11 ` Srivatsa S. Bhat 2011-11-19 21:09 ` Srivatsa S. Bhat 2011-11-19 21:53 ` NamJae Jeon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox