[PATCH 5/9] libceph: init osd->o_node in create_osd()
It turns out to be harmless but the red-black node o_node in the ceph osd structure is not initialized in create_osd(). Add a call to rb_init_node() initialize it. Signed-off-by: Alex Elder --- net/ceph/osd_client.c |1 + 1 file changed, 1 insertion(+) diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c index 60c74c1..470816c 100644 --- a/net/ceph/osd_client.c +++ b/net/ceph/osd_client.c @@ -642,6 +642,7 @@ static struct ceph_osd *create_osd(struct ceph_osd_client *osdc, int onum) atomic_set(&osd->o_ref, 1); osd->o_osdc = osdc; osd->o_osd = onum; + rb_init_node(&osd->o_node); INIT_LIST_HEAD(&osd->o_requests); INIT_LIST_HEAD(&osd->o_linger_requests); INIT_LIST_HEAD(&osd->o_osd_lru); -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/9] libceph: init osd->o_node in create_osd()
We should drop this one, I think. See upstream commit 4c199a93a2d36b277a9fd209a0f2793f8460a215. When we added the similar call on teh request tree it caused some noise in linux-next and then got removed. sage On Thu, 13 Dec 2012, Alex Elder wrote: > It turns out to be harmless but the red-black node o_node in the > ceph osd structure is not initialized in create_osd(). Add a > call to rb_init_node() initialize it. > > Signed-off-by: Alex Elder > --- > net/ceph/osd_client.c |1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > index 60c74c1..470816c 100644 > --- a/net/ceph/osd_client.c > +++ b/net/ceph/osd_client.c > @@ -642,6 +642,7 @@ static struct ceph_osd *create_osd(struct > ceph_osd_client *osdc, int onum) > atomic_set(&osd->o_ref, 1); > osd->o_osdc = osdc; > osd->o_osd = onum; > + rb_init_node(&osd->o_node); > INIT_LIST_HEAD(&osd->o_requests); > INIT_LIST_HEAD(&osd->o_linger_requests); > INIT_LIST_HEAD(&osd->o_osd_lru); > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/9] libceph: init osd->o_node in create_osd()
On 12/14/2012 07:43 PM, Sage Weil wrote: > We should drop this one, I think. See upstream commit > 4c199a93a2d36b277a9fd209a0f2793f8460a215. When we added the similar call > on teh request tree it caused some noise in linux-next and then got > removed. Well, we need to initialize it. In particular, there is a call to RB_EMPTY_NODE() in __unregister_request() which assumes that if a node is not in the tree, it has been initialized as it is in RB_CLEAR_NODE(). Even if that's not a normal path (and even if no current code path allows that condition), if we're using RB_EMPTY_NODE() we need to be sure any node we'll pass is properly set up or it's broken code. So it's fine with me if we use RB_CLEAR_NODE() instead. Or, we could stop using RB_EMPTY_NODE() (but that's more work to verify). The same goes for the event passed to __remove_event(), but that should be fixed in a separate patch. Please let me know what you think. -Alex > sage > > On Thu, 13 Dec 2012, Alex Elder wrote: > >> It turns out to be harmless but the red-black node o_node in the >> ceph osd structure is not initialized in create_osd(). Add a >> call to rb_init_node() initialize it. >> >> Signed-off-by: Alex Elder >> --- >> net/ceph/osd_client.c |1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c >> index 60c74c1..470816c 100644 >> --- a/net/ceph/osd_client.c >> +++ b/net/ceph/osd_client.c >> @@ -642,6 +642,7 @@ static struct ceph_osd *create_osd(struct >> ceph_osd_client *osdc, int onum) >> atomic_set(&osd->o_ref, 1); >> osd->o_osdc = osdc; >> osd->o_osd = onum; >> +rb_init_node(&osd->o_node); >> INIT_LIST_HEAD(&osd->o_requests); >> INIT_LIST_HEAD(&osd->o_linger_requests); >> INIT_LIST_HEAD(&osd->o_osd_lru); >> -- >> 1.7.9.5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/9] libceph: init osd->o_node in create_osd()
On Mon, 17 Dec 2012, Alex Elder wrote: > On 12/14/2012 07:43 PM, Sage Weil wrote: > > We should drop this one, I think. See upstream commit > > 4c199a93a2d36b277a9fd209a0f2793f8460a215. When we added the similar call > > on teh request tree it caused some noise in linux-next and then got > > removed. > > Well, we need to initialize it. In particular, there is a call > to RB_EMPTY_NODE() in __unregister_request() which assumes that > if a node is not in the tree, it has been initialized as it is > in RB_CLEAR_NODE(). Even if that's not a normal path (and even > if no current code path allows that condition), if we're using > RB_EMPTY_NODE() we need to be sure any node we'll pass is properly > set up or it's broken code. > > So it's fine with me if we use RB_CLEAR_NODE() instead. Or, we > could stop using RB_EMPTY_NODE() (but that's more work to verify). It sounds like RB_CLEAR_NODE is the right path. From the commit message it looks like rb_init_node() is broken and/or going away. Let's just switch to that? And I guess we probably need to do the same in the request tree, too? s > The same goes for the event passed to __remove_event(), but that > should be fixed in a separate patch. > > Please let me know what you think. > > -Alex > > > sage > > > > On Thu, 13 Dec 2012, Alex Elder wrote: > > > >> It turns out to be harmless but the red-black node o_node in the > >> ceph osd structure is not initialized in create_osd(). Add a > >> call to rb_init_node() initialize it. > >> > >> Signed-off-by: Alex Elder > >> --- > >> net/ceph/osd_client.c |1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c > >> index 60c74c1..470816c 100644 > >> --- a/net/ceph/osd_client.c > >> +++ b/net/ceph/osd_client.c > >> @@ -642,6 +642,7 @@ static struct ceph_osd *create_osd(struct > >> ceph_osd_client *osdc, int onum) > >>atomic_set(&osd->o_ref, 1); > >>osd->o_osdc = osdc; > >>osd->o_osd = onum; > >> + rb_init_node(&osd->o_node); > >>INIT_LIST_HEAD(&osd->o_requests); > >>INIT_LIST_HEAD(&osd->o_linger_requests); > >>INIT_LIST_HEAD(&osd->o_osd_lru); > >> -- > >> 1.7.9.5 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > >> the body of a message to majord...@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >> > >> > > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/9] libceph: init osd->o_node in create_osd()
On 12/17/2012 10:45 AM, Sage Weil wrote: > On Mon, 17 Dec 2012, Alex Elder wrote: >> On 12/14/2012 07:43 PM, Sage Weil wrote: >>> We should drop this one, I think. See upstream commit >>> 4c199a93a2d36b277a9fd209a0f2793f8460a215. When we added the similar call >>> on teh request tree it caused some noise in linux-next and then got >>> removed. >> >> Well, we need to initialize it. In particular, there is a call >> to RB_EMPTY_NODE() in __unregister_request() which assumes that >> if a node is not in the tree, it has been initialized as it is >> in RB_CLEAR_NODE(). Even if that's not a normal path (and even >> if no current code path allows that condition), if we're using >> RB_EMPTY_NODE() we need to be sure any node we'll pass is properly >> set up or it's broken code. >> >> So it's fine with me if we use RB_CLEAR_NODE() instead. Or, we >> could stop using RB_EMPTY_NODE() (but that's more work to verify). > > It sounds like RB_CLEAR_NODE is the right path. From the commit message > it looks like rb_init_node() is broken and/or going away. Let's just > switch to that? Until you questioned it I actually hadn't looked deep enough to know there was this good a reason to do the initialization... It was more that I prefer to see things initialized explicitly sort of thing. > And I guess we probably need to do the same in the request tree, too? Yes I already have a patch put together for that. I'll post it shortly, I was waiting to hear back first. -Alex > s > >> The same goes for the event passed to __remove_event(), but that >> should be fixed in a separate patch. >> >> Please let me know what you think. >> >> -Alex >> >>> sage >>> >>> On Thu, 13 Dec 2012, Alex Elder wrote: >>> It turns out to be harmless but the red-black node o_node in the ceph osd structure is not initialized in create_osd(). Add a call to rb_init_node() initialize it. Signed-off-by: Alex Elder --- net/ceph/osd_client.c |1 + 1 file changed, 1 insertion(+) diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c index 60c74c1..470816c 100644 --- a/net/ceph/osd_client.c +++ b/net/ceph/osd_client.c @@ -642,6 +642,7 @@ static struct ceph_osd *create_osd(struct ceph_osd_client *osdc, int onum) atomic_set(&osd->o_ref, 1); osd->o_osdc = osdc; osd->o_osd = onum; + rb_init_node(&osd->o_node); INIT_LIST_HEAD(&osd->o_requests); INIT_LIST_HEAD(&osd->o_linger_requests); INIT_LIST_HEAD(&osd->o_osd_lru); -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
