| Message ID | CAOi1vP-1EPasYZUPoqwh1Ykgq59xEAf1oeGZH5Y8Kpo9ooQMow@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ilya, On 02/13/2017 12:11 PM, Ilya Dryomov wrote: > Hi Artur, > > How about the attached patch? handle_timeout() is going to iterate > over all but the homeless OSD anyway; all it costs us is a couple of > tests, so I don't think a separate work is needed. > > abort_request() is a simple wrapper around complete_request(), making > it safe to call at any time -- replace it with complete_request() for > now if you want to try this out. Using one job for sending keepalive requests and completing stuck requests brings us to the need to check that osd_keepalive_timeout is not larger then osd_request_timeout. So we should not forget to say about it in the documentation. Is it worth creating a correlation between osd_keepalive_timeout and osd_request_timeout?
On Mon, Feb 13, 2017 at 10:54 AM, Artur Molchanov <artur.molchanov@synesis.ru> wrote: > Hi Ilya, > > On 02/13/2017 12:11 PM, Ilya Dryomov wrote: >> >> Hi Artur, >> >> How about the attached patch? handle_timeout() is going to iterate >> over all but the homeless OSD anyway; all it costs us is a couple of >> tests, so I don't think a separate work is needed. >> >> abort_request() is a simple wrapper around complete_request(), making >> it safe to call at any time -- replace it with complete_request() for >> now if you want to try this out. > > > Using one job for sending keepalive requests and completing stuck requests > brings us to the need to check that osd_keepalive_timeout is not larger then > osd_request_timeout. So we should not forget to say about it in the > documentation. I'll make a note to mention that it is osd_keepalive_timeout-precise. > Is it worth creating a correlation between osd_keepalive_timeout and > osd_request_timeout? No, probably not. osd_keepalive_timeout and osd_idle_ttl are similarly related and we don't have any. Thanks, Ilya -- 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
On 02/13/2017 05:15 PM, Ilya Dryomov wrote: > On Mon, Feb 13, 2017 at 10:54 AM, Artur Molchanov > <artur.molchanov@synesis.ru> wrote: >> Hi Ilya, >> >> On 02/13/2017 12:11 PM, Ilya Dryomov wrote: >>> >>> Hi Artur, >>> >>> How about the attached patch? handle_timeout() is going to iterate >>> over all but the homeless OSD anyway; all it costs us is a couple of >>> tests, so I don't think a separate work is needed. >>> >>> abort_request() is a simple wrapper around complete_request(), making >>> it safe to call at any time -- replace it with complete_request() for >>> now if you want to try this out. >> >> >> Using one job for sending keepalive requests and completing stuck requests >> brings us to the need to check that osd_keepalive_timeout is not larger then >> osd_request_timeout. So we should not forget to say about it in the >> documentation. > > I'll make a note to mention that it is osd_keepalive_timeout-precise. > >> Is it worth creating a correlation between osd_keepalive_timeout and >> osd_request_timeout? > > No, probably not. osd_keepalive_timeout and osd_idle_ttl are similarly > related and we don't have any. Your variant of patch works. As I said, using job handle_timeout to send keepalive requests and aborting stuck requests is not the perfect choice, but OK, it works. What should I do to make this patch merged to upstream?
On Mon, Feb 13, 2017 at 4:23 PM, Artur Molchanov <artur.molchanov@synesis.ru> wrote: > On 02/13/2017 05:15 PM, Ilya Dryomov wrote: >> >> On Mon, Feb 13, 2017 at 10:54 AM, Artur Molchanov >> <artur.molchanov@synesis.ru> wrote: >>> >>> Hi Ilya, >>> >>> On 02/13/2017 12:11 PM, Ilya Dryomov wrote: >>>> >>>> >>>> Hi Artur, >>>> >>>> How about the attached patch? handle_timeout() is going to iterate >>>> over all but the homeless OSD anyway; all it costs us is a couple of >>>> tests, so I don't think a separate work is needed. >>>> >>>> abort_request() is a simple wrapper around complete_request(), making >>>> it safe to call at any time -- replace it with complete_request() for >>>> now if you want to try this out. >>> >>> >>> >>> Using one job for sending keepalive requests and completing stuck >>> requests >>> brings us to the need to check that osd_keepalive_timeout is not larger >>> then >>> osd_request_timeout. So we should not forget to say about it in the >>> documentation. >> >> >> I'll make a note to mention that it is osd_keepalive_timeout-precise. >> >>> Is it worth creating a correlation between osd_keepalive_timeout and >>> osd_request_timeout? >> >> >> No, probably not. osd_keepalive_timeout and osd_idle_ttl are similarly >> related and we don't have any. > > > Your variant of patch works. > As I said, using job handle_timeout to send keepalive requests and aborting > stuck requests is not the perfect choice, but OK, it works. > > What should I do to make this patch merged to upstream? Nothing -- I'll apply it for 4.11 once we have abort_request(). Thanks, Ilya -- 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
diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h index 1816c5e26581..88cd5dc8e238 100644 --- a/include/linux/ceph/libceph.h +++ b/include/linux/ceph/libceph.h @@ -48,6 +48,7 @@ struct ceph_options { unsigned long mount_timeout; /* jiffies */ unsigned long osd_idle_ttl; /* jiffies */ unsigned long osd_keepalive_timeout; /* jiffies */ + unsigned long osd_request_timeout; /* jiffies */ /* * any type that can't be simply compared or doesn't need need @@ -68,6 +69,7 @@ struct ceph_options { #define CEPH_MOUNT_TIMEOUT_DEFAULT msecs_to_jiffies(60 * 1000) #define CEPH_OSD_KEEPALIVE_DEFAULT msecs_to_jiffies(5 * 1000) #define CEPH_OSD_IDLE_TTL_DEFAULT msecs_to_jiffies(60 * 1000) +#define CEPH_OSD_REQUEST_TIMEOUT_DEFAULT 0 /* no timeout */ #define CEPH_MONC_HUNT_INTERVAL msecs_to_jiffies(3 * 1000) #define CEPH_MONC_PING_INTERVAL msecs_to_jiffies(10 * 1000) diff --git a/include/linux/ceph/osd_client.h b/include/linux/ceph/osd_client.h index 03a6653d329a..4db031dd748d 100644 --- a/include/linux/ceph/osd_client.h +++ b/include/linux/ceph/osd_client.h @@ -193,6 +193,7 @@ struct ceph_osd_request { /* internal */ unsigned long r_stamp; /* jiffies, send or check time */ + unsigned long r_start_stamp; /* jiffies */ int r_attempts; struct ceph_eversion r_replay_version; /* aka reassert_version */ u32 r_last_force_resend; diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c index 464e88599b9d..108533859a53 100644 --- a/net/ceph/ceph_common.c +++ b/net/ceph/ceph_common.c @@ -230,6 +230,7 @@ enum { Opt_osdkeepalivetimeout, Opt_mount_timeout, Opt_osd_idle_ttl, + Opt_osd_request_timeout, Opt_last_int, /* int args above */ Opt_fsid, @@ -256,6 +257,7 @@ static match_table_t opt_tokens = { {Opt_osdkeepalivetimeout, "osdkeepalive=%d"}, {Opt_mount_timeout, "mount_timeout=%d"}, {Opt_osd_idle_ttl, "osd_idle_ttl=%d"}, + {Opt_osd_request_timeout, "osd_request_timeout=%d"}, /* int args above */ {Opt_fsid, "fsid=%s"}, {Opt_name, "name=%s"}, @@ -361,6 +363,7 @@ ceph_parse_options(char *options, const char *dev_name, opt->osd_keepalive_timeout = CEPH_OSD_KEEPALIVE_DEFAULT; opt->mount_timeout = CEPH_MOUNT_TIMEOUT_DEFAULT; opt->osd_idle_ttl = CEPH_OSD_IDLE_TTL_DEFAULT; + opt->osd_request_timeout = CEPH_OSD_REQUEST_TIMEOUT_DEFAULT; /* get mon ip(s) */ /* ip1[:port1][,ip2[:port2]...] */ @@ -473,6 +476,15 @@ ceph_parse_options(char *options, const char *dev_name, } opt->mount_timeout = msecs_to_jiffies(intval * 1000); break; + case Opt_osd_request_timeout: + /* 0 is "wait forever" (i.e. infinite timeout) */ + if (intval < 0 || intval > INT_MAX / 1000) { + pr_err("osd_request_timeout out of range\n"); + err = -EINVAL; + goto out; + } + opt->osd_request_timeout = msecs_to_jiffies(intval * 1000); + break; case Opt_share: opt->flags &= ~CEPH_OPT_NOSHARE; @@ -557,6 +569,9 @@ int ceph_print_client_options(struct seq_file *m, struct ceph_client *client) if (opt->osd_keepalive_timeout != CEPH_OSD_KEEPALIVE_DEFAULT) seq_printf(m, "osdkeepalivetimeout=%d,", jiffies_to_msecs(opt->osd_keepalive_timeout) / 1000); + if (opt->osd_request_timeout != CEPH_OSD_REQUEST_TIMEOUT_DEFAULT) + seq_printf(m, "osd_request_timeout=%d,", + jiffies_to_msecs(opt->osd_request_timeout) / 1000); /* drop redundant comma */ if (m->count != pos) diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c index b8da4279e745..1d580530c700 100644 --- a/net/ceph/osd_client.c +++ b/net/ceph/osd_client.c @@ -1717,6 +1717,8 @@ static void account_request(struct ceph_osd_request *req) WARN_ON(req->r_unsafe_callback && (req->r_flags & mask) != mask); atomic_inc(&req->r_osdc->num_requests); + + req->r_start_stamp = jiffies; } static void submit_request(struct ceph_osd_request *req, bool wrlocked) @@ -2504,6 +2506,7 @@ static void handle_timeout(struct work_struct *work) container_of(work, struct ceph_osd_client, timeout_work.work); struct ceph_options *opts = osdc->client->options; unsigned long cutoff = jiffies - opts->osd_keepalive_timeout; + unsigned long expiry_cutoff = jiffies - opts->osd_request_timeout; LIST_HEAD(slow_osds); struct rb_node *n, *p; @@ -2519,15 +2522,23 @@ static void handle_timeout(struct work_struct *work) struct ceph_osd *osd = rb_entry(n, struct ceph_osd, o_node); bool found = false; - for (p = rb_first(&osd->o_requests); p; p = rb_next(p)) { + for (p = rb_first(&osd->o_requests); p; ) { struct ceph_osd_request *req = rb_entry(p, struct ceph_osd_request, r_node); + p = rb_next(p); /* abort_request() */ + if (time_before(req->r_stamp, cutoff)) { dout(" req %p tid %llu on osd%d is laggy\n", req, req->r_tid, osd->o_osd); found = true; } + if (opts->osd_request_timeout && + time_before(req->r_start_stamp, expiry_cutoff)) { + pr_err_ratelimited("tid %llu on osd%d timeout\n", + req->r_tid, osd->o_osd); + abort_request(req, -ETIMEDOUT); + } } for (p = rb_first(&osd->o_linger_requests); p; p = rb_next(p)) { struct ceph_osd_linger_request *lreq = @@ -2547,6 +2558,21 @@ static void handle_timeout(struct work_struct *work) list_move_tail(&osd->o_keepalive_item, &slow_osds); } + if (opts->osd_request_timeout) { + for (p = rb_first(&osdc->homeless_osd.o_requests); p; ) { + struct ceph_osd_request *req = + rb_entry(p, struct ceph_osd_request, r_node); + + p = rb_next(p); /* abort_request() */ + + if (time_before(req->r_start_stamp, expiry_cutoff)) { + pr_err_ratelimited("tid %llu on osd%d timeout\n", + req->r_tid, osdc->homeless_osd.o_osd); + abort_request(req, -ETIMEDOUT); + } + } + } + if (atomic_read(&osdc->num_homeless) || !list_empty(&slow_osds)) maybe_request_map(osdc);