[PATCH] ext2: reduce redundant check of '*options'

[PATCH] ext2: reduce redundant check of '*options'

[Simon Xu]

We don't need to check whether '*options' equals to ',' twice.

Signed-off-by: Simon Xu <xu.simon@oracle.com>
---
 fs/ext2/super.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 7731695..bd5b66b 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -377,13 +377,13 @@ static unsigned long get_sb_block(void **data)
 		return 1;	/* Default location */
 	options += 3;
 	sb_block = simple_strtoul(options, &options, 0);
-	if (*options && *options != ',') {
+	if (*options == ',') {
+		options++;
+	} else if (*options) {
 		printk("EXT2-fs: Invalid sb specification: %s\n",
 		       (char *) *data);
 		return 1;
 	}
-	if (*options == ',')
-		options++;
 	*data = (void *) options;
 	return sb_block;
 }
-- 
1.7.3.5

Re: [PATCH] ext2: reduce redundant check of '*options'

[Jan Kara]

On Thu 13-01-11 20:39:22, Simon Xu wrote:
> We don't need to check whether '*options' equals to ',' twice.
Well, but the code is so rarely executed that even if the compiler would
not be able to optimize out the second test, it would not matter. And the
code is not better readable afterwards either. So I find this more or less
unnecessary code churn...

Seriously, if you want to do some useful fixing then have a look for
example at making ext3/4 (in fact JBD/JBD2) handle transaction allocation
failures gratefully.  Currently we just loop in start_this_handle()
infinitely until we can make the allocation which is a bit dumb. So it
would be good, to make as many places as possible able to handle ENOMEM
from start_this_handle() and propagate the error to user space. In some
cases, it might not be easily possible (e.g. during writeout of dirty
memory, where proper handling needs more thought) but lots of cases
should be rather simple and need just auditing the code paths. Thanks.

								Honza
> 
> Signed-off-by: Simon Xu <xu.simon@oracle.com>
> ---
>  fs/ext2/super.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> index 7731695..bd5b66b 100644
> --- a/fs/ext2/super.c
> +++ b/fs/ext2/super.c
> @@ -377,13 +377,13 @@ static unsigned long get_sb_block(void **data)
>  		return 1;	/* Default location */
>  	options += 3;
>  	sb_block = simple_strtoul(options, &options, 0);
> -	if (*options && *options != ',') {
> +	if (*options == ',') {
> +		options++;
> +	} else if (*options) {
>  		printk("EXT2-fs: Invalid sb specification: %s\n",
>  		       (char *) *data);
>  		return 1;
>  	}
> -	if (*options == ',')
> -		options++;
>  	*data = (void *) options;
>  	return sb_block;
>  }
> -- 
> 1.7.3.5
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] ext2: reduce redundant check of '*options'

[Ted Ts'o]

On Mon, Jan 17, 2011 at 06:02:03PM +0100, Jan Kara wrote:
> Seriously, if you want to do some useful fixing then have a look for
> example at making ext3/4 (in fact JBD/JBD2) handle transaction allocation
> failures gratefully.  Currently we just loop in start_this_handle()
> infinitely until we can make the allocation which is a bit dumb. So it
> would be good, to make as many places as possible able to handle ENOMEM
> from start_this_handle() and propagate the error to user space. In some
> cases, it might not be easily possible (e.g. during writeout of dirty
> memory, where proper handling needs more thought) but lots of cases
> should be rather simple and need just auditing the code paths. Thanks.

Uh, this is actually a rather subtle set of fixes that you are
proposing, because it won't be obvious to someone who hasn't paid very
careful attention to who calls certain functions (i.e., such as the
writepages function), which get propagated to user space, and which
will just cause mysterious data corruption of files --- especially
since the writeback daemon doesn't check error returns (!!!).  So if
someone changed writepages to return ENOMEM, and then said, "How
delightful!  My work is done...", it would result in nothing getting
back to userspace in most cases, and the writeback code would mark the
pages as clean, and on the next reboot, the data would be lost.

So this is *not* a change that I would recommend to people who are
just beginning to learn how to program the kernel, and have been
taught by some kernel programmers that the best thing to do is to take
trivial-style changes and submit them upstream.

					- Ted

Re: [PATCH] ext2: reduce redundant check of '*options'

[Jan Kara]

On Tue 18-01-11 08:43:45, Ted Ts'o wrote:
> On Mon, Jan 17, 2011 at 06:02:03PM +0100, Jan Kara wrote:
> > Seriously, if you want to do some useful fixing then have a look for
> > example at making ext3/4 (in fact JBD/JBD2) handle transaction allocation
> > failures gratefully.  Currently we just loop in start_this_handle()
> > infinitely until we can make the allocation which is a bit dumb. So it
> > would be good, to make as many places as possible able to handle ENOMEM
> > from start_this_handle() and propagate the error to user space. In some
> > cases, it might not be easily possible (e.g. during writeout of dirty
> > memory, where proper handling needs more thought) but lots of cases
> > should be rather simple and need just auditing the code paths. Thanks.
> 
> Uh, this is actually a rather subtle set of fixes that you are
> proposing, because it won't be obvious to someone who hasn't paid very
> careful attention to who calls certain functions (i.e., such as the
> writepages function), which get propagated to user space, and which
> will just cause mysterious data corruption of files --- especially
> since the writeback daemon doesn't check error returns (!!!).  So if
> someone changed writepages to return ENOMEM, and then said, "How
> delightful!  My work is done...", it would result in nothing getting
> back to userspace in most cases, and the writeback code would mark the
> pages as clean, and on the next reboot, the data would be lost.
  I agree that the patches will need a careful review because of catches
like you mention.

> So this is *not* a change that I would recommend to people who are
> just beginning to learn how to program the kernel, and have been
> taught by some kernel programmers that the best thing to do is to take
> trivial-style changes and submit them upstream.
 I agree, maybe I've shooted too high (feel free to suggest better topic),
but we'll see what the guy comes up with. Personally, I find it more useful
to correct bad patches trying to achieve good things than spending time
shooting down trivial code rearrangements...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR