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

Message ID 1432211706-10473-4-git-send-email-idryomov@gmail.com
State New
Headers show

Commit Message

Ilya Dryomov May 21, 2015, 12:35 p.m. UTC
- 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(-)

Comments

Alex Elder May 21, 2015, 1:29 p.m. UTC | #1
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) {
> 

--
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
Ilya Dryomov May 25, 2015, 10:38 a.m. UTC | #2
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
--
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
Alex Elder May 25, 2015, 3:40 p.m. UTC | #3
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
> 

--
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
diff mbox

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) {