| Message ID | 1432211706-10473-4-git-send-email-idryomov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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) {
- 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(-)