2011/11/20 Srivatsa S. Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>: > 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@xxxxxxxxx> >>> Signed-off-by: Amit Sahrawat <amit.sahrawat83@xxxxxxxxx> >>> --- >>> 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
