Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 27 Nov 2018 11:13:27 +0900
From:   Minchan Kim <minchan@kernel.org>
To:     Joey Pabalinas <joeypabalinas@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Subject: Re: [PATCH v2 5/7] zram: support idle/huge page writeback
Message-ID: <20181127021327.GD237537@google.com>
References: <20181126082813.81977-1-minchan@kernel.org>
 <20181126082813.81977-6-minchan@kernel.org>
 <20181126094737.sfudpeoigra2vir7@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181126094737.sfudpeoigra2vir7@gmail.com>
User-Agent: Mutt/1.10.1+60 (6df12dc1) (2018-08-07)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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