wip-objecter

wip-objecter

[Sage Weil]

Hey John,

I fixed up the mds osdmap wait stuff and squashed it into wip-objecter.  
That rebased out from underneath your branch; sorry.  Will try to look at 
the other patches shortly!

sage

Re: wip-objecter

[John Spray]

I think that for the calls where we check the epoch and then
conditionally call wait_for_map with the context, we have to change
them to do the wait_for_map call first and check the boolean response.
Otherwise, if the map we have is updated between the read of the epoch
and the call to wait_for_map, it could already be ready and never call
back our context.

My version is here (on top a rebase of wip-objecter to master on
branch wip-objecter-rebase):
https://github.com/ceph/ceph/commit/3cd82464ed6f13ec5b44da303849061648b9e3a1

John

On Sat, Jul 26, 2014 at 6:42 AM, Sage Weil <sweil@redhat.com> wrote:
> Hey John,
>
> I fixed up the mds osdmap wait stuff and squashed it into wip-objecter.
> That rebased out from underneath your branch; sorry.  Will try to look at
> the other patches shortly!
>
> sage

Re: wip-objecter

[Sage Weil]

On Sun, 27 Jul 2014, John Spray wrote:
> I think that for the calls where we check the epoch and then
> conditionally call wait_for_map with the context, we have to change
> them to do the wait_for_map call first and check the boolean response.
> Otherwise, if the map we have is updated between the read of the epoch
> and the call to wait_for_map, it could already be ready and never call
> back our context.
> 
> My version is here (on top a rebase of wip-objecter to master on branch 
> wip-objecter-rebase): 
> https://github.com/ceph/ceph/commit/3cd82464ed6f13ec5b44da303849061648b9e3a1

Okay, I finally got around to looking at this again, and it looks like we 
managed to collide on a few things.  The wip-objecter-rebase branch is 
missing the change in my series, specifically the old commit

 fa8d2d581b3da0fc5176a674623738cd555a3766

is replaced with

 7cb3cf98b09685ebd48451878ded3cbc212e0a12

in the (rebased) wip-objecter.  Also, the missing } was squashed into

 e596ad8e67719f7fb8e96b579c34a438f51c319f

Anyway, I rebased and resolved conflicts and pushed a new 
wip-objecter-rebased which looks like it has the right diff between the 
old and new versions.

Take a look?  The main change is that the set data pool virtual xattrs do 
objecter->wait_for_latest_osdmap() and the flawed MDS::request_osdmap() 
helper is now gone.

sage

Re: wip-objecter

[Sage Weil]

On Mon, 28 Jul 2014, Sage Weil wrote:
> On Sun, 27 Jul 2014, John Spray wrote:
> > I think that for the calls where we check the epoch and then
> > conditionally call wait_for_map with the context, we have to change
> > them to do the wait_for_map call first and check the boolean response.
> > Otherwise, if the map we have is updated between the read of the epoch
> > and the call to wait_for_map, it could already be ready and never call
> > back our context.
> > 
> > My version is here (on top a rebase of wip-objecter to master on branch 
> > wip-objecter-rebase): 
> > https://github.com/ceph/ceph/commit/3cd82464ed6f13ec5b44da303849061648b9e3a1
> 
> Okay, I finally got around to looking at this again, and it looks like we 
> managed to collide on a few things.  The wip-objecter-rebase branch is 
> missing the change in my series, specifically the old commit
> 
>  fa8d2d581b3da0fc5176a674623738cd555a3766
> 
> is replaced with
> 
>  7cb3cf98b09685ebd48451878ded3cbc212e0a12
> 
> in the (rebased) wip-objecter.  Also, the missing } was squashed into
> 
>  e596ad8e67719f7fb8e96b579c34a438f51c319f
> 
> Anyway, I rebased and resolved conflicts and pushed a new 
> wip-objecter-rebased which looks like it has the right diff between the 
> old and new versions.
> 
> Take a look?  The main change is that the set data pool virtual xattrs do 
> objecter->wait_for_latest_osdmap() and the flawed MDS::request_osdmap() 
> helper is now gone.

...and I pushed a couple more patches.  One fixes the wait_for_map callers 
so that they don't leak the completion when we already have the map.  The 
second applies the _IO_ naming thing to completions that have to retake 
the mds_lock.

But that makes me realize that a handful of the MDLog::submit_entry()
callers are passing in completions that take the lock, but there are a 
zillion other callers that aren't.

I wonder if a safer approach is to make a subclass of Context called 
RelockingContext, which takes an mds_lock * in the ctor, and then make all 
of the other completions subclasses of that.  That way we can use type 
checking to ensure that all Context's passed to MDLog::submit_entry() (or 
other places that make sense) are of the right type.  Hopefully there 
aren't any classes that are used in both and locking and non-locking 
context...

Alternatively, we could make a LockingFinisher class that takes the 
specified lock (mds_lock) before calling each Context.  That might be 
simpler?

sage

Re: wip-objecter

[John Spray]

On Mon, Jul 28, 2014 at 10:48 PM, Sage Weil <sweil@redhat.com> wrote:
> Anyway, I rebased and resolved conflicts and pushed a new
> wip-objecter-rebased which looks like it has the right diff between the
> old and new versions.
>
> Take a look?  The main change is that the set data pool virtual xattrs do
> objecter->wait_for_latest_osdmap() and the flawed MDS::request_osdmap()
> helper is now gone.

Yep, looks right.  I was going to do about the same squashup once
things were passing unit tests, but I had missed the leak of the
contexts.

John

Re: wip-objecter

[John Spray]

On Mon, Jul 28, 2014 at 11:17 PM, Sage Weil <sweil@redhat.com> wrote:
> I wonder if a safer approach is to make a subclass of Context called
> RelockingContext, which takes an mds_lock * in the ctor, and then make all
> of the other completions subclasses of that.  That way we can use type
> checking to ensure that all Context's passed to MDLog::submit_entry() (or
> other places that make sense) are of the right type.  Hopefully there
> aren't any classes that are used in both and locking and non-locking
> context...

Greg and I chatted about this sort of thing a bit last week, where the
same sort of mechanism might be used to distinguish completions that
need to go via a finisher vs. ones that are safe to call directly.  I
convinced myself that wasn't going to work last week (because we have
to call completions from the journaler in the right order), but the
general idea of declaratively distinguishing contexts like this is
still appealing.

The following are the interesting attributes of contexts:
  1 Contexts that just don't care how they're called (e.g.
C_OnFinisher can be called any which way)
  2 Contexts that will take mds_lock (if you're holding it, send via
finisher thread)
  3 Contexts that require mds_lock is already held
  4 Contexts that may call into Journaler (you may not call this while
holding Journaler.lock, if you're holding it, send via finisher
thread)
  5 Contexts that may do Objecter I/O (you may not call this from an
objecter callback, if you're in that situation you must go via a
finisher thread)

These are not all exclusive.  Number 5 goes away when we switch to
librados, because it's already going to be going via a finisher thread
before calling back our I/O completions.  For the moment we get the
same effect by using C_OnFinisher with all the C_IO_* contexts.

Number 4 goes away if Journaler sends all its completions via a
finisher, as seems to be the simplest thing to do right now.

> Alternatively, we could make a LockingFinisher class that takes the
> specified lock (mds_lock) before calling each Context.  That might be
> simpler?

Hmm, if we did that then completions might sometimes hop up multiple
finishers if they went e.g. first into a "take the mds lock" finisher
then subsequently via a "the mds lock must be held" finisher (second
one being strictly redundant but it sure would be nice to have the
check in there somehow).

The explicit Context subclasses has lots of appeal to me from the
compile-time assurance we would have that we were using the right kind
of thing in the right kind of place, e.g. when we do add_waiter calls
on an inode's lock we can check at compile time that we're passing a
"mds lock already held" subclass.

John

wip-objecter

[Sage Weil]

Hey Yehuda,

I merged the objecter branch into master and then reassembled the patch 
series so that all of the support bits are separate patches and there is 
just one commit that has all of the actual objecter changes.  See 
wip-objecter.  Hopefully this will make it easier to work with.

IIRC the remaining piece, besides general testing/reivew, is op 
cancellation?  And understanding what is causing Sushma's performance 
discrepancy...

sage