[PATCH] zram: check comp algorithm availability earlier

[PATCH] zram: check comp algorithm availability earlier

[Sergey Senozhatsky]

Improvement idea by Marcin Jabrzyk.

comp_algorithm_store() silently accepts any supplied algorithm
name, because zram performs algorithm availability check later,
during the device configuration phase in disksize_store() and
prints
  "zram: Cannot initialise %s compressing backend"
to syslog. this error line is somewhat generic and, besides,
can indicate a failed attempt to allocate compression backend's
working buffers.

make algorithm availability check earlier, in comp_algorithm_store(),
and be move verbose:

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

dmesg:
  zram: Error: unavailable compression algorithm: lzz

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Reported-by: Marcin Jabrzyk <m.jabrzyk@samsung.com>
---
 drivers/block/zram/zcomp.c    | 5 +++++
 drivers/block/zram/zcomp.h    | 1 +
 drivers/block/zram/zram_drv.c | 6 ++++++
 3 files changed, 12 insertions(+)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index a1a8b8e..e10e2b4 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -320,6 +320,11 @@ void zcomp_destroy(struct zcomp *comp)
 	kfree(comp);
 }
 
+bool zcomp_available_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..46e2b9f 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_available_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 28f6e46..e17b73e 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -378,6 +378,12 @@ 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)) {
+		pr_err("Error: unavailable compression algorithm: %s\n",
+				zram->compressor);
+		len = -EINVAL;
+	}
+
 	up_write(&zram->init_lock);
 	return len;
 }
-- 
2.4.1.314.g9532ead

Re: [PATCH] zram: check comp algorithm availability earlier

[Minchan Kim]

Hello Sergey,

On Tue, May 26, 2015 at 10:13:37PM +0900, Sergey Senozhatsky wrote:
> Improvement idea by Marcin Jabrzyk.
> 
> comp_algorithm_store() silently accepts any supplied algorithm
> name, because zram performs algorithm availability check later,
> during the device configuration phase in disksize_store() and
> prints
>   "zram: Cannot initialise %s compressing backend"
> to syslog. this error line is somewhat generic and, besides,
> can indicate a failed attempt to allocate compression backend's
> working buffers.
> 
> make algorithm availability check earlier, in comp_algorithm_store(),
> and be move verbose:
> 
>   echo lzz > /sys/block/zram0/comp_algorithm
>   -bash: echo: write error: Invalid argument
> 
> dmesg:
>   zram: Error: unavailable compression algorithm: lzz
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Reported-by: Marcin Jabrzyk <m.jabrzyk@samsung.com>
> ---
>  drivers/block/zram/zcomp.c    | 5 +++++
>  drivers/block/zram/zcomp.h    | 1 +
>  drivers/block/zram/zram_drv.c | 6 ++++++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index a1a8b8e..e10e2b4 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -320,6 +320,11 @@ void zcomp_destroy(struct zcomp *comp)
>  	kfree(comp);
>  }
>  
> +bool zcomp_available_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..46e2b9f 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_available_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 28f6e46..e17b73e 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -378,6 +378,12 @@ 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)) {
> +		pr_err("Error: unavailable compression algorithm: %s\n",
> +				zram->compressor);
> +		len = -EINVAL;
> +	}
> +

I'm not against this patch because it's better than old.
But let's think more about the pr_err part.

If user try to set wrong algo name, he can see EINVAL.
Isn't it enough?

I think every sane admin can think he passed wrong argument
if he sees -EINVAL.
So, I don't think we need to emit pr_err in here.

The reason I am paranoid about that is that I really don't want
to argue with syslog info which is part of ABI or not in future.
If possible, I don't want to depend on pr_xxx.


>  	up_write(&zram->init_lock);
>  	return len;
>  }
> -- 
> 2.4.1.314.g9532ead
> 

-- 
Kind regards,
Minchan Kim

Re: [PATCH] zram: check comp algorithm availability earlier

[Sergey Senozhatsky]

On (05/27/15 12:51), Minchan Kim wrote:
[..]
> > @@ -378,6 +378,12 @@ 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)) {
> > +		pr_err("Error: unavailable compression algorithm: %s\n",
> > +				zram->compressor);
> > +		len = -EINVAL;
> > +	}
> > +
> 
> I'm not against this patch because it's better than old.
> But let's think more about the pr_err part.
> 
> If user try to set wrong algo name, he can see EINVAL.
> Isn't it enough?
> 
> I think every sane admin can think he passed wrong argument
> if he sees -EINVAL.
> So, I don't think we need to emit pr_err in here.
> 

well, it's here simply to make failure investigation easier.
one surely will know that supplied string was not recognized
as a compression algorithm name, but what was it.. "$3 instead
of $2... or, wait, did $i contain something wrong?". zram knew
what was wrong.

/* and you asked to put this warn here in your previous email. */


sure, can remove it.


> The reason I am paranoid about that is that I really don't want
> to argue with syslog info which is part of ABI or not in future.
> If possible, I don't want to depend on pr_xxx.
> 

just for the record...  I don't understand this part.


ok. I'll resend later today.

	-ss

Re: [PATCH] zram: check comp algorithm availability earlier

[Minchan Kim]

On Wed, May 27, 2015 at 02:53:20PM +0900, Sergey Senozhatsky wrote:
> On (05/27/15 12:51), Minchan Kim wrote:
> [..]
> > > @@ -378,6 +378,12 @@ 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)) {
> > > +		pr_err("Error: unavailable compression algorithm: %s\n",
> > > +				zram->compressor);
> > > +		len = -EINVAL;
> > > +	}
> > > +
> > 
> > I'm not against this patch because it's better than old.
> > But let's think more about the pr_err part.
> > 
> > If user try to set wrong algo name, he can see EINVAL.
> > Isn't it enough?
> > 
> > I think every sane admin can think he passed wrong argument
> > if he sees -EINVAL.
> > So, I don't think we need to emit pr_err in here.
> > 
> 
> well, it's here simply to make failure investigation easier.
> one surely will know that supplied string was not recognized
> as a compression algorithm name, but what was it.. "$3 instead
> of $2... or, wait, did $i contain something wrong?". zram knew
> what was wrong.
> 
> /* and you asked to put this warn here in your previous email. */

Yes, Sorry about that. At that time, you put the warning in find_backend
and I didn't like it. Instead, I want to move it to in there.
But more thinking about it, I don't feel we need it.

> 
> 
> sure, can remove it.
> 
> 
> > The reason I am paranoid about that is that I really don't want
> > to argue with syslog info which is part of ABI or not in future.
> > If possible, I don't want to depend on pr_xxx.
> > 
> 
> just for the record...  I don't understand this part.

I meant if we remove the pr_err in future by some reason,
someone might shout

"No, it's ABI so if you guys removes it, it will break user interface's
semantic". Maybe he seems to depends on parse on dmesg.
That is not what I want.

> 
> 
> ok. I'll resend later today.
> 
> 	-ss

-- 
Kind regards,
Minchan Kim

Re: [PATCH] zram: check comp algorithm availability earlier

[Sergey Senozhatsky]

On (05/27/15 14:58), Minchan Kim wrote:
> > > I'm not against this patch because it's better than old.
> > > But let's think more about the pr_err part.
> > > 
> > > If user try to set wrong algo name, he can see EINVAL.
> > > Isn't it enough?
> > > 
> > > I think every sane admin can think he passed wrong argument
> > > if he sees -EINVAL.
> > > So, I don't think we need to emit pr_err in here.
> > > 
> > 
> > well, it's here simply to make failure investigation easier.
> > one surely will know that supplied string was not recognized
> > as a compression algorithm name, but what was it.. "$3 instead
> > of $2... or, wait, did $i contain something wrong?". zram knew
> > what was wrong.
> > 
> > /* and you asked to put this warn here in your previous email. */
> 
> Yes, Sorry about that. At that time, you put the warning in find_backend
> and I didn't like it. Instead, I want to move it to in there.
> But more thinking about it, I don't feel we need it.
> 

no problem.

> > > The reason I am paranoid about that is that I really don't want
> > > to argue with syslog info which is part of ABI or not in future.
> > > If possible, I don't want to depend on pr_xxx.
> > > 
> > 
> > just for the record...  I don't understand this part.
> 
> I meant if we remove the pr_err in future by some reason,
> someone might shout
> 
> "No, it's ABI so if you guys removes it, it will break user interface's
> semantic". Maybe he seems to depends on parse on dmesg.
>

ah, I see. well, hopefully no one does this. anyway, will remove it.


hm, I was thinking about some sort of a troubleshooting section in zram
documentation, where we can `decrypt' some of the errors that we return
back. but still did not convince myself that it will be of any use, just
doubt that people read documentation that often.

speaking of docs,
mentioning zramctl in zram docs is actully not entirely bad idea,
I think. zramctl has been in util-linux for (almost) a year now
and sooner or later will be widely available.

	-ss