* [PATCH] zram: propagate error to user @ 2014-03-07 1:56 Minchan Kim 2014-03-07 9:20 ` Sergey Senozhatsky 0 siblings, 1 reply; 6+ messages in thread From: Minchan Kim @ 2014-03-07 1:56 UTC (permalink / raw) To: Andrew Morton Cc: linux-kernel, Nitin Gupta, Jerome Marchand, Sergey Senozhatsky, Minchan Kim When we initialized zcomp with single, we couldn't change max_comp_streams without zram reset but current interface doesn't show any error to user and even it changes max_comp_streams's value without any effect so it would make user very confusing. This patch prevents max_comp_streams's change when zcomp was initialized as single zcomp and emit the error to user(ex, echo). Signed-off-by: Minchan Kim <minchan@kernel.org> --- Documentation/blockdev/zram.txt | 9 +++++---- drivers/block/zram/zcomp.c | 10 +++++----- drivers/block/zram/zcomp.h | 4 ++-- drivers/block/zram/zram_drv.c | 15 +++++++++++---- 4 files changed, 23 insertions(+), 15 deletions(-) diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt index 2604ffed51db..0595c3f56ccf 100644 --- a/Documentation/blockdev/zram.txt +++ b/Documentation/blockdev/zram.txt @@ -37,10 +37,11 @@ Note: In order to enable compression backend's multi stream support max_comp_streams must be initially set to desired concurrency level before ZRAM device initialisation. Once the device initialised as a single stream compression -backend (max_comp_streams equals to 0) changing the value of max_comp_streams -will not take any effect, because single stream compression backend implemented -as a special case and does not support dynamic max_comp_streams. Only multi -stream backend supports dynamic max_comp_streams adjustment. +backend (max_comp_streams equals to 1), you will see error if you try to change +the value of max_comp_streams because single stream compression backend +implemented as a special case by lock overhead issue and does not support +dynamic max_comp_streams. Only multi stream backend supports dynamic +max_comp_streams adjustment. 3) Select compression algorithm Using comp_algorithm device attribute one can see available and diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c index 92a83df40a27..15fe6a27781b 100644 --- a/drivers/block/zram/zcomp.c +++ b/drivers/block/zram/zcomp.c @@ -152,7 +152,7 @@ static void zcomp_strm_multi_release(struct zcomp *comp, struct zcomp_strm *zstr } /* change max_strm limit */ -static int zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) +static bool zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) { struct zcomp_strm_multi *zs = comp->stream; struct zcomp_strm *zstrm; @@ -171,7 +171,7 @@ static int zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) zs->avail_strm--; } spin_unlock(&zs->strm_lock); - return 0; + return true; } static void zcomp_strm_multi_destroy(struct zcomp *comp) @@ -231,10 +231,10 @@ static void zcomp_strm_single_release(struct zcomp *comp, mutex_unlock(&zs->strm_lock); } -static int zcomp_strm_single_set_max_streams(struct zcomp *comp, int num_strm) +static bool zcomp_strm_single_set_max_streams(struct zcomp *comp, int num_strm) { /* zcomp_strm_single support only max_comp_streams == 1 */ - return -ENOTSUPP; + return 0; } static void zcomp_strm_single_destroy(struct zcomp *comp) @@ -283,7 +283,7 @@ ssize_t zcomp_available_show(const char *comp, char *buf) return sz; } -int zcomp_set_max_streams(struct zcomp *comp, int num_strm) +bool zcomp_set_max_streams(struct zcomp *comp, int num_strm) { return comp->set_max_streams(comp, num_strm); } diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h index 8b8997f8613b..c59d1fca72c0 100644 --- a/drivers/block/zram/zcomp.h +++ b/drivers/block/zram/zcomp.h @@ -46,7 +46,7 @@ struct zcomp { struct zcomp_strm *(*strm_find)(struct zcomp *comp); void (*strm_release)(struct zcomp *comp, struct zcomp_strm *zstrm); - int (*set_max_streams)(struct zcomp *comp, int num_strm); + bool (*set_max_streams)(struct zcomp *comp, int num_strm); void (*destroy)(struct zcomp *comp); }; @@ -64,5 +64,5 @@ int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm, int zcomp_decompress(struct zcomp *comp, const unsigned char *src, size_t src_len, unsigned char *dst); -int zcomp_set_max_streams(struct zcomp *comp, int num_strm); +bool zcomp_set_max_streams(struct zcomp *comp, int num_strm); #endif /* _ZCOMP_H_ */ diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 7631ef0cbc41..eead9db9b100 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -128,17 +128,24 @@ static ssize_t max_comp_streams_store(struct device *dev, struct zram *zram = dev_to_zram(dev); if (kstrtoint(buf, 0, &num)) - return -EINVAL; + goto out; if (num < 1) - return -EINVAL; + goto out; + down_write(&zram->init_lock); if (init_done(zram)) { - if (zcomp_set_max_streams(zram->comp, num)) + if (!zcomp_set_max_streams(zram->comp, num)) { pr_info("Cannot change max compression streams\n"); + goto out_unlock; + } } + zram->max_comp_streams = num; - up_write(&zram->init_lock); return len; +out_unlock: + up_write(&zram->init_lock); +out: + return -EINVAL; } static ssize_t comp_algorithm_show(struct device *dev, -- 1.9.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] zram: propagate error to user 2014-03-07 1:56 [PATCH] zram: propagate error to user Minchan Kim @ 2014-03-07 9:20 ` Sergey Senozhatsky 2014-03-07 9:51 ` Minchan Kim 0 siblings, 1 reply; 6+ messages in thread From: Sergey Senozhatsky @ 2014-03-07 9:20 UTC (permalink / raw) To: Minchan Kim Cc: Andrew Morton, linux-kernel, Nitin Gupta, Jerome Marchand, Sergey Senozhatsky On (03/07/14 10:56), Minchan Kim wrote: > When we initialized zcomp with single, we couldn't change > max_comp_streams without zram reset but current interface doesn't > show any error to user and even it changes max_comp_streams's value > without any effect so it would make user very confusing. > > This patch prevents max_comp_streams's change when zcomp was > initialized as single zcomp and emit the error to user(ex, echo). > > Signed-off-by: Minchan Kim <minchan@kernel.org> > --- > Documentation/blockdev/zram.txt | 9 +++++---- > drivers/block/zram/zcomp.c | 10 +++++----- > drivers/block/zram/zcomp.h | 4 ++-- > drivers/block/zram/zram_drv.c | 15 +++++++++++---- > 4 files changed, 23 insertions(+), 15 deletions(-) > > diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt > index 2604ffed51db..0595c3f56ccf 100644 > --- a/Documentation/blockdev/zram.txt > +++ b/Documentation/blockdev/zram.txt > @@ -37,10 +37,11 @@ Note: > In order to enable compression backend's multi stream support max_comp_streams > must be initially set to desired concurrency level before ZRAM device > initialisation. Once the device initialised as a single stream compression > -backend (max_comp_streams equals to 0) changing the value of max_comp_streams > -will not take any effect, because single stream compression backend implemented > -as a special case and does not support dynamic max_comp_streams. Only multi > -stream backend supports dynamic max_comp_streams adjustment. > +backend (max_comp_streams equals to 1), you will see error if you try to change > +the value of max_comp_streams because single stream compression backend > +implemented as a special case by lock overhead issue and does not support > +dynamic max_comp_streams. Only multi stream backend supports dynamic > +max_comp_streams adjustment. > > 3) Select compression algorithm > Using comp_algorithm device attribute one can see available and > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c > index 92a83df40a27..15fe6a27781b 100644 > --- a/drivers/block/zram/zcomp.c > +++ b/drivers/block/zram/zcomp.c > @@ -152,7 +152,7 @@ static void zcomp_strm_multi_release(struct zcomp *comp, struct zcomp_strm *zstr > } > > /* change max_strm limit */ > -static int zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > +static bool zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > { > struct zcomp_strm_multi *zs = comp->stream; > struct zcomp_strm *zstrm; > @@ -171,7 +171,7 @@ static int zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > zs->avail_strm--; > } > spin_unlock(&zs->strm_lock); > - return 0; > + return true; > } > > static void zcomp_strm_multi_destroy(struct zcomp *comp) > @@ -231,10 +231,10 @@ static void zcomp_strm_single_release(struct zcomp *comp, > mutex_unlock(&zs->strm_lock); > } > > -static int zcomp_strm_single_set_max_streams(struct zcomp *comp, int num_strm) > +static bool zcomp_strm_single_set_max_streams(struct zcomp *comp, int num_strm) > { > /* zcomp_strm_single support only max_comp_streams == 1 */ > - return -ENOTSUPP; > + return 0; > } IMHO, -ENOTSUPP for unsupported operation fits better than `false'. yes, currently there are only two possible returns: 0 -- success -ENOTSUPP - not supported operation though, we can extend functions later and return additional codes, other than `false' and `true'. for example, -E2BIG if user requested extremly large number of streams, like 5000 streams. > > static void zcomp_strm_single_destroy(struct zcomp *comp) > @@ -283,7 +283,7 @@ ssize_t zcomp_available_show(const char *comp, char *buf) > return sz; > } > > -int zcomp_set_max_streams(struct zcomp *comp, int num_strm) > +bool zcomp_set_max_streams(struct zcomp *comp, int num_strm) > { > return comp->set_max_streams(comp, num_strm); > } > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h > index 8b8997f8613b..c59d1fca72c0 100644 > --- a/drivers/block/zram/zcomp.h > +++ b/drivers/block/zram/zcomp.h > @@ -46,7 +46,7 @@ struct zcomp { > > struct zcomp_strm *(*strm_find)(struct zcomp *comp); > void (*strm_release)(struct zcomp *comp, struct zcomp_strm *zstrm); > - int (*set_max_streams)(struct zcomp *comp, int num_strm); > + bool (*set_max_streams)(struct zcomp *comp, int num_strm); > void (*destroy)(struct zcomp *comp); > }; > > @@ -64,5 +64,5 @@ int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm, > int zcomp_decompress(struct zcomp *comp, const unsigned char *src, > size_t src_len, unsigned char *dst); > > -int zcomp_set_max_streams(struct zcomp *comp, int num_strm); > +bool zcomp_set_max_streams(struct zcomp *comp, int num_strm); > #endif /* _ZCOMP_H_ */ > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index 7631ef0cbc41..eead9db9b100 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -128,17 +128,24 @@ static ssize_t max_comp_streams_store(struct device *dev, > struct zram *zram = dev_to_zram(dev); > > if (kstrtoint(buf, 0, &num)) > - return -EINVAL; > + goto out; > if (num < 1) > - return -EINVAL; > + goto out; > + > down_write(&zram->init_lock); > if (init_done(zram)) { > - if (zcomp_set_max_streams(zram->comp, num)) > + if (!zcomp_set_max_streams(zram->comp, num)) { > pr_info("Cannot change max compression streams\n"); > + goto out_unlock; > + } > } > + > zram->max_comp_streams = num; agree, better to avoid this for a single stream. > - up_write(&zram->init_lock); > return len; > +out_unlock: > + up_write(&zram->init_lock); > +out: > + return -EINVAL; > } > > static ssize_t comp_algorithm_show(struct device *dev, > -- > 1.9.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] zram: propagate error to user 2014-03-07 9:20 ` Sergey Senozhatsky @ 2014-03-07 9:51 ` Minchan Kim 2014-03-09 16:58 ` Sergey Senozhatsky 0 siblings, 1 reply; 6+ messages in thread From: Minchan Kim @ 2014-03-07 9:51 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Andrew Morton, linux-kernel, Nitin Gupta, Jerome Marchand Hello Sergey! On Fri, Mar 07, 2014 at 12:20:45PM +0300, Sergey Senozhatsky wrote: > On (03/07/14 10:56), Minchan Kim wrote: > > When we initialized zcomp with single, we couldn't change > > max_comp_streams without zram reset but current interface doesn't > > show any error to user and even it changes max_comp_streams's value > > without any effect so it would make user very confusing. > > > > This patch prevents max_comp_streams's change when zcomp was > > initialized as single zcomp and emit the error to user(ex, echo). > > > > Signed-off-by: Minchan Kim <minchan@kernel.org> > > --- > > Documentation/blockdev/zram.txt | 9 +++++---- > > drivers/block/zram/zcomp.c | 10 +++++----- > > drivers/block/zram/zcomp.h | 4 ++-- > > drivers/block/zram/zram_drv.c | 15 +++++++++++---- > > 4 files changed, 23 insertions(+), 15 deletions(-) > > > > diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt > > index 2604ffed51db..0595c3f56ccf 100644 > > --- a/Documentation/blockdev/zram.txt > > +++ b/Documentation/blockdev/zram.txt > > @@ -37,10 +37,11 @@ Note: > > In order to enable compression backend's multi stream support max_comp_streams > > must be initially set to desired concurrency level before ZRAM device > > initialisation. Once the device initialised as a single stream compression > > -backend (max_comp_streams equals to 0) changing the value of max_comp_streams > > -will not take any effect, because single stream compression backend implemented > > -as a special case and does not support dynamic max_comp_streams. Only multi > > -stream backend supports dynamic max_comp_streams adjustment. > > +backend (max_comp_streams equals to 1), you will see error if you try to change > > +the value of max_comp_streams because single stream compression backend > > +implemented as a special case by lock overhead issue and does not support > > +dynamic max_comp_streams. Only multi stream backend supports dynamic > > +max_comp_streams adjustment. > > > > 3) Select compression algorithm > > Using comp_algorithm device attribute one can see available and > > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c > > index 92a83df40a27..15fe6a27781b 100644 > > --- a/drivers/block/zram/zcomp.c > > +++ b/drivers/block/zram/zcomp.c > > @@ -152,7 +152,7 @@ static void zcomp_strm_multi_release(struct zcomp *comp, struct zcomp_strm *zstr > > } > > > > /* change max_strm limit */ > > -static int zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > > +static bool zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > > { > > struct zcomp_strm_multi *zs = comp->stream; > > struct zcomp_strm *zstrm; > > @@ -171,7 +171,7 @@ static int zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > > zs->avail_strm--; > > } > > spin_unlock(&zs->strm_lock); > > - return 0; > > + return true; > > } > > > > static void zcomp_strm_multi_destroy(struct zcomp *comp) > > @@ -231,10 +231,10 @@ static void zcomp_strm_single_release(struct zcomp *comp, > > mutex_unlock(&zs->strm_lock); > > } > > > > -static int zcomp_strm_single_set_max_streams(struct zcomp *comp, int num_strm) > > +static bool zcomp_strm_single_set_max_streams(struct zcomp *comp, int num_strm) > > { > > /* zcomp_strm_single support only max_comp_streams == 1 */ > > - return -ENOTSUPP; > > + return 0; > > } > > IMHO, -ENOTSUPP for unsupported operation fits better than `false'. > yes, currently there are only two possible returns: > 0 -- success > -ENOTSUPP - not supported operation > > though, we can extend functions later and return additional codes, other > than `false' and `true'. Thing to expose to user isn't true and false but EINVAL. > > for example, -E2BIG if user requested extremly large number of streams, > like 5000 streams. I'm not sure it's right example for E2BIG. When I read the comment, it says "argument list too long". Anyway, when I tried ENOTSUPP, echo doesn't show meaningful error to user and I dont' know it's casual err number for userspace. Pz, comment about that. > > > > > static void zcomp_strm_single_destroy(struct zcomp *comp) > > @@ -283,7 +283,7 @@ ssize_t zcomp_available_show(const char *comp, char *buf) > > return sz; > > } > > > > -int zcomp_set_max_streams(struct zcomp *comp, int num_strm) > > +bool zcomp_set_max_streams(struct zcomp *comp, int num_strm) > > { > > return comp->set_max_streams(comp, num_strm); > > } > > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h > > index 8b8997f8613b..c59d1fca72c0 100644 > > --- a/drivers/block/zram/zcomp.h > > +++ b/drivers/block/zram/zcomp.h > > @@ -46,7 +46,7 @@ struct zcomp { > > > > struct zcomp_strm *(*strm_find)(struct zcomp *comp); > > void (*strm_release)(struct zcomp *comp, struct zcomp_strm *zstrm); > > - int (*set_max_streams)(struct zcomp *comp, int num_strm); > > + bool (*set_max_streams)(struct zcomp *comp, int num_strm); > > void (*destroy)(struct zcomp *comp); > > }; > > > > @@ -64,5 +64,5 @@ int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm, > > int zcomp_decompress(struct zcomp *comp, const unsigned char *src, > > size_t src_len, unsigned char *dst); > > > > -int zcomp_set_max_streams(struct zcomp *comp, int num_strm); > > +bool zcomp_set_max_streams(struct zcomp *comp, int num_strm); > > #endif /* _ZCOMP_H_ */ > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > > index 7631ef0cbc41..eead9db9b100 100644 > > --- a/drivers/block/zram/zram_drv.c > > +++ b/drivers/block/zram/zram_drv.c > > @@ -128,17 +128,24 @@ static ssize_t max_comp_streams_store(struct device *dev, > > struct zram *zram = dev_to_zram(dev); > > > > if (kstrtoint(buf, 0, &num)) > > - return -EINVAL; > > + goto out; > > if (num < 1) > > - return -EINVAL; > > + goto out; > > + > > down_write(&zram->init_lock); > > if (init_done(zram)) { > > - if (zcomp_set_max_streams(zram->comp, num)) > > + if (!zcomp_set_max_streams(zram->comp, num)) { > > pr_info("Cannot change max compression streams\n"); > > + goto out_unlock; > > + } > > } > > + > > zram->max_comp_streams = num; > > agree, better to avoid this for a single stream. Yeb. -- Kind regards, Minchan Kim ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] zram: propagate error to user 2014-03-07 9:51 ` Minchan Kim @ 2014-03-09 16:58 ` Sergey Senozhatsky 2014-03-10 2:01 ` Minchan Kim 0 siblings, 1 reply; 6+ messages in thread From: Sergey Senozhatsky @ 2014-03-09 16:58 UTC (permalink / raw) To: Minchan Kim Cc: Sergey Senozhatsky, Andrew Morton, linux-kernel, Nitin Gupta, Jerome Marchand Hello Minchan, On (03/07/14 18:51), Minchan Kim wrote: > Hello Sergey! > > On Fri, Mar 07, 2014 at 12:20:45PM +0300, Sergey Senozhatsky wrote: > > On (03/07/14 10:56), Minchan Kim wrote: > > > When we initialized zcomp with single, we couldn't change > > > max_comp_streams without zram reset but current interface doesn't > > > show any error to user and even it changes max_comp_streams's value > > > without any effect so it would make user very confusing. > > > > > > This patch prevents max_comp_streams's change when zcomp was > > > initialized as single zcomp and emit the error to user(ex, echo). > > > > > > Signed-off-by: Minchan Kim <minchan@kernel.org> > > > --- > > > Documentation/blockdev/zram.txt | 9 +++++---- > > > drivers/block/zram/zcomp.c | 10 +++++----- > > > drivers/block/zram/zcomp.h | 4 ++-- > > > drivers/block/zram/zram_drv.c | 15 +++++++++++---- > > > 4 files changed, 23 insertions(+), 15 deletions(-) > > > > > > diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt > > > index 2604ffed51db..0595c3f56ccf 100644 > > > --- a/Documentation/blockdev/zram.txt > > > +++ b/Documentation/blockdev/zram.txt > > > @@ -37,10 +37,11 @@ Note: > > > In order to enable compression backend's multi stream support max_comp_streams > > > must be initially set to desired concurrency level before ZRAM device > > > initialisation. Once the device initialised as a single stream compression > > > -backend (max_comp_streams equals to 0) changing the value of max_comp_streams > > > -will not take any effect, because single stream compression backend implemented > > > -as a special case and does not support dynamic max_comp_streams. Only multi > > > -stream backend supports dynamic max_comp_streams adjustment. > > > +backend (max_comp_streams equals to 1), you will see error if you try to change > > > +the value of max_comp_streams because single stream compression backend > > > +implemented as a special case by lock overhead issue and does not support > > > +dynamic max_comp_streams. Only multi stream backend supports dynamic > > > +max_comp_streams adjustment. > > > > > > 3) Select compression algorithm > > > Using comp_algorithm device attribute one can see available and > > > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c > > > index 92a83df40a27..15fe6a27781b 100644 > > > --- a/drivers/block/zram/zcomp.c > > > +++ b/drivers/block/zram/zcomp.c > > > @@ -152,7 +152,7 @@ static void zcomp_strm_multi_release(struct zcomp *comp, struct zcomp_strm *zstr > > > } > > > > > > /* change max_strm limit */ > > > -static int zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > > > +static bool zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > > > { > > > struct zcomp_strm_multi *zs = comp->stream; > > > struct zcomp_strm *zstrm; > > > @@ -171,7 +171,7 @@ static int zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > > > zs->avail_strm--; > > > } > > > spin_unlock(&zs->strm_lock); > > > - return 0; > > > + return true; > > > } > > > > > > static void zcomp_strm_multi_destroy(struct zcomp *comp) > > > @@ -231,10 +231,10 @@ static void zcomp_strm_single_release(struct zcomp *comp, > > > mutex_unlock(&zs->strm_lock); > > > } > > > > > > -static int zcomp_strm_single_set_max_streams(struct zcomp *comp, int num_strm) > > > +static bool zcomp_strm_single_set_max_streams(struct zcomp *comp, int num_strm) > > > { > > > /* zcomp_strm_single support only max_comp_streams == 1 */ > > > - return -ENOTSUPP; > > > + return 0; > > > } > > > > IMHO, -ENOTSUPP for unsupported operation fits better than `false'. > > yes, currently there are only two possible returns: > > 0 -- success > > -ENOTSUPP - not supported operation > > > > though, we can extend functions later and return additional codes, other > > than `false' and `true'. > > Thing to expose to user isn't true and false but EINVAL. > sure. I mean we can return actual zcomp_set_max_streams() error (if any) back to user from max_comp_streams_store(): [..] ret = zcomp_set_max_streams(...); [..] return ret; > > > > for example, -E2BIG if user requested extremly large number of streams, > > like 5000 streams. > > I'm not sure it's right example for E2BIG. > When I read the comment, it says "argument list too long". > Anyway, when I tried ENOTSUPP, echo doesn't show meaningful error to user > and I dont' know it's casual err number for userspace. > Pz, comment about that. > E2BIG (introduced by POSIX.1-2001) was a quick example and, probably, not the perfect one. In general, I'm not against this change and I can live with zcomp_set_max_streams() returning bool. -ss > > > > > > > > static void zcomp_strm_single_destroy(struct zcomp *comp) > > > @@ -283,7 +283,7 @@ ssize_t zcomp_available_show(const char *comp, char *buf) > > > return sz; > > > } > > > > > > -int zcomp_set_max_streams(struct zcomp *comp, int num_strm) > > > +bool zcomp_set_max_streams(struct zcomp *comp, int num_strm) > > > { > > > return comp->set_max_streams(comp, num_strm); > > > } > > > diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h > > > index 8b8997f8613b..c59d1fca72c0 100644 > > > --- a/drivers/block/zram/zcomp.h > > > +++ b/drivers/block/zram/zcomp.h > > > @@ -46,7 +46,7 @@ struct zcomp { > > > > > > struct zcomp_strm *(*strm_find)(struct zcomp *comp); > > > void (*strm_release)(struct zcomp *comp, struct zcomp_strm *zstrm); > > > - int (*set_max_streams)(struct zcomp *comp, int num_strm); > > > + bool (*set_max_streams)(struct zcomp *comp, int num_strm); > > > void (*destroy)(struct zcomp *comp); > > > }; > > > > > > @@ -64,5 +64,5 @@ int zcomp_compress(struct zcomp *comp, struct zcomp_strm *zstrm, > > > int zcomp_decompress(struct zcomp *comp, const unsigned char *src, > > > size_t src_len, unsigned char *dst); > > > > > > -int zcomp_set_max_streams(struct zcomp *comp, int num_strm); > > > +bool zcomp_set_max_streams(struct zcomp *comp, int num_strm); > > > #endif /* _ZCOMP_H_ */ > > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > > > index 7631ef0cbc41..eead9db9b100 100644 > > > --- a/drivers/block/zram/zram_drv.c > > > +++ b/drivers/block/zram/zram_drv.c > > > @@ -128,17 +128,24 @@ static ssize_t max_comp_streams_store(struct device *dev, > > > struct zram *zram = dev_to_zram(dev); > > > > > > if (kstrtoint(buf, 0, &num)) > > > - return -EINVAL; > > > + goto out; > > > if (num < 1) > > > - return -EINVAL; > > > + goto out; > > > + > > > down_write(&zram->init_lock); > > > if (init_done(zram)) { > > > - if (zcomp_set_max_streams(zram->comp, num)) > > > + if (!zcomp_set_max_streams(zram->comp, num)) { > > > pr_info("Cannot change max compression streams\n"); > > > + goto out_unlock; > > > + } > > > } > > > + > > > zram->max_comp_streams = num; > > > > agree, better to avoid this for a single stream. > > Yeb. > > -- > Kind regards, > Minchan Kim > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] zram: propagate error to user 2014-03-09 16:58 ` Sergey Senozhatsky @ 2014-03-10 2:01 ` Minchan Kim 2014-03-10 7:50 ` Sergey Senozhatsky 0 siblings, 1 reply; 6+ messages in thread From: Minchan Kim @ 2014-03-10 2:01 UTC (permalink / raw) To: Sergey Senozhatsky Cc: Andrew Morton, linux-kernel, Nitin Gupta, Jerome Marchand Hello Sergey, On Sun, Mar 09, 2014 at 07:58:51PM +0300, Sergey Senozhatsky wrote: > Hello Minchan, > > On (03/07/14 18:51), Minchan Kim wrote: > > Hello Sergey! > > > > On Fri, Mar 07, 2014 at 12:20:45PM +0300, Sergey Senozhatsky wrote: > > > On (03/07/14 10:56), Minchan Kim wrote: > > > > When we initialized zcomp with single, we couldn't change > > > > max_comp_streams without zram reset but current interface doesn't > > > > show any error to user and even it changes max_comp_streams's value > > > > without any effect so it would make user very confusing. > > > > > > > > This patch prevents max_comp_streams's change when zcomp was > > > > initialized as single zcomp and emit the error to user(ex, echo). > > > > > > > > Signed-off-by: Minchan Kim <minchan@kernel.org> > > > > --- > > > > Documentation/blockdev/zram.txt | 9 +++++---- > > > > drivers/block/zram/zcomp.c | 10 +++++----- > > > > drivers/block/zram/zcomp.h | 4 ++-- > > > > drivers/block/zram/zram_drv.c | 15 +++++++++++---- > > > > 4 files changed, 23 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt > > > > index 2604ffed51db..0595c3f56ccf 100644 > > > > --- a/Documentation/blockdev/zram.txt > > > > +++ b/Documentation/blockdev/zram.txt > > > > @@ -37,10 +37,11 @@ Note: > > > > In order to enable compression backend's multi stream support max_comp_streams > > > > must be initially set to desired concurrency level before ZRAM device > > > > initialisation. Once the device initialised as a single stream compression > > > > -backend (max_comp_streams equals to 0) changing the value of max_comp_streams > > > > -will not take any effect, because single stream compression backend implemented > > > > -as a special case and does not support dynamic max_comp_streams. Only multi > > > > -stream backend supports dynamic max_comp_streams adjustment. > > > > +backend (max_comp_streams equals to 1), you will see error if you try to change > > > > +the value of max_comp_streams because single stream compression backend > > > > +implemented as a special case by lock overhead issue and does not support > > > > +dynamic max_comp_streams. Only multi stream backend supports dynamic > > > > +max_comp_streams adjustment. > > > > > > > > 3) Select compression algorithm > > > > Using comp_algorithm device attribute one can see available and > > > > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c > > > > index 92a83df40a27..15fe6a27781b 100644 > > > > --- a/drivers/block/zram/zcomp.c > > > > +++ b/drivers/block/zram/zcomp.c > > > > @@ -152,7 +152,7 @@ static void zcomp_strm_multi_release(struct zcomp *comp, struct zcomp_strm *zstr > > > > } > > > > > > > > /* change max_strm limit */ > > > > -static int zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > > > > +static bool zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > > > > { > > > > struct zcomp_strm_multi *zs = comp->stream; > > > > struct zcomp_strm *zstrm; > > > > @@ -171,7 +171,7 @@ static int zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > > > > zs->avail_strm--; > > > > } > > > > spin_unlock(&zs->strm_lock); > > > > - return 0; > > > > + return true; > > > > } > > > > > > > > static void zcomp_strm_multi_destroy(struct zcomp *comp) > > > > @@ -231,10 +231,10 @@ static void zcomp_strm_single_release(struct zcomp *comp, > > > > mutex_unlock(&zs->strm_lock); > > > > } > > > > > > > > -static int zcomp_strm_single_set_max_streams(struct zcomp *comp, int num_strm) > > > > +static bool zcomp_strm_single_set_max_streams(struct zcomp *comp, int num_strm) > > > > { > > > > /* zcomp_strm_single support only max_comp_streams == 1 */ > > > > - return -ENOTSUPP; > > > > + return 0; > > > > } > > > > > > IMHO, -ENOTSUPP for unsupported operation fits better than `false'. > > > yes, currently there are only two possible returns: > > > 0 -- success > > > -ENOTSUPP - not supported operation > > > > > > though, we can extend functions later and return additional codes, other > > > than `false' and `true'. > > > > Thing to expose to user isn't true and false but EINVAL. > > > > sure. I mean we can return actual zcomp_set_max_streams() error (if any) > back to user from max_comp_streams_store(): > > [..] > ret = zcomp_set_max_streams(...); > [..] > return ret; > > > > > > > for example, -E2BIG if user requested extremly large number of streams, > > > like 5000 streams. > > > > I'm not sure it's right example for E2BIG. > > When I read the comment, it says "argument list too long". > > Anyway, when I tried ENOTSUPP, echo doesn't show meaningful error to user > > and I dont' know it's casual err number for userspace. > > Pz, comment about that. > > > > E2BIG (introduced by POSIX.1-2001) was a quick example and, probably, not > the perfect one. In general, I'm not against this change and I can live > with zcomp_set_max_streams() returning bool. Then, could you give me Acked-by? I am hungry with that. :) -- Kind regards, Minchan Kim ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] zram: propagate error to user 2014-03-10 2:01 ` Minchan Kim @ 2014-03-10 7:50 ` Sergey Senozhatsky 0 siblings, 0 replies; 6+ messages in thread From: Sergey Senozhatsky @ 2014-03-10 7:50 UTC (permalink / raw) To: Minchan Kim Cc: Sergey Senozhatsky, Andrew Morton, linux-kernel, Nitin Gupta, Jerome Marchand Hello, On (03/10/14 11:01), Minchan Kim wrote: > Hello Sergey, > > On Sun, Mar 09, 2014 at 07:58:51PM +0300, Sergey Senozhatsky wrote: > > Hello Minchan, > > > > On (03/07/14 18:51), Minchan Kim wrote: > > > Hello Sergey! > > > > > > On Fri, Mar 07, 2014 at 12:20:45PM +0300, Sergey Senozhatsky wrote: > > > > On (03/07/14 10:56), Minchan Kim wrote: > > > > > When we initialized zcomp with single, we couldn't change > > > > > max_comp_streams without zram reset but current interface doesn't > > > > > show any error to user and even it changes max_comp_streams's value > > > > > without any effect so it would make user very confusing. > > > > > > > > > > This patch prevents max_comp_streams's change when zcomp was > > > > > initialized as single zcomp and emit the error to user(ex, echo). > > > > > > > > > > Signed-off-by: Minchan Kim <minchan@kernel.org> > > > > > --- > > > > > Documentation/blockdev/zram.txt | 9 +++++---- > > > > > drivers/block/zram/zcomp.c | 10 +++++----- > > > > > drivers/block/zram/zcomp.h | 4 ++-- > > > > > drivers/block/zram/zram_drv.c | 15 +++++++++++---- > > > > > 4 files changed, 23 insertions(+), 15 deletions(-) > > > > > > > > > > diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt > > > > > index 2604ffed51db..0595c3f56ccf 100644 > > > > > --- a/Documentation/blockdev/zram.txt > > > > > +++ b/Documentation/blockdev/zram.txt > > > > > @@ -37,10 +37,11 @@ Note: > > > > > In order to enable compression backend's multi stream support max_comp_streams > > > > > must be initially set to desired concurrency level before ZRAM device > > > > > initialisation. Once the device initialised as a single stream compression > > > > > -backend (max_comp_streams equals to 0) changing the value of max_comp_streams > > > > > -will not take any effect, because single stream compression backend implemented > > > > > -as a special case and does not support dynamic max_comp_streams. Only multi > > > > > -stream backend supports dynamic max_comp_streams adjustment. > > > > > +backend (max_comp_streams equals to 1), you will see error if you try to change > > > > > +the value of max_comp_streams because single stream compression backend > > > > > +implemented as a special case by lock overhead issue and does not support > > > > > +dynamic max_comp_streams. Only multi stream backend supports dynamic > > > > > +max_comp_streams adjustment. > > > > > > > > > > 3) Select compression algorithm > > > > > Using comp_algorithm device attribute one can see available and > > > > > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c > > > > > index 92a83df40a27..15fe6a27781b 100644 > > > > > --- a/drivers/block/zram/zcomp.c > > > > > +++ b/drivers/block/zram/zcomp.c > > > > > @@ -152,7 +152,7 @@ static void zcomp_strm_multi_release(struct zcomp *comp, struct zcomp_strm *zstr > > > > > } > > > > > > > > > > /* change max_strm limit */ > > > > > -static int zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > > > > > +static bool zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > > > > > { > > > > > struct zcomp_strm_multi *zs = comp->stream; > > > > > struct zcomp_strm *zstrm; > > > > > @@ -171,7 +171,7 @@ static int zcomp_strm_multi_set_max_streams(struct zcomp *comp, int num_strm) > > > > > zs->avail_strm--; > > > > > } > > > > > spin_unlock(&zs->strm_lock); > > > > > - return 0; > > > > > + return true; > > > > > } > > > > > > > > > > static void zcomp_strm_multi_destroy(struct zcomp *comp) > > > > > @@ -231,10 +231,10 @@ static void zcomp_strm_single_release(struct zcomp *comp, > > > > > mutex_unlock(&zs->strm_lock); > > > > > } > > > > > > > > > > -static int zcomp_strm_single_set_max_streams(struct zcomp *comp, int num_strm) > > > > > +static bool zcomp_strm_single_set_max_streams(struct zcomp *comp, int num_strm) > > > > > { > > > > > /* zcomp_strm_single support only max_comp_streams == 1 */ > > > > > - return -ENOTSUPP; > > > > > + return 0; > > > > > } > > > > > > > > IMHO, -ENOTSUPP for unsupported operation fits better than `false'. > > > > yes, currently there are only two possible returns: > > > > 0 -- success > > > > -ENOTSUPP - not supported operation > > > > > > > > though, we can extend functions later and return additional codes, other > > > > than `false' and `true'. > > > > > > Thing to expose to user isn't true and false but EINVAL. > > > > > > > sure. I mean we can return actual zcomp_set_max_streams() error (if any) > > back to user from max_comp_streams_store(): > > > > [..] > > ret = zcomp_set_max_streams(...); > > [..] > > return ret; > > > > > > > > > > for example, -E2BIG if user requested extremly large number of streams, > > > > like 5000 streams. > > > > > > I'm not sure it's right example for E2BIG. > > > When I read the comment, it says "argument list too long". > > > Anyway, when I tried ENOTSUPP, echo doesn't show meaningful error to user > > > and I dont' know it's casual err number for userspace. > > > Pz, comment about that. > > > > > > > E2BIG (introduced by POSIX.1-2001) was a quick example and, probably, not > > the perfect one. In general, I'm not against this change and I can live > > with zcomp_set_max_streams() returning bool. > > Then, could you give me Acked-by? > I am hungry with that. :) > sure :) Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> -ss > -- > Kind regards, > Minchan Kim > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-03-10 7:54 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-07 1:56 [PATCH] zram: propagate error to user Minchan Kim 2014-03-07 9:20 ` Sergey Senozhatsky 2014-03-07 9:51 ` Minchan Kim 2014-03-09 16:58 ` Sergey Senozhatsky 2014-03-10 2:01 ` Minchan Kim 2014-03-10 7:50 ` Sergey Senozhatsky
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox