I didn't notice the bug. Guessing it was hidden because CephFS had been accessed by other daemons in my test environment. Thank you for the hint! The signed-off patches are resent, also including your fix. On Wed, Apr 17, 2013 at 4:06 AM, Gregory Farnum <greg@xxxxxxxxxxx> wrote: > On Mon, Apr 15, 2013 at 3:23 AM, Kuan Kai Chiu <big.chiu@xxxxxxxxxxx> wrote: >> MDS crashes while journaling dirty root inode in handle_client_setxattr >> and handle_client_removexattr. We should use journal_dirty_inode to >> safely log root inode here. >> --- >> src/mds/Server.cc | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/src/mds/Server.cc b/src/mds/Server.cc >> index 11ab834..1e62dd2 100644 >> --- a/src/mds/Server.cc >> +++ b/src/mds/Server.cc >> @@ -3907,8 +3907,7 @@ void Server::handle_client_setxattr(MDRequest *mdr) >> mdlog->start_entry(le); >> le->metablob.add_client_req(req->get_reqid(), req->get_oldest_client_tid()); >> mdcache->predirty_journal_parents(mdr, &le->metablob, cur, 0, PREDIRTY_PRIMARY, false); >> - mdcache->journal_cow_inode(mdr, &le->metablob, cur); >> - le->metablob.add_primary_dentry(cur->get_projected_parent_dn(), true, cur); >> + mdcache->journal_dirty_inode(mdr, &le->metablob, cur); >> >> journal_and_reply(mdr, cur, 0, le, new C_MDS_inode_update_finish(mds, mdr, cur)); >> } >> @@ -3964,8 +3963,7 @@ void Server::handle_client_removexattr(MDRequest *mdr) >> mdlog->start_entry(le); >> le->metablob.add_client_req(req->get_reqid(), req->get_oldest_client_tid()); >> mdcache->predirty_journal_parents(mdr, &le->metablob, cur, 0, PREDIRTY_PRIMARY, false); >> - mdcache->journal_cow_inode(mdr, &le->metablob, cur); >> - le->metablob.add_primary_dentry(cur->get_projected_parent_dn(), true, cur); >> + mdcache->journal_dirty_inode(mdr, &le->metablob, cur); >> >> journal_and_reply(mdr, cur, 0, le, new C_MDS_inode_update_finish(mds, mdr, cur)); >> } > > This is fine as far as it goes, but we'll need your sign-off for us to > incorporate it into the codebase. Also, have you run any tests with > it? The reason I ask is that when I apply this patch, set an xattr on > the root inode, and then restart the MDS and client, there are no > xattrs on the root any more. I think this should fix that, but there > may be other such issues: > diff --git a/src/mds/events/EMetaBlob.h b/src/mds/events/EMetaBlob.h > index 7065460..439bd78 100644 > --- a/src/mds/events/EMetaBlob.h > +++ b/src/mds/events/EMetaBlob.h > @@ -468,7 +468,7 @@ private: > > if (!pi) pi = in->get_projected_inode(); > if (!pdft) pdft = &in->dirfragtree; > - if (!px) px = &in->xattrs; > + if (!px) px = in->get_projected_xattrs(); > > bufferlist snapbl; > if (psnapbl) > > You've fallen victim to this new setup, incidentally — in the past the > root inode wasn't allowed to get any of these modifications because > it's not quite real in the way the rest of them are. We opened that up > when we made the virtual xattr interface, but we weren't very careful > about it so apparently we missed some side effects. > -Greg > Software Engineer #42 @ http://inktank.com | http://ceph.com -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
