OSD::op_is_discardable condition doubled

[Lakis, Jacek]

Hi ceph-devel!
We're checking OSD::op_is_discardable condition two times: in dispatcher 
(handle_op) and in worker threads 
(do_request->can_discard_request->can_discard_op)

  if (!op->get_connection()->is_connected() &&
  op->get_version().version == 0) {
    return true;
  }

Is there any purpose to do this? If not, which of those should be removed in 
your opinion? If condition in handle_op will remain, OP won't pass to the 
worker threads through the prioritized queue. On the other hand, leaving it in 
do_request will parallelize this check (I started parallelizing in PR 5211). 

Regards,
JJ



Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial 
Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | 
Kapital zakladowy 200.000 PLN.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i 
moze zawierac informacje poufne. W razie przypadkowego otrzymania tej 
wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; 
jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole 
use of the intended recipient(s). If you are not the intended recipient, please 
contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: OSD::op_is_discardable condition doubled

[Sage Weil]

On Tue, 22 Sep 2015, Lakis, Jacek wrote:
> Hi ceph-devel!
> We're checking OSD::op_is_discardable condition two times: in dispatcher 
> (handle_op) and in worker threads 
> (do_request->can_discard_request->can_discard_op)
> 
>   if (!op->get_connection()->is_connected() &&
>   op->get_version().version == 0) {
>     return true;
>   }
> 
> Is there any purpose to do this? If not, which of those should be 
> removed in your opinion? If condition in handle_op will remain, OP won't 
> pass to the worker threads through the prioritized queue. On the other 
> hand, leaving it in do_request will parallelize this check (I started 
> parallelizing in PR 5211).

Is there a reason not to do it twice?  It looks pretty cheap.

But, if we have to choose, I'd leave it in the worker thread.  It is 
unlikely to trigger on newly dispatched messages (that were *just* taken 
off the wire), and the worker thread check is critical to avoid wasting 
work on requeued/delayed messages that no longer have clients waiting on 
them.

sage

Re: OSD::op_is_discardable condition doubled

[Gregory Farnum]

On Tue, Sep 22, 2015 at 5:26 AM, Sage Weil  wrote:
> On Tue, 22 Sep 2015, Lakis, Jacek wrote:
>> Hi ceph-devel!
>> We're checking OSD::op_is_discardable condition two times: in dispatcher 
>> (handle_op) and in worker threads 
>> (do_request->can_discard_request->can_discard_op)
>>
>>   if (!op->get_connection()->is_connected() &&
>>   op->get_version().version == 0) {
>> return true;
>>   }
>>
>> Is there any purpose to do this? If not, which of those should be
>> removed in your opinion? If condition in handle_op will remain, OP won't
>> pass to the worker threads through the prioritized queue. On the other
>> hand, leaving it in do_request will parallelize this check (I started
>> parallelizing in PR 5211).
>
> Is there a reason not to do it twice?  It looks pretty cheap.
>
> But, if we have to choose, I'd leave it in the worker thread.  It is
> unlikely to trigger on newly dispatched messages (that were *just* taken
> off the wire), and the worker thread check is critical to avoid wasting
> work on requeued/delayed messages that no longer have clients waiting on
> them.

While this is definitely the right choice if eliminating one, I think
there was a specific reason to leave it in both. Perhaps just because
it's cheap, but I think maybe the rest of the dispatch work in that
thread relies on it having a connection. (Unless that got changed? We
are a lot more generous about lack of connections than we used to be.)
-Greg
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html