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

2015-09-08 Thread Sergey Senozhatsky
On (09/08/15 23:21), Minchan Kim wrote: [..] > > > If you ignore tailling space, your change would make a bug. > > > If you fix it, it looks good to me. > > > > that's why find_backend() calls sysfs_streq(), which takes care of > > a trailing new line. unless you're speaking about something else.

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

2015-09-08 Thread Minchan Kim
On Tue, Sep 08, 2015 at 10:44:01PM +0900, Sergey Senozhatsky wrote: > dammit, not going to waste my time on this BS anymore. > > > > > > --- a/drivers/block/zram/zram_drv.c > > > +++ b/drivers/block/zram/zram_drv.c > > > @@ -365,6 +365,9 @@ static ssize_t comp_algorithm_store(struct device > >

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

2015-09-08 Thread Sergey Senozhatsky
dammit, not going to waste my time on this BS anymore. > > --- a/drivers/block/zram/zram_drv.c > > +++ b/drivers/block/zram/zram_drv.c > > @@ -365,6 +365,9 @@ static ssize_t comp_algorithm_store(struct device *dev, > > struct zram *zram = dev_to_zram(dev); > > size_t sz; > > > > +

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

2015-09-08 Thread Minchan Kim
On Tue, Sep 08, 2015 at 06:54:28PM +0900, Sergey Senozhatsky wrote: > On (09/08/15 17:16), Minchan Kim wrote: > > we should help them to *correct* it rather than keeping such weired > > thing. > > A simple quiz > > A) > echo zzz > /sys/block/zram0/comp_algorithm > /dev/null > echo 1G >

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

2015-09-08 Thread Sergey Senozhatsky
On (09/08/15 11:08), Luis Henriques wrote: [..] > > > > /sys/block/sdb/queue/scheduler > > This one is actually a good example > ... its behaviour depends on the way the kernel uses that queue. # echo THIS_EXIST_ONLY_IN_MY_HEAD > /sys/devices/virtual/block/zram0/queue/scheduler # echo $? 0 #

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

2015-09-08 Thread Luis Henriques
things popped from my mind > at the moment. > > /sys/block/sdb/queue/scheduler This one is actually a good example, and its behaviour is exactly what I expecting in zram. But I believe To: Minchan Kim Cc: Sergey Senozhatsky , linux-kernel@vger.kernel.org, Sergey Senozhat

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

2015-09-08 Thread Sergey Senozhatsky
On (09/08/15 17:16), Minchan Kim wrote: > we should help them to *correct* it rather than keeping such weired > thing. A simple quiz A) echo zzz > /sys/block/zram0/comp_algorithm > /dev/null echo 1G > /sys/block/zram0/disksize -bash: echo: write error: Invalid argument echo $? 1 B) echo zzz >

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

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

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

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

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

2015-09-08 Thread Sergey Senozhatsky
On (09/08/15 17:16), Minchan Kim wrote: > we should help them to *correct* it rather than keeping such weired > thing. A simple quiz A) echo zzz > /sys/block/zram0/comp_algorithm > /dev/null echo 1G > /sys/block/zram0/disksize -bash: echo: write error: Invalid argument echo $? 1 B) echo zzz >

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

2015-09-08 Thread Sergey Senozhatsky
On (09/08/15 11:08), Luis Henriques wrote: [..] > > > > /sys/block/sdb/queue/scheduler > > This one is actually a good example > ... its behaviour depends on the way the kernel uses that queue. # echo THIS_EXIST_ONLY_IN_MY_HEAD > /sys/devices/virtual/block/zram0/queue/scheduler # echo $? 0 #

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

2015-09-08 Thread Luis Henriques
things popped from my mind > at the moment. > > /sys/block/sdb/queue/scheduler This one is actually a good example, and its behaviour is exactly what I expecting in zram. But I believe To: Minchan Kim <minc...@kernel.org> Cc: Sergey Senozhatsky <sergey.senozhatsky.w...@gm

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

2015-09-08 Thread Sergey Senozhatsky
dammit, not going to waste my time on this BS anymore. > > --- a/drivers/block/zram/zram_drv.c > > +++ b/drivers/block/zram/zram_drv.c > > @@ -365,6 +365,9 @@ static ssize_t comp_algorithm_store(struct device *dev, > > struct zram *zram = dev_to_zram(dev); > > size_t sz; > > > > +

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

2015-09-08 Thread Minchan Kim
On Tue, Sep 08, 2015 at 06:54:28PM +0900, Sergey Senozhatsky wrote: > On (09/08/15 17:16), Minchan Kim wrote: > > we should help them to *correct* it rather than keeping such weired > > thing. > > A simple quiz > > A) > echo zzz > /sys/block/zram0/comp_algorithm > /dev/null > echo 1G >

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

2015-09-08 Thread Sergey Senozhatsky
On (09/08/15 23:21), Minchan Kim wrote: [..] > > > If you ignore tailling space, your change would make a bug. > > > If you fix it, it looks good to me. > > > > that's why find_backend() calls sysfs_streq(), which takes care of > > a trailing new line. unless you're speaking about something else.

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

2015-09-08 Thread Minchan Kim
On Tue, Sep 08, 2015 at 10:44:01PM +0900, Sergey Senozhatsky wrote: > dammit, not going to waste my time on this BS anymore. > > > > > > --- a/drivers/block/zram/zram_drv.c > > > +++ b/drivers/block/zram/zram_drv.c > > > @@ -365,6 +365,9 @@ static ssize_t comp_algorithm_store(struct device > >

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

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

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

2015-09-07 Thread Sergey Senozhatsky
On (09/08/15 13:50), Minchan Kim wrote: [..] > And it's straightforward/consistent to change the thing's state > only if is successful. > what for? I provided several good reasons not to do this, because it makes life easier for users. we added this check in Jun 25, 2015 while this functionality

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

2015-09-07 Thread Minchan Kim
On Tue, Sep 08, 2015 at 10:58:31AM +0900, Sergey Senozhatsky wrote: > On (09/08/15 10:33), Sergey Senozhatsky wrote: > > > I don't understand your concern. To me, this patch makes sense to me. > > > Could you explain your point clearly, again? > > > > OK. suppose someone landed a typo in a 'zram

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

2015-09-07 Thread Sergey Senozhatsky
On (09/08/15 10:33), Sergey Senozhatsky wrote: > > I don't understand your concern. To me, this patch makes sense to me. > > Could you explain your point clearly, again? > > OK. suppose someone landed a typo in a 'zram device management' script > > echo llzo > /sys/block/zram0/comp_algorithm >

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

2015-09-07 Thread Sergey Senozhatsky
On (09/08/15 10:14), Minchan Kim wrote: [..] > > NACK. > > > > This is intentional. We haven't returned 'invalid compression algorithm' > > error from comp_algorithm_store() historically, so someone's script can > > simply ignore it. However, the script will fail to init the device and > > user

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

2015-09-07 Thread Minchan Kim
Hey Sergey, On Tue, Sep 08, 2015 at 08:56:35AM +0900, Sergey Senozhatsky wrote: > On (09/07/15 21:48), Luis Henriques wrote: > > Validate the new compression algorithm before copying it into the zram > > 'compressor' field, keeping the old one if it's invalid. > > > > NACK. > > This is

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

2015-09-07 Thread Sergey Senozhatsky
On (09/07/15 21:48), Luis Henriques wrote: > Validate the new compression algorithm before copying it into the zram > 'compressor' field, keeping the old one if it's invalid. > NACK. This is intentional. We haven't returned 'invalid compression algorithm' error from comp_algorithm_store()

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

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

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

2015-09-07 Thread Sergey Senozhatsky
On (09/07/15 21:48), Luis Henriques wrote: > Validate the new compression algorithm before copying it into the zram > 'compressor' field, keeping the old one if it's invalid. > NACK. This is intentional. We haven't returned 'invalid compression algorithm' error from comp_algorithm_store()

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

2015-09-07 Thread Sergey Senozhatsky
On (09/08/15 10:33), Sergey Senozhatsky wrote: > > I don't understand your concern. To me, this patch makes sense to me. > > Could you explain your point clearly, again? > > OK. suppose someone landed a typo in a 'zram device management' script > > echo llzo > /sys/block/zram0/comp_algorithm >

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

2015-09-07 Thread Minchan Kim
Hey Sergey, On Tue, Sep 08, 2015 at 08:56:35AM +0900, Sergey Senozhatsky wrote: > On (09/07/15 21:48), Luis Henriques wrote: > > Validate the new compression algorithm before copying it into the zram > > 'compressor' field, keeping the old one if it's invalid. > > > > NACK. > > This is

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

2015-09-07 Thread Minchan Kim
On Tue, Sep 08, 2015 at 10:58:31AM +0900, Sergey Senozhatsky wrote: > On (09/08/15 10:33), Sergey Senozhatsky wrote: > > > I don't understand your concern. To me, this patch makes sense to me. > > > Could you explain your point clearly, again? > > > > OK. suppose someone landed a typo in a 'zram

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

2015-09-07 Thread Sergey Senozhatsky
On (09/08/15 10:14), Minchan Kim wrote: [..] > > NACK. > > > > This is intentional. We haven't returned 'invalid compression algorithm' > > error from comp_algorithm_store() historically, so someone's script can > > simply ignore it. However, the script will fail to init the device and > > user

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

2015-09-07 Thread Sergey Senozhatsky
On (09/08/15 13:50), Minchan Kim wrote: [..] > And it's straightforward/consistent to change the thing's state > only if is successful. > what for? I provided several good reasons not to do this, because it makes life easier for users. we added this check in Jun 25, 2015 while this functionality