Re: [PATCH] zram: don't copy invalid compression algorithms
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. > > You're right. Your patch totally makes sense. Sorry for the niose. > Please resend it on another thread out of this BS thread with Ccing > Andrew. patch resends better happen in separate threads. Luis, please Cc Andrew. I think the commit message also should document that this patch does change a user space visible behaviour in some cases (doesn't matter how seriously we take it, it better be 'documented'). -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: don't copy invalid compression algorithms
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 > > > *dev, > > > struct zram *zram = dev_to_zram(dev); > > > size_t sz; > > > > > > + if (!zcomp_available_algorithm(buf)) > > > + return -EINVAL; > > > > 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. You're right. Your patch totally makes sense. Sorry for the niose. Please resend it on another thread out of this BS thread with Ccing Andrew. > > -ss -- 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: don't copy invalid compression algorithms
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; > > > > + if (!zcomp_available_algorithm(buf)) > > + return -EINVAL; > > 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. -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: don't copy invalid compression algorithms
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 > /sys/block/zram0/disksize > -bash: echo: write error: Invalid argument > echo $? > 1 > > > B) > echo zzz > /sys/block/zram0/comp_algorithm > /dev/null > echo 1G > /sys/block/zram0/disksize > echo $? > 0 > > > which one *DOES* help finding and correcting an error and which one *DOES > NOT*? > a million dolla question. Wrong quiz. For the helping finding, user should do this. echo zzz > /sys/block/zram0/comp_algorithm echo $? If he want to not show error message, he should do. echo zzz > /sys/block/zram/comp_algorithm 2> /dev/null echo $? > > the difference between comp_algorithm store and any other store function - is > that comp_algorithm_store DOES ABSOLUTELY NOTHING. it does not allocate > memory, > free memory, register or unregister anything, change backends, etc., etc., > etc. > it does none of those. its only purpose is to strcpy() given data. this data > will be used later by a completely different function as a result of > additional > actions taken by user space. You are saying implementation of kernel, not interface itself. Nothing different. It's a interface to say something from the user to the kenrel. If user passes wrong input, normally, kernel should return a error and user should check it. It's a general rule for the programming for several decades. > > Returning back to our quiz. I do suspect that the answer is... "B"! > > > So, I still NACK the patch. It introduces a goto label, etc. In fact all > we need to do is to move zcomp_available_algorithm() up, before we grab > the ->init_lock. zcomp_available_algorithm() does not depend on anything > that requires a ->init_lock protection. > > Next, the patch lacks a reasoning/motivation in its commit message. What > we do in fact here is we introduce compression algorithm fallback to a > previously selected (knowingly supported, which has already passed > zcomp_available_algorithm()) or the `default_compressor'. > > Summarizing, it's something like this: > > --- > > From: Sergey SENOZHATSKY > Subject: [PATCH] zram: introduce comp algorithm fallback functionality > > When user supplies an unsupported compression algorithm, keep > a previously selected one (knowingly supported) or the default > one (in case if compression algorithm hasn't been changed before). > --- > 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 55e09db..255d68b 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; If you ignore tailling space, your change would make a bug. If you fix it, it looks good to me. I hope Luis can send it with his SOB and indication of your credit about checking with avoiding unnecessary locking if you don't mind. Thanks, Guys! > + > down_write(>init_lock); > if (init_done(zram)) { > up_write(>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(>init_lock); > return len; > } > -- > 2.5.1.454.g1616360 > -- 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: don't copy invalid compression algorithms
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 # echo hm > /sys/devices/virtual/block/zram0/queue/scheduler # echo $? 0 -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: don't copy invalid compression algorithms
On Tue, Sep 08, 2015 at 05:16:10PM +0900, Minchan Kim wrote: > 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 good reasons? > > I just heard you claim to take care of scripts which don't check > function's success at the moment function is called but check it > later via reading the knob later. > If we changes it, it breaks such scripts. > So, with your claim, there are two assumption. > > 1. script doesn't check return val at the moment function completes > 2. Instead, script checks it later via reading the knob again. > So, conclusion is we should keep wrong input in kernel side for them. > > It seems you insist on "we should keep wrong input from the userspace > in the kernel to show it if user *might* ask for his debug later" > What makes you think like above? > > I think such assumption is really from your brain, not real usecases. > Ok, I admit i'm not a god but if there is such thing in real practice, > we should help them to *correct* it rather than keeping such weired > thing. From the beginning, they should check his action's result with > return value, not dmesg, not reading the knob later, again. > > > it makes life easier for users. we added this check in Jun 25, 2015 > > No, it could make more bad scripts which not checks the result > of the action but rely on current awkward zram's interface. > Consider other knobs in the kernel. A few 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 Senozhatsky Bcc: Subject: Re: [PATCH] zram: don't copy invalid compression algorithms Reply-To: In-Reply-To: <20150908081610.GA8633@bbox> On Tue, Sep 08, 2015 at 05:16:10PM +0900, Minchan Kim wrote: > 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 good reasons? > > I just heard you claim to take care of scripts which don't check > function's success at the moment function is called but check it > later via reading the knob later. > If we changes it, it breaks such scripts. > So, with your claim, there are two assumption. > > 1. script doesn't check return val at the moment function completes > 2. Instead, script checks it later via reading the knob again. > So, conclusion is we should keep wrong input in kernel side for them. > > It seems you insist on "we should keep wrong input from the userspace > in the kernel to show it if user *might* ask for his debug later" > What makes you think like above? > > I think such assumption is really from your brain, not real usecases. > Ok, I admit i'm not a god but if there is such thing in real practice, > we should help them to *correct* it rather than keeping such weired > thing. From the beginning, they should check his action's result with > return value, not dmesg, not reading the knob later, again. > > > it makes life easier for users. we added this check in Jun 25, 2015 > > No, it could make more bad scripts which not checks the result > of the action but rely on current awkward zram's interface. > Consider other knobs in the kernel. A few 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 was expecting in zram. Of course, the semantics of this file is very different, but not changing the system status if invalid input is provided seems to be the sane default behaviour. For this reason, I still think my patch is doing the right thing. HOWEVER, I also believe Sergey has a very valid point: my patch changes the ABI with user-space, and this could actually be considered a regression. This all depends on how stable this sysfs interface is :-) So, if you guys decide to drop my patch, I would at least update the following files to describe the current behaviour: - Documentation/ABI/testing/sysfs-block-zram - Documentation/blockdev/zram.txt (I'm OK sending a patch to update these files once there is a decision on what to do.) Cheers, -- Luís > /sys/ke
Re: [PATCH] zram: don't copy invalid compression algorithms
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 > /sys/block/zram0/comp_algorithm > /dev/null echo 1G > /sys/block/zram0/disksize echo $? 0 which one *DOES* help finding and correcting an error and which one *DOES NOT*? a million dolla question. the difference between comp_algorithm store and any other store function - is that comp_algorithm_store DOES ABSOLUTELY NOTHING. it does not allocate memory, free memory, register or unregister anything, change backends, etc., etc., etc. it does none of those. its only purpose is to strcpy() given data. this data will be used later by a completely different function as a result of additional actions taken by user space. Returning back to our quiz. I do suspect that the answer is... "B"! So, I still NACK the patch. It introduces a goto label, etc. In fact all we need to do is to move zcomp_available_algorithm() up, before we grab the ->init_lock. zcomp_available_algorithm() does not depend on anything that requires a ->init_lock protection. Next, the patch lacks a reasoning/motivation in its commit message. What we do in fact here is we introduce compression algorithm fallback to a previously selected (knowingly supported, which has already passed zcomp_available_algorithm()) or the `default_compressor'. Summarizing, it's something like this: --- From: Sergey SENOZHATSKY Subject: [PATCH] zram: introduce comp algorithm fallback functionality When user supplies an unsupported compression algorithm, keep a previously selected one (knowingly supported) or the default one (in case if compression algorithm hasn't been changed before). --- 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 55e09db..255d68b 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(>init_lock); if (init_done(zram)) { up_write(>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(>init_lock); return len; } -- 2.5.1.454.g1616360 -- 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: don't copy invalid compression algorithms
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 good reasons? I just heard you claim to take care of scripts which don't check function's success at the moment function is called but check it later via reading the knob later. If we changes it, it breaks such scripts. So, with your claim, there are two assumption. 1. script doesn't check return val at the moment function completes 2. Instead, script checks it later via reading the knob again. So, conclusion is we should keep wrong input in kernel side for them. It seems you insist on "we should keep wrong input from the userspace in the kernel to show it if user *might* ask for his debug later" What makes you think like above? I think such assumption is really from your brain, not real usecases. Ok, I admit i'm not a god but if there is such thing in real practice, we should help them to *correct* it rather than keeping such weired thing. From the beginning, they should check his action's result with return value, not dmesg, not reading the knob later, again. > it makes life easier for users. we added this check in Jun 25, 2015 No, it could make more bad scripts which not checks the result of the action but rely on current awkward zram's interface. Consider other knobs in the kernel. A few things popped from my mind at the moment. /sys/block/sdb/queue/scheduler /sys/kernel/mm/transparent_hugepage/enabled /sys/kernel/debug/tracing/current_tracer /sys/devices/system/clocksource/clocksource/clocksource0/current_clocksource They are not showing wrong input user have passed although it was failed. Could you say a example of kernel interface did intentionally like you said? > while this functionality and scripts have been around for years, and > apparently now it's users' problem and they must go and do something. I believe anyone shouldn't rely on it. But who knows? However, I want to make it sane(ie, only change compressor name if the action is successful). Please, let's discuss how we do it rather than whether it's useful or not. > > > seriously, what improvement this change brings in the first place? > what does it make better and for whom? As I mentioned, it makes zram's ABI consistent with others in kernel space so it makes user feel zram is straight-forward and sane like others. > > -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: don't copy invalid compression algorithms
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 good reasons? I just heard you claim to take care of scripts which don't check function's success at the moment function is called but check it later via reading the knob later. If we changes it, it breaks such scripts. So, with your claim, there are two assumption. 1. script doesn't check return val at the moment function completes 2. Instead, script checks it later via reading the knob again. So, conclusion is we should keep wrong input in kernel side for them. It seems you insist on "we should keep wrong input from the userspace in the kernel to show it if user *might* ask for his debug later" What makes you think like above? I think such assumption is really from your brain, not real usecases. Ok, I admit i'm not a god but if there is such thing in real practice, we should help them to *correct* it rather than keeping such weired thing. From the beginning, they should check his action's result with return value, not dmesg, not reading the knob later, again. > it makes life easier for users. we added this check in Jun 25, 2015 No, it could make more bad scripts which not checks the result of the action but rely on current awkward zram's interface. Consider other knobs in the kernel. A few things popped from my mind at the moment. /sys/block/sdb/queue/scheduler /sys/kernel/mm/transparent_hugepage/enabled /sys/kernel/debug/tracing/current_tracer /sys/devices/system/clocksource/clocksource/clocksource0/current_clocksource They are not showing wrong input user have passed although it was failed. Could you say a example of kernel interface did intentionally like you said? > while this functionality and scripts have been around for years, and > apparently now it's users' problem and they must go and do something. I believe anyone shouldn't rely on it. But who knows? However, I want to make it sane(ie, only change compressor name if the action is successful). Please, let's discuss how we do it rather than whether it's useful or not. > > > seriously, what improvement this change brings in the first place? > what does it make better and for whom? As I mentioned, it makes zram's ABI consistent with others in kernel space so it makes user feel zram is straight-forward and sane like others. > > -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: don't copy invalid compression algorithms
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 > /sys/block/zram0/comp_algorithm > /dev/null echo 1G > /sys/block/zram0/disksize echo $? 0 which one *DOES* help finding and correcting an error and which one *DOES NOT*? a million dolla question. the difference between comp_algorithm store and any other store function - is that comp_algorithm_store DOES ABSOLUTELY NOTHING. it does not allocate memory, free memory, register or unregister anything, change backends, etc., etc., etc. it does none of those. its only purpose is to strcpy() given data. this data will be used later by a completely different function as a result of additional actions taken by user space. Returning back to our quiz. I do suspect that the answer is... "B"! So, I still NACK the patch. It introduces a goto label, etc. In fact all we need to do is to move zcomp_available_algorithm() up, before we grab the ->init_lock. zcomp_available_algorithm() does not depend on anything that requires a ->init_lock protection. Next, the patch lacks a reasoning/motivation in its commit message. What we do in fact here is we introduce compression algorithm fallback to a previously selected (knowingly supported, which has already passed zcomp_available_algorithm()) or the `default_compressor'. Summarizing, it's something like this: --- From: Sergey SENOZHATSKYSubject: [PATCH] zram: introduce comp algorithm fallback functionality When user supplies an unsupported compression algorithm, keep a previously selected one (knowingly supported) or the default one (in case if compression algorithm hasn't been changed before). --- 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 55e09db..255d68b 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(>init_lock); if (init_done(zram)) { up_write(>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(>init_lock); return len; } -- 2.5.1.454.g1616360 -- 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: don't copy invalid compression algorithms
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 # echo hm > /sys/devices/virtual/block/zram0/queue/scheduler # echo $? 0 -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: don't copy invalid compression algorithms
On Tue, Sep 08, 2015 at 05:16:10PM +0900, Minchan Kim wrote: > 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 good reasons? > > I just heard you claim to take care of scripts which don't check > function's success at the moment function is called but check it > later via reading the knob later. > If we changes it, it breaks such scripts. > So, with your claim, there are two assumption. > > 1. script doesn't check return val at the moment function completes > 2. Instead, script checks it later via reading the knob again. > So, conclusion is we should keep wrong input in kernel side for them. > > It seems you insist on "we should keep wrong input from the userspace > in the kernel to show it if user *might* ask for his debug later" > What makes you think like above? > > I think such assumption is really from your brain, not real usecases. > Ok, I admit i'm not a god but if there is such thing in real practice, > we should help them to *correct* it rather than keeping such weired > thing. From the beginning, they should check his action's result with > return value, not dmesg, not reading the knob later, again. > > > it makes life easier for users. we added this check in Jun 25, 2015 > > No, it could make more bad scripts which not checks the result > of the action but rely on current awkward zram's interface. > Consider other knobs in the kernel. A few 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...@gmail.com>, linux-kernel@vger.kernel.org, Sergey Senozhatsky <sergey.senozhat...@gmail.com> Bcc: Subject: Re: [PATCH] zram: don't copy invalid compression algorithms Reply-To: In-Reply-To: <20150908081610.GA8633@bbox> On Tue, Sep 08, 2015 at 05:16:10PM +0900, Minchan Kim wrote: > 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 good reasons? > > I just heard you claim to take care of scripts which don't check > function's success at the moment function is called but check it > later via reading the knob later. > If we changes it, it breaks such scripts. > So, with your claim, there are two assumption. > > 1. script doesn't check return val at the moment function completes > 2. Instead, script checks it later via reading the knob again. > So, conclusion is we should keep wrong input in kernel side for them. > > It seems you insist on "we should keep wrong input from the userspace > in the kernel to show it if user *might* ask for his debug later" > What makes you think like above? > > I think such assumption is really from your brain, not real usecases. > Ok, I admit i'm not a god but if there is such thing in real practice, > we should help them to *correct* it rather than keeping such weired > thing. From the beginning, they should check his action's result with > return value, not dmesg, not reading the knob later, again. > > > it makes life easier for users. we added this check in Jun 25, 2015 > > No, it could make more bad scripts which not checks the result > of the action but rely on current awkward zram's interface. > Consider other knobs in the kernel. A few 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 was expecting in zram. Of course, the semantics of this file is very different, but not changing the system status if invalid input is provided seems to be the sane default behaviour. For this reason, I still think my patch is doing the right thing. HOWEVER, I also believe Sergey has a very valid point: my patch changes the ABI with user-space, and this could actually be considered a regression. This all depends on how stable this sysfs interface is :-) So, if you guys decide to drop my patch, I would at least update the following files to describe the current behaviour: - Documentation/ABI/testing/sysfs-block-zram - Documentation/blockdev/zram.txt (I'm OK sending a patch
Re: [PATCH] zram: don't copy invalid compression algorithms
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; > > > > + if (!zcomp_available_algorithm(buf)) > > + return -EINVAL; > > 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. -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: don't copy invalid compression algorithms
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 > /sys/block/zram0/disksize > -bash: echo: write error: Invalid argument > echo $? > 1 > > > B) > echo zzz > /sys/block/zram0/comp_algorithm > /dev/null > echo 1G > /sys/block/zram0/disksize > echo $? > 0 > > > which one *DOES* help finding and correcting an error and which one *DOES > NOT*? > a million dolla question. Wrong quiz. For the helping finding, user should do this. echo zzz > /sys/block/zram0/comp_algorithm echo $? If he want to not show error message, he should do. echo zzz > /sys/block/zram/comp_algorithm 2> /dev/null echo $? > > the difference between comp_algorithm store and any other store function - is > that comp_algorithm_store DOES ABSOLUTELY NOTHING. it does not allocate > memory, > free memory, register or unregister anything, change backends, etc., etc., > etc. > it does none of those. its only purpose is to strcpy() given data. this data > will be used later by a completely different function as a result of > additional > actions taken by user space. You are saying implementation of kernel, not interface itself. Nothing different. It's a interface to say something from the user to the kenrel. If user passes wrong input, normally, kernel should return a error and user should check it. It's a general rule for the programming for several decades. > > Returning back to our quiz. I do suspect that the answer is... "B"! > > > So, I still NACK the patch. It introduces a goto label, etc. In fact all > we need to do is to move zcomp_available_algorithm() up, before we grab > the ->init_lock. zcomp_available_algorithm() does not depend on anything > that requires a ->init_lock protection. > > Next, the patch lacks a reasoning/motivation in its commit message. What > we do in fact here is we introduce compression algorithm fallback to a > previously selected (knowingly supported, which has already passed > zcomp_available_algorithm()) or the `default_compressor'. > > Summarizing, it's something like this: > > --- > > From: Sergey SENOZHATSKY> Subject: [PATCH] zram: introduce comp algorithm fallback functionality > > When user supplies an unsupported compression algorithm, keep > a previously selected one (knowingly supported) or the default > one (in case if compression algorithm hasn't been changed before). > --- > 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 55e09db..255d68b 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; If you ignore tailling space, your change would make a bug. If you fix it, it looks good to me. I hope Luis can send it with his SOB and indication of your credit about checking with avoiding unnecessary locking if you don't mind. Thanks, Guys! > + > down_write(>init_lock); > if (init_done(zram)) { > up_write(>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(>init_lock); > return len; > } > -- > 2.5.1.454.g1616360 > -- 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: don't copy invalid compression algorithms
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. > > You're right. Your patch totally makes sense. Sorry for the niose. > Please resend it on another thread out of this BS thread with Ccing > Andrew. patch resends better happen in separate threads. Luis, please Cc Andrew. I think the commit message also should document that this patch does change a user space visible behaviour in some cases (doesn't matter how seriously we take it, it better be 'documented'). -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: don't copy invalid compression algorithms
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 > > > *dev, > > > struct zram *zram = dev_to_zram(dev); > > > size_t sz; > > > > > > + if (!zcomp_available_algorithm(buf)) > > > + return -EINVAL; > > > > 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. You're right. Your patch totally makes sense. Sorry for the niose. Please resend it on another thread out of this BS thread with Ccing Andrew. > > -ss -- 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: don't copy invalid compression algorithms
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 > /sys/block/zram0/max_comp_streams clever or stupid? do we control mem_limit_store()? no. do we contol mem_used_max_store()? no. if user asks to redefine a "default" value we just let him do so. -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: don't copy invalid compression algorithms
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 and scripts have been around for years, and apparently now it's users' problem and they must go and do something. seriously, what improvement this change brings in the first place? what does it make better and for whom? -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: don't copy invalid compression algorithms
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 device management' script > > > > echo llzo > /sys/block/zram0/comp_algorithm > > -bash: echo: write error: Invalid argument > > > > > > but the script ignores 'echo: write error'. > > Because we added compression algorithm name check recently. > > > > then the script does > > > > echo 200M > /sys/block/zram0/disksize > > -bash: echo: write error: Invalid argument > > > > > > doing a simple dmesg reveals the problem > > > > [ 7076.657184] zram: Cannot initialise llzo compressing backend > > > > note that zram provides 'llzo' here, which is convenient. > > ah, forgot to mention. there is another misleading thing. > > suppose the script checks the comp_algorithm() error code. > and it attempts to do somthing like > echo llzo > /sys/block/zram0/comp_algorithm > -bash: echo: write error: Device or resource busy > > > so user knows that comp_algorithm failed. so now > he/she goes and checks zram > > cat /sys/block/zram0/comp_algorithm > [lzo] lz4 > > > and finds out... that [lzo] is supported and selected for usage. > > so what't the problem then? so user wrongly assumes now that the > script has provided 'lzo' as input to zram... false! > > > > the existing scheme of things will provide additional hint. > > #current implementation > cat /sys/block/zram0/comp_algorithm > lzo lz4 > > so, none of the supported compression algorithms is selected. > aha, that is obviously lead us to a conclusion that something > wrong was with the input that script provided to zram. correct! The problem is caused by that user skipped check of whether his action was successful or not. IOW, script should have chekcked status of "echo llzo > ". User shouldn't rely on dmesg. So, I don't think it's good idea to paper over user's mistake. And it's straightforward/consistent to change the thing's state only if is successful. 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 return value of doing. It doesn't sound to make sense to me. > > -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: don't copy invalid compression algorithms
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 > -bash: echo: write error: Invalid argument > > > but the script ignores 'echo: write error'. > Because we added compression algorithm name check recently. > > then the script does > > echo 200M > /sys/block/zram0/disksize > -bash: echo: write error: Invalid argument > > > doing a simple dmesg reveals the problem > > [ 7076.657184] zram: Cannot initialise llzo compressing backend > > note that zram provides 'llzo' here, which is convenient. ah, forgot to mention. there is another misleading thing. suppose the script checks the comp_algorithm() error code. and it attempts to do somthing like echo llzo > /sys/block/zram0/comp_algorithm -bash: echo: write error: Device or resource busy so user knows that comp_algorithm failed. so now he/she goes and checks zram cat /sys/block/zram0/comp_algorithm [lzo] lz4 and finds out... that [lzo] is supported and selected for usage. so what't the problem then? so user wrongly assumes now that the script has provided 'lzo' as input to zram... false! the existing scheme of things will provide additional hint. #current implementation cat /sys/block/zram0/comp_algorithm lzo lz4 so, none of the supported compression algorithms is selected. aha, that is obviously lead us to a conclusion that something wrong was with the input that script provided to zram. correct! -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: don't copy invalid compression algorithms
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 will be able to figure out the root cause, because zram will report > > to syslog an actually requested alg name. > > > > Example > > > > [ 1669.473296] zram: Cannot initialise llzo compressing backend > > 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 -bash: echo: write error: Invalid argument but the script ignores 'echo: write error'. Because we added compression algorithm name check recently. then the script does echo 200M > /sys/block/zram0/disksize -bash: echo: write error: Invalid argument doing a simple dmesg reveals the problem [ 7076.657184] zram: Cannot initialise llzo compressing backend note that zram provides 'llzo' here, which is convenient. With this change the semantics is changing and zram now swallows (hides) the user space error. echo llzo > /sys/block/zram0/comp_algorithm -bash: echo: write error: Invalid argument echo 200M > /sys/block/zram0/disksize instead of using a requested 'llzo' zram for some reason switched to 'lzo'. I don't like this behaviour change. User requested to change the 'default' value, and that new value didn't work out. No reason to hide 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: don't copy invalid compression algorithms
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 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 will be able to figure out the root cause, because zram will report > to syslog an actually requested alg name. > > Example > > [ 1669.473296] zram: Cannot initialise llzo compressing backend I don't understand your concern. To me, this patch makes sense to me. Could you explain your point clearly, again? Thanks. > > > -ss > > > The error path code is also slightly refactored. > > > > Signed-off-by: Luis Henriques > > --- > > drivers/block/zram/zram_drv.c | 13 - > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > > index 9c01f5bfa33f..33551ec9e7f5 100644 > > --- a/drivers/block/zram/zram_drv.c > > +++ b/drivers/block/zram/zram_drv.c > > @@ -367,10 +367,15 @@ static ssize_t comp_algorithm_store(struct device > > *dev, > > > > down_write(>init_lock); > > if (init_done(zram)) { > > - up_write(>init_lock); > > pr_info("Can't change algorithm for initialized device\n"); > > - return -EBUSY; > > + len = -EBUSY; > > + goto out; > > + } > > + if (!zcomp_available_algorithm(buf)) { > > + len = -EINVAL; > > + goto out; > > } > > + > > strlcpy(zram->compressor, buf, sizeof(zram->compressor)); > > > > /* ignore trailing newline */ > > @@ -378,9 +383,7 @@ 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; > > - > > +out: > > 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: don't copy invalid compression algorithms
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() historically, so someone's script can simply ignore it. However, the script will fail to init the device and user will be able to figure out the root cause, because zram will report to syslog an actually requested alg name. Example [ 1669.473296] zram: Cannot initialise llzo compressing backend -ss > The error path code is also slightly refactored. > > Signed-off-by: Luis Henriques > --- > drivers/block/zram/zram_drv.c | 13 - > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index 9c01f5bfa33f..33551ec9e7f5 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -367,10 +367,15 @@ static ssize_t comp_algorithm_store(struct device *dev, > > down_write(>init_lock); > if (init_done(zram)) { > - up_write(>init_lock); > pr_info("Can't change algorithm for initialized device\n"); > - return -EBUSY; > + len = -EBUSY; > + goto out; > + } > + if (!zcomp_available_algorithm(buf)) { > + len = -EINVAL; > + goto out; > } > + > strlcpy(zram->compressor, buf, sizeof(zram->compressor)); > > /* ignore trailing newline */ > @@ -378,9 +383,7 @@ 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; > - > +out: > 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: don't copy invalid compression algorithms
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 > /sys/block/zram0/max_comp_streams clever or stupid? do we control mem_limit_store()? no. do we contol mem_used_max_store()? no. if user asks to redefine a "default" value we just let him do so. -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: don't copy invalid compression algorithms
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() historically, so someone's script can simply ignore it. However, the script will fail to init the device and user will be able to figure out the root cause, because zram will report to syslog an actually requested alg name. Example [ 1669.473296] zram: Cannot initialise llzo compressing backend -ss > The error path code is also slightly refactored. > > Signed-off-by: Luis Henriques> --- > drivers/block/zram/zram_drv.c | 13 - > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index 9c01f5bfa33f..33551ec9e7f5 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -367,10 +367,15 @@ static ssize_t comp_algorithm_store(struct device *dev, > > down_write(>init_lock); > if (init_done(zram)) { > - up_write(>init_lock); > pr_info("Can't change algorithm for initialized device\n"); > - return -EBUSY; > + len = -EBUSY; > + goto out; > + } > + if (!zcomp_available_algorithm(buf)) { > + len = -EINVAL; > + goto out; > } > + > strlcpy(zram->compressor, buf, sizeof(zram->compressor)); > > /* ignore trailing newline */ > @@ -378,9 +383,7 @@ 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; > - > +out: > 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: don't copy invalid compression algorithms
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 > -bash: echo: write error: Invalid argument > > > but the script ignores 'echo: write error'. > Because we added compression algorithm name check recently. > > then the script does > > echo 200M > /sys/block/zram0/disksize > -bash: echo: write error: Invalid argument > > > doing a simple dmesg reveals the problem > > [ 7076.657184] zram: Cannot initialise llzo compressing backend > > note that zram provides 'llzo' here, which is convenient. ah, forgot to mention. there is another misleading thing. suppose the script checks the comp_algorithm() error code. and it attempts to do somthing like echo llzo > /sys/block/zram0/comp_algorithm -bash: echo: write error: Device or resource busy so user knows that comp_algorithm failed. so now he/she goes and checks zram cat /sys/block/zram0/comp_algorithm [lzo] lz4 and finds out... that [lzo] is supported and selected for usage. so what't the problem then? so user wrongly assumes now that the script has provided 'lzo' as input to zram... false! the existing scheme of things will provide additional hint. #current implementation cat /sys/block/zram0/comp_algorithm lzo lz4 so, none of the supported compression algorithms is selected. aha, that is obviously lead us to a conclusion that something wrong was with the input that script provided to zram. correct! -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: don't copy invalid compression algorithms
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 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 will be able to figure out the root cause, because zram will report > to syslog an actually requested alg name. > > Example > > [ 1669.473296] zram: Cannot initialise llzo compressing backend I don't understand your concern. To me, this patch makes sense to me. Could you explain your point clearly, again? Thanks. > > > -ss > > > The error path code is also slightly refactored. > > > > Signed-off-by: Luis Henriques> > --- > > drivers/block/zram/zram_drv.c | 13 - > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > > index 9c01f5bfa33f..33551ec9e7f5 100644 > > --- a/drivers/block/zram/zram_drv.c > > +++ b/drivers/block/zram/zram_drv.c > > @@ -367,10 +367,15 @@ static ssize_t comp_algorithm_store(struct device > > *dev, > > > > down_write(>init_lock); > > if (init_done(zram)) { > > - up_write(>init_lock); > > pr_info("Can't change algorithm for initialized device\n"); > > - return -EBUSY; > > + len = -EBUSY; > > + goto out; > > + } > > + if (!zcomp_available_algorithm(buf)) { > > + len = -EINVAL; > > + goto out; > > } > > + > > strlcpy(zram->compressor, buf, sizeof(zram->compressor)); > > > > /* ignore trailing newline */ > > @@ -378,9 +383,7 @@ 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; > > - > > +out: > > 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: don't copy invalid compression algorithms
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 device management' script > > > > echo llzo > /sys/block/zram0/comp_algorithm > > -bash: echo: write error: Invalid argument > > > > > > but the script ignores 'echo: write error'. > > Because we added compression algorithm name check recently. > > > > then the script does > > > > echo 200M > /sys/block/zram0/disksize > > -bash: echo: write error: Invalid argument > > > > > > doing a simple dmesg reveals the problem > > > > [ 7076.657184] zram: Cannot initialise llzo compressing backend > > > > note that zram provides 'llzo' here, which is convenient. > > ah, forgot to mention. there is another misleading thing. > > suppose the script checks the comp_algorithm() error code. > and it attempts to do somthing like > echo llzo > /sys/block/zram0/comp_algorithm > -bash: echo: write error: Device or resource busy > > > so user knows that comp_algorithm failed. so now > he/she goes and checks zram > > cat /sys/block/zram0/comp_algorithm > [lzo] lz4 > > > and finds out... that [lzo] is supported and selected for usage. > > so what't the problem then? so user wrongly assumes now that the > script has provided 'lzo' as input to zram... false! > > > > the existing scheme of things will provide additional hint. > > #current implementation > cat /sys/block/zram0/comp_algorithm > lzo lz4 > > so, none of the supported compression algorithms is selected. > aha, that is obviously lead us to a conclusion that something > wrong was with the input that script provided to zram. correct! The problem is caused by that user skipped check of whether his action was successful or not. IOW, script should have chekcked status of "echo llzo > ". User shouldn't rely on dmesg. So, I don't think it's good idea to paper over user's mistake. And it's straightforward/consistent to change the thing's state only if is successful. 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 return value of doing. It doesn't sound to make sense to me. > > -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: don't copy invalid compression algorithms
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 will be able to figure out the root cause, because zram will report > > to syslog an actually requested alg name. > > > > Example > > > > [ 1669.473296] zram: Cannot initialise llzo compressing backend > > 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 -bash: echo: write error: Invalid argument but the script ignores 'echo: write error'. Because we added compression algorithm name check recently. then the script does echo 200M > /sys/block/zram0/disksize -bash: echo: write error: Invalid argument doing a simple dmesg reveals the problem [ 7076.657184] zram: Cannot initialise llzo compressing backend note that zram provides 'llzo' here, which is convenient. With this change the semantics is changing and zram now swallows (hides) the user space error. echo llzo > /sys/block/zram0/comp_algorithm -bash: echo: write error: Invalid argument echo 200M > /sys/block/zram0/disksize instead of using a requested 'llzo' zram for some reason switched to 'lzo'. I don't like this behaviour change. User requested to change the 'default' value, and that new value didn't work out. No reason to hide 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: don't copy invalid compression algorithms
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 and scripts have been around for years, and apparently now it's users' problem and they must go and do something. seriously, what improvement this change brings in the first place? what does it make better and for whom? -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/