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

[Luis Henriques <luis.henriques@canonical.com>]

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 <minchan@kernel.org>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
	linux-kernel@vger.kernel.org,
	Sergey Senozhatsky <sergey.senozhatsky@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 to update these files once there is a decision on
what to do.)

Cheers,
--
Luís

> /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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/