[PATCH 0/5] libceph: timeout handling fixes

[PATCH 0/5] libceph: timeout handling fixes

[Ilya Dryomov]

Hello,

With libceph timeouts becoming more and more of a user-exposed thing
rather than a debugging thing it's time we harden the supporting code.
The issues addressed in this series are described in 2/5.

5/5 is just rebase of an already posted and reviewed patch.

Zheng, please have a look at fs/ceph parts.

Thanks,

                Ilya


Ilya Dryomov (5):
  libceph: nuke time_sub()
  libceph: store timeouts in jiffies, verify user input
  libceph: a couple tweaks for wait loops
  ceph: simplify two mount_timeout sites
  rbd: timeout watch teardown on unmap with mount_timeout

 drivers/block/rbd.c          | 43 ++++++++++++++++++++++++++++-----------
 fs/ceph/dir.c                | 16 +++++----------
 fs/ceph/mds_client.c         | 26 +++++++++++++-----------
 fs/ceph/mds_client.h         |  2 +-
 fs/ceph/super.c              |  2 +-
 include/linux/ceph/libceph.h | 20 ++++++++----------
 net/ceph/ceph_common.c       | 48 +++++++++++++++++++++++++++++++-------------
 net/ceph/mon_client.c        | 13 +++++++++---
 net/ceph/osd_client.c        | 15 +++++++-------
 9 files changed, 111 insertions(+), 74 deletions(-)

-- 
1.9.3

[PATCH 1/5] libceph: nuke time_sub()

[Ilya Dryomov]

Unused since ceph got merged into mainline I guess.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 include/linux/ceph/libceph.h | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
index 30f92cefaa72..85ae9a889a3f 100644
--- a/include/linux/ceph/libceph.h
+++ b/include/linux/ceph/libceph.h
@@ -93,15 +93,6 @@ enum {
 	CEPH_MOUNT_SHUTDOWN,
 };
 
-/*
- * subtract jiffies
- */
-static inline unsigned long time_sub(unsigned long a, unsigned long b)
-{
-	BUG_ON(time_after(b, a));
-	return (long)a - (long)b;
-}
-
 struct ceph_mds_client;
 
 /*
-- 
1.9.3

Re: [PATCH 1/5] libceph: nuke time_sub()

[Alex Elder]

On 05/21/2015 07:35 AM, Ilya Dryomov wrote:
> Unused since ceph got merged into mainline I guess.
> 
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>

Looks good.

Reviewed-by: Alex Elder <elder@linaro.org>

> ---
>  include/linux/ceph/libceph.h | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
> index 30f92cefaa72..85ae9a889a3f 100644
> --- a/include/linux/ceph/libceph.h
> +++ b/include/linux/ceph/libceph.h
> @@ -93,15 +93,6 @@ enum {
>  	CEPH_MOUNT_SHUTDOWN,
>  };
>  
> -/*
> - * subtract jiffies
> - */
> -static inline unsigned long time_sub(unsigned long a, unsigned long b)
> -{
> -	BUG_ON(time_after(b, a));
> -	return (long)a - (long)b;
> -}
> -
>  struct ceph_mds_client;
>  
>  /*
> 

[PATCH 2/5] libceph: store timeouts in jiffies, verify user input

[Ilya Dryomov]

There are currently three libceph-level timeouts that the user can
specify on mount: mount_timeout, osd_idle_ttl and osdkeepalive.  All of
these are in seconds and no checking is done on user input: negative
values are accepted, we multiply them all by HZ which may or may not
overflow, arbitrarily large jiffies then get added together, etc.

There is also a bug in the way mount_timeout=0 is handled.  It's
supposed to mean "infinite timeout", but that's not how wait.h APIs
treat it and so __ceph_open_session() for example will busy loop
without much chance of being interrupted if none of ceph-mons are
there.

Fix all this by verifying user input, storing timeouts capped by
msecs_to_jiffies() in jiffies and using the new ceph_timeout_jiffies()
helper for all user-specified waits to handle infinite timeouts
correctly.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 drivers/block/rbd.c          |  5 +++--
 fs/ceph/dir.c                |  4 ++--
 fs/ceph/mds_client.c         | 12 ++++++------
 fs/ceph/mds_client.h         |  2 +-
 fs/ceph/super.c              |  2 +-
 include/linux/ceph/libceph.h | 17 +++++++++++------
 net/ceph/ceph_common.c       | 41 +++++++++++++++++++++++++++++++----------
 net/ceph/mon_client.c        | 11 +++++++++--
 net/ceph/osd_client.c        | 15 +++++++--------
 9 files changed, 71 insertions(+), 38 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 349115ae3bc2..992683b6b299 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -4963,8 +4963,8 @@ out_err:
  */
 static int rbd_add_get_pool_id(struct rbd_client *rbdc, const char *pool_name)
 {
+	struct ceph_options *opts = rbdc->client->options;
 	u64 newest_epoch;
-	unsigned long timeout = rbdc->client->options->mount_timeout * HZ;
 	int tries = 0;
 	int ret;
 
@@ -4979,7 +4979,8 @@ again:
 		if (rbdc->client->osdc.osdmap->epoch < newest_epoch) {
 			ceph_monc_request_next_osdmap(&rbdc->client->monc);
 			(void) ceph_monc_wait_osdmap(&rbdc->client->monc,
-						     newest_epoch, timeout);
+						     newest_epoch,
+						     opts->mount_timeout);
 			goto again;
 		} else {
 			/* the osdmap we have is new enough */
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 4248307fea90..173dd4b58c71 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1259,8 +1259,8 @@ static int ceph_dir_fsync(struct file *file, loff_t start, loff_t end,
 		     inode, req->r_tid, last_tid);
 		if (req->r_timeout) {
 			unsigned long time_left = wait_for_completion_timeout(
-							&req->r_safe_completion,
-							req->r_timeout);
+					&req->r_safe_completion,
+					ceph_timeout_jiffies(req->r_timeout));
 			if (time_left > 0)
 				ret = 0;
 			else
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 69a36f40517f..0b0e0a9a81c0 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2268,7 +2268,8 @@ int ceph_mdsc_do_request(struct ceph_mds_client *mdsc,
 	dout("do_request waiting\n");
 	if (req->r_timeout) {
 		err = (long)wait_for_completion_killable_timeout(
-			&req->r_completion, req->r_timeout);
+					&req->r_completion,
+					ceph_timeout_jiffies(req->r_timeout));
 		if (err == 0)
 			err = -EIO;
 	} else if (req->r_wait_for_completion) {
@@ -3424,8 +3425,8 @@ int ceph_mdsc_init(struct ceph_fs_client *fsc)
  */
 static void wait_requests(struct ceph_mds_client *mdsc)
 {
+	struct ceph_options *opts = mdsc->fsc->client->options;
 	struct ceph_mds_request *req;
-	struct ceph_fs_client *fsc = mdsc->fsc;
 
 	mutex_lock(&mdsc->mutex);
 	if (__get_oldest_req(mdsc)) {
@@ -3433,7 +3434,7 @@ static void wait_requests(struct ceph_mds_client *mdsc)
 
 		dout("wait_requests waiting for requests\n");
 		wait_for_completion_timeout(&mdsc->safe_umount_waiters,
-				    fsc->client->options->mount_timeout * HZ);
+				    ceph_timeout_jiffies(opts->mount_timeout));
 
 		/* tear down remaining requests */
 		mutex_lock(&mdsc->mutex);
@@ -3556,10 +3557,9 @@ static bool done_closing_sessions(struct ceph_mds_client *mdsc)
  */
 void ceph_mdsc_close_sessions(struct ceph_mds_client *mdsc)
 {
+	struct ceph_options *opts = mdsc->fsc->client->options;
 	struct ceph_mds_session *session;
 	int i;
-	struct ceph_fs_client *fsc = mdsc->fsc;
-	unsigned long timeout = fsc->client->options->mount_timeout * HZ;
 
 	dout("close_sessions\n");
 
@@ -3580,7 +3580,7 @@ void ceph_mdsc_close_sessions(struct ceph_mds_client *mdsc)
 
 	dout("waiting for sessions to close\n");
 	wait_event_timeout(mdsc->session_close_wq, done_closing_sessions(mdsc),
-			   timeout);
+			   ceph_timeout_jiffies(opts->mount_timeout));
 
 	/* tear down remaining sessions */
 	mutex_lock(&mdsc->mutex);
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index 2ef799961ebb..509d6822e9b1 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -227,7 +227,7 @@ struct ceph_mds_request {
 	int r_err;
 	bool r_aborted;
 
-	unsigned long r_timeout;  /* optional.  jiffies */
+	unsigned long r_timeout;  /* optional.  jiffies, 0 is "wait forever" */
 	unsigned long r_started;  /* start time to measure timeout against */
 	unsigned long r_request_started; /* start time for mds request only,
 					    used to measure lease durations */
diff --git a/fs/ceph/super.c b/fs/ceph/super.c
index 9a5350030af8..edeb83c43112 100644
--- a/fs/ceph/super.c
+++ b/fs/ceph/super.c
@@ -742,7 +742,7 @@ static struct dentry *open_root_dentry(struct ceph_fs_client *fsc,
 	req->r_ino1.ino = CEPH_INO_ROOT;
 	req->r_ino1.snap = CEPH_NOSNAP;
 	req->r_started = started;
-	req->r_timeout = fsc->client->options->mount_timeout * HZ;
+	req->r_timeout = fsc->client->options->mount_timeout;
 	req->r_args.getattr.mask = cpu_to_le32(CEPH_STAT_CAP_INODE);
 	req->r_num_caps = 2;
 	err = ceph_mdsc_do_request(mdsc, NULL, req);
diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
index 85ae9a889a3f..d73a569f9bf5 100644
--- a/include/linux/ceph/libceph.h
+++ b/include/linux/ceph/libceph.h
@@ -43,9 +43,9 @@ struct ceph_options {
 	int flags;
 	struct ceph_fsid fsid;
 	struct ceph_entity_addr my_addr;
-	int mount_timeout;
-	int osd_idle_ttl;
-	int osd_keepalive_timeout;
+	unsigned long mount_timeout;		/* jiffies */
+	unsigned long osd_idle_ttl;		/* jiffies */
+	unsigned long osd_keepalive_timeout;	/* jiffies */
 
 	/*
 	 * any type that can't be simply compared or doesn't need need
@@ -63,9 +63,9 @@ struct ceph_options {
 /*
  * defaults
  */
-#define CEPH_MOUNT_TIMEOUT_DEFAULT  60
-#define CEPH_OSD_KEEPALIVE_DEFAULT  5
-#define CEPH_OSD_IDLE_TTL_DEFAULT    60
+#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_MSG_MAX_FRONT_LEN	(16*1024*1024)
 #define CEPH_MSG_MAX_MIDDLE_LEN	(16*1024*1024)
@@ -93,6 +93,11 @@ enum {
 	CEPH_MOUNT_SHUTDOWN,
 };
 
+static inline unsigned long ceph_timeout_jiffies(unsigned long timeout)
+{
+	return timeout ?: MAX_SCHEDULE_TIMEOUT;
+}
+
 struct ceph_mds_client;
 
 /*
diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
index 79e8f71aef5b..a80e91c2c9a3 100644
--- a/net/ceph/ceph_common.c
+++ b/net/ceph/ceph_common.c
@@ -352,8 +352,8 @@ ceph_parse_options(char *options, const char *dev_name,
 	/* start with defaults */
 	opt->flags = CEPH_OPT_DEFAULT;
 	opt->osd_keepalive_timeout = CEPH_OSD_KEEPALIVE_DEFAULT;
-	opt->mount_timeout = CEPH_MOUNT_TIMEOUT_DEFAULT; /* seconds */
-	opt->osd_idle_ttl = CEPH_OSD_IDLE_TTL_DEFAULT;   /* seconds */
+	opt->mount_timeout = CEPH_MOUNT_TIMEOUT_DEFAULT;
+	opt->osd_idle_ttl = CEPH_OSD_IDLE_TTL_DEFAULT;
 
 	/* get mon ip(s) */
 	/* ip1[:port1][,ip2[:port2]...] */
@@ -439,13 +439,32 @@ ceph_parse_options(char *options, const char *dev_name,
 			pr_warn("ignoring deprecated osdtimeout option\n");
 			break;
 		case Opt_osdkeepalivetimeout:
-			opt->osd_keepalive_timeout = intval;
+			/* 0 isn't well defined right now, reject it */
+			if (intval < 1 || intval > INT_MAX / 1000) {
+				pr_err("osdkeepalive out of range\n");
+				err = -EINVAL;
+				goto out;
+			}
+			opt->osd_keepalive_timeout =
+					msecs_to_jiffies(intval * 1000);
 			break;
 		case Opt_osd_idle_ttl:
-			opt->osd_idle_ttl = intval;
+			/* 0 isn't well defined right now, reject it */
+			if (intval < 1 || intval > INT_MAX / 1000) {
+				pr_err("osd_idle_ttl out of range\n");
+				err = -EINVAL;
+				goto out;
+			}
+			opt->osd_idle_ttl = msecs_to_jiffies(intval * 1000);
 			break;
 		case Opt_mount_timeout:
-			opt->mount_timeout = intval;
+			/* 0 is "wait forever" (i.e. infinite timeout) */
+			if (intval < 0 || intval > INT_MAX / 1000) {
+				pr_err("mount_timeout out of range\n");
+				err = -EINVAL;
+				goto out;
+			}
+			opt->mount_timeout = msecs_to_jiffies(intval * 1000);
 			break;
 
 		case Opt_share:
@@ -512,12 +531,14 @@ int ceph_print_client_options(struct seq_file *m, struct ceph_client *client)
 		seq_puts(m, "notcp_nodelay,");
 
 	if (opt->mount_timeout != CEPH_MOUNT_TIMEOUT_DEFAULT)
-		seq_printf(m, "mount_timeout=%d,", opt->mount_timeout);
+		seq_printf(m, "mount_timeout=%d,",
+			   jiffies_to_msecs(opt->mount_timeout) / 1000);
 	if (opt->osd_idle_ttl != CEPH_OSD_IDLE_TTL_DEFAULT)
-		seq_printf(m, "osd_idle_ttl=%d,", opt->osd_idle_ttl);
+		seq_printf(m, "osd_idle_ttl=%d,",
+			   jiffies_to_msecs(opt->osd_idle_ttl) / 1000);
 	if (opt->osd_keepalive_timeout != CEPH_OSD_KEEPALIVE_DEFAULT)
 		seq_printf(m, "osdkeepalivetimeout=%d,",
-			   opt->osd_keepalive_timeout);
+		    jiffies_to_msecs(opt->osd_keepalive_timeout) / 1000);
 
 	/* drop redundant comma */
 	if (m->count != pos)
@@ -627,7 +648,7 @@ static int have_mon_and_osd_map(struct ceph_client *client)
 int __ceph_open_session(struct ceph_client *client, unsigned long started)
 {
 	int err;
-	unsigned long timeout = client->options->mount_timeout * HZ;
+	unsigned long timeout = client->options->mount_timeout;
 
 	/* open session, and wait for mon and osd maps */
 	err = ceph_monc_open_session(&client->monc);
@@ -643,7 +664,7 @@ int __ceph_open_session(struct ceph_client *client, unsigned long started)
 		dout("mount waiting for mon_map\n");
 		err = wait_event_interruptible_timeout(client->auth_wq,
 			have_mon_and_osd_map(client) || (client->auth_err < 0),
-			timeout);
+			ceph_timeout_jiffies(timeout));
 		if (err == -EINTR || err == -ERESTARTSYS)
 			return err;
 		if (client->auth_err < 0)
diff --git a/net/ceph/mon_client.c b/net/ceph/mon_client.c
index 2b3cf05e87b0..0da3bdc116f7 100644
--- a/net/ceph/mon_client.c
+++ b/net/ceph/mon_client.c
@@ -298,6 +298,12 @@ void ceph_monc_request_next_osdmap(struct ceph_mon_client *monc)
 }
 EXPORT_SYMBOL(ceph_monc_request_next_osdmap);
 
+/*
+ * Wait for an osdmap with a given epoch.
+ *
+ * @epoch: epoch to wait for
+ * @timeout: in jiffies, 0 means "wait forever"
+ */
 int ceph_monc_wait_osdmap(struct ceph_mon_client *monc, u32 epoch,
 			  unsigned long timeout)
 {
@@ -308,11 +314,12 @@ int ceph_monc_wait_osdmap(struct ceph_mon_client *monc, u32 epoch,
 	while (monc->have_osdmap < epoch) {
 		mutex_unlock(&monc->mutex);
 
-		if (timeout != 0 && time_after_eq(jiffies, started + timeout))
+		if (timeout && time_after_eq(jiffies, started + timeout))
 			return -ETIMEDOUT;
 
 		ret = wait_event_interruptible_timeout(monc->client->auth_wq,
-					 monc->have_osdmap >= epoch, timeout);
+						monc->have_osdmap >= epoch,
+						ceph_timeout_jiffies(timeout));
 		if (ret < 0)
 			return ret;
 
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 4cb4fab46e4f..50033677c0fa 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -1097,7 +1097,7 @@ static void __move_osd_to_lru(struct ceph_osd_client *osdc,
 	BUG_ON(!list_empty(&osd->o_osd_lru));
 
 	list_add_tail(&osd->o_osd_lru, &osdc->osd_lru);
-	osd->lru_ttl = jiffies + osdc->client->options->osd_idle_ttl * HZ;
+	osd->lru_ttl = jiffies + osdc->client->options->osd_idle_ttl;
 }
 
 static void maybe_move_osd_to_lru(struct ceph_osd_client *osdc,
@@ -1208,7 +1208,7 @@ static struct ceph_osd *__lookup_osd(struct ceph_osd_client *osdc, int o)
 static void __schedule_osd_timeout(struct ceph_osd_client *osdc)
 {
 	schedule_delayed_work(&osdc->timeout_work,
-			osdc->client->options->osd_keepalive_timeout * HZ);
+			      osdc->client->options->osd_keepalive_timeout);
 }
 
 static void __cancel_osd_timeout(struct ceph_osd_client *osdc)
@@ -1576,10 +1576,9 @@ static void handle_timeout(struct work_struct *work)
 {
 	struct ceph_osd_client *osdc =
 		container_of(work, struct ceph_osd_client, timeout_work.work);
+	struct ceph_options *opts = osdc->client->options;
 	struct ceph_osd_request *req;
 	struct ceph_osd *osd;
-	unsigned long keepalive =
-		osdc->client->options->osd_keepalive_timeout * HZ;
 	struct list_head slow_osds;
 	dout("timeout\n");
 	down_read(&osdc->map_sem);
@@ -1595,7 +1594,8 @@ static void handle_timeout(struct work_struct *work)
 	 */
 	INIT_LIST_HEAD(&slow_osds);
 	list_for_each_entry(req, &osdc->req_lru, r_req_lru_item) {
-		if (time_before(jiffies, req->r_stamp + keepalive))
+		if (time_before(jiffies,
+				req->r_stamp + opts->osd_keepalive_timeout))
 			break;
 
 		osd = req->r_osd;
@@ -1622,8 +1622,7 @@ static void handle_osds_timeout(struct work_struct *work)
 	struct ceph_osd_client *osdc =
 		container_of(work, struct ceph_osd_client,
 			     osds_timeout_work.work);
-	unsigned long delay =
-		osdc->client->options->osd_idle_ttl * HZ >> 2;
+	unsigned long delay = osdc->client->options->osd_idle_ttl / 4;
 
 	dout("osds timeout\n");
 	down_read(&osdc->map_sem);
@@ -2628,7 +2627,7 @@ int ceph_osdc_init(struct ceph_osd_client *osdc, struct ceph_client *client)
 	osdc->event_count = 0;
 
 	schedule_delayed_work(&osdc->osds_timeout_work,
-	   round_jiffies_relative(osdc->client->options->osd_idle_ttl * HZ));
+	    round_jiffies_relative(osdc->client->options->osd_idle_ttl));
 
 	err = -ENOMEM;
 	osdc->req_mempool = mempool_create_kmalloc_pool(10,
-- 
1.9.3

Re: [PATCH 2/5] libceph: store timeouts in jiffies, verify user input

[Alex Elder]

On 05/21/2015 07:35 AM, Ilya Dryomov wrote:
> There are currently three libceph-level timeouts that the user can
> specify on mount: mount_timeout, osd_idle_ttl and osdkeepalive.  All of
> these are in seconds and no checking is done on user input: negative
> values are accepted, we multiply them all by HZ which may or may not
> overflow, arbitrarily large jiffies then get added together, etc.
> 
> There is also a bug in the way mount_timeout=0 is handled.  It's
> supposed to mean "infinite timeout", but that's not how wait.h APIs
> treat it and so __ceph_open_session() for example will busy loop
> without much chance of being interrupted if none of ceph-mons are
> there.
> 
> Fix all this by verifying user input, storing timeouts capped by
> msecs_to_jiffies() in jiffies and using the new ceph_timeout_jiffies()
> helper for all user-specified waits to handle infinite timeouts
> correctly.

This looks good.  I like your use of local variables for
the options pointer; it makes it easy to see in the code
where the timeout values come from.

You could have handled timeout option checking and
error reporting generically, but I'm not sure that
would be better.

Reviewed-by: Alex Elder <elder@linaro.org>

> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
>  drivers/block/rbd.c          |  5 +++--
>  fs/ceph/dir.c                |  4 ++--
>  fs/ceph/mds_client.c         | 12 ++++++------
>  fs/ceph/mds_client.h         |  2 +-
>  fs/ceph/super.c              |  2 +-
>  include/linux/ceph/libceph.h | 17 +++++++++++------
>  net/ceph/ceph_common.c       | 41 +++++++++++++++++++++++++++++++----------
>  net/ceph/mon_client.c        | 11 +++++++++--
>  net/ceph/osd_client.c        | 15 +++++++--------
>  9 files changed, 71 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 349115ae3bc2..992683b6b299 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -4963,8 +4963,8 @@ out_err:
>   */
>  static int rbd_add_get_pool_id(struct rbd_client *rbdc, const char *pool_name)
>  {
> +	struct ceph_options *opts = rbdc->client->options;
>  	u64 newest_epoch;
> -	unsigned long timeout = rbdc->client->options->mount_timeout * HZ;
>  	int tries = 0;
>  	int ret;
>  
> @@ -4979,7 +4979,8 @@ again:
>  		if (rbdc->client->osdc.osdmap->epoch < newest_epoch) {
>  			ceph_monc_request_next_osdmap(&rbdc->client->monc);
>  			(void) ceph_monc_wait_osdmap(&rbdc->client->monc,
> -						     newest_epoch, timeout);
> +						     newest_epoch,
> +						     opts->mount_timeout);
>  			goto again;
>  		} else {
>  			/* the osdmap we have is new enough */
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 4248307fea90..173dd4b58c71 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -1259,8 +1259,8 @@ static int ceph_dir_fsync(struct file *file, loff_t start, loff_t end,
>  		     inode, req->r_tid, last_tid);
>  		if (req->r_timeout) {
>  			unsigned long time_left = wait_for_completion_timeout(
> -							&req->r_safe_completion,
> -							req->r_timeout);
> +					&req->r_safe_completion,
> +					ceph_timeout_jiffies(req->r_timeout));
>  			if (time_left > 0)
>  				ret = 0;
>  			else
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 69a36f40517f..0b0e0a9a81c0 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -2268,7 +2268,8 @@ int ceph_mdsc_do_request(struct ceph_mds_client *mdsc,
>  	dout("do_request waiting\n");
>  	if (req->r_timeout) {
>  		err = (long)wait_for_completion_killable_timeout(
> -			&req->r_completion, req->r_timeout);
> +					&req->r_completion,
> +					ceph_timeout_jiffies(req->r_timeout));
>  		if (err == 0)
>  			err = -EIO;
>  	} else if (req->r_wait_for_completion) {
> @@ -3424,8 +3425,8 @@ int ceph_mdsc_init(struct ceph_fs_client *fsc)
>   */
>  static void wait_requests(struct ceph_mds_client *mdsc)
>  {
> +	struct ceph_options *opts = mdsc->fsc->client->options;
>  	struct ceph_mds_request *req;
> -	struct ceph_fs_client *fsc = mdsc->fsc;
>  
>  	mutex_lock(&mdsc->mutex);
>  	if (__get_oldest_req(mdsc)) {
> @@ -3433,7 +3434,7 @@ static void wait_requests(struct ceph_mds_client *mdsc)
>  
>  		dout("wait_requests waiting for requests\n");
>  		wait_for_completion_timeout(&mdsc->safe_umount_waiters,
> -				    fsc->client->options->mount_timeout * HZ);
> +				    ceph_timeout_jiffies(opts->mount_timeout));
>  
>  		/* tear down remaining requests */
>  		mutex_lock(&mdsc->mutex);
> @@ -3556,10 +3557,9 @@ static bool done_closing_sessions(struct ceph_mds_client *mdsc)
>   */
>  void ceph_mdsc_close_sessions(struct ceph_mds_client *mdsc)
>  {
> +	struct ceph_options *opts = mdsc->fsc->client->options;
>  	struct ceph_mds_session *session;
>  	int i;
> -	struct ceph_fs_client *fsc = mdsc->fsc;
> -	unsigned long timeout = fsc->client->options->mount_timeout * HZ;
>  
>  	dout("close_sessions\n");
>  
> @@ -3580,7 +3580,7 @@ void ceph_mdsc_close_sessions(struct ceph_mds_client *mdsc)
>  
>  	dout("waiting for sessions to close\n");
>  	wait_event_timeout(mdsc->session_close_wq, done_closing_sessions(mdsc),
> -			   timeout);
> +			   ceph_timeout_jiffies(opts->mount_timeout));
>  
>  	/* tear down remaining sessions */
>  	mutex_lock(&mdsc->mutex);
> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> index 2ef799961ebb..509d6822e9b1 100644
> --- a/fs/ceph/mds_client.h
> +++ b/fs/ceph/mds_client.h
> @@ -227,7 +227,7 @@ struct ceph_mds_request {
>  	int r_err;
>  	bool r_aborted;
>  
> -	unsigned long r_timeout;  /* optional.  jiffies */
> +	unsigned long r_timeout;  /* optional.  jiffies, 0 is "wait forever" */
>  	unsigned long r_started;  /* start time to measure timeout against */
>  	unsigned long r_request_started; /* start time for mds request only,
>  					    used to measure lease durations */
> diff --git a/fs/ceph/super.c b/fs/ceph/super.c
> index 9a5350030af8..edeb83c43112 100644
> --- a/fs/ceph/super.c
> +++ b/fs/ceph/super.c
> @@ -742,7 +742,7 @@ static struct dentry *open_root_dentry(struct ceph_fs_client *fsc,
>  	req->r_ino1.ino = CEPH_INO_ROOT;
>  	req->r_ino1.snap = CEPH_NOSNAP;
>  	req->r_started = started;
> -	req->r_timeout = fsc->client->options->mount_timeout * HZ;
> +	req->r_timeout = fsc->client->options->mount_timeout;
>  	req->r_args.getattr.mask = cpu_to_le32(CEPH_STAT_CAP_INODE);
>  	req->r_num_caps = 2;
>  	err = ceph_mdsc_do_request(mdsc, NULL, req);
> diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
> index 85ae9a889a3f..d73a569f9bf5 100644
> --- a/include/linux/ceph/libceph.h
> +++ b/include/linux/ceph/libceph.h
> @@ -43,9 +43,9 @@ struct ceph_options {
>  	int flags;
>  	struct ceph_fsid fsid;
>  	struct ceph_entity_addr my_addr;
> -	int mount_timeout;
> -	int osd_idle_ttl;
> -	int osd_keepalive_timeout;
> +	unsigned long mount_timeout;		/* jiffies */
> +	unsigned long osd_idle_ttl;		/* jiffies */
> +	unsigned long osd_keepalive_timeout;	/* jiffies */
>  
>  	/*
>  	 * any type that can't be simply compared or doesn't need need
> @@ -63,9 +63,9 @@ struct ceph_options {
>  /*
>   * defaults
>   */
> -#define CEPH_MOUNT_TIMEOUT_DEFAULT  60
> -#define CEPH_OSD_KEEPALIVE_DEFAULT  5
> -#define CEPH_OSD_IDLE_TTL_DEFAULT    60
> +#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_MSG_MAX_FRONT_LEN	(16*1024*1024)
>  #define CEPH_MSG_MAX_MIDDLE_LEN	(16*1024*1024)
> @@ -93,6 +93,11 @@ enum {
>  	CEPH_MOUNT_SHUTDOWN,
>  };
>  
> +static inline unsigned long ceph_timeout_jiffies(unsigned long timeout)
> +{
> +	return timeout ?: MAX_SCHEDULE_TIMEOUT;
> +}
> +
>  struct ceph_mds_client;
>  
>  /*
> diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
> index 79e8f71aef5b..a80e91c2c9a3 100644
> --- a/net/ceph/ceph_common.c
> +++ b/net/ceph/ceph_common.c
> @@ -352,8 +352,8 @@ ceph_parse_options(char *options, const char *dev_name,
>  	/* start with defaults */
>  	opt->flags = CEPH_OPT_DEFAULT;
>  	opt->osd_keepalive_timeout = CEPH_OSD_KEEPALIVE_DEFAULT;
> -	opt->mount_timeout = CEPH_MOUNT_TIMEOUT_DEFAULT; /* seconds */
> -	opt->osd_idle_ttl = CEPH_OSD_IDLE_TTL_DEFAULT;   /* seconds */
> +	opt->mount_timeout = CEPH_MOUNT_TIMEOUT_DEFAULT;
> +	opt->osd_idle_ttl = CEPH_OSD_IDLE_TTL_DEFAULT;
>  
>  	/* get mon ip(s) */
>  	/* ip1[:port1][,ip2[:port2]...] */
> @@ -439,13 +439,32 @@ ceph_parse_options(char *options, const char *dev_name,
>  			pr_warn("ignoring deprecated osdtimeout option\n");
>  			break;
>  		case Opt_osdkeepalivetimeout:
> -			opt->osd_keepalive_timeout = intval;
> +			/* 0 isn't well defined right now, reject it */
> +			if (intval < 1 || intval > INT_MAX / 1000) {
> +				pr_err("osdkeepalive out of range\n");
> +				err = -EINVAL;
> +				goto out;
> +			}
> +			opt->osd_keepalive_timeout =
> +					msecs_to_jiffies(intval * 1000);
>  			break;
>  		case Opt_osd_idle_ttl:
> -			opt->osd_idle_ttl = intval;
> +			/* 0 isn't well defined right now, reject it */
> +			if (intval < 1 || intval > INT_MAX / 1000) {
> +				pr_err("osd_idle_ttl out of range\n");
> +				err = -EINVAL;
> +				goto out;
> +			}
> +			opt->osd_idle_ttl = msecs_to_jiffies(intval * 1000);
>  			break;
>  		case Opt_mount_timeout:
> -			opt->mount_timeout = intval;
> +			/* 0 is "wait forever" (i.e. infinite timeout) */
> +			if (intval < 0 || intval > INT_MAX / 1000) {
> +				pr_err("mount_timeout out of range\n");
> +				err = -EINVAL;
> +				goto out;
> +			}
> +			opt->mount_timeout = msecs_to_jiffies(intval * 1000);
>  			break;
>  
>  		case Opt_share:
> @@ -512,12 +531,14 @@ int ceph_print_client_options(struct seq_file *m, struct ceph_client *client)
>  		seq_puts(m, "notcp_nodelay,");
>  
>  	if (opt->mount_timeout != CEPH_MOUNT_TIMEOUT_DEFAULT)
> -		seq_printf(m, "mount_timeout=%d,", opt->mount_timeout);
> +		seq_printf(m, "mount_timeout=%d,",
> +			   jiffies_to_msecs(opt->mount_timeout) / 1000);
>  	if (opt->osd_idle_ttl != CEPH_OSD_IDLE_TTL_DEFAULT)
> -		seq_printf(m, "osd_idle_ttl=%d,", opt->osd_idle_ttl);
> +		seq_printf(m, "osd_idle_ttl=%d,",
> +			   jiffies_to_msecs(opt->osd_idle_ttl) / 1000);
>  	if (opt->osd_keepalive_timeout != CEPH_OSD_KEEPALIVE_DEFAULT)
>  		seq_printf(m, "osdkeepalivetimeout=%d,",
> -			   opt->osd_keepalive_timeout);
> +		    jiffies_to_msecs(opt->osd_keepalive_timeout) / 1000);
>  
>  	/* drop redundant comma */
>  	if (m->count != pos)
> @@ -627,7 +648,7 @@ static int have_mon_and_osd_map(struct ceph_client *client)
>  int __ceph_open_session(struct ceph_client *client, unsigned long started)
>  {
>  	int err;
> -	unsigned long timeout = client->options->mount_timeout * HZ;
> +	unsigned long timeout = client->options->mount_timeout;
>  
>  	/* open session, and wait for mon and osd maps */
>  	err = ceph_monc_open_session(&client->monc);
> @@ -643,7 +664,7 @@ int __ceph_open_session(struct ceph_client *client, unsigned long started)
>  		dout("mount waiting for mon_map\n");
>  		err = wait_event_interruptible_timeout(client->auth_wq,
>  			have_mon_and_osd_map(client) || (client->auth_err < 0),
> -			timeout);
> +			ceph_timeout_jiffies(timeout));
>  		if (err == -EINTR || err == -ERESTARTSYS)
>  			return err;
>  		if (client->auth_err < 0)
> diff --git a/net/ceph/mon_client.c b/net/ceph/mon_client.c
> index 2b3cf05e87b0..0da3bdc116f7 100644
> --- a/net/ceph/mon_client.c
> +++ b/net/ceph/mon_client.c
> @@ -298,6 +298,12 @@ void ceph_monc_request_next_osdmap(struct ceph_mon_client *monc)
>  }
>  EXPORT_SYMBOL(ceph_monc_request_next_osdmap);
>  
> +/*
> + * Wait for an osdmap with a given epoch.
> + *
> + * @epoch: epoch to wait for
> + * @timeout: in jiffies, 0 means "wait forever"
> + */
>  int ceph_monc_wait_osdmap(struct ceph_mon_client *monc, u32 epoch,
>  			  unsigned long timeout)
>  {
> @@ -308,11 +314,12 @@ int ceph_monc_wait_osdmap(struct ceph_mon_client *monc, u32 epoch,
>  	while (monc->have_osdmap < epoch) {
>  		mutex_unlock(&monc->mutex);
>  
> -		if (timeout != 0 && time_after_eq(jiffies, started + timeout))
> +		if (timeout && time_after_eq(jiffies, started + timeout))
>  			return -ETIMEDOUT;
>  
>  		ret = wait_event_interruptible_timeout(monc->client->auth_wq,
> -					 monc->have_osdmap >= epoch, timeout);
> +						monc->have_osdmap >= epoch,
> +						ceph_timeout_jiffies(timeout));
>  		if (ret < 0)
>  			return ret;
>  
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index 4cb4fab46e4f..50033677c0fa 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -1097,7 +1097,7 @@ static void __move_osd_to_lru(struct ceph_osd_client *osdc,
>  	BUG_ON(!list_empty(&osd->o_osd_lru));
>  
>  	list_add_tail(&osd->o_osd_lru, &osdc->osd_lru);
> -	osd->lru_ttl = jiffies + osdc->client->options->osd_idle_ttl * HZ;
> +	osd->lru_ttl = jiffies + osdc->client->options->osd_idle_ttl;
>  }
>  
>  static void maybe_move_osd_to_lru(struct ceph_osd_client *osdc,
> @@ -1208,7 +1208,7 @@ static struct ceph_osd *__lookup_osd(struct ceph_osd_client *osdc, int o)
>  static void __schedule_osd_timeout(struct ceph_osd_client *osdc)
>  {
>  	schedule_delayed_work(&osdc->timeout_work,
> -			osdc->client->options->osd_keepalive_timeout * HZ);
> +			      osdc->client->options->osd_keepalive_timeout);
>  }
>  
>  static void __cancel_osd_timeout(struct ceph_osd_client *osdc)
> @@ -1576,10 +1576,9 @@ static void handle_timeout(struct work_struct *work)
>  {
>  	struct ceph_osd_client *osdc =
>  		container_of(work, struct ceph_osd_client, timeout_work.work);
> +	struct ceph_options *opts = osdc->client->options;
>  	struct ceph_osd_request *req;
>  	struct ceph_osd *osd;
> -	unsigned long keepalive =
> -		osdc->client->options->osd_keepalive_timeout * HZ;
>  	struct list_head slow_osds;
>  	dout("timeout\n");
>  	down_read(&osdc->map_sem);
> @@ -1595,7 +1594,8 @@ static void handle_timeout(struct work_struct *work)
>  	 */
>  	INIT_LIST_HEAD(&slow_osds);
>  	list_for_each_entry(req, &osdc->req_lru, r_req_lru_item) {
> -		if (time_before(jiffies, req->r_stamp + keepalive))
> +		if (time_before(jiffies,
> +				req->r_stamp + opts->osd_keepalive_timeout))
>  			break;
>  
>  		osd = req->r_osd;
> @@ -1622,8 +1622,7 @@ static void handle_osds_timeout(struct work_struct *work)
>  	struct ceph_osd_client *osdc =
>  		container_of(work, struct ceph_osd_client,
>  			     osds_timeout_work.work);
> -	unsigned long delay =
> -		osdc->client->options->osd_idle_ttl * HZ >> 2;
> +	unsigned long delay = osdc->client->options->osd_idle_ttl / 4;
>  
>  	dout("osds timeout\n");
>  	down_read(&osdc->map_sem);
> @@ -2628,7 +2627,7 @@ int ceph_osdc_init(struct ceph_osd_client *osdc, struct ceph_client *client)
>  	osdc->event_count = 0;
>  
>  	schedule_delayed_work(&osdc->osds_timeout_work,
> -	   round_jiffies_relative(osdc->client->options->osd_idle_ttl * HZ));
> +	    round_jiffies_relative(osdc->client->options->osd_idle_ttl));
>  
>  	err = -ENOMEM;
>  	osdc->req_mempool = mempool_create_kmalloc_pool(10,
> 

[PATCH 3/5] libceph: a couple tweaks for wait loops

[Ilya Dryomov]

- return -ETIMEDOUT instead of -EIO in case of timeout
- wait_event_interruptible_timeout() returns time left until timeout
  and since it can be almost LONG_MAX we had better assign it to long

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 net/ceph/ceph_common.c | 7 +++----
 net/ceph/mon_client.c  | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
index a80e91c2c9a3..925d0c890b80 100644
--- a/net/ceph/ceph_common.c
+++ b/net/ceph/ceph_common.c
@@ -647,8 +647,8 @@ static int have_mon_and_osd_map(struct ceph_client *client)
  */
 int __ceph_open_session(struct ceph_client *client, unsigned long started)
 {
-	int err;
 	unsigned long timeout = client->options->mount_timeout;
+	long err;
 
 	/* open session, and wait for mon and osd maps */
 	err = ceph_monc_open_session(&client->monc);
@@ -656,16 +656,15 @@ int __ceph_open_session(struct ceph_client *client, unsigned long started)
 		return err;
 
 	while (!have_mon_and_osd_map(client)) {
-		err = -EIO;
 		if (timeout && time_after_eq(jiffies, started + timeout))
-			return err;
+			return -ETIMEDOUT;
 
 		/* wait */
 		dout("mount waiting for mon_map\n");
 		err = wait_event_interruptible_timeout(client->auth_wq,
 			have_mon_and_osd_map(client) || (client->auth_err < 0),
 			ceph_timeout_jiffies(timeout));
-		if (err == -EINTR || err == -ERESTARTSYS)
+		if (err < 0)
 			return err;
 		if (client->auth_err < 0)
 			return client->auth_err;
diff --git a/net/ceph/mon_client.c b/net/ceph/mon_client.c
index 0da3bdc116f7..9d6ff1215928 100644
--- a/net/ceph/mon_client.c
+++ b/net/ceph/mon_client.c
@@ -308,7 +308,7 @@ int ceph_monc_wait_osdmap(struct ceph_mon_client *monc, u32 epoch,
 			  unsigned long timeout)
 {
 	unsigned long started = jiffies;
-	int ret;
+	long ret;
 
 	mutex_lock(&monc->mutex);
 	while (monc->have_osdmap < epoch) {
-- 
1.9.3

Re: [PATCH 3/5] libceph: a couple tweaks for wait loops

[Alex Elder]

On 05/21/2015 07:35 AM, Ilya Dryomov wrote:
> - return -ETIMEDOUT instead of -EIO in case of timeout
> - wait_event_interruptible_timeout() returns time left until timeout
>   and since it can be almost LONG_MAX we had better assign it to long

Any error returned by wait_event_interruptible_timeout()
can now be returned by __ceph_open_session().  It looks
like that may, in fact, be only -EINTR and -ERESTARTSYS.
But it's a change you could note in the log message.

It turns out the only caller ignores the return value of
ceph_monc_wait_osdmap() anyway.  That should maybe be fixed.

In any case, this looks good.

Reviewed-by: Alex Elder <elder@linaro.org>

> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
>  net/ceph/ceph_common.c | 7 +++----
>  net/ceph/mon_client.c  | 2 +-
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
> index a80e91c2c9a3..925d0c890b80 100644
> --- a/net/ceph/ceph_common.c
> +++ b/net/ceph/ceph_common.c
> @@ -647,8 +647,8 @@ static int have_mon_and_osd_map(struct ceph_client *client)
>   */
>  int __ceph_open_session(struct ceph_client *client, unsigned long started)
>  {
> -	int err;
>  	unsigned long timeout = client->options->mount_timeout;
> +	long err;
>  
>  	/* open session, and wait for mon and osd maps */
>  	err = ceph_monc_open_session(&client->monc);
> @@ -656,16 +656,15 @@ int __ceph_open_session(struct ceph_client *client, unsigned long started)
>  		return err;
>  
>  	while (!have_mon_and_osd_map(client)) {
> -		err = -EIO;
>  		if (timeout && time_after_eq(jiffies, started + timeout))
> -			return err;
> +			return -ETIMEDOUT;
>  
>  		/* wait */
>  		dout("mount waiting for mon_map\n");
>  		err = wait_event_interruptible_timeout(client->auth_wq,
>  			have_mon_and_osd_map(client) || (client->auth_err < 0),
>  			ceph_timeout_jiffies(timeout));
> -		if (err == -EINTR || err == -ERESTARTSYS)
> +		if (err < 0)
>  			return err;
>  		if (client->auth_err < 0)
>  			return client->auth_err;
> diff --git a/net/ceph/mon_client.c b/net/ceph/mon_client.c
> index 0da3bdc116f7..9d6ff1215928 100644
> --- a/net/ceph/mon_client.c
> +++ b/net/ceph/mon_client.c
> @@ -308,7 +308,7 @@ int ceph_monc_wait_osdmap(struct ceph_mon_client *monc, u32 epoch,
>  			  unsigned long timeout)
>  {
>  	unsigned long started = jiffies;
> -	int ret;
> +	long ret;
>  
>  	mutex_lock(&monc->mutex);
>  	while (monc->have_osdmap < epoch) {
> 

Re: [PATCH 3/5] libceph: a couple tweaks for wait loops

[Ilya Dryomov]

On Thu, May 21, 2015 at 4:29 PM, Alex Elder <elder@ieee.org> wrote:
> On 05/21/2015 07:35 AM, Ilya Dryomov wrote:
>> - return -ETIMEDOUT instead of -EIO in case of timeout
>> - wait_event_interruptible_timeout() returns time left until timeout
>>   and since it can be almost LONG_MAX we had better assign it to long
>
> Any error returned by wait_event_interruptible_timeout()
> can now be returned by __ceph_open_session().  It looks
> like that may, in fact, be only -EINTR and -ERESTARTSYS.
> But it's a change you could note in the log message.

I think it's just -ERESTARTSYS so I didn't bother.

>
> It turns out the only caller ignores the return value of
> ceph_monc_wait_osdmap() anyway.  That should maybe be fixed.

That's on purpose - rbd map tries to wait for a new enough osdmap only
if the pool that the image is supposed to be in doesn't exist and we
know we have a stale osdmap.  We ignore wait retval because if we
timeout we should return "this pool doesn't exist", not -ETIMEDOUT.

Thanks,

                Ilya

Re: [PATCH 3/5] libceph: a couple tweaks for wait loops

[Alex Elder]

On 05/25/2015 05:38 AM, Ilya Dryomov wrote:
> On Thu, May 21, 2015 at 4:29 PM, Alex Elder <elder@ieee.org> wrote:
>> On 05/21/2015 07:35 AM, Ilya Dryomov wrote:
>>> - return -ETIMEDOUT instead of -EIO in case of timeout
>>> - wait_event_interruptible_timeout() returns time left until timeout
>>>   and since it can be almost LONG_MAX we had better assign it to long
>>
>> Any error returned by wait_event_interruptible_timeout()
>> can now be returned by __ceph_open_session().  It looks
>> like that may, in fact, be only -EINTR and -ERESTARTSYS.
>> But it's a change you could note in the log message.
> 
> I think it's just -ERESTARTSYS so I didn't bother.

My point was almost a little more philosophical.  It's conceivable
(though not likely) that wait_event_interruptible_timeout() could
be changed to return a value that your caller here does not expect.

>> It turns out the only caller ignores the return value of
>> ceph_monc_wait_osdmap() anyway.  That should maybe be fixed.
> 
> That's on purpose - rbd map tries to wait for a new enough osdmap only
> if the pool that the image is supposed to be in doesn't exist and we
> know we have a stale osdmap.  We ignore wait retval because if we
> timeout we should return "this pool doesn't exist", not -ETIMEDOUT.

Yes I realize that.  This second part of my response was
following on to my previous thought.  That is, the caller
might get a different return value that it didn't expect;
but since the only caller ignores what gets returned, it's
a moot point.

					-Alex

> Thanks,
> 
>                 Ilya
> 

[PATCH 4/5] ceph: simplify two mount_timeout sites

[Ilya Dryomov]

No need to bifurcate wait now that we've got ceph_timeout_jiffies().

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 fs/ceph/dir.c        | 14 ++++----------
 fs/ceph/mds_client.c | 18 ++++++++++--------
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 173dd4b58c71..3dec27e36417 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1257,17 +1257,11 @@ static int ceph_dir_fsync(struct file *file, loff_t start, loff_t end,
 
 		dout("dir_fsync %p wait on tid %llu (until %llu)\n",
 		     inode, req->r_tid, last_tid);
-		if (req->r_timeout) {
-			unsigned long time_left = wait_for_completion_timeout(
-					&req->r_safe_completion,
+		ret = !wait_for_completion_timeout(&req->r_safe_completion,
 					ceph_timeout_jiffies(req->r_timeout));
-			if (time_left > 0)
-				ret = 0;
-			else
-				ret = -EIO;  /* timed out */
-		} else {
-			wait_for_completion(&req->r_safe_completion);
-		}
+		if (ret)
+			ret = -EIO;  /* timed out */
+
 		ceph_mdsc_put_request(req);
 
 		spin_lock(&ci->i_unsafe_lock);
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 0b0e0a9a81c0..5be2d287a26c 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2266,16 +2266,18 @@ int ceph_mdsc_do_request(struct ceph_mds_client *mdsc,
 	/* wait */
 	mutex_unlock(&mdsc->mutex);
 	dout("do_request waiting\n");
-	if (req->r_timeout) {
-		err = (long)wait_for_completion_killable_timeout(
-					&req->r_completion,
-					ceph_timeout_jiffies(req->r_timeout));
-		if (err == 0)
-			err = -EIO;
-	} else if (req->r_wait_for_completion) {
+	if (!req->r_timeout && req->r_wait_for_completion) {
 		err = req->r_wait_for_completion(mdsc, req);
 	} else {
-		err = wait_for_completion_killable(&req->r_completion);
+		long timeleft = wait_for_completion_killable_timeout(
+					&req->r_completion,
+					ceph_timeout_jiffies(req->r_timeout));
+		if (timeleft > 0)
+			err = 0;
+		else if (!timeleft)
+			err = -EIO;  /* timed out */
+		else
+			err = timeleft;  /* killed */
 	}
 	dout("do_request waited, got %d\n", err);
 	mutex_lock(&mdsc->mutex);
-- 
1.9.3

Re: [PATCH 4/5] ceph: simplify two mount_timeout sites

[Alex Elder]

On 05/21/2015 07:35 AM, Ilya Dryomov wrote:
> No need to bifurcate wait now that we've got ceph_timeout_jiffies().

Looks good.

Reviewed-by: Alex Elder <elder@linaro.org>

> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
>  fs/ceph/dir.c        | 14 ++++----------
>  fs/ceph/mds_client.c | 18 ++++++++++--------
>  2 files changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 173dd4b58c71..3dec27e36417 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -1257,17 +1257,11 @@ static int ceph_dir_fsync(struct file *file, loff_t start, loff_t end,
>  
>  		dout("dir_fsync %p wait on tid %llu (until %llu)\n",
>  		     inode, req->r_tid, last_tid);
> -		if (req->r_timeout) {
> -			unsigned long time_left = wait_for_completion_timeout(
> -					&req->r_safe_completion,
> +		ret = !wait_for_completion_timeout(&req->r_safe_completion,
>  					ceph_timeout_jiffies(req->r_timeout));
> -			if (time_left > 0)
> -				ret = 0;
> -			else
> -				ret = -EIO;  /* timed out */
> -		} else {
> -			wait_for_completion(&req->r_safe_completion);
> -		}
> +		if (ret)
> +			ret = -EIO;  /* timed out */
> +
>  		ceph_mdsc_put_request(req);
>  
>  		spin_lock(&ci->i_unsafe_lock);
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 0b0e0a9a81c0..5be2d287a26c 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -2266,16 +2266,18 @@ int ceph_mdsc_do_request(struct ceph_mds_client *mdsc,
>  	/* wait */
>  	mutex_unlock(&mdsc->mutex);
>  	dout("do_request waiting\n");
> -	if (req->r_timeout) {
> -		err = (long)wait_for_completion_killable_timeout(
> -					&req->r_completion,
> -					ceph_timeout_jiffies(req->r_timeout));
> -		if (err == 0)
> -			err = -EIO;
> -	} else if (req->r_wait_for_completion) {
> +	if (!req->r_timeout && req->r_wait_for_completion) {
>  		err = req->r_wait_for_completion(mdsc, req);
>  	} else {
> -		err = wait_for_completion_killable(&req->r_completion);
> +		long timeleft = wait_for_completion_killable_timeout(
> +					&req->r_completion,
> +					ceph_timeout_jiffies(req->r_timeout));
> +		if (timeleft > 0)
> +			err = 0;
> +		else if (!timeleft)
> +			err = -EIO;  /* timed out */
> +		else
> +			err = timeleft;  /* killed */
>  	}
>  	dout("do_request waited, got %d\n", err);
>  	mutex_lock(&mdsc->mutex);
> 

Re: [PATCH 4/5] ceph: simplify two mount_timeout sites

[Yan, Zheng]

On Thu, May 21, 2015 at 8:35 PM, Ilya Dryomov <idryomov@gmail.com> wrote:
> No need to bifurcate wait now that we've got ceph_timeout_jiffies().
>
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
>  fs/ceph/dir.c        | 14 ++++----------
>  fs/ceph/mds_client.c | 18 ++++++++++--------
>  2 files changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 173dd4b58c71..3dec27e36417 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -1257,17 +1257,11 @@ static int ceph_dir_fsync(struct file *file, loff_t start, loff_t end,
>
>                 dout("dir_fsync %p wait on tid %llu (until %llu)\n",
>                      inode, req->r_tid, last_tid);
> -               if (req->r_timeout) {
> -                       unsigned long time_left = wait_for_completion_timeout(
> -                                       &req->r_safe_completion,
> +               ret = !wait_for_completion_timeout(&req->r_safe_completion,
>                                         ceph_timeout_jiffies(req->r_timeout));
> -                       if (time_left > 0)
> -                               ret = 0;
> -                       else
> -                               ret = -EIO;  /* timed out */
> -               } else {
> -                       wait_for_completion(&req->r_safe_completion);
> -               }
> +               if (ret)
> +                       ret = -EIO;  /* timed out */
> +
>                 ceph_mdsc_put_request(req);
>
>                 spin_lock(&ci->i_unsafe_lock);
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 0b0e0a9a81c0..5be2d287a26c 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -2266,16 +2266,18 @@ int ceph_mdsc_do_request(struct ceph_mds_client *mdsc,
>         /* wait */
>         mutex_unlock(&mdsc->mutex);
>         dout("do_request waiting\n");
> -       if (req->r_timeout) {
> -               err = (long)wait_for_completion_killable_timeout(
> -                                       &req->r_completion,
> -                                       ceph_timeout_jiffies(req->r_timeout));
> -               if (err == 0)
> -                       err = -EIO;
> -       } else if (req->r_wait_for_completion) {
> +       if (!req->r_timeout && req->r_wait_for_completion) {
>                 err = req->r_wait_for_completion(mdsc, req);
>         } else {
> -               err = wait_for_completion_killable(&req->r_completion);
> +               long timeleft = wait_for_completion_killable_timeout(
> +                                       &req->r_completion,
> +                                       ceph_timeout_jiffies(req->r_timeout));
> +               if (timeleft > 0)
> +                       err = 0;
> +               else if (!timeleft)
> +                       err = -EIO;  /* timed out */
> +               else
> +                       err = timeleft;  /* killed */
>         }
>         dout("do_request waited, got %d\n", err);
>         mutex_lock(&mdsc->mutex);
> --
> 1.9.3
>

Reviewed-by: Yan, Zheng <zyan@redhat.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

[PATCH v2 5/5] rbd: timeout watch teardown on unmap with mount_timeout

[Ilya Dryomov]

As part of unmap sequence, kernel client has to talk to the OSDs to
teardown watch on the header object.  If none of the OSDs are available
it would hang forever, until interrupted by a signal - when that
happens we follow through with the rest of unmap procedure (i.e.
unregister the device and put all the data structures) and the unmap is
still considired successful (rbd cli tool exits with 0).  The watch on
the userspace side should eventually timeout so that's fine.

This isn't very nice, because various userspace tools (pacemaker rbd
resource agent, for example) then have to worry about setting up their
own timeouts.  Timeout it with mount_timeout (60 seconds by default).

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
Reviewed-by: Sage Weil <sage@redhat.com>
---
 drivers/block/rbd.c | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 992683b6b299..89fe8a4bc02e 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1563,22 +1563,39 @@ static void rbd_obj_request_end(struct rbd_obj_request *obj_request)
 /*
  * Wait for an object request to complete.  If interrupted, cancel the
  * underlying osd request.
+ *
+ * @timeout: in jiffies, 0 means "wait forever"
  */
-static int rbd_obj_request_wait(struct rbd_obj_request *obj_request)
+static int __rbd_obj_request_wait(struct rbd_obj_request *obj_request,
+				  unsigned long timeout)
 {
-	int ret;
+	long ret;
 
 	dout("%s %p\n", __func__, obj_request);
-
-	ret = wait_for_completion_interruptible(&obj_request->completion);
-	if (ret < 0) {
-		dout("%s %p interrupted\n", __func__, obj_request);
+	ret = wait_for_completion_interruptible_timeout(
+					&obj_request->completion,
+					ceph_timeout_jiffies(timeout));
+	if (ret <= 0) {
+		if (ret == 0)
+			ret = -ETIMEDOUT;
 		rbd_obj_request_end(obj_request);
-		return ret;
+	} else {
+		ret = 0;
 	}
 
-	dout("%s %p done\n", __func__, obj_request);
-	return 0;
+	dout("%s %p ret %d\n", __func__, obj_request, (int)ret);
+	return ret;
+}
+
+static int rbd_obj_request_wait(struct rbd_obj_request *obj_request)
+{
+	return __rbd_obj_request_wait(obj_request, 0);
+}
+
+static int rbd_obj_request_wait_timeout(struct rbd_obj_request *obj_request,
+					unsigned long timeout)
+{
+	return __rbd_obj_request_wait(obj_request, timeout);
 }
 
 static void rbd_img_request_complete(struct rbd_img_request *img_request)
@@ -3122,6 +3139,7 @@ static struct rbd_obj_request *rbd_obj_watch_request_helper(
 						bool watch)
 {
 	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
+	struct ceph_options *opts = osdc->client->options;
 	struct rbd_obj_request *obj_request;
 	int ret;
 
@@ -3148,7 +3166,7 @@ static struct rbd_obj_request *rbd_obj_watch_request_helper(
 	if (ret)
 		goto out;
 
-	ret = rbd_obj_request_wait(obj_request);
+	ret = rbd_obj_request_wait_timeout(obj_request, opts->mount_timeout);
 	if (ret)
 		goto out;
 
-- 
1.9.3

Re: [PATCH v2 5/5] rbd: timeout watch teardown on unmap with mount_timeout

[Alex Elder]

On 05/21/2015 07:35 AM, Ilya Dryomov wrote:
> As part of unmap sequence, kernel client has to talk to the OSDs to
> teardown watch on the header object.  If none of the OSDs are available
> it would hang forever, until interrupted by a signal - when that
> happens we follow through with the rest of unmap procedure (i.e.
> unregister the device and put all the data structures) and the unmap is
> still considired successful (rbd cli tool exits with 0).  The watch on
> the userspace side should eventually timeout so that's fine.
> 
> This isn't very nice, because various userspace tools (pacemaker rbd
> resource agent, for example) then have to worry about setting up their
> own timeouts.  Timeout it with mount_timeout (60 seconds by default).
> 
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> Reviewed-by: Sage Weil <sage@redhat.com>

You can now add:

Reviewed-by: Alex Elder <elder@linaro.org>

> ---
>  drivers/block/rbd.c | 38 ++++++++++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 992683b6b299..89fe8a4bc02e 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -1563,22 +1563,39 @@ static void rbd_obj_request_end(struct rbd_obj_request *obj_request)
>  /*
>   * Wait for an object request to complete.  If interrupted, cancel the
>   * underlying osd request.
> + *
> + * @timeout: in jiffies, 0 means "wait forever"
>   */
> -static int rbd_obj_request_wait(struct rbd_obj_request *obj_request)
> +static int __rbd_obj_request_wait(struct rbd_obj_request *obj_request,
> +				  unsigned long timeout)
>  {
> -	int ret;
> +	long ret;
>  
>  	dout("%s %p\n", __func__, obj_request);
> -
> -	ret = wait_for_completion_interruptible(&obj_request->completion);
> -	if (ret < 0) {
> -		dout("%s %p interrupted\n", __func__, obj_request);
> +	ret = wait_for_completion_interruptible_timeout(
> +					&obj_request->completion,
> +					ceph_timeout_jiffies(timeout));
> +	if (ret <= 0) {
> +		if (ret == 0)
> +			ret = -ETIMEDOUT;
>  		rbd_obj_request_end(obj_request);
> -		return ret;
> +	} else {
> +		ret = 0;
>  	}
>  
> -	dout("%s %p done\n", __func__, obj_request);
> -	return 0;
> +	dout("%s %p ret %d\n", __func__, obj_request, (int)ret);
> +	return ret;
> +}
> +
> +static int rbd_obj_request_wait(struct rbd_obj_request *obj_request)
> +{
> +	return __rbd_obj_request_wait(obj_request, 0);
> +}
> +
> +static int rbd_obj_request_wait_timeout(struct rbd_obj_request *obj_request,
> +					unsigned long timeout)
> +{
> +	return __rbd_obj_request_wait(obj_request, timeout);
>  }
>  
>  static void rbd_img_request_complete(struct rbd_img_request *img_request)
> @@ -3122,6 +3139,7 @@ static struct rbd_obj_request *rbd_obj_watch_request_helper(
>  						bool watch)
>  {
>  	struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
> +	struct ceph_options *opts = osdc->client->options;
>  	struct rbd_obj_request *obj_request;
>  	int ret;
>  
> @@ -3148,7 +3166,7 @@ static struct rbd_obj_request *rbd_obj_watch_request_helper(
>  	if (ret)
>  		goto out;
>  
> -	ret = rbd_obj_request_wait(obj_request);
> +	ret = rbd_obj_request_wait_timeout(obj_request, opts->mount_timeout);
>  	if (ret)
>  		goto out;
>  
>