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

[Joey Pabalinas]

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).

> +	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".

> +		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 :)

-- 
Cheers,
Joey Pabalinas

Attachment: signature.asc
Description: PGP signature