From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754204AbbIIApP (ORCPT ); Tue, 8 Sep 2015 20:45:15 -0400 Received: from mail-pa0-f52.google.com ([209.85.220.52]:35939 "EHLO mail-pa0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752642AbbIIAoi (ORCPT ); Tue, 8 Sep 2015 20:44:38 -0400 Date: Wed, 9 Sep 2015 09:45:18 +0900 From: Sergey Senozhatsky To: Luis Henriques Cc: Minchan Kim , Sergey Senozhatsky , Andrew Morton , linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCH v2] zram: introduce comp algorithm fallback functionality Message-ID: <20150909004518.GA1883@swordfish> References: <1441737776-25280-1-git-send-email-luis.henriques@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1441737776-25280-1-git-send-email-luis.henriques@canonical.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On (09/08/15 19:42), Luis Henriques wrote: > > Note that previously this operation (i.e. setting an invalid algorithm) > would result in no algorithm being selected, which means that this > represents a small change in the default behaviour. > previously it would result in guaranteed to fail echo xxx > /sys/block/zram/disksize and thus in no device, not just "in no algorithm being selected". comp_algorithm historically wasn't returning anything back to user space and it always copied in its input (what later would have been used as a compression algorithm name). yes, "wasn't returning anything back" is not entirely correct, there was (and still is) a chance to receive -EBUSY, but that was (and still is) is absolutely equivalent to /sys/block/zram/initstate != 0 (well, I never had a clear understanding of why comp_algorithm and other attrs return -EBUSY instead of simply silently ignoring the input; is it really an error? there is a initstate attr for that, but I never had enough reasons to change it either). anyway, believe me or not, that's what my toy scripts were doing for years (and hopefully it's not because of the fact that I'm completely unaware of the trivial basics of software development, as we found out yesterday, but because zram ABI was letting me to do so). # head -17 create-zram --- #!/bin/sh rmmod zram modprobe zram if [ -e /sys/block/zram0/initstate ]; then initdone=`cat /sys/block/zram0/initstate` if [ $initdone = 1 ]; then echo "init done" exit 1 fi fi echo 8 > /sys/block/zram0/max_comp_streams echo $1 > /sys/block/zram0/comp_algorithm cat /sys/block/zram0/comp_algorithm --- So, "./create-zram LLZZOO 2G" will have different result now. Other than that Acked-by: Sergey Senozhatsky -ss > Cc: Sergey Senozhatsky > Signed-off-by: Luis Henriques > --- > Changes since v1: > * Moved the algorithm check further up, before mutex lock > * Dropped error path refactoring > * Updated commit text to explicitly refer to the ABI change > (changes suggested by Sergey and Minchan) > > drivers/block/zram/zram_drv.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index 9c01f5bfa33f..1caa8f793e51 100644 > --- 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; > > + if (!zcomp_available_algorithm(buf)) > + return -EINVAL; > + > down_write(&zram->init_lock); > if (init_done(zram)) { > up_write(&zram->init_lock); > @@ -378,9 +381,6 @@ static ssize_t comp_algorithm_store(struct device *dev, > if (sz > 0 && zram->compressor[sz - 1] == '\n') > zram->compressor[sz - 1] = 0x00; > > - if (!zcomp_available_algorithm(zram->compressor)) > - len = -EINVAL; > - > up_write(&zram->init_lock); > return len; > } >