Re: [PATCH V2] mds: fix CDir::_commit_partial() bug

["Yan, Zheng" <zheng.z.yan@intel.com>]

On 11/25/2012 01:51 AM, Sage Weil wrote:
> On Sun, 25 Nov 2012, Yan, Zheng wrote:
>> On 11/25/2012 12:21 AM, Sage Weil wrote:
>>> On Sat, 24 Nov 2012, Yan, Zheng wrote:
>>>> Hi, Sage
>>>>
>>>> I found this fix is still not enough because we don't know if a dentry has been
>>>> successfully deleted from the directory fragment after MDS restart. There are several
>>>> options to fix this, which one do you like?
>>>>
>>>> 1. Make OSD ignore the TMAP_RM commands if it can't the find items.
>>>
>>> This is certainly the simplest.  Since TMAP is effectively deprecated and 
>>> everyone should be using the omap stuff anyway, it gets my vote.
>>>
>>> Speaking of, we should transition the MDS to use the omap operations soon 
>>> as well, since TMAP is horribly inefficient for large directories.  Also, 
>>> the rmkey method in that case does not have this problem.
>>
>> I can do this, it should be easy.
>>
>>>
>>> Let's ignore enoent on TMAP_RM for bobtail, and transition to omap in the 
>>> next release... do you want to test the patch that drops the !key_exists 
>>> check?
>>
>> I will modify the code and test it. but I think it is hiding bugs ;)
>>
>> Another issue of tmap is that we may get different results when comparing two dentry_key_t
>> and their string representation. So the dentries in tmap can be out of order, setkey method
>> may add duplicated dentry and rmkey method may fail to find existing dentry. Since tmap is
>> so buggy, let's switch the MDS to use the omap ASAP.
> 
> Yeah.  I think the question is what is the minimal set of fixes we should 
> do to the TMAP implementation for bobtail before changing to omap.  The 
> dentry_key_t ordering fix you posted the other day and ignoring the rm 
> failures should catch these, right?

Yes, that bug was caught by the !key_exists check. Actually the patch I previously sent
is buggy too, it breaks CDir::lookup. I have a patch that sorts dentries in CDir::_commit_full
and CDir::_commit_partial. It survives over night fsstress test, but change is ugly and
relatively large. I will send it out tomorrow.

Regards
Yan, Zheng
> 
> sage
> 
> 
>>
>> Regards
>> Yan, Zheng
>>
>>
>>>
>>> Thanks!
>>> sage
>>>
>>>  
>>>> 2. Add a new item deletion command for tmap, the new command doesn't make whole tmap
>>>>    update operation fail if it can't the item. When committing directory fragments,
>>>>    use the new command for null dentries that were added by MDS log replay.
>>>>
>>>> 3. When committing a directory fragment, Re-fetch it if it contains dirty+null dentries
>>>>    that were added by MDS log replay. Find already deleted items and mark corresponding
>>>>    dentries as new.
>>>>
>>>> Regards
>>>> Yan, Zheng
>>>>
>>>> On 11/23/2012 12:52 AM, Yan, Zheng wrote:
>>>>> From: "Yan, Zheng" <zheng.z.yan@intel.com>
>>>>>
>>>>> When a null dentry is encountered, CDir::_commit_partial() adds
>>>>> a OSD_TMAP_RM command to delete the dentry. But if the dentry is
>>>>> new, the osd will not find the dentry when handling the command
>>>>> and the tmap update operation will fail totally.
>>>>>
>>>>> This patch also makes sure dentries are properly marked as new
>>>>> when preparing new dentries and exporting dentries.
>>>>>
>>>>> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
>>>>> ---
>>>>>  src/mds/CDentry.h  |  2 ++
>>>>>  src/mds/CDir.cc    | 11 ++++++++---
>>>>>  src/mds/CDir.h     |  2 +-
>>>>>  src/mds/MDCache.cc |  9 ++++++---
>>>>>  src/mds/Server.cc  |  3 +++
>>>>>  5 files changed, 20 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/src/mds/CDentry.h b/src/mds/CDentry.h
>>>>> index 480e562..5755c55 100644
>>>>> --- a/src/mds/CDentry.h
>>>>> +++ b/src/mds/CDentry.h
>>>>> @@ -347,6 +347,8 @@ public:
>>>>>      // twiddle
>>>>>      state = 0;
>>>>>      state_set(CDentry::STATE_AUTH);
>>>>> +    if (nstate & STATE_NEW)
>>>>> +      mark_new();
>>>>>      if (nstate & STATE_DIRTY)
>>>>>        _mark_dirty(ls);
>>>>>      if (!replica_map.empty())
>>>>> diff --git a/src/mds/CDir.cc b/src/mds/CDir.cc
>>>>> index c5220ed..411d864 100644
>>>>> --- a/src/mds/CDir.cc
>>>>> +++ b/src/mds/CDir.cc
>>>>> @@ -1696,7 +1696,7 @@ class C_Dir_Committed : public Context {
>>>>>  public:
>>>>>    C_Dir_Committed(CDir *d, version_t v, version_t lrv) : dir(d), version(v), last_renamed_version(lrv) { }
>>>>>    void finish(int r) {
>>>>> -    dir->_committed(version, last_renamed_version);
>>>>> +    dir->_committed(version, last_renamed_version, r);
>>>>>    }
>>>>>  };
>>>>>  
>>>>> @@ -1802,6 +1802,10 @@ CDir::map_t::iterator CDir::_commit_partial(ObjectOperation& m,
>>>>>        continue;  // skip clean dentries
>>>>>  
>>>>>      if (dn->get_linkage()->is_null()) {
>>>>> +      if (dn->is_new()) {
>>>>> +	dn->mark_clean();
>>>>> +	continue;
>>>>> +      }
>>>>>        dout(10) << " rm " << dn->name << " " << *dn << dendl;
>>>>>        finalbl.append(CEPH_OSD_TMAP_RM);
>>>>>        dn->key().encode(finalbl);
>>>>> @@ -1997,10 +2001,11 @@ void CDir::_commit(version_t want)
>>>>>   *
>>>>>   * @param v version i just committed
>>>>>   */
>>>>> -void CDir::_committed(version_t v, version_t lrv)
>>>>> +void CDir::_committed(version_t v, version_t lrv, int ret)
>>>>>  {
>>>>> -  dout(10) << "_committed v " << v << " (last renamed " << lrv << ") on " << *this << dendl;
>>>>> +  dout(10) << "_committed ret " << ret << " v " << v << " (last renamed " << lrv << ") on " << *this << dendl;
>>>>>    assert(is_auth());
>>>>> +  assert(ret == 0);
>>>>>  
>>>>>    bool stray = inode->is_stray();
>>>>>  
>>>>> diff --git a/src/mds/CDir.h b/src/mds/CDir.h
>>>>> index 2222418..274e38b 100644
>>>>> --- a/src/mds/CDir.h
>>>>> +++ b/src/mds/CDir.h
>>>>> @@ -487,7 +487,7 @@ private:
>>>>>                         unsigned max_write_size=-1,
>>>>>                         map_t::iterator last_committed_dn=map_t::iterator());
>>>>>    void _encode_dentry(CDentry *dn, bufferlist& bl, const set<snapid_t> *snaps);
>>>>> -  void _committed(version_t v, version_t last_renamed_version);
>>>>> +  void _committed(version_t v, version_t last_renamed_version, int ret);
>>>>>    void wait_for_commit(Context *c, version_t v=0);
>>>>>  
>>>>>    // -- dirtyness --
>>>>> diff --git a/src/mds/MDCache.cc b/src/mds/MDCache.cc
>>>>> index f8b1c8f..e69a49f 100644
>>>>> --- a/src/mds/MDCache.cc
>>>>> +++ b/src/mds/MDCache.cc
>>>>> @@ -657,12 +657,15 @@ CDentry *MDCache::get_or_create_stray_dentry(CInode *in)
>>>>>    CDir *straydir = strayi->get_dirfrag(fg);
>>>>>    assert(straydir);
>>>>>    CDentry *straydn = straydir->lookup(straydname);
>>>>> -  if (!straydn) {
>>>>> +
>>>>> +  if (!straydn)
>>>>>      straydn = straydir->add_null_dentry(straydname);
>>>>> -    straydn->mark_new();
>>>>> -  } else 
>>>>> +  else
>>>>>      assert(straydn->get_projected_linkage()->is_null());
>>>>>  
>>>>> +  if (!straydn->is_dirty())
>>>>> +    straydn->mark_new();
>>>>> +
>>>>>    return straydn;
>>>>>  }
>>>>>  
>>>>> diff --git a/src/mds/Server.cc b/src/mds/Server.cc
>>>>> index ec0d5d5..228fede 100644
>>>>> --- a/src/mds/Server.cc
>>>>> +++ b/src/mds/Server.cc
>>>>> @@ -1685,6 +1685,9 @@ CDentry* Server::prepare_null_dentry(MDRequest *mdr, CDir *dir, const string& dn
>>>>>        }
>>>>>      }
>>>>>  
>>>>> +    if (!dn->is_dirty())
>>>>> +      dn->mark_new();
>>>>> +
>>>>>      return dn;
>>>>>    }
>>>>>  
>>>>>
>>>>
>>>>
>>
>>