[PATCH] zram: don't copy invalid compression algorithms

[PATCH] zram: don't copy invalid compression algorithms

[Luis Henriques]

Validate the new compression algorithm before copying it into the zram
'compressor' field, keeping the old one if it's invalid.

The error path code is also slightly refactored.

Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
---
 drivers/block/zram/zram_drv.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 9c01f5bfa33f..33551ec9e7f5 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -367,10 +367,15 @@ static ssize_t comp_algorithm_store(struct device *dev,
 
 	down_write(&zram->init_lock);
 	if (init_done(zram)) {
-		up_write(&zram->init_lock);
 		pr_info("Can't change algorithm for initialized device\n");
-		return -EBUSY;
+		len = -EBUSY;
+		goto out;
+	}
+	if (!zcomp_available_algorithm(buf)) {
+		len = -EINVAL;
+		goto out;
 	}
+
 	strlcpy(zram->compressor, buf, sizeof(zram->compressor));
 
 	/* ignore trailing newline */
@@ -378,9 +383,7 @@ static ssize_t comp_algorithm_store(struct device *dev,
 	if (sz > 0 && zram->compressor[sz - 1] == '\n')
 		zram->compressor[sz - 1] = 0x00;
 
-	if (!zcomp_available_algorithm(zram->compressor))
-		len = -EINVAL;
-
+out:
 	up_write(&zram->init_lock);
 	return len;
 }

Re: [PATCH] zram: don't copy invalid compression algorithms

[Sergey Senozhatsky]

On (09/07/15 21:48), Luis Henriques wrote:
> Validate the new compression algorithm before copying it into the zram
> 'compressor' field, keeping the old one if it's invalid.
> 

NACK.

This is intentional. We haven't returned 'invalid compression algorithm'
error from comp_algorithm_store() historically, so someone's script can
simply ignore it. However, the script will fail to init the device and
user will be able to figure out the root cause, because zram will report
to syslog an actually requested alg name.

Example

[ 1669.473296] zram: Cannot initialise llzo compressing backend


	-ss

> The error path code is also slightly refactored.
> 
> Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
> ---
>  drivers/block/zram/zram_drv.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 9c01f5bfa33f..33551ec9e7f5 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -367,10 +367,15 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  
>  	down_write(&zram->init_lock);
>  	if (init_done(zram)) {
> -		up_write(&zram->init_lock);
>  		pr_info("Can't change algorithm for initialized device\n");
> -		return -EBUSY;
> +		len = -EBUSY;
> +		goto out;
> +	}
> +	if (!zcomp_available_algorithm(buf)) {
> +		len = -EINVAL;
> +		goto out;
>  	}
> +
>  	strlcpy(zram->compressor, buf, sizeof(zram->compressor));
>  
>  	/* ignore trailing newline */
> @@ -378,9 +383,7 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  	if (sz > 0 && zram->compressor[sz - 1] == '\n')
>  		zram->compressor[sz - 1] = 0x00;
>  
> -	if (!zcomp_available_algorithm(zram->compressor))
> -		len = -EINVAL;
> -
> +out:
>  	up_write(&zram->init_lock);
>  	return len;
>  }
> 

Re: [PATCH] zram: don't copy invalid compression algorithms

[Minchan Kim]

Hey Sergey,

On Tue, Sep 08, 2015 at 08:56:35AM +0900, Sergey Senozhatsky wrote:
> On (09/07/15 21:48), Luis Henriques wrote:
> > Validate the new compression algorithm before copying it into the zram
> > 'compressor' field, keeping the old one if it's invalid.
> > 
> 
> NACK.
> 
> This is intentional. We haven't returned 'invalid compression algorithm'
> error from comp_algorithm_store() historically, so someone's script can
> simply ignore it. However, the script will fail to init the device and
> user will be able to figure out the root cause, because zram will report
> to syslog an actually requested alg name.
> 
> Example
> 
> [ 1669.473296] zram: Cannot initialise llzo compressing backend

I don't understand your concern. To me, this patch makes sense to me.
Could you explain your point clearly, again?

Thanks.
> 
> 
> 	-ss
> 
> > The error path code is also slightly refactored.
> > 
> > Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
> > ---
> >  drivers/block/zram/zram_drv.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 9c01f5bfa33f..33551ec9e7f5 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -367,10 +367,15 @@ static ssize_t comp_algorithm_store(struct device *dev,
> >  
> >  	down_write(&zram->init_lock);
> >  	if (init_done(zram)) {
> > -		up_write(&zram->init_lock);
> >  		pr_info("Can't change algorithm for initialized device\n");
> > -		return -EBUSY;
> > +		len = -EBUSY;
> > +		goto out;
> > +	}
> > +	if (!zcomp_available_algorithm(buf)) {
> > +		len = -EINVAL;
> > +		goto out;
> >  	}
> > +
> >  	strlcpy(zram->compressor, buf, sizeof(zram->compressor));
> >  
> >  	/* ignore trailing newline */
> > @@ -378,9 +383,7 @@ static ssize_t comp_algorithm_store(struct device *dev,
> >  	if (sz > 0 && zram->compressor[sz - 1] == '\n')
> >  		zram->compressor[sz - 1] = 0x00;
> >  
> > -	if (!zcomp_available_algorithm(zram->compressor))
> > -		len = -EINVAL;
> > -
> > +out:
> >  	up_write(&zram->init_lock);
> >  	return len;
> >  }
> > 

Re: [PATCH] zram: don't copy invalid compression algorithms

[Sergey Senozhatsky]

On (09/08/15 10:14), Minchan Kim wrote:
[..]
> > NACK.
> > 
> > This is intentional. We haven't returned 'invalid compression algorithm'
> > error from comp_algorithm_store() historically, so someone's script can
> > simply ignore it. However, the script will fail to init the device and
> > user will be able to figure out the root cause, because zram will report
> > to syslog an actually requested alg name.
> > 
> > Example
> > 
> > [ 1669.473296] zram: Cannot initialise llzo compressing backend
> 
> I don't understand your concern. To me, this patch makes sense to me.
> Could you explain your point clearly, again?

OK. suppose someone landed a typo in a 'zram device management' script

echo llzo > /sys/block/zram0/comp_algorithm
-bash: echo: write error: Invalid argument


but the script ignores 'echo: write error'.
Because we added compression algorithm name check recently.

then the script does

echo 200M > /sys/block/zram0/disksize
-bash: echo: write error: Invalid argument


doing a simple dmesg reveals the problem

[ 7076.657184] zram: Cannot initialise llzo compressing backend

note that zram provides 'llzo' here, which is convenient.


With this change the semantics is changing and zram now swallows
(hides) the user space error.

echo llzo > /sys/block/zram0/comp_algorithm
-bash: echo: write error: Invalid argument

echo 200M > /sys/block/zram0/disksize

instead of using a requested 'llzo' zram for some reason
switched to 'lzo'.


I don't like this behaviour change. User requested to change
the 'default' value, and that new value didn't work out. No
reason to hide it.

	-ss

Re: [PATCH] zram: don't copy invalid compression algorithms

[Sergey Senozhatsky]

On (09/08/15 10:33), Sergey Senozhatsky wrote:
> > I don't understand your concern. To me, this patch makes sense to me.
> > Could you explain your point clearly, again?
> 
> OK. suppose someone landed a typo in a 'zram device management' script
> 
> echo llzo > /sys/block/zram0/comp_algorithm
> -bash: echo: write error: Invalid argument
> 
> 
> but the script ignores 'echo: write error'.
> Because we added compression algorithm name check recently.
> 
> then the script does
> 
> echo 200M > /sys/block/zram0/disksize
> -bash: echo: write error: Invalid argument
> 
> 
> doing a simple dmesg reveals the problem
> 
> [ 7076.657184] zram: Cannot initialise llzo compressing backend
> 
> note that zram provides 'llzo' here, which is convenient.

ah, forgot to mention. there is another misleading thing.

suppose the script checks the comp_algorithm() error code.
and it attempts to do somthing like
echo llzo > /sys/block/zram0/comp_algorithm
-bash: echo: write error: Device or resource busy


so user knows that comp_algorithm failed. so now
he/she goes and checks zram

cat /sys/block/zram0/comp_algorithm
[lzo] lz4


and finds out... that [lzo] is supported and selected for usage.

so what't the problem then? so user wrongly assumes now that the
script has provided 'lzo' as input to zram...             false!



the existing scheme of things will provide additional hint.

#current implementation
cat /sys/block/zram0/comp_algorithm
lzo lz4

so, none of the supported compression algorithms is selected.
aha, that is obviously lead us to a conclusion that something
wrong was with the input that script provided to zram.  correct!

	-ss

Re: [PATCH] zram: don't copy invalid compression algorithms

[Minchan Kim]

On Tue, Sep 08, 2015 at 10:58:31AM +0900, Sergey Senozhatsky wrote:
> On (09/08/15 10:33), Sergey Senozhatsky wrote:
> > > I don't understand your concern. To me, this patch makes sense to me.
> > > Could you explain your point clearly, again?
> > 
> > OK. suppose someone landed a typo in a 'zram device management' script
> > 
> > echo llzo > /sys/block/zram0/comp_algorithm
> > -bash: echo: write error: Invalid argument
> > 
> > 
> > but the script ignores 'echo: write error'.
> > Because we added compression algorithm name check recently.
> > 
> > then the script does
> > 
> > echo 200M > /sys/block/zram0/disksize
> > -bash: echo: write error: Invalid argument
> > 
> > 
> > doing a simple dmesg reveals the problem
> > 
> > [ 7076.657184] zram: Cannot initialise llzo compressing backend
> > 
> > note that zram provides 'llzo' here, which is convenient.
> 
> ah, forgot to mention. there is another misleading thing.
> 
> suppose the script checks the comp_algorithm() error code.
> and it attempts to do somthing like
> echo llzo > /sys/block/zram0/comp_algorithm
> -bash: echo: write error: Device or resource busy
> 
> 
> so user knows that comp_algorithm failed. so now
> he/she goes and checks zram
> 
> cat /sys/block/zram0/comp_algorithm
> [lzo] lz4
> 
> 
> and finds out... that [lzo] is supported and selected for usage.
> 
> so what't the problem then? so user wrongly assumes now that the
> script has provided 'lzo' as input to zram...             false!
> 
> 
> 
> the existing scheme of things will provide additional hint.
> 
> #current implementation
> cat /sys/block/zram0/comp_algorithm
> lzo lz4
> 
> so, none of the supported compression algorithms is selected.
> aha, that is obviously lead us to a conclusion that something
> wrong was with the input that script provided to zram.  correct!

The problem is caused by that user skipped check of whether his
action was successful or not. IOW, script should have chekcked
status of "echo llzo > xxxx". User shouldn't rely on dmesg.

So, I don't think it's good idea to paper over user's mistake.
And it's straightforward/consistent to change the thing's state
only if is successful.

For exmaple, disksize, max_comp_streams are changed only if
it is successful.
If your logic were right approach, we should change
max_comp_streams for *stupid* script although it doesn't check
return value of doing. It doesn't sound to make sense to me.


> 
> 	-ss

Re: [PATCH] zram: don't copy invalid compression algorithms

[Sergey Senozhatsky]

On (09/08/15 13:50), Minchan Kim wrote:
[..]
> And it's straightforward/consistent to change the thing's state
> only if is successful.
> 

what for? I provided several good reasons not to do this, because
it makes life easier for users. we added this check in Jun 25, 2015
while this functionality and scripts have been around for years, and
apparently now it's users' problem and they must go and do something.


seriously, what improvement this change brings in the first place?
what does it make better and for whom?

	-ss

Re: [PATCH] zram: don't copy invalid compression algorithms

[Minchan Kim]

On Tue, Sep 08, 2015 at 02:04:42PM +0900, Sergey Senozhatsky wrote:
> On (09/08/15 13:50), Minchan Kim wrote:
> [..]
> > And it's straightforward/consistent to change the thing's state
> > only if is successful.
> > 
> 
> what for? I provided several good reasons not to do this, because

Several good reasons?

I just heard you claim to take care of scripts which don't check
function's success at the moment function is called but check it
later via reading the knob later.
If we changes it, it breaks such scripts.
So, with your claim, there are two assumption.

1. script doesn't check return val at the moment function completes
2. Instead, script checks it later via reading the knob again.
So, conclusion is we should keep wrong input in kernel side for them.

It seems you insist on "we should keep wrong input from the userspace
in the kernel to show it if user *might* ask for his debug later"
What makes you think like above?

I think such assumption is really from your brain, not real usecases.
Ok, I admit i'm not a god but if there is such thing in real practice,
we should help them to *correct* it rather than keeping such weired
thing. From the beginning, they should check his action's result with
return value, not dmesg, not reading the knob later, again.

> it makes life easier for users. we added this check in Jun 25, 2015

No, it could make more bad scripts which not checks the result
of the action but rely on current awkward zram's interface.
Consider other knobs in the kernel. A few things popped from my mind
at the moment.

/sys/block/sdb/queue/scheduler
/sys/kernel/mm/transparent_hugepage/enabled
/sys/kernel/debug/tracing/current_tracer
/sys/devices/system/clocksource/clocksource/clocksource0/current_clocksource

They are not showing wrong input user have passed although it was failed.
Could you say a example of kernel interface did intentionally like you said?

> while this functionality and scripts have been around for years, and
> apparently now it's users' problem and they must go and do something.

I believe anyone shouldn't rely on it. But who knows?
However, I want to make it sane(ie, only change compressor name
if the action is successful).
Please, let's discuss how we do it rather than whether it's useful
or not.

> 
> 
> seriously, what improvement this change brings in the first place?
> what does it make better and for whom?

As I mentioned, it makes zram's ABI consistent with others
in kernel space so it makes user feel zram is straight-forward
and sane like others.


> 
> 	-ss

Re: [PATCH] zram: don't copy invalid compression algorithms

[Sergey Senozhatsky]

On (09/08/15 17:16), Minchan Kim wrote:
> we should help them to *correct* it rather than keeping such weired
> thing.

A simple quiz

A)
echo zzz > /sys/block/zram0/comp_algorithm > /dev/null
echo 1G > /sys/block/zram0/disksize
-bash: echo: write error: Invalid argument
echo $?
1


B)
echo zzz > /sys/block/zram0/comp_algorithm > /dev/null
echo 1G > /sys/block/zram0/disksize
echo $?
0


which one *DOES* help finding and correcting an error and which one *DOES NOT*?
a million dolla question.

the difference between comp_algorithm store and any other store function - is
that comp_algorithm_store DOES ABSOLUTELY NOTHING. it does not allocate memory,
free memory, register or unregister anything, change backends, etc., etc., etc.
it does none of those. its only purpose is to strcpy() given data. this data
will be used later by a completely different function as a result of additional
actions taken by user space.

Returning back to our quiz. I do suspect that the answer is... "B"!


So, I still NACK the patch. It introduces a goto label, etc. In fact all
we need to do is to move zcomp_available_algorithm() up, before we grab
the ->init_lock. zcomp_available_algorithm() does not depend on anything
that requires a ->init_lock protection.

Next, the patch lacks a reasoning/motivation in its commit message. What
we do in fact here is we introduce compression algorithm fallback to a
previously selected (knowingly supported, which has already passed
zcomp_available_algorithm()) or the `default_compressor'.

Summarizing, it's something like this:

---

From: Sergey SENOZHATSKY <sergey.senozhatsky@gmail.com>
Subject: [PATCH] zram: introduce comp algorithm fallback functionality

When user supplies an unsupported compression algorithm, keep
a previously selected one (knowingly supported) or the default
one (in case if compression algorithm hasn't been changed before).
---
 drivers/block/zram/zram_drv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 55e09db..255d68b 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -365,6 +365,9 @@ static ssize_t comp_algorithm_store(struct device *dev,
 	struct zram *zram = dev_to_zram(dev);
 	size_t sz;
 
+	if (!zcomp_available_algorithm(buf))
+		return -EINVAL;
+
 	down_write(&zram->init_lock);
 	if (init_done(zram)) {
 		up_write(&zram->init_lock);
@@ -378,9 +381,6 @@ static ssize_t comp_algorithm_store(struct device *dev,
 	if (sz > 0 && zram->compressor[sz - 1] == '\n')
 		zram->compressor[sz - 1] = 0x00;
 
-	if (!zcomp_available_algorithm(zram->compressor))
-		len = -EINVAL;
-
 	up_write(&zram->init_lock);
 	return len;
 }
-- 
2.5.1.454.g1616360

Re: [PATCH] zram: don't copy invalid compression algorithms

[Minchan Kim]

On Tue, Sep 08, 2015 at 06:54:28PM +0900, Sergey Senozhatsky wrote:
> On (09/08/15 17:16), Minchan Kim wrote:
> > we should help them to *correct* it rather than keeping such weired
> > thing.
> 
> A simple quiz
> 
> A)
> echo zzz > /sys/block/zram0/comp_algorithm > /dev/null
> echo 1G > /sys/block/zram0/disksize
> -bash: echo: write error: Invalid argument
> echo $?
> 1
> 
> 
> B)
> echo zzz > /sys/block/zram0/comp_algorithm > /dev/null
> echo 1G > /sys/block/zram0/disksize
> echo $?
> 0
> 
> 
> which one *DOES* help finding and correcting an error and which one *DOES NOT*?
> a million dolla question.

Wrong quiz.
For the helping finding, user should do this.

echo zzz > /sys/block/zram0/comp_algorithm
echo $?

If he want to not show error message, he should do.
echo zzz > /sys/block/zram/comp_algorithm 2> /dev/null
echo $?

> 
> the difference between comp_algorithm store and any other store function - is
> that comp_algorithm_store DOES ABSOLUTELY NOTHING. it does not allocate memory,
> free memory, register or unregister anything, change backends, etc., etc., etc.
> it does none of those. its only purpose is to strcpy() given data. this data
> will be used later by a completely different function as a result of additional
> actions taken by user space.

You are saying implementation of kernel, not interface itself.
Nothing different. It's a interface to say something from the user
to the kenrel. If user passes wrong input, normally, kernel should return
a error and user should check it.
It's a general rule for the programming for several decades.

> 
> Returning back to our quiz. I do suspect that the answer is... "B"!
> 
> 
> So, I still NACK the patch. It introduces a goto label, etc. In fact all
> we need to do is to move zcomp_available_algorithm() up, before we grab
> the ->init_lock. zcomp_available_algorithm() does not depend on anything
> that requires a ->init_lock protection.
> 
> Next, the patch lacks a reasoning/motivation in its commit message. What
> we do in fact here is we introduce compression algorithm fallback to a
> previously selected (knowingly supported, which has already passed
> zcomp_available_algorithm()) or the `default_compressor'.
> 
> Summarizing, it's something like this:
> 
> ---
> 
> From: Sergey SENOZHATSKY <sergey.senozhatsky@gmail.com>
> Subject: [PATCH] zram: introduce comp algorithm fallback functionality
> 
> When user supplies an unsupported compression algorithm, keep
> a previously selected one (knowingly supported) or the default
> one (in case if compression algorithm hasn't been changed before).
> ---
>  drivers/block/zram/zram_drv.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 55e09db..255d68b 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -365,6 +365,9 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  	struct zram *zram = dev_to_zram(dev);
>  	size_t sz;
>  
> +	if (!zcomp_available_algorithm(buf))
> +		return -EINVAL;

If you ignore tailling space, your change would make a bug.
If you fix it, it looks good to me.
I hope Luis can send it with his SOB and indication of
your credit about checking with avoiding unnecessary locking if you don't mind.

Thanks, Guys!

> +
>  	down_write(&zram->init_lock);
>  	if (init_done(zram)) {
>  		up_write(&zram->init_lock);
> @@ -378,9 +381,6 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  	if (sz > 0 && zram->compressor[sz - 1] == '\n')
>  		zram->compressor[sz - 1] = 0x00;
>  
> -	if (!zcomp_available_algorithm(zram->compressor))
> -		len = -EINVAL;
> -
>  	up_write(&zram->init_lock);
>  	return len;
>  }
> -- 
> 2.5.1.454.g1616360
> 

Re: [PATCH] zram: don't copy invalid compression algorithms

[Sergey Senozhatsky]

dammit, not going to waste my time on this BS anymore.



> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -365,6 +365,9 @@ static ssize_t comp_algorithm_store(struct device *dev,
> >  	struct zram *zram = dev_to_zram(dev);
> >  	size_t sz;
> >  
> > +	if (!zcomp_available_algorithm(buf))
> > +		return -EINVAL;
> 
> If you ignore tailling space, your change would make a bug.
> If you fix it, it looks good to me.

that's why find_backend() calls sysfs_streq(), which takes care of
a trailing new line. unless you're speaking about something else.

	-ss

Re: [PATCH] zram: don't copy invalid compression algorithms

[Minchan Kim]

On Tue, Sep 08, 2015 at 10:44:01PM +0900, Sergey Senozhatsky wrote:
> dammit, not going to waste my time on this BS anymore.
> 
> 
> 
> > > --- a/drivers/block/zram/zram_drv.c
> > > +++ b/drivers/block/zram/zram_drv.c
> > > @@ -365,6 +365,9 @@ static ssize_t comp_algorithm_store(struct device *dev,
> > >  	struct zram *zram = dev_to_zram(dev);
> > >  	size_t sz;
> > >  
> > > +	if (!zcomp_available_algorithm(buf))
> > > +		return -EINVAL;
> > 
> > If you ignore tailling space, your change would make a bug.
> > If you fix it, it looks good to me.
> 
> that's why find_backend() calls sysfs_streq(), which takes care of
> a trailing new line. unless you're speaking about something else.

You're right. Your patch totally makes sense. Sorry for the niose.
Please resend it on another thread out of this BS thread with Ccing
Andrew.

> 
> 	-ss

-- 
Kind regards,
Minchan Kim

Re: [PATCH] zram: don't copy invalid compression algorithms

[Sergey Senozhatsky]

On (09/08/15 23:21), Minchan Kim wrote:
[..]
> > > If you ignore tailling space, your change would make a bug.
> > > If you fix it, it looks good to me.
> > 
> > that's why find_backend() calls sysfs_streq(), which takes care of
> > a trailing new line. unless you're speaking about something else.
> 
> You're right. Your patch totally makes sense. Sorry for the niose.
> Please resend it on another thread out of this BS thread with Ccing
> Andrew.

patch resends better happen in separate threads. Luis, please Cc Andrew.

I think the commit message also should document that this patch does change
a user space visible behaviour in some cases (doesn't matter how seriously
we take it, it better be 'documented').

	-ss

Re: [PATCH] zram: don't copy invalid compression algorithms

[Luis Henriques]

On Tue, Sep 08, 2015 at 05:16:10PM +0900, Minchan Kim wrote:
> On Tue, Sep 08, 2015 at 02:04:42PM +0900, Sergey Senozhatsky wrote:
> > On (09/08/15 13:50), Minchan Kim wrote:
> > [..]
> > > And it's straightforward/consistent to change the thing's state
> > > only if is successful.
> > > 
> > 
> > what for? I provided several good reasons not to do this, because
> 
> Several good reasons?
> 
> I just heard you claim to take care of scripts which don't check
> function's success at the moment function is called but check it
> later via reading the knob later.
> If we changes it, it breaks such scripts.
> So, with your claim, there are two assumption.
> 
> 1. script doesn't check return val at the moment function completes
> 2. Instead, script checks it later via reading the knob again.
> So, conclusion is we should keep wrong input in kernel side for them.
> 
> It seems you insist on "we should keep wrong input from the userspace
> in the kernel to show it if user *might* ask for his debug later"
> What makes you think like above?
> 
> I think such assumption is really from your brain, not real usecases.
> Ok, I admit i'm not a god but if there is such thing in real practice,
> we should help them to *correct* it rather than keeping such weired
> thing. From the beginning, they should check his action's result with
> return value, not dmesg, not reading the knob later, again.
> 
> > it makes life easier for users. we added this check in Jun 25, 2015
> 
> No, it could make more bad scripts which not checks the result
> of the action but rely on current awkward zram's interface.
> Consider other knobs in the kernel. A few things popped from my mind
> at the moment.
> 
> /sys/block/sdb/queue/scheduler

This one is actually a good example, and its behaviour is exactly what I
expecting in zram.

But I believe To: Minchan Kim <minchan@kernel.org>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
	linux-kernel@vger.kernel.org,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Bcc: 
Subject: Re: [PATCH] zram: don't copy invalid compression algorithms
Reply-To: 
In-Reply-To: <20150908081610.GA8633@bbox>

On Tue, Sep 08, 2015 at 05:16:10PM +0900, Minchan Kim wrote:
> On Tue, Sep 08, 2015 at 02:04:42PM +0900, Sergey Senozhatsky wrote:
> > On (09/08/15 13:50), Minchan Kim wrote:
> > [..]
> > > And it's straightforward/consistent to change the thing's state
> > > only if is successful.
> > > 
> > 
> > what for? I provided several good reasons not to do this, because
> 
> Several good reasons?
> 
> I just heard you claim to take care of scripts which don't check
> function's success at the moment function is called but check it
> later via reading the knob later.
> If we changes it, it breaks such scripts.
> So, with your claim, there are two assumption.
> 
> 1. script doesn't check return val at the moment function completes
> 2. Instead, script checks it later via reading the knob again.
> So, conclusion is we should keep wrong input in kernel side for them.
> 
> It seems you insist on "we should keep wrong input from the userspace
> in the kernel to show it if user *might* ask for his debug later"
> What makes you think like above?
> 
> I think such assumption is really from your brain, not real usecases.
> Ok, I admit i'm not a god but if there is such thing in real practice,
> we should help them to *correct* it rather than keeping such weired
> thing. From the beginning, they should check his action's result with
> return value, not dmesg, not reading the knob later, again.
> 
> > it makes life easier for users. we added this check in Jun 25, 2015
> 
> No, it could make more bad scripts which not checks the result
> of the action but rely on current awkward zram's interface.
> Consider other knobs in the kernel. A few things popped from my mind
> at the moment.
> 
> /sys/block/sdb/queue/scheduler

This one is actually a good example, and its behaviour is exactly what I
was expecting in zram.  Of course, the semantics of this file is very
different, but not changing the system status if invalid input is provided
seems to be the sane default behaviour.  For this reason, I still think my
patch is doing the right thing.

HOWEVER, I also believe Sergey has a very valid point: my patch changes
the ABI with user-space, and this could actually be considered a
regression.  This all depends on how stable this sysfs interface is :-)

So, if you guys decide to drop my patch, I would at least update the
following files to describe the current behaviour:

 - Documentation/ABI/testing/sysfs-block-zram
 - Documentation/blockdev/zram.txt

(I'm OK sending a patch to update these files once there is a decision on
what to do.)

Cheers,
--
Luís

> /sys/kernel/mm/transparent_hugepage/enabled
> /sys/kernel/debug/tracing/current_tracer
> /sys/devices/system/clocksource/clocksource/clocksource0/current_clocksource
> 
> They are not showing wrong input user have passed although it was failed.
> Could you say a example of kernel interface did intentionally like you said?
> 
> > while this functionality and scripts have been around for years, and
> > apparently now it's users' problem and they must go and do something.
> 
> I believe anyone shouldn't rely on it. But who knows?
> However, I want to make it sane(ie, only change compressor name
> if the action is successful).
> Please, let's discuss how we do it rather than whether it's useful
> or not.
> 
> > 
> > 
> > seriously, what improvement this change brings in the first place?
> > what does it make better and for whom?
> 
> As I mentioned, it makes zram's ABI consistent with others
> in kernel space so it makes user feel zram is straight-forward
> and sane like others.
> 
> 
> > 
> > 	-ss
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] zram: don't copy invalid compression algorithms

[Sergey Senozhatsky]

On (09/08/15 11:08), Luis Henriques wrote:
[..]
> > 
> > /sys/block/sdb/queue/scheduler
> 
> This one is actually a good example
>

... its behaviour depends on the way the kernel uses that queue.

# echo THIS_EXIST_ONLY_IN_MY_HEAD > /sys/devices/virtual/block/zram0/queue/scheduler
# echo $?
0

# echo hmmmmm > /sys/devices/virtual/block/zram0/queue/scheduler
# echo $?
0

	-ss

Re: [PATCH] zram: don't copy invalid compression algorithms

[Sergey Senozhatsky]

On (09/08/15 13:50), Minchan Kim wrote:
> For exmaple, disksize, max_comp_streams are changed only if
> it is successful.
> If your logic were right approach, we should change
> max_comp_streams for *stupid* script although it doesn't check

define stupid.

is   echo 2100000 > /sys/block/zram0/max_comp_streams   clever or stupid?

do we control mem_limit_store()? no.
do we contol mem_used_max_store()? no.

if user asks to redefine a "default" value we just let him
do so.

	-ss