Re: [Gluster-devel] Proposal to change locking in data-self-heal

[Pranith Kumar Karampuri]

----- Original Message -----
> From: "Anand Avati" <address@hidden>
> To: "Pranith Kumar Karampuri" <address@hidden>
> Cc: "Jeff Darcy" <address@hidden>, "Anand Avati" <address@hidden>, "Raghavan 
> Pichai" <address@hidden>,
> "Ravishankar Narayanankutty" <address@hidden>, "devel" <address@hidden>
> Sent: Thursday, May 23, 2013 7:02:09 AM
> Subject: Re: [Gluster-devel] Proposal to change locking in data-self-heal
> 
> On Wed, May 22, 2013 at 5:57 AM, Pranith Kumar Karampuri <
> address@hidden> wrote:
> 
> >
> >
> > ----- Original Message -----
> > > From: "Anand Avati" <address@hidden>
> > > To: "Jeff Darcy" <address@hidden>
> > > Cc: "Pranith Kumar Karampuri" <address@hidden>, "Anand Avati" <
> > address@hidden>, "Raghavan Pichai"
> > > <address@hidden>, "Ravishankar Narayanankutty" <address@hidden>,
> > "devel" <address@hidden>
> > > Sent: Wednesday, May 22, 2013 1:19:19 AM
> > > Subject: Re: [Gluster-devel] Proposal to change locking in data-self-heal
> > >
> > > On Tue, May 21, 2013 at 7:05 AM, Jeff Darcy <address@hidden> wrote:
> > >
> > > > On 05/21/2013 09:10 AM, Pranith Kumar Karampuri wrote:
> > > >
> > > >> scenario-1 won't happen because there exists a chance for it to
> > acquire
> > > >> truncate's full file lock after any 128k range sync happens.
> > > >>
> > > >> Scenario-2 won't happen because extra self-heals that are launched on
> > the
> > > >> same file will be blocked in self-heal-domain so the data-path's
> > locks are
> > > >> not affected by this.
> > > >>
> > > >
> > > > This is two changes for two scenarios:
> > > >
> > > > * Change granular-lock order to avoid scenario 1.
> > > >
> > > > * Add a new lock to avoid scenario 2.
> > > >
> > > > I'm totally fine with the second part, but the first part makes me a
> > bit
> > > > uneasy.  As I recall, the "chained" locking (lock second range before
> > > > unlocking the first) was a deliberate choice to avoid windows where
> > > > somebody could jump in with a truncate during self-heal.  It certainly
> > > > wasn't the obvious thing to do, suggesting there was a specific reason.
> > >
> > >
> > > Chained locking really was to avoid the start of a second self-heal while
> > > one is in progress. By giving up the big lock (after holding the small
> > > lock), regional operations can progress but the second self-heal waiting
> > to
> > > hold the big lock is still held off. Truncating isn't really an issue as
> > > long as the truncate transaction locks from new EOF till infinity (which
> > it
> > > is) and self-heal will not race in those regions. Of course, with chained
> > > locking, full-file truncates can starve.
> > >
> > > It's not obvious what use case Pranith is facing where self-heal and
> > > ftruncate() are competing. Is it just an artificial/hand-crafted test
> > case,
> > > or a real-life access pattern?
> >
> > artificial.
> >
> > >
> > >
> > > > Until we recall what that reason was I don't think we can determine
> > > > whether this is a bug or a feature.  If it's a feature, or if we don't
> > > > know, this change is likely to break something instead of fixing it.
> > >
> > >
> > > The sole reason was to prevent the start of a second self-heal. Having
> > > self-heals serialize in a separate domain solves the problem (except if
> > we
> > > are looking at runtime compatibility across multiple versions in this
> > > scenario.. aargh!)
> >
> > So you guys are OK with this proposal if we solve version compatibility
> > issues?
> >
> >
> I guess you can even just ignore the backward compatibility issue in this
> case. Old servers will "safely" block themselves against the big data-lock
> causing starvation just the way things work today. Once all servers are
> upgraded, all self-heals will gracefully block themselves in the new
> domain. There isn't much compatibility handling actually.

   If afr with new locking approach triggers self-heal first then old afr can 
still get
the full-file lock. This is because new locking approach unlocks the range-lock 
without acquiring
next range-lock. So 2 self-heals will be in progress in parallel. This will 
lead to incorrect
extended attributes because of double decrements of changelogs one by the new 
afr self-heal
and the other with old afr self-heal. Let me know if I am missing something.

   Since scenario-1(the one with truncate) is not widely seen, may be we should 
just solve
scenario-2 which can be done by acquiring a full-lock in self-heal domain then 
unlock after
completion of self-heal, without changing locking in data-domain. 

> 
> Avati
>