>>> On 6/21/2012 at 12:04 AM, in message <[email protected]>, Sage Weil <[email protected]> wrote: > On Wed, 20 Jun 2012, Guan Jun He wrote: >> >> >> >>> On 6/19/2012 at 11:33 PM, in message >> <[email protected]>, Sage Weil >> <[email protected]> wrote: >> > [Sorry for not responding earlier!] >> > >> > On Tue, 19 Jun 2012, Guan Jun He wrote: >> >> Hi, >> >> >> >> Do you think this is needed? >> >> The osdmap update need to hold this sem.As in function >> >> void ceph_osdc_handle_map(struct ceph_osd_client *osdc, struct ceph_msg >> > *msg) >> >> and >> >> function static int __map_request(), >> >> etc. >> > >> > I'm skeptical that this is necessary. At the point that we're tearing >> > down the osd_client there shouldn't be anybody else referencing us, or the >> > osdmap. >> > >> > Actually, looking at the code a bit, I think the only kind of race we need >> > to worry about is an incoming message. The correct fix is probably a call >> > to the messenger to drop all incoming traffic before we start to shut >> > everything else down. Even with this, we might still end up with a new >> > osdmap that we leak, but stopping the msgr earlier would fix both issues. >> >> Yes, stopping the msgr earlier is a better fix. >> I will do some test and produce a new patch to this. > > Ok. The trick here is that I think we want to set a flag to discard > incoming messages and then flush the workqueue, do all this other > tear down for osd_client etc., and then tear down the messenger. I don't > think we can tear down the msgr fully before that, though, because > osd_client etc have their fingers in it. We just need to prevent the race > of incoming work during teardown...
Have produced a new patch and sent in another mail, and the title is 'client: prevent the race of incoming work during teardown' thanks, Guanjun > > Thanks for looking at this! > sage > >> thanks, >> Guanjun >> >> >> >> >> > >> > sage >> > >> >> >> >> Thanks a lot for your reply! >> >> >> >> thanks, >> >> Guanjun >> >> >> >> >>> On 6/15/2012 at 04:32 PM, in message >> >> <[email protected]>, Guanjun He >> >> <[email protected]> wrote: >> >> > From: Guanjun He <[email protected]> >> >> > >> >> > I think the osdmap destroy code shoule be protected by map_sem. >> >> > If I was wrong, please correct me, thanks! >> >> > >> >> > Signed-off-by: Guanjun He <[email protected]> >> >> > --- >> >> > net/ceph/osd_client.c | 2 ++ >> >> > 1 files changed, 2 insertions(+), 0 deletions(-) >> >> > >> >> > diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c >> >> > index 1ffebed..718b100 100644 >> >> > --- a/net/ceph/osd_client.c >> >> > +++ b/net/ceph/osd_client.c >> >> > @@ -1872,11 +1872,13 @@ void ceph_osdc_stop(struct ceph_osd_client >> >> > *osdc) >> >> > destroy_workqueue(osdc->notify_wq); >> >> > cancel_delayed_work_sync(&osdc->timeout_work); >> >> > cancel_delayed_work_sync(&osdc->osds_timeout_work); >> >> > + down_write(&osdc->map_sem); >> >> > if (osdc->osdmap) { >> >> > ceph_osdmap_destroy(osdc->osdmap); >> >> > osdc->osdmap = NULL; >> >> > } >> >> > remove_all_osds(osdc); >> >> > + up_write(&osdc->map_sem); >> >> > mempool_destroy(osdc->req_mempool); >> >> > ceph_msgpool_destroy(&osdc->msgpool_op); >> >> > ceph_msgpool_destroy(&osdc->msgpool_op_reply); >> >> >> >> >> >> >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in >> the body of a message to [email protected] >> 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 [email protected] > 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
