Re: [PATCH v2] io-wq: handle hashed writes in chains

Pavel Begunkov
2020-03-22T20:57:15Z

[-- Attachment #1.1: Type: text/plain, Size: 1919 bytes --]

On 19/03/2020 21:56, Jens Axboe wrote:

> if (hash != -1U) {
> + /*
> + * If the local list is non-empty, then we
> + * have work that hashed to the same key.
> + * No need for a lock round-trip, or fiddling
> + * the the free/busy state of the worker, or
> + * clearing the hashed state. Just process the
> + * next one.
> + */
> + if (!work) {
> + work = wq_first_entry(&list,
> + struct io_wq_work,
> + list);
> + if (work) {
> + wq_node_del(&list, &work->list);
> + goto got_work;
> + }
> + }
> +
> spin_lock_irq(&wqe->lock);
> wqe->hash_map &= ~BIT_ULL(hash);
> wqe->flags &= ~IO_WQE_FLAG_STALLED;

Let's have an example, where "->" is a link
req0(hash=0) -> req1(not_hashed)
req2(hash=0)

1. it grabs @req0 (@work=@req0) and @req1 (in the local @list)
2. it do @req0->func(), sets @work=@req1 and goes to the hash updating code (see
above).

3. ```if (!work)``` check fails, and it clears @hash_map, even though there is
one of the same hash in the list. It messes up @hash_map accounting, but
probably even can continue working fine.

4. Next, it goes for the second iteration (work == req1), do ->func().
Then checks @hash, which is -1 for non-hashed req1, and exits leaving req2 in
the @list.

Can miss something by looking at diff only, but there are 2 potential points to
fix.

BTW, yours patch executes all linked requests first and then goes to the next
hashed. Mine do hashed first, and re-enqueue linked requests. The downside in my
version is the extra re-enqueue. And your approach can't do some cases in
parallel, e.g. the following is purely sequential:

req0(hash0) -> ... long link ->
req1(hash0) -> ... long link ->
req2(hash0) -> ... long link ->
req3(hash0) -> ... long link ->

It's not hard to convert, though

--
Pavel Begunkov

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]