[Ocfs2-devel] [PATCH 1/1] ocfs2: remove redundant and incorrect mlog_error

[Ocfs2-devel] [PATCH 1/1] ocfs2: remove redundant and incorrect mlog_error

[Tiger Yang]

We have already mlog all error and positive status is not error.

Signed-off-by: Tiger Yang <tiger.yang@oracle.com>
---
 fs/ocfs2/quota_global.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
index 92fcd57..0a86e30 100644
--- a/fs/ocfs2/quota_global.c
+++ b/fs/ocfs2/quota_global.c
@@ -399,8 +399,6 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
 			      msecs_to_jiffies(oinfo->dqi_syncms));
 
 out_err:
-	if (status)
-		mlog_errno(status);
 	return status;
 out_unlock:
 	ocfs2_unlock_global_qf(oinfo, 0);
-- 
1.7.4.4

[Ocfs2-devel] [PATCH 1/1] ocfs2: remove redundant and incorrect mlog_error

[Joel Becker]

On Sat, May 28, 2011 at 12:34:52AM +0800, Tiger Yang wrote:
> We have already mlog all error and positive status is not error.

	Positive status is turned into -EIO.  There are actually a
couple of places in this function that do not mlog_errno(status) and
rely on this print.  I think you should add them to your patch.
	For example:

371         status = ocfs2_qinfo_lock(oinfo, 0);
372         if (status < 0)
373                 goto out_unlock;

Joel
 
> Signed-off-by: Tiger Yang <tiger.yang@oracle.com>
> ---
>  fs/ocfs2/quota_global.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
> index 92fcd57..0a86e30 100644
> --- a/fs/ocfs2/quota_global.c
> +++ b/fs/ocfs2/quota_global.c
> @@ -399,8 +399,6 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
>  			      msecs_to_jiffies(oinfo->dqi_syncms));
>  
>  out_err:
> -	if (status)
> -		mlog_errno(status);
>  	return status;
>  out_unlock:
>  	ocfs2_unlock_global_qf(oinfo, 0);
> -- 
> 1.7.4.4
> 

-- 

"Copy from one, it's plagiarism; copy from two, it's research."
        - Wilson Mizner

			http://www.jlbec.org/
			jlbec at evilplan.org

[Ocfs2-devel] [PATCH 1/1] ocfs2: remove redundant and incorrect mlog_error

[Tiger Yang]

On 06/01/2011 09:43 AM, Joel Becker wrote:
> On Sat, May 28, 2011 at 12:34:52AM +0800, Tiger Yang wrote:
>> We have already mlog all error and positive status is not error.
> 	Positive status is turned into -EIO.  There are actually a
> couple of places in this function that do not mlog_errno(status) and
> rely on this print.  I think you should add them to your patch.
> 	For example:
>
> 371         status = ocfs2_qinfo_lock(oinfo, 0);
> 372         if (status<  0)
> 373                 goto out_unlock;
>
> Joel
>
>
Hi, Joel,
There is something devious about this function.
1 If status == sizeof(struct ocfs2_global_disk_dqinfo)) then positive 
status will be return.
2 After goto out_unlock, they goto out_err again.
I make a new one to fix this.  please review the attached patch.

Thanks,
Tiger


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-ocfs2-remove-redundant-and-incorrect-mlog_error.patch
Url: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20110602/88479be9/attachment.pl 

[Ocfs2-devel] [PATCH 1/1] ocfs2: remove redundant and incorrect mlog_error

[Tiger Yang]

out_err already logged all the negative status, so remove mlog_errno
before goto there. If quota_read succeed, positive (non zero)
status should be return and it's not a error.

Signed-off-by: Tiger Yang <tiger.yang@oracle.com>
---
 fs/ocfs2/quota_global.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
index 92fcd57..cb0f017 100644
--- a/fs/ocfs2/quota_global.c
+++ b/fs/ocfs2/quota_global.c
@@ -358,10 +358,8 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
 	oinfo->dqi_gqi_count = 0;
 	oinfo->dqi_gqinode = gqinode;
 	status = ocfs2_lock_global_qf(oinfo, 0);
-	if (status < 0) {
-		mlog_errno(status);
+	if (status < 0)
 		goto out_err;
-	}
 
 	status = ocfs2_extent_map_get_blocks(gqinode, 0, &oinfo->dqi_giblk,
 					     &pcount, NULL);
@@ -381,7 +379,6 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
 		     status);
 		if (status >= 0)
 			status = -EIO;
-		mlog_errno(status);
 		goto out_err;
 	}
 	info->dqi_bgrace = le32_to_cpu(dinfo.dqi_bgrace);
@@ -398,14 +395,12 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
 	schedule_delayed_work(&oinfo->dqi_sync_work,
 			      msecs_to_jiffies(oinfo->dqi_syncms));
 
-out_err:
-	if (status)
-		mlog_errno(status);
 	return status;
 out_unlock:
 	ocfs2_unlock_global_qf(oinfo, 0);
+out_err:
 	mlog_errno(status);
-	goto out_err;
+	return status;
 }
 
 /* Write information to global quota file. Expects exlusive lock on quota
-- 
1.7.4.4

[Ocfs2-devel] [PATCH 1/1] ocfs2: remove redundant and incorrect mlog_error

[Joel Becker]

I have Jan's patch, which keeps mlog_errno near the errors.  this is
good for locating the problem.

Joel

On Mon, Jun 25, 2012 at 04:03:24PM +0800, Tiger Yang wrote:
> out_err already logged all the negative status, so remove mlog_errno
> before goto there. If quota_read succeed, positive (non zero)
> status should be return and it's not a error.
> 
> Signed-off-by: Tiger Yang <tiger.yang@oracle.com>
> ---
>  fs/ocfs2/quota_global.c |   11 +++--------
>  1 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
> index 92fcd57..cb0f017 100644
> --- a/fs/ocfs2/quota_global.c
> +++ b/fs/ocfs2/quota_global.c
> @@ -358,10 +358,8 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
>  	oinfo->dqi_gqi_count = 0;
>  	oinfo->dqi_gqinode = gqinode;
>  	status = ocfs2_lock_global_qf(oinfo, 0);
> -	if (status < 0) {
> -		mlog_errno(status);
> +	if (status < 0)
>  		goto out_err;
> -	}
>  
>  	status = ocfs2_extent_map_get_blocks(gqinode, 0, &oinfo->dqi_giblk,
>  					     &pcount, NULL);
> @@ -381,7 +379,6 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
>  		     status);
>  		if (status >= 0)
>  			status = -EIO;
> -		mlog_errno(status);
>  		goto out_err;
>  	}
>  	info->dqi_bgrace = le32_to_cpu(dinfo.dqi_bgrace);
> @@ -398,14 +395,12 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
>  	schedule_delayed_work(&oinfo->dqi_sync_work,
>  			      msecs_to_jiffies(oinfo->dqi_syncms));
>  
> -out_err:
> -	if (status)
> -		mlog_errno(status);
>  	return status;
>  out_unlock:
>  	ocfs2_unlock_global_qf(oinfo, 0);
> +out_err:
>  	mlog_errno(status);
> -	goto out_err;
> +	return status;
>  }
>  
>  /* Write information to global quota file. Expects exlusive lock on quota
> -- 
> 1.7.4.4
> 

-- 

Life's Little Instruction Book #451

	"Don't be afraid to say, 'I'm sorry.'"

			http://www.jlbec.org/
			jlbec at evilplan.org

[Ocfs2-devel] [PATCH 1/1] ocfs2: remove redundant and incorrect mlog_error

[Joel Becker]

On Thu, Jun 02, 2011 at 12:43:40PM +0800, Tiger Yang wrote:
> On 06/01/2011 09:43 AM, Joel Becker wrote:
> >On Sat, May 28, 2011 at 12:34:52AM +0800, Tiger Yang wrote:
> >>We have already mlog all error and positive status is not error.
> >	Positive status is turned into -EIO.  There are actually a
> >couple of places in this function that do not mlog_errno(status) and
> >rely on this print.  I think you should add them to your patch.
> >	For example:
> >
> >371         status = ocfs2_qinfo_lock(oinfo, 0);
> >372         if (status<  0)
> >373                 goto out_unlock;
> >
> >Joel
> >
> >
> Hi, Joel,
> There is something devious about this function.
> 1 If status == sizeof(struct ocfs2_global_disk_dqinfo)) then
> positive status will be return.
> 2 After goto out_unlock, they goto out_err again.
> I make a new one to fix this.  please review the attached patch.
> 
> Thanks,
> Tiger
> 
> 

> >From 36951746bb68250ce13a62fd2a3290fa8af30c54 Mon Sep 17 00:00:00 2001
> From: Tiger Yang <tiger.yang@oracle.com>
> Date: Thu, 2 Jun 2011 11:27:11 +0800
> Subject: [PATCH 1/1] ocfs2: remove redundant and incorrect mlog_error
> 
> We have already mlog all error and positive status is not error.
> 
> Signed-off-by: Tiger Yang <tiger.yang@oracle.com>

This patch is now part of the fixes branch of ocfs2.git.

Joel

> ---
>  fs/ocfs2/quota_global.c |   11 +++--------
>  1 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
> index 92fcd57..cb0f017 100644
> --- a/fs/ocfs2/quota_global.c
> +++ b/fs/ocfs2/quota_global.c
> @@ -358,10 +358,8 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
>  	oinfo->dqi_gqi_count = 0;
>  	oinfo->dqi_gqinode = gqinode;
>  	status = ocfs2_lock_global_qf(oinfo, 0);
> -	if (status < 0) {
> -		mlog_errno(status);
> +	if (status < 0)
>  		goto out_err;
> -	}
>  
>  	status = ocfs2_extent_map_get_blocks(gqinode, 0, &oinfo->dqi_giblk,
>  					     &pcount, NULL);
> @@ -381,7 +379,6 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
>  		     status);
>  		if (status >= 0)
>  			status = -EIO;
> -		mlog_errno(status);
>  		goto out_err;
>  	}
>  	info->dqi_bgrace = le32_to_cpu(dinfo.dqi_bgrace);
> @@ -398,14 +395,12 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
>  	schedule_delayed_work(&oinfo->dqi_sync_work,
>  			      msecs_to_jiffies(oinfo->dqi_syncms));
>  
> -out_err:
> -	if (status)
> -		mlog_errno(status);
>  	return status;
>  out_unlock:
>  	ocfs2_unlock_global_qf(oinfo, 0);
> +out_err:
>  	mlog_errno(status);
> -	goto out_err;
> +	return status;
>  }
>  
>  /* Write information to global quota file. Expects exlusive lock on quota
> -- 
> 1.7.4.4
> 


-- 

"The opposite of a correct statement is a false statement. The
 opposite of a profound truth may well be another profound truth."
         - Niels Bohr 

			http://www.jlbec.org/
			jlbec at evilplan.org

[Ocfs2-devel] [PATCH 1/1] ocfs2: remove redundant and incorrect mlog_error

[Joel Becker]

On Tue, Aug 14, 2012 at 11:37:11PM -0700, Joel Becker wrote:
> On Thu, Jun 02, 2011 at 12:43:40PM +0800, Tiger Yang wrote:
> > On 06/01/2011 09:43 AM, Joel Becker wrote:
> > >On Sat, May 28, 2011 at 12:34:52AM +0800, Tiger Yang wrote:
> > >>We have already mlog all error and positive status is not error.
> > >	Positive status is turned into -EIO.  There are actually a
> > >couple of places in this function that do not mlog_errno(status) and
> > >rely on this print.  I think you should add them to your patch.
> > >	For example:
> > >
> > >371         status = ocfs2_qinfo_lock(oinfo, 0);
> > >372         if (status<  0)
> > >373                 goto out_unlock;
> > >
> > >Joel
> > >
> > >
> > Hi, Joel,
> > There is something devious about this function.
> > 1 If status == sizeof(struct ocfs2_global_disk_dqinfo)) then
> > positive status will be return.
> > 2 After goto out_unlock, they goto out_err again.
> > I make a new one to fix this.  please review the attached patch.
> > 
> > Thanks,
> > Tiger
> > 
> > 
> 
> > >From 36951746bb68250ce13a62fd2a3290fa8af30c54 Mon Sep 17 00:00:00 2001
> > From: Tiger Yang <tiger.yang@oracle.com>
> > Date: Thu, 2 Jun 2011 11:27:11 +0800
> > Subject: [PATCH 1/1] ocfs2: remove redundant and incorrect mlog_error
> > 
> > We have already mlog all error and positive status is not error.
> > 
> > Signed-off-by: Tiger Yang <tiger.yang@oracle.com>
> 
> This patch is now part of the fixes branch of ocfs2.git.

I lied.  It conflicted and did not get pushed.  This is still not
correct.  Removing mlog_errno() calls means we can't find out which line
caused the problem.

Joel

> 
> Joel
> 
> > ---
> >  fs/ocfs2/quota_global.c |   11 +++--------
> >  1 files changed, 3 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/ocfs2/quota_global.c b/fs/ocfs2/quota_global.c
> > index 92fcd57..cb0f017 100644
> > --- a/fs/ocfs2/quota_global.c
> > +++ b/fs/ocfs2/quota_global.c
> > @@ -358,10 +358,8 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
> >  	oinfo->dqi_gqi_count = 0;
> >  	oinfo->dqi_gqinode = gqinode;
> >  	status = ocfs2_lock_global_qf(oinfo, 0);
> > -	if (status < 0) {
> > -		mlog_errno(status);
> > +	if (status < 0)
> >  		goto out_err;
> > -	}
> >  
> >  	status = ocfs2_extent_map_get_blocks(gqinode, 0, &oinfo->dqi_giblk,
> >  					     &pcount, NULL);
> > @@ -381,7 +379,6 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
> >  		     status);
> >  		if (status >= 0)
> >  			status = -EIO;
> > -		mlog_errno(status);
> >  		goto out_err;
> >  	}
> >  	info->dqi_bgrace = le32_to_cpu(dinfo.dqi_bgrace);
> > @@ -398,14 +395,12 @@ int ocfs2_global_read_info(struct super_block *sb, int type)
> >  	schedule_delayed_work(&oinfo->dqi_sync_work,
> >  			      msecs_to_jiffies(oinfo->dqi_syncms));
> >  
> > -out_err:
> > -	if (status)
> > -		mlog_errno(status);
> >  	return status;
> >  out_unlock:
> >  	ocfs2_unlock_global_qf(oinfo, 0);
> > +out_err:
> >  	mlog_errno(status);
> > -	goto out_err;
> > +	return status;
> >  }
> >  
> >  /* Write information to global quota file. Expects exlusive lock on quota
> > -- 
> > 1.7.4.4
> > 
> 
> 
> -- 
> 
> "The opposite of a correct statement is a false statement. The
>  opposite of a profound truth may well be another profound truth."
>          - Niels Bohr 
> 
> 			http://www.jlbec.org/
> 			jlbec at evilplan.org
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel

-- 

Life's Little Instruction Book #450

	"Don't be afraid to say, 'I need help.'"

			http://www.jlbec.org/
			jlbec at evilplan.org