[PATCH] zram: check compressor name before setting it

[PATCH] zram: check compressor name before setting it

[Marcin Jabrzyk]

Zram sysfs interface was not making any check of
proper compressor name when setting it.
Any name is accepted, but further tries of device
creation would end up with not very meaningfull error.
eg.

echo lz0 > comp_algorithm
echo 200M > disksize
echo: write error: Invalid argument

This commit fixes that behaviour with returning
EINVAL and proper error message.

Signed-off-by: Marcin Jabrzyk <m.jabrzyk@samsung.com>
---
 drivers/block/zram/zcomp.c    | 22 +++++++++++-----------
 drivers/block/zram/zcomp.h    |  1 +
 drivers/block/zram/zram_drv.c |  5 +++++
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index f1ff39a3d1c1..f81a2b5fef43 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -51,17 +51,6 @@ static struct zcomp_backend *backends[] = {
 	NULL
 };
 
-static struct zcomp_backend *find_backend(const char *compress)
-{
-	int i = 0;
-	while (backends[i]) {
-		if (sysfs_streq(compress, backends[i]->name))
-			break;
-		i++;
-	}
-	return backends[i];
-}
-
 static void zcomp_strm_free(struct zcomp *comp, struct zcomp_strm *zstrm)
 {
 	if (zstrm->private)
@@ -267,6 +256,17 @@ static int zcomp_strm_single_create(struct zcomp *comp)
 	return 0;
 }
 
+struct zcomp_backend *find_backend(const char *compress)
+{
+	int i = 0;
+	while (backends[i]) {
+		if (sysfs_streq(compress, backends[i]->name))
+			break;
+		i++;
+	}
+	return backends[i];
+}
+
 /* show available compressors */
 ssize_t zcomp_available_show(const char *comp, char *buf)
 {
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index c59d1fca72c0..a531350858d0 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -50,6 +50,7 @@ struct zcomp {
 	void (*destroy)(struct zcomp *comp);
 };
 
+struct zcomp_backend *find_backend(const char *compress);
 ssize_t zcomp_available_show(const char *comp, char *buf);
 
 struct zcomp *zcomp_create(const char *comp, int max_strm);
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 01ec6945c2a9..ef4acd6e52d1 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -268,6 +268,11 @@ static ssize_t comp_algorithm_store(struct device *dev,
 {
 	struct zram *zram = dev_to_zram(dev);
 	down_write(&zram->init_lock);
+	if (!find_backend(buf)) {
+		up_write(&zram->init_lock);
+		pr_info("There is no such compression algorithm\n");
+		return -EINVAL;
+	}
 	if (init_done(zram)) {
 		up_write(&zram->init_lock);
 		pr_info("Can't change algorithm for initialized device\n");
-- 
1.9.1

Re: [PATCH] zram: check compressor name before setting it

[Sergey Senozhatsky]

On (05/22/15 10:31), Marcin Jabrzyk wrote:
> Zram sysfs interface was not making any check of
> proper compressor name when setting it.
> Any name is accepted, but further tries of device
> creation would end up with not very meaningfull error.
> eg.
> 
> echo lz0 > comp_algorithm
> echo 200M > disksize
> echo: write error: Invalid argument
> 

no.

zram already complains about failed comp backend creation.
it's in dmesg (or syslog, etc.):

	"zram: Cannot initialise %s compressing backend"

second, there is not much value in exposing zcomp internals,
especially when the result is just another line in dmesg output.

	-ss

Re: [PATCH] zram: check compressor name before setting it

[Marcin Jabrzyk]

Hi,

On 22/05/15 10:56, Sergey Senozhatsky wrote:
> On (05/22/15 10:31), Marcin Jabrzyk wrote:
>> Zram sysfs interface was not making any check of
>> proper compressor name when setting it.
>> Any name is accepted, but further tries of device
>> creation would end up with not very meaningfull error.
>> eg.
>>
>> echo lz0 > comp_algorithm
>> echo 200M > disksize
>> echo: write error: Invalid argument
>>
>
> no.
>
> zram already complains about failed comp backend creation.
> it's in dmesg (or syslog, etc.):
>
> 	"zram: Cannot initialise %s compressing backend"
>
OK, now I see that. Sorry for the noise.

> second, there is not much value in exposing zcomp internals,
> especially when the result is just another line in dmesg output.

 From the other hand, the only valid values that can be written are
in 'comp_algorithm'.
So when writing other one, returning -EINVAL seems to be reasonable.
The user would get immediately information that he can't do that,
now the information can be very deferred in time.
I'm not for exposing more internals, but getting -EINVAL would be nice I 
think.

>
> 	-ss
>

Best regards,
Marcin Jabrzyk

Re: [PATCH] zram: check compressor name before setting it

[Sergey Senozhatsky]

On (05/22/15 11:12), Marcin Jabrzyk wrote:
> >
> >no.
> >
> >zram already complains about failed comp backend creation.
> >it's in dmesg (or syslog, etc.):
> >
> >	"zram: Cannot initialise %s compressing backend"
> >
> OK, now I see that. Sorry for the noise.
> 
> >second, there is not much value in exposing zcomp internals,
> >especially when the result is just another line in dmesg output.
> 
> From the other hand, the only valid values that can be written are
> in 'comp_algorithm'.
> So when writing other one, returning -EINVAL seems to be reasonable.
> The user would get immediately information that he can't do that,
> now the information can be very deferred in time.

it's not.
the error message appears in syslog right before we return -EINVAL
back to user.

	-ss

> I'm not for exposing more internals, but getting -EINVAL would be nice I

Re: [PATCH] zram: check compressor name before setting it

[Minchan Kim]

Hello Sergey,

On Fri, May 22, 2015 at 09:44:11PM +0900, Sergey Senozhatsky wrote:
> On (05/22/15 11:12), Marcin Jabrzyk wrote:
> > >
> > >no.
> > >
> > >zram already complains about failed comp backend creation.
> > >it's in dmesg (or syslog, etc.):
> > >
> > >	"zram: Cannot initialise %s compressing backend"
> > >
> > OK, now I see that. Sorry for the noise.
> > 
> > >second, there is not much value in exposing zcomp internals,
> > >especially when the result is just another line in dmesg output.
> > 
> > From the other hand, the only valid values that can be written are
> > in 'comp_algorithm'.
> > So when writing other one, returning -EINVAL seems to be reasonable.
> > The user would get immediately information that he can't do that,
> > now the information can be very deferred in time.
> 
> it's not.
> the error message appears in syslog right before we return -EINVAL
> back to user.

Although Marcin's description is rather misleading, I like the patch.
Every admin doesn't watch dmesg output. Even people could change loglevel
simply so KERN_INFO would be void in that case.

Instant error propagation is more strighforward for user point of view
rather than delaying with depending on another event.

Thanks.

> 
> 	-ss
> 
> > I'm not for exposing more internals, but getting -EINVAL would be nice I

-- 
Kind regards,
Minchan Kim

Re: [PATCH] zram: check compressor name before setting it

[Marcin Jabrzyk]

On 22/05/15 14:44, Sergey Senozhatsky wrote:
> On (05/22/15 11:12), Marcin Jabrzyk wrote:
>>>
>>> no.
>>>
>>> zram already complains about failed comp backend creation.
>>> it's in dmesg (or syslog, etc.):
>>>
>>> 	"zram: Cannot initialise %s compressing backend"
>>>
>> OK, now I see that. Sorry for the noise.
>>
>>> second, there is not much value in exposing zcomp internals,
>>> especially when the result is just another line in dmesg output.
>>
>>  From the other hand, the only valid values that can be written are
>> in 'comp_algorithm'.
>> So when writing other one, returning -EINVAL seems to be reasonable.
>> The user would get immediately information that he can't do that,
>> now the information can be very deferred in time.
>
> it's not.
> the error message appears in syslog right before we return -EINVAL
> back to user.

Yes I've read up the code more detailed and I saw that error message
just before returning to user with error value.

But this happens when 'disksize' is wirtten, not when 'comp_algorithm'.
I understood, the error message in dmesg is clear there is no such 
algorithm.

But this is not an immediate error, when setting the 'comp_algorithm',
where we already know that it's wrong, not existing etc.
Anything after that moment would be wrong and would not work at all.

 From what I saw 'comp_algorithm_store' is the only *_store in zram that
believes user that he writes proper value and just makes strlcpy.

So what I've ing mind is to provide direct feedback, you have
written wrong name of compressor, you got -EINVAL, please write
correct value. This would be very useful when scripting.

Sorry for being so confusing.

Best regards,
Marcin Jabrzyk

>
> 	-ss
>
>> I'm not for exposing more internals, but getting -EINVAL would be nice I
>

Re: [PATCH] zram: check compressor name before setting it

[Marcin Jabrzyk]

Hello Minchan,

On 22/05/15 15:14, Minchan Kim wrote:
> Hello Sergey,
>
> On Fri, May 22, 2015 at 09:44:11PM +0900, Sergey Senozhatsky wrote:
>> On (05/22/15 11:12), Marcin Jabrzyk wrote:
>>>>
>>>> no.
>>>>
>>>> zram already complains about failed comp backend creation.
>>>> it's in dmesg (or syslog, etc.):
>>>>
>>>> 	"zram: Cannot initialise %s compressing backend"
>>>>
>>> OK, now I see that. Sorry for the noise.
>>>
>>>> second, there is not much value in exposing zcomp internals,
>>>> especially when the result is just another line in dmesg output.
>>>
>>>  From the other hand, the only valid values that can be written are
>>> in 'comp_algorithm'.
>>> So when writing other one, returning -EINVAL seems to be reasonable.
>>> The user would get immediately information that he can't do that,
>>> now the information can be very deferred in time.
>>
>> it's not.
>> the error message appears in syslog right before we return -EINVAL
>> back to user.
>
> Although Marcin's description is rather misleading, I like the patch.
> Every admin doesn't watch dmesg output. Even people could change loglevel
> simply so KERN_INFO would be void in that case.
Sorry for being confusing, at the first time I've overlooked that error 
message in syslog.
I didn't thought about looking for handling exactly this error in 
completely different place.

>
> Instant error propagation is more strighforward for user point of view
> rather than delaying with depending on another event.

Yes this was my exact motivation.
Instant value can be detected in scripts etc. Easier to debug in
automated environment.

>
> Thanks.
>
>>
>> 	-ss
>>
>>> I'm not for exposing more internals, but getting -EINVAL would be nice I
>

If this would be ok, I can prepare v2 with better description and with
less exposing zcomp internals.

Best regards,
Marcin Jabrzyk

Re: [PATCH] zram: check compressor name before setting it

[Sergey Senozhatsky]

On (05/22/15 22:14), Minchan Kim wrote:
> > > >second, there is not much value in exposing zcomp internals,
> > > >especially when the result is just another line in dmesg output.
> > > 
> > > From the other hand, the only valid values that can be written are
> > > in 'comp_algorithm'.
> > > So when writing other one, returning -EINVAL seems to be reasonable.
> > > The user would get immediately information that he can't do that,
> > > now the information can be very deferred in time.
> > 
> > it's not.
> > the error message appears in syslog right before we return -EINVAL
> > back to user.
> 
> Although Marcin's description is rather misleading, I like the patch.
> Every admin doesn't watch dmesg output. Even people could change loglevel
> simply so KERN_INFO would be void in that case.

there is no -EUNSPPORTEDCOMPRESSIONALGORITHM errno that we can return
back to userspace and expect it [userspace] to magically transform it
into a meaningful error message; users must check syslog/dmesg. that's
the way it is.

# echo LZ4 > /sys/block/zram0/comp_algorithm
# -bash: echo: write error: Device or resource busy

- hm.... why?
- well, that's why:
dmesg
[  249.745335] zram: Can't change algorithm for initialized device


> Instant error propagation is more strighforward for user point of view
> rather than delaying with depending on another event.

I'd rather just add two lines of code, w/o making zcomp internals visible.

it seems that we are trying to solve a problem that does not really
exist. I think what we really need to do is to rewrite zram documentation
and to propose zramctl usage as a recommended way of managing zram devices.
zramctl does not do `typo' errors. if somebody wants to configure zram
manually, then he simply must check syslog. it's simple.

---

 drivers/block/zram/zcomp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index a1a8b8e..d96da53 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -54,11 +54,16 @@ static struct zcomp_backend *backends[] = {
 static struct zcomp_backend *find_backend(const char *compress)
 {
 	int i = 0;
+
 	while (backends[i]) {
 		if (sysfs_streq(compress, backends[i]->name))
 			break;
 		i++;
 	}
+
+	if (!backends[i])
+		pr_err("Error: unknown compression algorithm: %s\n",
+				compress);
 	return backends[i];
 }
 

Re: [PATCH] zram: check compressor name before setting it

[Sergey Senozhatsky]

On (05/22/15 15:26), Marcin Jabrzyk wrote:
> >> From the other hand, the only valid values that can be written are
> >>in 'comp_algorithm'.
> >>So when writing other one, returning -EINVAL seems to be reasonable.
> >>The user would get immediately information that he can't do that,
> >>now the information can be very deferred in time.
> >
> >it's not.
> >the error message appears in syslog right before we return -EINVAL
> >back to user.
> 
> Yes I've read up the code more detailed and I saw that error message
> just before returning to user with error value.
> 
> But this happens when 'disksize' is wirtten, not when 'comp_algorithm'.
> I understood, the error message in dmesg is clear there is no such
> algorithm.
> 
> But this is not an immediate error, when setting the 'comp_algorithm',
> where we already know that it's wrong, not existing etc.
> Anything after that moment would be wrong and would not work at all.
> 
> From what I saw 'comp_algorithm_store' is the only *_store in zram that
> believes user that he writes proper value and just makes strlcpy.
> 
> So what I've ing mind is to provide direct feedback, you have
> written wrong name of compressor, you got -EINVAL, please write
> correct value. This would be very useful when scripting.
> 

I can't see how printing error 0.0012 seconds earlier helps. really.
if one sets a compression algorithm the very next thing to do is to
set device's disksize. even if he/she usually watch a baseball game in
between, then the error message appears right when it's needed anyway:
during `setup my device and make it usable' stage.


>I'm not for exposing more internals, but getting -EINVAL would be nice I

you are.

find_backend() returns back to its caller a raw and completely initialized
zcomp_backend pointer. this is very dangerous zcomp internals, which should
never be exposed. from zcomp layer we return either ERR_PTR() or a usable
zcomp_backend pointer. that's the rule.


if you guys still insist that this is critical and very important change,
then there should be a small helper function instead with a clear name
(starting with zcomp_ to indicate its place) which will simply return bool
TRUE for `algorithm is known' case and FALSE otherwise (aka `algorithm is
unknown').


something like below:


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

  dmesg
  [ 7440.544852] Error: unknown compression algorithm: LZ5


p.s. but, I still see a very little value.
p.p.s notice a necessary and inevitable `dmesg'. (back to 'no one reads
syslog' argument).

---

 drivers/block/zram/zcomp.c    | 10 ++++++++++
 drivers/block/zram/zcomp.h    |  1 +
 drivers/block/zram/zram_drv.c |  3 +++
 3 files changed, 14 insertions(+)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index a1a8b8e..b68b16f 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -54,11 +54,16 @@ static struct zcomp_backend *backends[] = {
 static struct zcomp_backend *find_backend(const char *compress)
 {
 	int i = 0;
+
 	while (backends[i]) {
 		if (sysfs_streq(compress, backends[i]->name))
 			break;
 		i++;
 	}
+
+	if (!backends[i])
+		pr_err("Error: unknown compression algorithm: %s\n",
+				compress);
 	return backends[i];
 }
 
@@ -320,6 +325,11 @@ void zcomp_destroy(struct zcomp *comp)
 	kfree(comp);
 }
 
+bool zcomp_known_algorithm(const char *comp)
+{
+	return find_backend(comp) != NULL;
+}
+
 /*
  * search available compressors for requested algorithm.
  * allocate new zcomp and initialize it. return compressing
diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
index c59d1fc..773bdf1 100644
--- a/drivers/block/zram/zcomp.h
+++ b/drivers/block/zram/zcomp.h
@@ -51,6 +51,7 @@ struct zcomp {
 };
 
 ssize_t zcomp_available_show(const char *comp, char *buf);
+bool zcomp_known_algorithm(const char *comp);
 
 struct zcomp *zcomp_create(const char *comp, int max_strm);
 void zcomp_destroy(struct zcomp *comp);
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index f750e34..2197a81 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -378,6 +378,9 @@ static ssize_t comp_algorithm_store(struct device *dev,
 	if (sz > 0 && zram->compressor[sz - 1] == '\n')
 		zram->compressor[sz - 1] = 0x00;
 
+	if (!zcomp_known_algorithm(zram->compressor))
+		len = -EINVAL;
+
 	up_write(&zram->init_lock);
 	return len;
 }

Re: [PATCH] zram: check compressor name before setting it

[Sergey Senozhatsky]

On (05/25/15 15:18), Sergey Senozhatsky wrote:
> find_backend() returns back to its caller a raw and completely initialized

*UN-initialized.  a typo.

	-ss

Re: [PATCH] zram: check compressor name before setting it

[Marcin Jabrzyk]

Hi Sergey,

On 25/05/15 08:18, Sergey Senozhatsky wrote:
> On (05/22/15 15:26), Marcin Jabrzyk wrote:
>>>>  From the other hand, the only valid values that can be written are
>>>> in 'comp_algorithm'.
>>>> So when writing other one, returning -EINVAL seems to be reasonable.
>>>> The user would get immediately information that he can't do that,
>>>> now the information can be very deferred in time.
>>>
>>> it's not.
>>> the error message appears in syslog right before we return -EINVAL
>>> back to user.
>>
>> Yes I've read up the code more detailed and I saw that error message
>> just before returning to user with error value.
>>
>> But this happens when 'disksize' is wirtten, not when 'comp_algorithm'.
>> I understood, the error message in dmesg is clear there is no such
>> algorithm.
>>
>> But this is not an immediate error, when setting the 'comp_algorithm',
>> where we already know that it's wrong, not existing etc.
>> Anything after that moment would be wrong and would not work at all.
>>
>>  From what I saw 'comp_algorithm_store' is the only *_store in zram that
>> believes user that he writes proper value and just makes strlcpy.
>>
>> So what I've ing mind is to provide direct feedback, you have
>> written wrong name of compressor, you got -EINVAL, please write
>> correct value. This would be very useful when scripting.
>>
>
> I can't see how printing error 0.0012 seconds earlier helps. really.
> if one sets a compression algorithm the very next thing to do is to
> set device's disksize. even if he/she usually watch a baseball game in
> between, then the error message appears right when it's needed anyway:
> during `setup my device and make it usable' stage.
>
>
>> I'm not for exposing more internals, but getting -EINVAL would be nice I
>
> you are.
>
> find_backend() returns back to its caller a raw and completely initialized
> zcomp_backend pointer. this is very dangerous zcomp internals, which should
> never be exposed. from zcomp layer we return either ERR_PTR() or a usable
> zcomp_backend pointer. that's the rule.
>
>
> if you guys still insist that this is critical and very important change,
> then there should be a small helper function instead with a clear name
> (starting with zcomp_ to indicate its place) which will simply return bool
> TRUE for `algorithm is known' case and FALSE otherwise (aka `algorithm is
> unknown').
>
This is exactly the idea I've in my mind, when thinking about much 
proper solution.
Simple function that will return bool and just check if name is correct 
or not. Without presenting find_backend or anything from zcomp.

>
> something like below:
>
>
>    # echo LZ5 > /sys/block/zram0/comp_algorithm
>    -bash: echo: write error: Invalid argument
>
>    dmesg
>    [ 7440.544852] Error: unknown compression algorithm: LZ5
>
>
> p.s. but, I still see a very little value.
> p.p.s notice a necessary and inevitable `dmesg'. (back to 'no one reads
> syslog' argument).
>
> ---
>
>   drivers/block/zram/zcomp.c    | 10 ++++++++++
>   drivers/block/zram/zcomp.h    |  1 +
>   drivers/block/zram/zram_drv.c |  3 +++
>   3 files changed, 14 insertions(+)
>
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index a1a8b8e..b68b16f 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -54,11 +54,16 @@ static struct zcomp_backend *backends[] = {
>   static struct zcomp_backend *find_backend(const char *compress)
>   {
>   	int i = 0;
> +
>   	while (backends[i]) {
>   		if (sysfs_streq(compress, backends[i]->name))
>   			break;
>   		i++;
>   	}
> +
> +	if (!backends[i])
> +		pr_err("Error: unknown compression algorithm: %s\n",
> +				compress);
>   	return backends[i];
>   }
>
> @@ -320,6 +325,11 @@ void zcomp_destroy(struct zcomp *comp)
>   	kfree(comp);
>   }
>
> +bool zcomp_known_algorithm(const char *comp)
> +{
> +	return find_backend(comp) != NULL;
> +}
> +
>   /*
>    * search available compressors for requested algorithm.
>    * allocate new zcomp and initialize it. return compressing
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> index c59d1fc..773bdf1 100644
> --- a/drivers/block/zram/zcomp.h
> +++ b/drivers/block/zram/zcomp.h
> @@ -51,6 +51,7 @@ struct zcomp {
>   };
>
>   ssize_t zcomp_available_show(const char *comp, char *buf);
> +bool zcomp_known_algorithm(const char *comp);
>
>   struct zcomp *zcomp_create(const char *comp, int max_strm);
>   void zcomp_destroy(struct zcomp *comp);
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index f750e34..2197a81 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -378,6 +378,9 @@ static ssize_t comp_algorithm_store(struct device *dev,
>   	if (sz > 0 && zram->compressor[sz - 1] == '\n')
>   		zram->compressor[sz - 1] = 0x00;
>
> +	if (!zcomp_known_algorithm(zram->compressor))
> +		len = -EINVAL;
> +
>   	up_write(&zram->init_lock);
>   	return len;
>   }
>
I'm perfectly fine with this solution. It just does what
I'd expect.

Best regards,
Marcin Jabrzyk

Re: [PATCH] zram: check compressor name before setting it

[Sergey Senozhatsky]

On (05/25/15 09:15), Marcin Jabrzyk wrote:
[..]
> >
> I'm perfectly fine with this solution. It just does what
> I'd expect.

cool, let's hear from Minchan.

btw, if we decide to move on, how do you guys want to route
it? do you want Marcin (I don't mind)  or me  (of course,
with the appropriate credit to Marcin) to submit it?

	-ss

Re: [PATCH] zram: check compressor name before setting it

[Marcin Jabrzyk]

On 25/05/15 09:34, Sergey Senozhatsky wrote:
> On (05/25/15 09:15), Marcin Jabrzyk wrote:
> [..]
>>>
>> I'm perfectly fine with this solution. It just does what
>> I'd expect.
>
> cool, let's hear from Minchan.
>
> btw, if we decide to move on, how do you guys want to route
> it? do you want Marcin (I don't mind)  or me  (of course,
> with the appropriate credit to Marcin) to submit it?
>
It this get accepted, then I'm fine with you to submit it.

Best regards,
Marcin Jabrzyk

> 	-ss
>

Re: [PATCH] zram: check compressor name before setting it

[Minchan Kim]

Hello Sergey,

On Mon, May 25, 2015 at 01:03:04PM +0900, Sergey Senozhatsky wrote:
> On (05/22/15 22:14), Minchan Kim wrote:
> > > > >second, there is not much value in exposing zcomp internals,
> > > > >especially when the result is just another line in dmesg output.
> > > >
> > > > From the other hand, the only valid values that can be written are
> > > > in 'comp_algorithm'.
> > > > So when writing other one, returning -EINVAL seems to be reasonable.
> > > > The user would get immediately information that he can't do that,
> > > > now the information can be very deferred in time.
> > >
> > > it's not.
> > > the error message appears in syslog right before we return -EINVAL
> > > back to user.
> >
> > Although Marcin's description is rather misleading, I like the patch.
> > Every admin doesn't watch dmesg output. Even people could change loglevel
> > simply so KERN_INFO would be void in that case.
>
> there is no -EUNSPPORTEDCOMPRESSIONALGORITHM errno that we can return
> back to userspace and expect it [userspace] to magically transform it
> into a meaningful error message; users must check syslog/dmesg. that's
> the way it is.
>
> # echo LZ4 > /sys/block/zram0/comp_algorithm
> # -bash: echo: write error: Device or resource busy

The problem is that user cannot notice the fail until he try to
set disksize. It's not straightforward as interface.

>
> - hm.... why?
> - well, that's why:
> dmesg
> [  249.745335] zram: Can't change algorithm for initialized device
>
>
> > Instant error propagation is more strighforward for user point of view
> > rather than delaying with depending on another event.
>
> I'd rather just add two lines of code, w/o making zcomp internals visible.
>
> it seems that we are trying to solve a problem that does not really
> exist. I think what we really need to do is to rewrite zram documentation
> and to propose zramctl usage as a recommended way of managing zram devices.

Zramctl is useful for people to handle zram's inteface consistenly but
it couldn't be a justification for awkard error propagation which depends
another event(ie, disksize setting).

> zramctl does not do `typo' errors. if somebody wants to configure zram
> manually, then he simply must check syslog. it's simple.

If any action fails instanly, it makes sense to check it with syslog.
But the problem is echo lz5 > /sys/block/zram0/comp_algorithm didn't
emit any error message but could notice it when we set disksize.
It's not good.
Of course, if we need a lot code churn to fix it, I will think it over
seriously but we could fix it simply. Why not?

Frankly speaking, I didn't use zramctl ever because I didn't know when it
was merged into util-linux unforunately and my distro's util-linux seems
to not include it.
And so many products have used zram in my company are too old to use
recent util-linux. They are reluctant to update their util-linux
for just zramctl.
Time goes by, I belive zramctl would be standard way to use zram but
it couldn't cover 100% for the world usecases.
So, my point is that we should make the interface sane without dependency
of zramctl.

>
> ---
>
>  drivers/block/zram/zcomp.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index a1a8b8e..d96da53 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -54,11 +54,16 @@ static struct zcomp_backend *backends[] = {
>  static struct zcomp_backend *find_backend(const char *compress)
>  {
>   int i = 0;
> +
>   while (backends[i]) {
>   if (sysfs_streq(compress, backends[i]->name))
>   break;
>   i++;
>   }
> +
> + if (!backends[i])
> + pr_err("Error: unknown compression algorithm: %s\n",
> + compress);
>   return backends[i];
>  }
>  
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Kind regards,
Minchan Kim

Re: [PATCH] zram: check compressor name before setting it

[Minchan Kim]

On Mon, May 25, 2015 at 03:18:38PM +0900, Sergey Senozhatsky wrote:
> On (05/22/15 15:26), Marcin Jabrzyk wrote:
> > >> From the other hand, the only valid values that can be written are
> > >>in 'comp_algorithm'.
> > >>So when writing other one, returning -EINVAL seems to be reasonable.
> > >>The user would get immediately information that he can't do that,
> > >>now the information can be very deferred in time.
> > >
> > >it's not.
> > >the error message appears in syslog right before we return -EINVAL
> > >back to user.
> > 
> > Yes I've read up the code more detailed and I saw that error message
> > just before returning to user with error value.
> > 
> > But this happens when 'disksize' is wirtten, not when 'comp_algorithm'.
> > I understood, the error message in dmesg is clear there is no such
> > algorithm.
> > 
> > But this is not an immediate error, when setting the 'comp_algorithm',
> > where we already know that it's wrong, not existing etc.
> > Anything after that moment would be wrong and would not work at all.
> > 
> > From what I saw 'comp_algorithm_store' is the only *_store in zram that
> > believes user that he writes proper value and just makes strlcpy.
> > 
> > So what I've ing mind is to provide direct feedback, you have
> > written wrong name of compressor, you got -EINVAL, please write
> > correct value. This would be very useful when scripting.
> > 
> 
> I can't see how printing error 0.0012 seconds earlier helps. really.
> if one sets a compression algorithm the very next thing to do is to
> set device's disksize. even if he/she usually watch a baseball game in
> between, then the error message appears right when it's needed anyway:
> during `setup my device and make it usable' stage.
> 
> 
> >I'm not for exposing more internals, but getting -EINVAL would be nice I
> 
> you are.
> 
> find_backend() returns back to its caller a raw and completely initialized
> zcomp_backend pointer. this is very dangerous zcomp internals, which should
> never be exposed. from zcomp layer we return either ERR_PTR() or a usable
> zcomp_backend pointer. that's the rule.
> 
> 
> if you guys still insist that this is critical and very important change,
> then there should be a small helper function instead with a clear name
> (starting with zcomp_ to indicate its place) which will simply return bool
> TRUE for `algorithm is known' case and FALSE otherwise (aka `algorithm is
> unknown').
> 
> 
> something like below:
> 
> 
>   # echo LZ5 > /sys/block/zram0/comp_algorithm
>   -bash: echo: write error: Invalid argument
> 
>   dmesg
>   [ 7440.544852] Error: unknown compression algorithm: LZ5
> 
> 
> p.s. but, I still see a very little value.
> p.p.s notice a necessary and inevitable `dmesg'. (back to 'no one reads
> syslog' argument).
> 
> ---
> 
>  drivers/block/zram/zcomp.c    | 10 ++++++++++
>  drivers/block/zram/zcomp.h    |  1 +
>  drivers/block/zram/zram_drv.c |  3 +++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index a1a8b8e..b68b16f 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -54,11 +54,16 @@ static struct zcomp_backend *backends[] = {
>  static struct zcomp_backend *find_backend(const char *compress)
>  {
>  	int i = 0;
> +
>  	while (backends[i]) {
>  		if (sysfs_streq(compress, backends[i]->name))
>  			break;
>  		i++;
>  	}
> +
> +	if (!backends[i])
> +		pr_err("Error: unknown compression algorithm: %s\n",
> +				compress);

find_backend is just utility function to get zcomp_backend.
IOW, it might be used for several cases in future so I want
make error report as caller's work.

>  	return backends[i];
>  }
>  
> @@ -320,6 +325,11 @@ void zcomp_destroy(struct zcomp *comp)
>  	kfree(comp);
>  }
>  
> +bool zcomp_known_algorithm(const char *comp)
> +{
> +	return find_backend(comp) != NULL;
> +}
> +
>  /*
>   * search available compressors for requested algorithm.
>   * allocate new zcomp and initialize it. return compressing
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> index c59d1fc..773bdf1 100644
> --- a/drivers/block/zram/zcomp.h
> +++ b/drivers/block/zram/zcomp.h
> @@ -51,6 +51,7 @@ struct zcomp {
>  };
>  
>  ssize_t zcomp_available_show(const char *comp, char *buf);
> +bool zcomp_known_algorithm(const char *comp);
>  
>  struct zcomp *zcomp_create(const char *comp, int max_strm);
>  void zcomp_destroy(struct zcomp *comp);
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index f750e34..2197a81 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -378,6 +378,9 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  	if (sz > 0 && zram->compressor[sz - 1] == '\n')
>  		zram->compressor[sz - 1] = 0x00;
>  
> +	if (!zcomp_known_algorithm(zram->compressor))

In here, we could report back to the user.

> +		len = -EINVAL;
> +
>  	up_write(&zram->init_lock);
>  	return len;
>  }
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Kind regards,
Minchan Kim

Re: [PATCH] zram: check compressor name before setting it

[Sergey Senozhatsky]

Hi,

On (05/25/15 23:21), Minchan Kim wrote:
[..]
> find_backend is just utility function to get zcomp_backend.
> IOW, it might be used for several cases in future so I want
> make error report as caller's work.

[..]
> >  	if (sz > 0 && zram->compressor[sz - 1] == '\n')
> >  		zram->compressor[sz - 1] = 0x00;
> >  
> > +	if (!zcomp_known_algorithm(zram->compressor))
> 
> In here, we could report back to the user.

the motivation was that we actually change user land interface
here and it's quite possible that none of the existing scripts
handle errors returned from `echo X > /../comp_algorithm`, simply
because it has never issued any errors; not counting -BUSY, which
may be not relevant for the vast majority of the scripts:

  #!/bin/sh
  modprobe zram
  echo $1 > /sys/block/zram0/max_comp_streams
  echo $2 > /sys/block/zram0/comp_algorithm

  [..]

  echo $3 > /sys/block/zram0/disksize
  if [ $? ... ]
     ...
  fi

  mkfs.xxx /dev/zram0
  mount -o xxx /dev/zram0 /xxx


`echo $2 > /sys/block/zram0/comp_algorithm` -EINVAL return can be
ignored (and, thus, syslog message as well); because `comp_algorithm`
has never returned anything for that trivial script. so that's why I
wanted to print extra `unknown compression algorithm` message during
disksize store.


	-ss

> > +		len = -EINVAL;
> > +
> >  	up_write(&zram->init_lock);
> >  	return len;
> >  }
> > 
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to majordomo@kvack.org.  For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 
> -- 
> Kind regards,
> Minchan Kim
>