Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1592554 > unrolled thread

[PATCH RFC 00/14] Add the BFQ I/O Scheduler to blk-mq

Back to article view | Back to linux.kernel

Contents

  [PATCH RFC 00/14] Add the BFQ I/O Scheduler to blk-mq Paolo Valente <paolo.valente@linaro.org> - 2017-03-04 17:10 +0100
    Re: [PATCH RFC 01/14] block, bfq: introduce the BFQ-v0 I/O scheduler  as an extra scheduler Jens Axboe <axboe@kernel.dk> - 2017-03-05 16:30 +0100
      Re: [PATCH RFC 01/14] block, bfq: introduce the BFQ-v0 I/O scheduler as an extra scheduler Paolo Valente <paolo.valente@linaro.org> - 2017-03-05 17:10 +0100
        Re: [PATCH RFC 01/14] block, bfq: introduce the BFQ-v0 I/O scheduler  as an extra scheduler Jens Axboe <axboe@kernel.dk> - 2017-03-06 23:30 +0100
    Re: [PATCH RFC 00/14] Add the BFQ I/O Scheduler to blk-mq Markus Trippelsdorf <markus@trippelsdorf.de> - 2017-03-06 08:50 +0100
    Re: [PATCH RFC 10/14] block, bfq: add Early Queue Merge (EQM) Jens Axboe <axboe@kernel.dk> - 2017-03-07 20:40 +0100
    Re: [PATCH RFC 01/14] block, bfq: introduce the BFQ-v0 I/O scheduler  as an extra scheduler Jens Axboe <axboe@kernel.dk> - 2017-03-08 00:30 +0100

#1592554 — [PATCH RFC 00/14] Add the BFQ I/O Scheduler to blk-mq

Hi,
at last, here is my first patch series meant for merging. It adds BFQ
to blk-mq. Don't worry, in this message I won't bore you again with
the wonderful properties of BFQ :)

A quick update on the status of the code: thanks to Murphy's laws, in
the last handful of days,
1) A kind of rare failure, reported only once by a user, several
months ago, has been reported again. Fortunately, the bug reporter
provided an oops this time.
2) An unexpected bandwidth unbalance between greedy reads and writes
has been noted.
I have chosen however to submit these patches before attacking these
new problems.

Let me also recall the limitations of the current version of BFQ. On
average CPUs, it can handle, without loss of throughput or fairness
guarantees, devices performing at most ~30K IOPS; at most ~50 KIOPS on
faster CPUs. Just to put this into context, these are about the same
limits as CFQ in blk. A second intrinsic problem is the current need
for device idling when differentiated bandwidth distribution must be
guaranteed.  Over the last months, I have seen that, fortunately,
there is room for significant improvements with both these
limitations. I plan to work on these improvements after we are done
(and if everything goes well) with merging BFQ.

Finally, a few details on the patchset.

The first two patches introduce BFQ-v0, which is more or less the
first version of BFQ submitted a few years ago [1].  The remaining
patches turn progressively BFQ-v0 into BFQ-v8r8, the current version
of BFQ.

Some patch generates WARNINGS with checkpatch.pl, but these WARNINGS
seem to be either unavoidable for the involved pieces of code (which
the patch just extends), or false positives.

Thanks,
Paolo

[1] https://lkml.org/lkml/2008/4/1/234

Arianna Avanzini (4):
  block, bfq: add full hierarchical scheduling and cgroups support
  block, bfq: add Early Queue Merge (EQM)
  block, bfq: reduce idling only in symmetric scenarios
  block, bfq: handle bursts of queue activations

Paolo Valente (10):
  block, bfq: introduce the BFQ-v0 I/O scheduler as an extra scheduler
  block, bfq: improve throughput boosting
  block, bfq: modify the peak-rate estimator
  block, bfq: add more fairness with writes and slow processes
  block, bfq: improve responsiveness
  block, bfq: reduce I/O latency for soft real-time applications
  block, bfq: preserve a low latency also with NCQ-capable drives
  block, bfq: reduce latency during request-pool saturation
  block, bfq: boost the throughput on NCQ-capable flash-based devices
  block, bfq: boost the throughput with random I/O on NCQ-capable HDDs

 Documentation/block/00-INDEX        |    2 +
 Documentation/block/bfq-iosched.txt |  530 +++
 block/Kconfig.iosched               |   21 +
 block/Makefile                      |    1 +
 block/bfq-iosched.c                 | 8751 +++++++++++++++++++++++++++++++++++
 block/elevator.c                    |   16 +-
 include/linux/blkdev.h              |    2 +-
 7 files changed, 9317 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/block/bfq-iosched.txt
 create mode 100644 block/bfq-iosched.c

--
2.10.0

[toc] | [next] | [standalone]

#1592781 — Re: [PATCH RFC 01/14] block, bfq: introduce the BFQ-v0 I/O scheduler as an extra scheduler

On 03/04/2017 09:01 AM, Paolo Valente wrote:
> We tag as v0 the version of BFQ containing only BFQ's engine plus
> hierarchical support. BFQ's engine is introduced by this commit, while
> hierarchical support is added by next commit. We use the v0 tag to
> distinguish this minimal version of BFQ from the versions containing
> also the features and the improvements added by next commits. BFQ-v0
> coincides with the version of BFQ submitted a few years ago [1], apart
> from the introduction of preemption, described below.
> 
> BFQ is a proportional-share I/O scheduler, whose general structure,
> plus a lot of code, are borrowed from CFQ.

I'll take a closer look at this in the coming week. But one quick
comment - don't default to BFQ. Both because it might not be fully
stable yet, and also because the performance limitation of it is
quite severe. Whereas deadline doesn't really hurt single queue
flash at all, BFQ will.

Generally, I think that sort of logic should go into a udev rule. If
a device is rotational it should default to BFQ once the dust has
settled.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1592786 — Re: [PATCH RFC 01/14] block, bfq: introduce the BFQ-v0 I/O scheduler as an extra scheduler

> Il giorno 05 mar 2017, alle ore 16:16, Jens Axboe <axboe@kernel.dk> ha scritto:
> 
> On 03/04/2017 09:01 AM, Paolo Valente wrote:
>> We tag as v0 the version of BFQ containing only BFQ's engine plus
>> hierarchical support. BFQ's engine is introduced by this commit, while
>> hierarchical support is added by next commit. We use the v0 tag to
>> distinguish this minimal version of BFQ from the versions containing
>> also the features and the improvements added by next commits. BFQ-v0
>> coincides with the version of BFQ submitted a few years ago [1], apart
>> from the introduction of preemption, described below.
>> 
>> BFQ is a proportional-share I/O scheduler, whose general structure,
>> plus a lot of code, are borrowed from CFQ.
> 
> I'll take a closer look at this in the coming week.

ok

> But one quick
> comment - don't default to BFQ. Both because it might not be fully
> stable yet, and also because the performance limitation of it is
> quite severe. Whereas deadline doesn't really hurt single queue
> flash at all, BFQ will.
> 

Ok, sorry.  I was doubtful on what to do, but, to not bother you on
every details, I went for setting it as default, because I thought
people would have preferred to test it, even from boot, in this
preliminary stage.  I reset elevator.c in the submission, unless you
want me to do it even before receiving your and others' reviews.

> Generally, I think that sort of logic should go into a udev rule. If
> a device is rotational it should default to BFQ once the dust has
> settled.
> 

ok

Looking forward for your feedback,
Paolo

> -- 
> Jens Axboe
> 

[toc] | [prev] | [next] | [standalone]

#1593771 — Re: [PATCH RFC 01/14] block, bfq: introduce the BFQ-v0 I/O scheduler as an extra scheduler

On 03/05/2017 09:02 AM, Paolo Valente wrote:
> 
>> Il giorno 05 mar 2017, alle ore 16:16, Jens Axboe <axboe@kernel.dk> ha scritto:
>>
>> On 03/04/2017 09:01 AM, Paolo Valente wrote:
>>> We tag as v0 the version of BFQ containing only BFQ's engine plus
>>> hierarchical support. BFQ's engine is introduced by this commit, while
>>> hierarchical support is added by next commit. We use the v0 tag to
>>> distinguish this minimal version of BFQ from the versions containing
>>> also the features and the improvements added by next commits. BFQ-v0
>>> coincides with the version of BFQ submitted a few years ago [1], apart
>>> from the introduction of preemption, described below.
>>>
>>> BFQ is a proportional-share I/O scheduler, whose general structure,
>>> plus a lot of code, are borrowed from CFQ.
>>
>> I'll take a closer look at this in the coming week.
> 
> ok
> 
>> But one quick
>> comment - don't default to BFQ. Both because it might not be fully
>> stable yet, and also because the performance limitation of it is
>> quite severe. Whereas deadline doesn't really hurt single queue
>> flash at all, BFQ will.
>>
> 
> Ok, sorry.  I was doubtful on what to do, but, to not bother you on
> every details, I went for setting it as default, because I thought
> people would have preferred to test it, even from boot, in this
> preliminary stage.  I reset elevator.c in the submission, unless you
> want me to do it even before receiving your and others' reviews.

I don't think it's stable enough for that yet, it's seen very little
testing outside of your own testing. Given that, it's much better that
people opt in to testing BFQ, so they at least know they can expect
crashes.

Speaking of testing, I ran into this bug:

[ 9469.621413] general protection fault: 0000 [#1] PREEMPT SMP
[ 9469.627872] Modules linked in: loop dm_mod xfs libcrc32c bfq_iosched x86_pkg_temp_thermal btrfe
[ 9469.648196] CPU: 0 PID: 2114 Comm: kworker/0:1H Tainted: G        W       4.11.0-rc1+ #249
[ 9469.657873] Hardware name: Dell Inc. PowerEdge T630/0NT78X, BIOS 2.3.4 11/09/2016
[ 9469.666742] Workqueue: xfs-log/nvme4n1p1 xfs_buf_ioend_work [xfs]
[ 9469.674213] task: ffff881fe97646c0 task.stack: ffff881ff13d0000
[ 9469.681053] RIP: 0010:__bfq_bfqq_expire+0xb3/0x110 [bfq_iosched]
[ 9469.687991] RSP: 0018:ffff881fff603dd8 EFLAGS: 00010082
[ 9469.694052] RAX: 6b6b6b6b6b6b6b6b RBX: ffff883fe8e0eb58 RCX: 0000000000010004
[ 9469.702251] RDX: 0000000000010004 RSI: 0000000000000000 RDI: 00000000ffffffff
[ 9469.710456] RBP: ffff881fff603de8 R08: 0000000000000000 R09: 0000000000000001
[ 9469.718659] R10: 0000000000000001 R11: 0000000000000000 R12: ffff883fe8dbf4e8
[ 9469.726863] R13: 0000000000000000 R14: 0000000000000001 R15: 0000000000007904
[ 9469.735063] FS:  0000000000000000(0000) GS:ffff881fff600000(0000) knlGS:0000000000000000
[ 9469.744539] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 9469.751189] CR2: 00000000020c0018 CR3: 0000001fec1db000 CR4: 00000000003406f0
[ 9469.759392] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 9469.767596] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 9469.775794] Call Trace:
[ 9469.778748]  <IRQ>
[ 9469.781222]  bfq_bfqq_expire+0x104/0x2f0 [bfq_iosched]
[ 9469.787193]  ? bfq_idle_slice_timer+0x2a/0xc0 [bfq_iosched]
[ 9469.793650]  bfq_idle_slice_timer+0x7c/0xc0 [bfq_iosched]
[ 9469.799914]  __hrtimer_run_queues+0xd9/0x500
[ 9469.804911]  ? bfq_rq_enqueued+0x340/0x340 [bfq_iosched]
[ 9469.811072]  hrtimer_interrupt+0xb0/0x200
[ 9469.815781]  local_apic_timer_interrupt+0x31/0x50
[ 9469.821264]  smp_apic_timer_interrupt+0x33/0x50
[ 9469.826555]  apic_timer_interrupt+0x90/0xa0

just running the xfstest suite. It's test generic/299. Have you done
full runs of xfstest? I'd greatly recommend that for shaking out bugs.
Run a full loop with xfs, one with btrfs, and one with ext4 for better
confidence in the stability of the code.

(gdb) l *__bfq_bfqq_expire+0xb3
0x5983 is in __bfq_bfqq_expire (block/bfq-iosched.c:2664).
2659		 * been properly deactivated or requeued, so we can safely
2660		 * execute the final step: reset in_service_entity along the
2661		 * path from entity to the root.
2662		 */
2663		for_each_entity(entity)
2664			entity->sched_data->in_service_entity = NULL;
2665	}
2666	
2667	static void bfq_deactivate_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq,
2668					bool ins_into_idle_tree, bool expiration)

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1592975

On 2017.03.04 at 17:01 +0100, Paolo Valente wrote:
> Hi,
> at last, here is my first patch series meant for merging. It adds BFQ
> to blk-mq. Don't worry, in this message I won't bore you again with
> the wonderful properties of BFQ :)

I gave BFQ a quick try. Unfortunately it hangs when I try to delete
btrfs snapshots:

root       124  0.0  0.0      0     0 ?        D    07:19   0:03 [btrfs-cleaner]  
root       125  0.0  0.0      0     0 ?        D    07:19   0:00 [btrfs-transacti]

[ 4372.880116] sysrq: SysRq : Show Blocked State
[ 4372.880125]   task                        PC stack   pid father
[ 4372.880148] btrfs-cleaner   D    0   124      2 0x00000000
[ 4372.880156] Call Trace:
[ 4372.880166]  ? __schedule+0x160/0x7c0
[ 4372.880174]  ? io_schedule+0x64/0xe0
[ 4372.880179]  ? wait_on_page_bit+0x7a/0x100
[ 4372.880183]  ? devm_memunmap+0x40/0x40
[ 4372.880189]  ? read_extent_buffer_pages+0x25c/0x2c0
[ 4372.880195]  ? run_one_async_done+0xc0/0xc0
[ 4372.880200]  ? btree_read_extent_buffer_pages+0x60/0x2e0
[ 4372.880206]  ? read_tree_block+0x2c/0x60
[ 4372.880211]  ? read_block_for_search.isra.38+0xec/0x3a0
[ 4372.880217]  ? btrfs_search_slot+0x214/0xbc0
[ 4372.880221]  ? lookup_inline_extent_backref+0xfb/0x8c0
[ 4372.880225]  ? __btrfs_free_extent.isra.74+0xe9/0xdc0
[ 4372.880231]  ? btrfs_merge_delayed_refs+0x57/0x6e0
[ 4372.880235]  ? __btrfs_run_delayed_refs+0x60d/0x1340
[ 4372.880239]  ? btrfs_run_delayed_refs+0x64/0x280
[ 4372.880243]  ? btrfs_should_end_transaction+0x3b/0xa0
[ 4372.880247]  ? btrfs_drop_snapshot+0x3b2/0x800
[ 4372.880251]  ? __schedule+0x168/0x7c0
[ 4372.880254]  ? btrfs_clean_one_deleted_snapshot+0xa4/0xe0
[ 4372.880259]  ? cleaner_kthread+0x13a/0x180
[ 4372.880264]  ? btree_invalidatepage+0xc0/0xc0
[ 4372.880268]  ? kthread+0x144/0x180
[ 4372.880272]  ? kthread_flush_work_fn+0x20/0x20
[ 4372.880277]  ? ret_from_fork+0x23/0x30
[ 4372.880280] btrfs-transacti D    0   125      2 0x00000000
[ 4372.880285] Call Trace:
[ 4372.880290]  ? __schedule+0x160/0x7c0
[ 4372.880295]  ? io_schedule+0x64/0xe0
[ 4372.880300]  ? wait_on_page_bit_common.constprop.57+0x160/0x180
[ 4372.880303]  ? devm_memunmap+0x40/0x40
[ 4372.880307]  ? __filemap_fdatawait_range+0xd3/0x140
[ 4372.880311]  ? clear_state_bit.constprop.82+0xf7/0x180
[ 4372.880315]  ? __clear_extent_bit.constprop.79+0x138/0x3c0
[ 4372.880319]  ? filemap_fdatawait_range+0x9/0x60
[ 4372.880323]  ? __btrfs_wait_marked_extents.isra.18+0xc1/0x100
[ 4372.880327]  ? btrfs_write_and_wait_marked_extents.constprop.23+0x49/0x80
[ 4372.880331]  ? btrfs_commit_transaction+0x8e1/0xb00
[ 4372.880334]  ? join_transaction.constprop.24+0x10/0xa0
[ 4372.880340]  ? wake_bit_function+0x60/0x60
[ 4372.880345]  ? transaction_kthread+0x185/0x1a0
[ 4372.880350]  ? btrfs_cleanup_transaction+0x500/0x500
[ 4372.880354]  ? kthread+0x144/0x180
[ 4372.880358]  ? kthread_flush_work_fn+0x20/0x20
[ 4372.880362]  ? ret_from_fork+0x23/0x30
[ 4372.880367] ntpd            D    0   175      1 0x00000004
[ 4372.880372] Call Trace:
[ 4372.880375]  ? __schedule+0x160/0x7c0
[ 4372.880379]  ? schedule_preempt_disabled+0x2d/0x80
[ 4372.880383]  ? __mutex_lock.isra.5+0x17b/0x4c0
[ 4372.880386]  ? wait_current_trans+0x15/0xc0
[ 4372.880391]  ? btrfs_free_path+0xe/0x20
[ 4372.880395]  ? btrfs_pin_log_trans+0x14/0x40
[ 4372.880400]  ? btrfs_rename2+0x28e/0x19c0
[ 4372.880404]  ? path_init+0x187/0x3e0
[ 4372.880407]  ? unlazy_walk+0x4b/0x100
[ 4372.880410]  ? terminate_walk+0x8d/0x100
[ 4372.880414]  ? filename_parentat+0x1e9/0x2c0
[ 4372.880420]  ? __kmalloc_track_caller+0xc4/0x100
[ 4372.880424]  ? vfs_rename+0x33f/0x7e0
[ 4372.880428]  ? SYSC_renameat2+0x53c/0x680
[ 4372.880433]  ? entry_SYSCALL_64_fastpath+0x13/0x94
[ 4372.880437] fcron           D    0   178      1 0x00000000
[ 4372.880441] Call Trace:
[ 4372.880445]  ? __schedule+0x160/0x7c0
[ 4372.880448]  ? schedule_preempt_disabled+0x2d/0x80
[ 4372.880452]  ? __mutex_lock.isra.5+0x17b/0x4c0
[ 4372.880458]  ? pagevec_lookup_tag+0x18/0x20
[ 4372.880462]  ? btrfs_log_dentry_safe+0x4cd/0xac0
[ 4372.880466]  ? btrfs_start_transaction+0x249/0x460
[ 4372.880470]  ? btrfs_sync_file+0x288/0x3c0
[ 4372.880475]  ? btrfs_file_write_iter+0x3a9/0x4e0
[ 4372.880479]  ? vfs_write+0x26c/0x2c0
[ 4372.880483]  ? SyS_write+0x3d/0xa0
[ 4372.880486]  ? SyS_fchown+0x7b/0xa0
[ 4372.880491]  ? entry_SYSCALL_64_fastpath+0x13/0x94
[ 4372.880508] kworker/u8:8    D    0   759      2 0x00000000
[ 4372.880518] Workqueue: btrfs-submit btrfs_submit_helper
[ 4372.880520] Call Trace:
[ 4372.880524]  ? __schedule+0x160/0x7c0
[ 4372.880529]  ? io_schedule+0x64/0xe0
[ 4372.880534]  ? blk_mq_get_tag+0x212/0x320
[ 4372.880538]  ? wake_bit_function+0x60/0x60
[ 4372.880544]  ? __blk_mq_alloc_request+0x11/0x1c0
[ 4372.880548]  ? blk_mq_sched_get_request+0x17e/0x220
[ 4372.880553]  ? blk_sq_make_request+0xd3/0x4c0
[ 4372.880557]  ? blk_mq_sched_dispatch_requests+0x104/0x160
[ 4372.880561]  ? generic_make_request+0xc3/0x2e0
[ 4372.880564]  ? submit_bio+0x58/0x100
[ 4372.880569]  ? run_scheduled_bios+0x1a6/0x500
[ 4372.880574]  ? btrfs_worker_helper+0x129/0x1c0
[ 4372.880580]  ? process_one_work+0x1bc/0x400
[ 4372.880585]  ? worker_thread+0x42/0x540
[ 4372.880588]  ? __schedule+0x168/0x7c0
[ 4372.880592]  ? process_one_work+0x400/0x400
[ 4372.880596]  ? kthread+0x144/0x180
[ 4372.880600]  ? kthread_flush_work_fn+0x20/0x20
[ 4372.880605]  ? ret_from_fork+0x23/0x30

I could get it going again by running:
 echo "mq-deadline" > /sys/block/sdb/queue/scheduler

--
Markus

[toc] | [prev] | [next] | [standalone]

#1594580 — Re: [PATCH RFC 10/14] block, bfq: add Early Queue Merge (EQM)

On 03/04/2017 09:01 AM, Paolo Valente wrote:
> @@ -560,6 +600,15 @@ struct bfq_data {
>  	struct bfq_io_cq *bio_bic;
>  	/* bfqq associated with the task issuing current bio for merging */
>  	struct bfq_queue *bio_bfqq;
> +
> +	/*
> +	 * io context to put right after bfqd->lock is released. This
> +	 * filed is used to perform put_io_context, when needed, to
> +	 * after the scheduler lock has been released, and thus
> +	 * prevent an ioc->lock from being possibly taken while the
> +	 * scheduler lock is being held.
> +	 */
> +	struct io_context *ioc_to_put;
>  };

The logic around this is nasty, effectively you end up having locking
around sections of code instea of structures, which is never a good
idea.

The helper functions for unlocking and dropping the ioc add to the mess
as well.

Can't we simply pass back a pointer to an ioc to free? That should be
possible, given that we must have grabbed the bfqd lock ourselves
further up in the call chain. So we _know_ that we'll drop it later on.
If that wasn't the case, the existing logic wouldn't work.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1594719 — Re: [PATCH RFC 01/14] block, bfq: introduce the BFQ-v0 I/O scheduler as an extra scheduler

> +/**
> + * bfq_entity_of - get an entity from a node.
> + * @node: the node field of the entity.
> + *
> + * Convert a node pointer to the relative entity.  This is used only
> + * to simplify the logic of some functions and not as the generic
> + * conversion mechanism because, e.g., in the tree walking functions,
> + * the check for a %NULL value would be redundant.
> + */
> +static struct bfq_entity *bfq_entity_of(struct rb_node *node)
> +{
> +	struct bfq_entity *entity = NULL;
> +
> +	if (node)
> +		entity = rb_entry(node, struct bfq_entity, rb_node);
> +
> +	return entity;
> +}

Get rid of pointless wrappers like this, just use rb_entry() in the
caller. It's harmful to the readability of the code to have to lookup
things like this, if it's a list_entry or rb_entry() in the caller you
know exactly what is going on immediately.

-- 
Jens Axboe

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web