review request : ReplicatedPG::AccessMode::wake removal

review request : ReplicatedPG::AccessMode::wake removal

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

Hi Sam,

IIRC, a few weeks ago you told me that 

https://github.com/ceph/ceph/blob/962b64a83037ff79855c5261325de0cd1541f582/src/osd/ReplicatedPG.cc#L4626

  if (mode.wake) {
    requeue_ops(mode.waiting);
    for (list<Cond*>::iterator p = mode.waiting_cond.begin(); p != mode.waiting_cond.end(); ++p)
      (*p)->Signal();
    mode.wake = false;
  } 

was not used and could be discarded. In the context of https://github.com/ceph/ceph/pull/414 I originally kept calling it wherever put_object_context() was called, just to make sure no opportunity to signal the waiters was missed. 

Since that could lead to very subtle bugs if I misunderstood what you meant, I would very much appreciate a review of this specific commit. 

https://github.com/dachary/ceph/commit/014997d55aa8d614fe38a1f86faa246d3c13e21b

I've not hit anything suspicious yet while running teuthology. But that's really the only part of this patch series that worries me.

Cheers

-- 
Loïc Dachary, Artisan Logiciel Libre
All that is necessary for the triumph of evil is that good people do nothing.


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

Re: review request : ReplicatedPG::AccessMode::wake removal

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 1373 bytes --]

I removed the code in a minimal way instead of removing the data member itself, as you suggested.

https://github.com/dachary/ceph/commit/2f73eea0b8d7d4633d5887ba725d258ce81f8ac0

On 14/08/2013 12:17, Loic Dachary wrote:
> Hi Sam,
> 
> IIRC, a few weeks ago you told me that 
> 
> https://github.com/ceph/ceph/blob/962b64a83037ff79855c5261325de0cd1541f582/src/osd/ReplicatedPG.cc#L4626
> 
>   if (mode.wake) {
>     requeue_ops(mode.waiting);
>     for (list<Cond*>::iterator p = mode.waiting_cond.begin(); p != mode.waiting_cond.end(); ++p)
>       (*p)->Signal();
>     mode.wake = false;
>   } 
> 
> was not used and could be discarded. In the context of https://github.com/ceph/ceph/pull/414 I originally kept calling it wherever put_object_context() was called, just to make sure no opportunity to signal the waiters was missed. 
> 
> Since that could lead to very subtle bugs if I misunderstood what you meant, I would very much appreciate a review of this specific commit. 
> 
> https://github.com/dachary/ceph/commit/014997d55aa8d614fe38a1f86faa246d3c13e21b
> 
> I've not hit anything suspicious yet while running teuthology. But that's really the only part of this patch series that worries me.
> 
> Cheers
> 

-- 
Loïc Dachary, Artisan Logiciel Libre
All that is necessary for the triumph of evil is that good people do nothing.


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