Re: [PATCH v2 5/7] zram: support idle/huge page writeback

[Minchan Kim <minchan@kernel.org>]

On Sun, Nov 25, 2018 at 11:47:37PM -1000, Joey Pabalinas wrote:
> On Mon, Nov 26, 2018 at 05:28:11PM +0900, Minchan Kim wrote:
> > +	strlcpy(mode_buf, buf, sizeof(mode_buf));
> > +	/* ignore trailing newline */
> > +	sz = strlen(mode_buf);
> 
> One possible idea would be to use strscpy() instead and directly assign
> the return value to sz, avoiding an extra strlen() call (though you would
> have to check if `sz == -E2BIG` and do `sz = sizeof(mode_buf) - 1` in that
> case).

Thanks for the suggstion.
If I limit destination buffer smaller, I couldn't meet -E2BIG?

> 
> > +	if (!strcmp(mode_buf, "idle"))
> > +		mode = IDLE_WRITEBACK;
> > +	if (!strcmp(mode_buf, "huge"))
> > +		mode = HUGE_WRITEBACK;
> 
> Maybe using `else if (!strcmp(mode_buf, "huge"))` would be slightly
> better here, avoiding a second strcmp() if mode_buf has already
> matched "idle".

I considered "huge|idle" as an option. Anyway, in that case, mode should
"mode |= ". At this moment, yes, lets use "else if" since I don't have
strong opinion to support "idle|huge".

> 
> > +		if ((mode & IDLE_WRITEBACK &&
> > +			  !zram_test_flag(zram, index, ZRAM_IDLE)) &&
> > +		    (mode & HUGE_WRITEBACK &&
> > +			  !zram_test_flag(zram, index, ZRAM_HUGE)))
> > +			goto next;
> 
> Wouldn't writing this as `mode & (IDLE_WRITEBACK | HUGE_WRITEBACK)`
> be a bit easier to read as well as slightly more compact?
> 
> > +	ret = len;
> > +	 __free_page(page);
> > +release_init_lock:
> > +	up_read(&zram->init_lock);
> > +	return ret;
> 
> Hm, I noticed that this function either returns an error or just the passed
> in len on success, and I'm left wondering if there might be other useful
> information which could be passed back to the caller instead. I can't
> immediately think of any such information, though, so it's possible I'm
> just daydreaming :)

It is write syscall semantic of sysfs so not sure it's doable to pass
other value to user.

> 
> -- 
> Cheers,
> Joey Pabalinas