Re: [PATCH] zram: check compressor name before setting it
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(>init_lock); > > return len; > > } > > > > -- > > To unsubscribe, send a message with 'unsubscribe linux-mm' in > > the body to majord...@kvack.org. For more info on Linux MM, > > see: http://www.linux-mm.org/ . > > Don't email: mailto:"d...@kvack.org;> em...@kvack.org > > -- > Kind regards, > Minchan Kim > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: check compressor name before setting it
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
Re: [PATCH] zram: check compressor name before setting it
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 majord...@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: mailto:"d...@kvack.org;> em...@kvack.org -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: check compressor name before setting it
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(>init_lock); return len; } I'm perfectly fine with this solution. It just does what I'd expect. Best regards, Marcin Jabrzyk -- To unsubscribe from this list: send the
Re: [PATCH] zram: check compressor name before setting it
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: check compressor name before setting it
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(>init_lock); return len; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: check compressor name before setting it
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; } -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line
Re: [PATCH] zram: check compressor name before setting it
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: check compressor name before setting it
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 majord...@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: a href=mailto:d...@kvack.org; em...@kvack.org /a -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: check compressor name before setting it
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
Re: [PATCH] zram: check compressor name before setting it
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 majord...@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: a href=mailto:d...@kvack.org; em...@kvack.org /a -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: check compressor name before setting it
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]; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: check compressor name before setting it
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]; } -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] zram: check compressor name before setting it
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 --- 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(>init_lock); + if (!find_backend(buf)) { + up_write(>init_lock); + pr_info("There is no such compression algorithm\n"); + return -EINVAL; + } if (init_done(zram)) { up_write(>init_lock); pr_info("Can't change algorithm for initialized device\n"); -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] zram: check compressor name before setting it
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.jabr...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: check compressor name before setting it
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/