On Fri, Jun 30, 2017 at 01:42:56PM -0700, bcache@xxxxxxxxxxxxxxxxxx wrote:
> From: Eric Wheeler <git@xxxxxxxxxxxxxxxxxx>
>
> Add sysfs entries to support to hint for bypass/writeback by the ioprio
> assigned to the bio. If the bio is unassigned, use current's io-context
> ioprio for cache writeback or bypass (configured per-process with
> `ionice`).
>
> Having idle IOs bypass the cache can increase performance elsewhere
> since you probably don't care about their performance. In addition,
> this prevents idle IOs from promoting into (polluting) your cache and
> evicting blocks that are more important elsewhere.
>
> If you really nead the performance at the expense of SSD wearout,
> then configure ioprio_writeback and set your `ionice` appropriately.
>
> For example:
> echo 2,7 > /sys/block/bcache0/bcache/ioprio_bypass
> echo 2,0 > /sys/block/bcache0/bcache/ioprio_writeback
>
> See the documentation commit for details.
I'm really worried about this interface, as it basically uses the
ioprio field for side channel communication - your app must know
which value it wants, and you need to configure bcache to fit
exacltly that scheme.
> + /* If the ioprio already exists on the bio, use that. We assume that
> + * the upper layer properly assigned the calling process's ioprio to
> + * the bio being passed to bcache. Otherwise, use current's ioc. */
Please make this fit the normal kernel comment style.
> + ioprio = bio_prio(bio);
> + if (!ioprio_valid(ioprio)) {
> + ioc = get_task_io_context(current, GFP_NOIO, NUMA_NO_NODE);
> + if (ioc) {
> + if (ioprio_valid(ioc->ioprio))
> + ioprio = ioc->ioprio;
> + put_io_context(ioc);
> + ioc = NULL;
> + }
> + }
While get_task_io_context currently is exported it really should not
be - we should only allocate on when setting the io priority or when
forking.
What this code really wants is the ioprio related lines of code from
blk_init_request_from_bio, which should be factored into a new helper.
> + if (ioprio_valid(ioprio) && ioprio_valid(dc->ioprio_writeback)
> + && ioprio >= dc->ioprio_bypass) {
> + return true;
> + }
Incorrect indentation, this shold be:
if (ioprio_valid(ioprio) && ioprio_valid(dc->ioprio_writeback) &&
ioprio >= dc->ioprio_bypass)
return true;
And there is some more of this in this and the following patches.
Please run them through something like checkpatch.pl
> +
> SHOW(__bch_cached_dev)
> {
> struct cached_dev *dc = container_of(kobj, struct cached_dev,
> @@ -183,6 +186,17 @@ SHOW(__bch_cached_dev)
> return strlen(buf);
> }
>
> + if (attr == &sysfs_ioprio_bypass)
> + return snprintf(buf, PAGE_SIZE-1, "%d,%ld\n",
> + IOPRIO_PRIO_CLASS(dc->ioprio_bypass),
> + IOPRIO_PRIO_DATA(dc->ioprio_bypass));
> +
> + if (attr == &sysfs_ioprio_writeback)
> + return snprintf(buf, PAGE_SIZE-1, "%d,%ld\n",
> + IOPRIO_PRIO_CLASS(dc->ioprio_writeback),
> + IOPRIO_PRIO_DATA(dc->ioprio_writeback));
> +
> +
Please implement separate sysfs show and store function for your new
attributes instead of overloading all of them into a giant mess.
