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:
signature.asc
Description: OpenPGP digital signature
