All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mds: fix setting/removing xattrs on root
@ 2013-04-15 10:22 Kuan Kai Chiu
  0 siblings, 0 replies; 5+ messages in thread
From: Kuan Kai Chiu @ 2013-04-15 10:22 UTC (permalink / raw)
  To: ceph-devel; +Cc: henry, Kuan Kai Chiu

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.

Kuan Kai Chiu (1):
  mds: fix setting/removing xattrs on root

 src/mds/Server.cc |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] mds: fix setting/removing xattrs on root
  2013-04-16 20:06   ` Gregory Farnum
@ 2013-04-18  7:16     ` Big Chiu
  0 siblings, 0 replies; 5+ messages in thread
From: Big Chiu @ 2013-04-18  7:16 UTC (permalink / raw)
  To: Gregory Farnum; +Cc: ceph-devel, Henry Chang

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@inktank.com> wrote:
> On Mon, Apr 15, 2013 at 3:23 AM, Kuan Kai Chiu <big.chiu@bigtera.com> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] mds: fix setting/removing xattrs on root
  2013-04-15 10:23 ` Kuan Kai Chiu
@ 2013-04-16 20:06   ` Gregory Farnum
  2013-04-18  7:16     ` Big Chiu
  0 siblings, 1 reply; 5+ messages in thread
From: Gregory Farnum @ 2013-04-16 20:06 UTC (permalink / raw)
  To: Kuan Kai Chiu; +Cc: ceph-devel, henry

On Mon, Apr 15, 2013 at 3:23 AM, Kuan Kai Chiu <big.chiu@bigtera.com> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH] mds: fix setting/removing xattrs on root
  2013-04-15 10:23 Kuan Kai Chiu
@ 2013-04-15 10:23 ` Kuan Kai Chiu
  2013-04-16 20:06   ` Gregory Farnum
  0 siblings, 1 reply; 5+ messages in thread
From: Kuan Kai Chiu @ 2013-04-15 10:23 UTC (permalink / raw)
  To: ceph-devel; +Cc: henry, Kuan Kai Chiu

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));
 }
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH] mds: fix setting/removing xattrs on root
@ 2013-04-15 10:23 Kuan Kai Chiu
  2013-04-15 10:23 ` Kuan Kai Chiu
  0 siblings, 1 reply; 5+ messages in thread
From: Kuan Kai Chiu @ 2013-04-15 10:23 UTC (permalink / raw)
  To: ceph-devel; +Cc: henry, Kuan Kai Chiu

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.

Kuan Kai Chiu (1):
  mds: fix setting/removing xattrs on root

 src/mds/Server.cc |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-04-18  7:16 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-04-15 10:22 [PATCH] mds: fix setting/removing xattrs on root Kuan Kai Chiu
2013-04-15 10:23 Kuan Kai Chiu
2013-04-15 10:23 ` Kuan Kai Chiu
2013-04-16 20:06   ` Gregory Farnum
2013-04-18  7:16     ` Big Chiu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.