| Message ID | 1522074175-29449-1-git-send-email-dongsheng.yang@easystack.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 26, 2018 at 4:22 PM, Dongsheng Yang <dongsheng.yang@easystack.cn> wrote: > currently, the rbd_wait_state_locked() will wait forever if we > can't get our state locked. Example: > > rbd map --exclusive test1 --> /dev/rbd0 > rbd map test1 --> /dev/rbd1 > dd if=/dev/zero of=/dev/rbd1 bs=1M count=1 --> IO blocked > > To avoid this problem, this patch introduce a timeout design > in rbd_wait_state_locked(). Then rbd_wait_state_locked() will > return error when we reach a timeout. > > This patch allow user to set the lock_timeout in rbd mapping. > > Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn> > --- > drivers/block/rbd.c | 35 +++++++++++++++++++++++++++++++---- > 1 file changed, 31 insertions(+), 4 deletions(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index f1d9e60..58edead 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -726,6 +726,7 @@ static struct rbd_client *rbd_client_find(struct ceph_options *ceph_opts) > */ > enum { > Opt_queue_depth, > + Opt_lock_timeout, > Opt_last_int, > /* int args above */ > Opt_last_string, > @@ -739,6 +740,7 @@ enum { > > static match_table_t rbd_opts_tokens = { > {Opt_queue_depth, "queue_depth=%d"}, > + {Opt_lock_timeout, "lock_timeout=%d"}, > /* int args above */ > /* string args above */ > {Opt_read_only, "read_only"}, > @@ -752,6 +754,7 @@ enum { > > struct rbd_options { > int queue_depth; > + unsigned long lock_timeout; > bool read_only; > bool lock_on_read; > bool exclusive; > @@ -761,6 +764,7 @@ struct rbd_options { > #define RBD_READ_ONLY_DEFAULT false > #define RBD_LOCK_ON_READ_DEFAULT false > #define RBD_EXCLUSIVE_DEFAULT false > +#define RBD_LOCK_TIMEOUT_DEFAULT 0 /* no timeout */ > > static int parse_rbd_opts_token(char *c, void *private) > { > @@ -790,6 +794,14 @@ static int parse_rbd_opts_token(char *c, void *private) > } > rbd_opts->queue_depth = intval; > break; > + case Opt_lock_timeout: > + /* 0 is "wait forever" (i.e. infinite timeout) */ > + if (intval < 0 || intval > INT_MAX / 1000) { > + pr_err("lock_timeout out of range\n"); > + return -EINVAL; > + } > + rbd_opts->lock_timeout = msecs_to_jiffies(intval * 1000); > + break; > case Opt_read_only: > rbd_opts->read_only = true; > break; > @@ -3494,8 +3506,9 @@ static int rbd_obj_method_sync(struct rbd_device *rbd_dev, > /* > * lock_rwsem must be held for read > */ > -static void rbd_wait_state_locked(struct rbd_device *rbd_dev) > +static int rbd_wait_state_locked(struct rbd_device *rbd_dev) > { > + unsigned long timeout = 0; > DEFINE_WAIT(wait); > > do { > @@ -3508,12 +3521,19 @@ static void rbd_wait_state_locked(struct rbd_device *rbd_dev) > prepare_to_wait_exclusive(&rbd_dev->lock_waitq, &wait, > TASK_UNINTERRUPTIBLE); > up_read(&rbd_dev->lock_rwsem); > - schedule(); > + timeout = schedule_timeout(ceph_timeout_jiffies( > + rbd_dev->opts->lock_timeout)); > down_read(&rbd_dev->lock_rwsem); > + if (!timeout) { > + finish_wait(&rbd_dev->lock_waitq, &wait); > + rbd_warn(rbd_dev, "timed out in waiting for lock"); > + return -ETIMEDOUT; > + } > } while (rbd_dev->lock_state != RBD_LOCK_STATE_LOCKED && > !test_bit(RBD_DEV_FLAG_BLACKLISTED, &rbd_dev->flags)); > > finish_wait(&rbd_dev->lock_waitq, &wait); > + return 0; > } > > static void rbd_queue_workfn(struct work_struct *work) > @@ -3606,7 +3626,9 @@ static void rbd_queue_workfn(struct work_struct *work) > result = -EROFS; > goto err_unlock; > } > - rbd_wait_state_locked(rbd_dev); > + result = rbd_wait_state_locked(rbd_dev); > + if (result) > + goto err_unlock; > } > if (test_bit(RBD_DEV_FLAG_BLACKLISTED, &rbd_dev->flags)) { > result = -EBLACKLISTED; > @@ -5139,6 +5161,7 @@ static int rbd_add_parse_args(const char *buf, > > rbd_opts->read_only = RBD_READ_ONLY_DEFAULT; > rbd_opts->queue_depth = RBD_QUEUE_DEPTH_DEFAULT; > + rbd_opts->lock_timeout = RBD_LOCK_TIMEOUT_DEFAULT; > rbd_opts->lock_on_read = RBD_LOCK_ON_READ_DEFAULT; > rbd_opts->exclusive = RBD_EXCLUSIVE_DEFAULT; > > @@ -5209,6 +5232,8 @@ static void rbd_dev_image_unlock(struct rbd_device *rbd_dev) > > static int rbd_add_acquire_lock(struct rbd_device *rbd_dev) > { > + int ret = 0; > + > if (!(rbd_dev->header.features & RBD_FEATURE_EXCLUSIVE_LOCK)) { > rbd_warn(rbd_dev, "exclusive-lock feature is not enabled"); > return -EINVAL; > @@ -5216,8 +5241,10 @@ static int rbd_add_acquire_lock(struct rbd_device *rbd_dev) > > /* FIXME: "rbd map --exclusive" should be in interruptible */ > down_read(&rbd_dev->lock_rwsem); > - rbd_wait_state_locked(rbd_dev); > + ret = rbd_wait_state_locked(rbd_dev); > up_read(&rbd_dev->lock_rwsem); > + if (ret) > + return ret; > if (test_bit(RBD_DEV_FLAG_BLACKLISTED, &rbd_dev->flags)) { > rbd_warn(rbd_dev, "failed to acquire exclusive lock"); > return -EROFS; Looks good to me. I don't like that rbd_wait_state_locked() has kind of two return values now: the error code and RBD_DEV_FLAG_BLACKLISTED flag which needs to be tested afterwards, but I'll fix it myself. 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 03/26/2018 10:53 PM, Ilya Dryomov wrote: > On Mon, Mar 26, 2018 at 4:22 PM, Dongsheng Yang > <dongsheng.yang@easystack.cn> wrote: >> currently, the rbd_wait_state_locked() will wait forever if we >> can't get our state locked. Example: >> >> rbd map --exclusive test1 --> /dev/rbd0 >> rbd map test1 --> /dev/rbd1 >> dd if=/dev/zero of=/dev/rbd1 bs=1M count=1 --> IO blocked >> >> To avoid this problem, this patch introduce a timeout design >> in rbd_wait_state_locked(). Then rbd_wait_state_locked() will >> return error when we reach a timeout. >> >> This patch allow user to set the lock_timeout in rbd mapping. >> >> Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn> >> --- >> drivers/block/rbd.c | 35 +++++++++++++++++++++++++++++++---- >> 1 file changed, 31 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c >> index f1d9e60..58edead 100644 >> --- a/drivers/block/rbd.c >> +++ b/drivers/block/rbd.c >> @@ -726,6 +726,7 @@ static struct rbd_client *rbd_client_find(struct ceph_options *ceph_opts) >> */ >> enum { >> Opt_queue_depth, >> + Opt_lock_timeout, >> Opt_last_int, >> /* int args above */ >> Opt_last_string, >> @@ -739,6 +740,7 @@ enum { >> >> static match_table_t rbd_opts_tokens = { >> {Opt_queue_depth, "queue_depth=%d"}, >> + {Opt_lock_timeout, "lock_timeout=%d"}, >> /* int args above */ >> /* string args above */ >> {Opt_read_only, "read_only"}, >> @@ -752,6 +754,7 @@ enum { >> >> struct rbd_options { >> int queue_depth; >> + unsigned long lock_timeout; >> bool read_only; >> bool lock_on_read; >> bool exclusive; >> @@ -761,6 +764,7 @@ struct rbd_options { >> #define RBD_READ_ONLY_DEFAULT false >> #define RBD_LOCK_ON_READ_DEFAULT false >> #define RBD_EXCLUSIVE_DEFAULT false >> +#define RBD_LOCK_TIMEOUT_DEFAULT 0 /* no timeout */ >> >> static int parse_rbd_opts_token(char *c, void *private) >> { >> @@ -790,6 +794,14 @@ static int parse_rbd_opts_token(char *c, void *private) >> } >> rbd_opts->queue_depth = intval; >> break; >> + case Opt_lock_timeout: >> + /* 0 is "wait forever" (i.e. infinite timeout) */ >> + if (intval < 0 || intval > INT_MAX / 1000) { >> + pr_err("lock_timeout out of range\n"); >> + return -EINVAL; >> + } >> + rbd_opts->lock_timeout = msecs_to_jiffies(intval * 1000); >> + break; >> case Opt_read_only: >> rbd_opts->read_only = true; >> break; >> @@ -3494,8 +3506,9 @@ static int rbd_obj_method_sync(struct rbd_device *rbd_dev, >> /* >> * lock_rwsem must be held for read >> */ >> -static void rbd_wait_state_locked(struct rbd_device *rbd_dev) >> +static int rbd_wait_state_locked(struct rbd_device *rbd_dev) >> { >> + unsigned long timeout = 0; >> DEFINE_WAIT(wait); >> >> do { >> @@ -3508,12 +3521,19 @@ static void rbd_wait_state_locked(struct rbd_device *rbd_dev) >> prepare_to_wait_exclusive(&rbd_dev->lock_waitq, &wait, >> TASK_UNINTERRUPTIBLE); >> up_read(&rbd_dev->lock_rwsem); >> - schedule(); >> + timeout = schedule_timeout(ceph_timeout_jiffies( >> + rbd_dev->opts->lock_timeout)); >> down_read(&rbd_dev->lock_rwsem); >> + if (!timeout) { >> + finish_wait(&rbd_dev->lock_waitq, &wait); >> + rbd_warn(rbd_dev, "timed out in waiting for lock"); >> + return -ETIMEDOUT; >> + } >> } while (rbd_dev->lock_state != RBD_LOCK_STATE_LOCKED && >> !test_bit(RBD_DEV_FLAG_BLACKLISTED, &rbd_dev->flags)); >> >> finish_wait(&rbd_dev->lock_waitq, &wait); >> + return 0; >> } >> >> static void rbd_queue_workfn(struct work_struct *work) >> @@ -3606,7 +3626,9 @@ static void rbd_queue_workfn(struct work_struct *work) >> result = -EROFS; >> goto err_unlock; >> } >> - rbd_wait_state_locked(rbd_dev); >> + result = rbd_wait_state_locked(rbd_dev); >> + if (result) >> + goto err_unlock; >> } >> if (test_bit(RBD_DEV_FLAG_BLACKLISTED, &rbd_dev->flags)) { >> result = -EBLACKLISTED; >> @@ -5139,6 +5161,7 @@ static int rbd_add_parse_args(const char *buf, >> >> rbd_opts->read_only = RBD_READ_ONLY_DEFAULT; >> rbd_opts->queue_depth = RBD_QUEUE_DEPTH_DEFAULT; >> + rbd_opts->lock_timeout = RBD_LOCK_TIMEOUT_DEFAULT; >> rbd_opts->lock_on_read = RBD_LOCK_ON_READ_DEFAULT; >> rbd_opts->exclusive = RBD_EXCLUSIVE_DEFAULT; >> >> @@ -5209,6 +5232,8 @@ static void rbd_dev_image_unlock(struct rbd_device *rbd_dev) >> >> static int rbd_add_acquire_lock(struct rbd_device *rbd_dev) >> { >> + int ret = 0; >> + >> if (!(rbd_dev->header.features & RBD_FEATURE_EXCLUSIVE_LOCK)) { >> rbd_warn(rbd_dev, "exclusive-lock feature is not enabled"); >> return -EINVAL; >> @@ -5216,8 +5241,10 @@ static int rbd_add_acquire_lock(struct rbd_device *rbd_dev) >> >> /* FIXME: "rbd map --exclusive" should be in interruptible */ >> down_read(&rbd_dev->lock_rwsem); >> - rbd_wait_state_locked(rbd_dev); >> + ret = rbd_wait_state_locked(rbd_dev); >> up_read(&rbd_dev->lock_rwsem); >> + if (ret) >> + return ret; >> if (test_bit(RBD_DEV_FLAG_BLACKLISTED, &rbd_dev->flags)) { >> rbd_warn(rbd_dev, "failed to acquire exclusive lock"); >> return -EROFS; > Looks good to me. > > I don't like that rbd_wait_state_locked() has kind of two return values > now: the error code and RBD_DEV_FLAG_BLACKLISTED flag which needs to be > tested afterwards, but I'll fix it myself. That's okey, thanx. Yang > > 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 03/26/2018 10:53 PM, Ilya Dryomov wrote: > On Mon, Mar 26, 2018 at 4:22 PM, Dongsheng Yang > <dongsheng.yang@easystack.cn> wrote: ... > Looks good to me. > > I don't like that rbd_wait_state_locked() has kind of two return values > now: the error code and RBD_DEV_FLAG_BLACKLISTED flag which needs to be > tested afterwards, but I'll fix it myself. Hi Ilya, will this patch be in 3.17 for-linus pull? Because I did not find it in ceph-client/testing, I want to know your plan. thanx Yang > > 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 > -- 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 Wed, Mar 28, 2018 at 9:49 AM, Dongsheng Yang <dongsheng.yang@easystack.cn> wrote: > > > On 03/26/2018 10:53 PM, Ilya Dryomov wrote: >> >> On Mon, Mar 26, 2018 at 4:22 PM, Dongsheng Yang >> <dongsheng.yang@easystack.cn> wrote: > > ... >> >> Looks good to me. >> >> I don't like that rbd_wait_state_locked() has kind of two return values >> now: the error code and RBD_DEV_FLAG_BLACKLISTED flag which needs to be >> tested afterwards, but I'll fix it myself. > > > Hi Ilya, > will this patch be in 3.17 for-linus pull? Because I did not find it in > ceph-client/testing, I want to know your plan. Yes, lock_timeout will be pushed for 4.17. 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 03/28/2018 04:11 PM, Ilya Dryomov wrote: > On Wed, Mar 28, 2018 at 9:49 AM, Dongsheng Yang > <dongsheng.yang@easystack.cn> wrote: >> >> On 03/26/2018 10:53 PM, Ilya Dryomov wrote: >>> On Mon, Mar 26, 2018 at 4:22 PM, Dongsheng Yang >>> <dongsheng.yang@easystack.cn> wrote: >> ... >>> Looks good to me. >>> >>> I don't like that rbd_wait_state_locked() has kind of two return values >>> now: the error code and RBD_DEV_FLAG_BLACKLISTED flag which needs to be >>> tested afterwards, but I'll fix it myself. >> >> Hi Ilya, >> will this patch be in 3.17 for-linus pull? Because I did not find it in >> ceph-client/testing, I want to know your plan. > Yes, lock_timeout will be pushed for 4.17. Great, thanx > > 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 > -- 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 Mon, Mar 26, 2018 at 4:53 PM, Ilya Dryomov <idryomov@gmail.com> wrote: > On Mon, Mar 26, 2018 at 4:22 PM, Dongsheng Yang > <dongsheng.yang@easystack.cn> wrote: >> currently, the rbd_wait_state_locked() will wait forever if we >> can't get our state locked. Example: >> >> rbd map --exclusive test1 --> /dev/rbd0 >> rbd map test1 --> /dev/rbd1 >> dd if=/dev/zero of=/dev/rbd1 bs=1M count=1 --> IO blocked >> >> To avoid this problem, this patch introduce a timeout design >> in rbd_wait_state_locked(). Then rbd_wait_state_locked() will >> return error when we reach a timeout. >> >> This patch allow user to set the lock_timeout in rbd mapping. >> >> Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn> >> --- >> drivers/block/rbd.c | 35 +++++++++++++++++++++++++++++++---- >> 1 file changed, 31 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c >> index f1d9e60..58edead 100644 >> --- a/drivers/block/rbd.c >> +++ b/drivers/block/rbd.c >> @@ -726,6 +726,7 @@ static struct rbd_client *rbd_client_find(struct ceph_options *ceph_opts) >> */ >> enum { >> Opt_queue_depth, >> + Opt_lock_timeout, >> Opt_last_int, >> /* int args above */ >> Opt_last_string, >> @@ -739,6 +740,7 @@ enum { >> >> static match_table_t rbd_opts_tokens = { >> {Opt_queue_depth, "queue_depth=%d"}, >> + {Opt_lock_timeout, "lock_timeout=%d"}, >> /* int args above */ >> /* string args above */ >> {Opt_read_only, "read_only"}, >> @@ -752,6 +754,7 @@ enum { >> >> struct rbd_options { >> int queue_depth; >> + unsigned long lock_timeout; >> bool read_only; >> bool lock_on_read; >> bool exclusive; >> @@ -761,6 +764,7 @@ struct rbd_options { >> #define RBD_READ_ONLY_DEFAULT false >> #define RBD_LOCK_ON_READ_DEFAULT false >> #define RBD_EXCLUSIVE_DEFAULT false >> +#define RBD_LOCK_TIMEOUT_DEFAULT 0 /* no timeout */ >> >> static int parse_rbd_opts_token(char *c, void *private) >> { >> @@ -790,6 +794,14 @@ static int parse_rbd_opts_token(char *c, void *private) >> } >> rbd_opts->queue_depth = intval; >> break; >> + case Opt_lock_timeout: >> + /* 0 is "wait forever" (i.e. infinite timeout) */ >> + if (intval < 0 || intval > INT_MAX / 1000) { >> + pr_err("lock_timeout out of range\n"); >> + return -EINVAL; >> + } >> + rbd_opts->lock_timeout = msecs_to_jiffies(intval * 1000); >> + break; >> case Opt_read_only: >> rbd_opts->read_only = true; >> break; >> @@ -3494,8 +3506,9 @@ static int rbd_obj_method_sync(struct rbd_device *rbd_dev, >> /* >> * lock_rwsem must be held for read >> */ >> -static void rbd_wait_state_locked(struct rbd_device *rbd_dev) >> +static int rbd_wait_state_locked(struct rbd_device *rbd_dev) >> { >> + unsigned long timeout = 0; >> DEFINE_WAIT(wait); >> >> do { >> @@ -3508,12 +3521,19 @@ static void rbd_wait_state_locked(struct rbd_device *rbd_dev) >> prepare_to_wait_exclusive(&rbd_dev->lock_waitq, &wait, >> TASK_UNINTERRUPTIBLE); >> up_read(&rbd_dev->lock_rwsem); >> - schedule(); >> + timeout = schedule_timeout(ceph_timeout_jiffies( >> + rbd_dev->opts->lock_timeout)); >> down_read(&rbd_dev->lock_rwsem); >> + if (!timeout) { >> + finish_wait(&rbd_dev->lock_waitq, &wait); >> + rbd_warn(rbd_dev, "timed out in waiting for lock"); >> + return -ETIMEDOUT; >> + } >> } while (rbd_dev->lock_state != RBD_LOCK_STATE_LOCKED && >> !test_bit(RBD_DEV_FLAG_BLACKLISTED, &rbd_dev->flags)); >> >> finish_wait(&rbd_dev->lock_waitq, &wait); >> + return 0; >> } >> >> static void rbd_queue_workfn(struct work_struct *work) >> @@ -3606,7 +3626,9 @@ static void rbd_queue_workfn(struct work_struct *work) >> result = -EROFS; >> goto err_unlock; >> } >> - rbd_wait_state_locked(rbd_dev); >> + result = rbd_wait_state_locked(rbd_dev); >> + if (result) >> + goto err_unlock; >> } >> if (test_bit(RBD_DEV_FLAG_BLACKLISTED, &rbd_dev->flags)) { >> result = -EBLACKLISTED; >> @@ -5139,6 +5161,7 @@ static int rbd_add_parse_args(const char *buf, >> >> rbd_opts->read_only = RBD_READ_ONLY_DEFAULT; >> rbd_opts->queue_depth = RBD_QUEUE_DEPTH_DEFAULT; >> + rbd_opts->lock_timeout = RBD_LOCK_TIMEOUT_DEFAULT; >> rbd_opts->lock_on_read = RBD_LOCK_ON_READ_DEFAULT; >> rbd_opts->exclusive = RBD_EXCLUSIVE_DEFAULT; >> >> @@ -5209,6 +5232,8 @@ static void rbd_dev_image_unlock(struct rbd_device *rbd_dev) >> >> static int rbd_add_acquire_lock(struct rbd_device *rbd_dev) >> { >> + int ret = 0; >> + >> if (!(rbd_dev->header.features & RBD_FEATURE_EXCLUSIVE_LOCK)) { >> rbd_warn(rbd_dev, "exclusive-lock feature is not enabled"); >> return -EINVAL; >> @@ -5216,8 +5241,10 @@ static int rbd_add_acquire_lock(struct rbd_device *rbd_dev) >> >> /* FIXME: "rbd map --exclusive" should be in interruptible */ >> down_read(&rbd_dev->lock_rwsem); >> - rbd_wait_state_locked(rbd_dev); >> + ret = rbd_wait_state_locked(rbd_dev); >> up_read(&rbd_dev->lock_rwsem); >> + if (ret) >> + return ret; >> if (test_bit(RBD_DEV_FLAG_BLACKLISTED, &rbd_dev->flags)) { >> rbd_warn(rbd_dev, "failed to acquire exclusive lock"); >> return -EROFS; > > Looks good to me. > > I don't like that rbd_wait_state_locked() has kind of two return values > now: the error code and RBD_DEV_FLAG_BLACKLISTED flag which needs to be > tested afterwards, but I'll fix it myself. Hi Dongsheng, I pushed the updated patch to testing, please take a look: https://github.com/ceph/ceph-client/commit/4eb7d6b3554a7c2f39c588a4c636d8eef6784665 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 04/09/2018 08:35 PM, Ilya Dryomov wrote: > On Mon, Mar 26, 2018 at 4:53 PM, Ilya Dryomov <idryomov@gmail.com> wrote: >> On Mon, Mar 26, 2018 at 4:22 PM, Dongsheng Yang >> <dongsheng.yang@easystack.cn> wrote: >>> currently, the rbd_wait_state_locked() will wait forever if we >>> can't get our state locked. Example: ... >>> Looks good to me. >>> >>> I don't like that rbd_wait_state_locked() has kind of two return values >>> now: the error code and RBD_DEV_FLAG_BLACKLISTED flag which needs to be >>> tested afterwards, but I'll fix it myself. > Hi Dongsheng, > > I pushed the updated patch to testing, please take a look: > > https://github.com/ceph/ceph-client/commit/4eb7d6b3554a7c2f39c588a4c636d8eef6784665 > > Thanks, Thanx Ilya, that looks good to me. BTW, I took a look at the preparation commit: "rbd: refactor rbd_wait_state_locked() " bad477c8ce9748dbd762eda93e3c2281c8d2eabf That looks good to me too. But could you add a comment for the parameter of may_queue. may_queue means whether we will try to acquire lock. When the lock_state is not RBD_LOCK_STATE_LOCKED, if may_queue is true, we will queue a lock work and wait lock_state to be locked; if may_queue is false, we will return -EROFS immediately. Do I understand it correctly? Thanx Dongsheng > > 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 Tue, Apr 10, 2018 at 5:14 AM, Dongsheng Yang <dongsheng.yang@easystack.cn> wrote: > > > On 04/09/2018 08:35 PM, Ilya Dryomov wrote: >> >> On Mon, Mar 26, 2018 at 4:53 PM, Ilya Dryomov <idryomov@gmail.com> wrote: >>> >>> On Mon, Mar 26, 2018 at 4:22 PM, Dongsheng Yang >>> <dongsheng.yang@easystack.cn> wrote: >>>> >>>> currently, the rbd_wait_state_locked() will wait forever if we >>>> can't get our state locked. Example: > > ... >>>> >>>> Looks good to me. >>>> >>>> I don't like that rbd_wait_state_locked() has kind of two return values >>>> now: the error code and RBD_DEV_FLAG_BLACKLISTED flag which needs to be >>>> tested afterwards, but I'll fix it myself. >> >> Hi Dongsheng, >> >> I pushed the updated patch to testing, please take a look: >> >> >> https://github.com/ceph/ceph-client/commit/4eb7d6b3554a7c2f39c588a4c636d8eef6784665 >> >> Thanks, > > > Thanx Ilya, that looks good to me. > > BTW, I took a look at the preparation commit: "rbd: refactor > rbd_wait_state_locked() " > bad477c8ce9748dbd762eda93e3c2281c8d2eabf > > That looks good to me too. But could you add a comment for the parameter of > may_queue. > > may_queue means whether we will try to acquire lock. When the lock_state is > not RBD_LOCK_STATE_LOCKED, > if may_queue is true, we will queue a lock work and wait lock_state to be > locked; > if may_queue is false, we will return -EROFS immediately. > > Do I understand it correctly? Yes. I have renamed it to may_acquire for clarity. 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 04/10/2018 09:54 PM, Ilya Dryomov wrote: > On Tue, Apr 10, 2018 at 5:14 AM, Dongsheng Yang > <dongsheng.yang@easystack.cn> wrote: >> >> On 04/09/2018 08:35 PM, Ilya Dryomov wrote: >>> On Mon, Mar 26, 2018 at 4:53 PM, Ilya Dryomov <idryomov@gmail.com> wrote: >>>> On Mon, Mar 26, 2018 at 4:22 PM, Dongsheng Yang >>>> <dongsheng.yang@easystack.cn> wrote: >>>>> currently, the rbd_wait_state_locked() will wait forever if we >>>>> can't get our state locked. Example: >> ... >>>>> Looks good to me. >>>>> >>>>> I don't like that rbd_wait_state_locked() has kind of two return values >>>>> now: the error code and RBD_DEV_FLAG_BLACKLISTED flag which needs to be >>>>> tested afterwards, but I'll fix it myself. >>> Hi Dongsheng, >>> >>> I pushed the updated patch to testing, please take a look: >>> >>> >>> https://github.com/ceph/ceph-client/commit/4eb7d6b3554a7c2f39c588a4c636d8eef6784665 >>> >>> Thanks, >> >> Thanx Ilya, that looks good to me. >> >> BTW, I took a look at the preparation commit: "rbd: refactor >> rbd_wait_state_locked() " >> bad477c8ce9748dbd762eda93e3c2281c8d2eabf >> >> That looks good to me too. But could you add a comment for the parameter of >> may_queue. >> >> may_queue means whether we will try to acquire lock. When the lock_state is >> not RBD_LOCK_STATE_LOCKED, >> if may_queue is true, we will queue a lock work and wait lock_state to be >> locked; >> if may_queue is false, we will return -EROFS immediately. >> >> Do I understand it correctly? > Yes. I have renamed it to may_acquire for clarity. Okey, sounds better. Thanx Dongsheng > > 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/drivers/block/rbd.c b/drivers/block/rbd.c index f1d9e60..58edead 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -726,6 +726,7 @@ static struct rbd_client *rbd_client_find(struct ceph_options *ceph_opts) */ enum { Opt_queue_depth, + Opt_lock_timeout, Opt_last_int, /* int args above */ Opt_last_string, @@ -739,6 +740,7 @@ enum { static match_table_t rbd_opts_tokens = { {Opt_queue_depth, "queue_depth=%d"}, + {Opt_lock_timeout, "lock_timeout=%d"}, /* int args above */ /* string args above */ {Opt_read_only, "read_only"}, @@ -752,6 +754,7 @@ enum { struct rbd_options { int queue_depth; + unsigned long lock_timeout; bool read_only; bool lock_on_read; bool exclusive; @@ -761,6 +764,7 @@ struct rbd_options { #define RBD_READ_ONLY_DEFAULT false #define RBD_LOCK_ON_READ_DEFAULT false #define RBD_EXCLUSIVE_DEFAULT false +#define RBD_LOCK_TIMEOUT_DEFAULT 0 /* no timeout */ static int parse_rbd_opts_token(char *c, void *private) { @@ -790,6 +794,14 @@ static int parse_rbd_opts_token(char *c, void *private) } rbd_opts->queue_depth = intval; break; + case Opt_lock_timeout: + /* 0 is "wait forever" (i.e. infinite timeout) */ + if (intval < 0 || intval > INT_MAX / 1000) { + pr_err("lock_timeout out of range\n"); + return -EINVAL; + } + rbd_opts->lock_timeout = msecs_to_jiffies(intval * 1000); + break; case Opt_read_only: rbd_opts->read_only = true; break; @@ -3494,8 +3506,9 @@ static int rbd_obj_method_sync(struct rbd_device *rbd_dev, /* * lock_rwsem must be held for read */ -static void rbd_wait_state_locked(struct rbd_device *rbd_dev) +static int rbd_wait_state_locked(struct rbd_device *rbd_dev) { + unsigned long timeout = 0; DEFINE_WAIT(wait); do { @@ -3508,12 +3521,19 @@ static void rbd_wait_state_locked(struct rbd_device *rbd_dev) prepare_to_wait_exclusive(&rbd_dev->lock_waitq, &wait, TASK_UNINTERRUPTIBLE); up_read(&rbd_dev->lock_rwsem); - schedule(); + timeout = schedule_timeout(ceph_timeout_jiffies( + rbd_dev->opts->lock_timeout)); down_read(&rbd_dev->lock_rwsem); + if (!timeout) { + finish_wait(&rbd_dev->lock_waitq, &wait); + rbd_warn(rbd_dev, "timed out in waiting for lock"); + return -ETIMEDOUT; + } } while (rbd_dev->lock_state != RBD_LOCK_STATE_LOCKED && !test_bit(RBD_DEV_FLAG_BLACKLISTED, &rbd_dev->flags)); finish_wait(&rbd_dev->lock_waitq, &wait); + return 0; } static void rbd_queue_workfn(struct work_struct *work) @@ -3606,7 +3626,9 @@ static void rbd_queue_workfn(struct work_struct *work) result = -EROFS; goto err_unlock; } - rbd_wait_state_locked(rbd_dev); + result = rbd_wait_state_locked(rbd_dev); + if (result) + goto err_unlock; } if (test_bit(RBD_DEV_FLAG_BLACKLISTED, &rbd_dev->flags)) { result = -EBLACKLISTED; @@ -5139,6 +5161,7 @@ static int rbd_add_parse_args(const char *buf, rbd_opts->read_only = RBD_READ_ONLY_DEFAULT; rbd_opts->queue_depth = RBD_QUEUE_DEPTH_DEFAULT; + rbd_opts->lock_timeout = RBD_LOCK_TIMEOUT_DEFAULT; rbd_opts->lock_on_read = RBD_LOCK_ON_READ_DEFAULT; rbd_opts->exclusive = RBD_EXCLUSIVE_DEFAULT; @@ -5209,6 +5232,8 @@ static void rbd_dev_image_unlock(struct rbd_device *rbd_dev) static int rbd_add_acquire_lock(struct rbd_device *rbd_dev) { + int ret = 0; + if (!(rbd_dev->header.features & RBD_FEATURE_EXCLUSIVE_LOCK)) { rbd_warn(rbd_dev, "exclusive-lock feature is not enabled"); return -EINVAL; @@ -5216,8 +5241,10 @@ static int rbd_add_acquire_lock(struct rbd_device *rbd_dev) /* FIXME: "rbd map --exclusive" should be in interruptible */ down_read(&rbd_dev->lock_rwsem); - rbd_wait_state_locked(rbd_dev); + ret = rbd_wait_state_locked(rbd_dev); up_read(&rbd_dev->lock_rwsem); + if (ret) + return ret; if (test_bit(RBD_DEV_FLAG_BLACKLISTED, &rbd_dev->flags)) { rbd_warn(rbd_dev, "failed to acquire exclusive lock"); return -EROFS;
currently, the rbd_wait_state_locked() will wait forever if we can't get our state locked. Example: rbd map --exclusive test1 --> /dev/rbd0 rbd map test1 --> /dev/rbd1 dd if=/dev/zero of=/dev/rbd1 bs=1M count=1 --> IO blocked To avoid this problem, this patch introduce a timeout design in rbd_wait_state_locked(). Then rbd_wait_state_locked() will return error when we reach a timeout. This patch allow user to set the lock_timeout in rbd mapping. Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn> --- drivers/block/rbd.c | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-)