* [PATCH] Remove: container should wait for an array to release a drive
@ 2016-07-18 12:11 Tomasz Majchrzak
2016-07-18 13:37 ` John Stoffel
0 siblings, 1 reply; 6+ messages in thread
From: Tomasz Majchrzak @ 2016-07-18 12:11 UTC (permalink / raw)
To: linux-raid
Cc: Jes.Sorensen, aleksey.obitotskiy, pawel.baldysiak, artur.paszkiewicz
A 'faulty' drive is being removed from a container after it has been
released by an array, however there is a race there. The drive is
released asynchronously by a monitor but sometimes it doesn't happen
before container checks it. It results in a container refusing to remove
a drive as it still seems to be a part of some array.
It seems 'ping_monitor' could be a solution here to assure monitor has
had a chance to process the events, however it doesn't resolve the
problem - sometimes an array has to request a release of the drive few
times (as the array is busy) and single 'ping_monitor' call is not
sufficient. As there is no way to query monitor progress, it forces us
to retry a check several times before an error is returned.
Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
---
Manage.c | 38 +++++++++++++++++++++++++-------------
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/Manage.c b/Manage.c
index e2e88b8..7f8eb88 100644
--- a/Manage.c
+++ b/Manage.c
@@ -1125,19 +1125,31 @@ int Manage_remove(struct supertype *tst, int fd, struct mddev_dev *dv,
*/
if (rdev == 0)
ret = -1;
- else
- ret = sysfs_unique_holder(devnm, rdev);
- if (ret == 0) {
- pr_err("%s is not a member, cannot remove.\n",
- dv->devname);
- close(lfd);
- return -1;
- }
- if (ret >= 2) {
- pr_err("%s is still in use, cannot remove.\n",
- dv->devname);
- close(lfd);
- return -1;
+ else {
+ /* The drive has already been set to 'faulty', however monitor might
+ * not have had time to process it and the drive might still have
+ * an entry in the 'holders' directory. Try a few times to avoid
+ * a false error */
+ int count = 20;
+ do {
+ ret = sysfs_unique_holder(devnm, rdev);
+ if (ret < 2)
+ break;
+ usleep(100000);
+ } while (--count > 0);
+
+ if (ret == 0) {
+ pr_err("%s is not a member, cannot remove.\n",
+ dv->devname);
+ close(lfd);
+ return -1;
+ }
+ if (ret >= 2) {
+ pr_err("%s is still in use, cannot remove.\n",
+ dv->devname);
+ close(lfd);
+ return -1;
+ }
}
}
/* FIXME check that it is a current member */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Remove: container should wait for an array to release a drive
2016-07-18 12:11 [PATCH] Remove: container should wait for an array to release a drive Tomasz Majchrzak
@ 2016-07-18 13:37 ` John Stoffel
2016-07-18 20:55 ` Jes Sorensen
0 siblings, 1 reply; 6+ messages in thread
From: John Stoffel @ 2016-07-18 13:37 UTC (permalink / raw)
To: Tomasz Majchrzak
Cc: linux-raid, Jes.Sorensen, aleksey.obitotskiy, pawel.baldysiak,
artur.paszkiewicz
>>>>> "Tomasz" == Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
Tomasz> A 'faulty' drive is being removed from a container after it
Tomasz> has been released by an array, however there is a race
Tomasz> there. The drive is released asynchronously by a monitor but
Tomasz> sometimes it doesn't happen before container checks it. It
Tomasz> results in a container refusing to remove a drive as it still
Tomasz> seems to be a part of some array.
Tomasz> It seems 'ping_monitor' could be a solution here to assure
Tomasz> monitor has had a chance to process the events, however it
Tomasz> doesn't resolve the problem - sometimes an array has to
Tomasz> request a release of the drive few times (as the array is
Tomasz> busy) and single 'ping_monitor' call is not sufficient. As
Tomasz> there is no way to query monitor progress, it forces us to
Tomasz> retry a check several times before an error is returned.
Tomasz> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
Tomasz> ---
Tomasz> Manage.c | 38 +++++++++++++++++++++++++-------------
Tomasz> 1 file changed, 25 insertions(+), 13 deletions(-)
Tomasz> diff --git a/Manage.c b/Manage.c
Tomasz> index e2e88b8..7f8eb88 100644
Tomasz> --- a/Manage.c
Tomasz> +++ b/Manage.c
Tomasz> @@ -1125,19 +1125,31 @@ int Manage_remove(struct supertype *tst, int fd, struct mddev_dev *dv,
Tomasz> */
Tomasz> if (rdev == 0)
Tomasz> ret = -1;
Tomasz> - else
Tomasz> - ret = sysfs_unique_holder(devnm, rdev);
Tomasz> - if (ret == 0) {
Tomasz> - pr_err("%s is not a member, cannot remove.\n",
Tomasz> - dv->devname);
Tomasz> - close(lfd);
Tomasz> - return -1;
Tomasz> - }
Tomasz> - if (ret >= 2) {
Tomasz> - pr_err("%s is still in use, cannot remove.\n",
Tomasz> - dv->devname);
Tomasz> - close(lfd);
Tomasz> - return -1;
Tomasz> + else {
Tomasz> + /* The drive has already been set to 'faulty', however monitor might
Tomasz> + * not have had time to process it and the drive might still have
Tomasz> + * an entry in the 'holders' directory. Try a few times to avoid
Tomasz> + * a false error */
Tomasz> + int count = 20;
Tomasz> + do {
Tomasz> + ret = sysfs_unique_holder(devnm, rdev);
Tomasz> + if (ret < 2)
Tomasz> + break;
Tomasz> + usleep(100000);
Really, you're sleeping 10 seconds without telling the user? That
seems to be a bit obnoxious. Logging something here would be good.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Remove: container should wait for an array to release a drive
2016-07-18 13:37 ` John Stoffel
@ 2016-07-18 20:55 ` Jes Sorensen
2016-07-19 7:23 ` Tomasz Majchrzak
0 siblings, 1 reply; 6+ messages in thread
From: Jes Sorensen @ 2016-07-18 20:55 UTC (permalink / raw)
To: John Stoffel
Cc: Tomasz Majchrzak, linux-raid, aleksey.obitotskiy,
pawel.baldysiak, artur.paszkiewicz
"John Stoffel" <john@stoffel.org> writes:
>>>>>> "Tomasz" == Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
>
> Tomasz> A 'faulty' drive is being removed from a container after it
> Tomasz> has been released by an array, however there is a race
> Tomasz> there. The drive is released asynchronously by a monitor but
> Tomasz> sometimes it doesn't happen before container checks it. It
> Tomasz> results in a container refusing to remove a drive as it still
> Tomasz> seems to be a part of some array.
>
> Tomasz> It seems 'ping_monitor' could be a solution here to assure
> Tomasz> monitor has had a chance to process the events, however it
> Tomasz> doesn't resolve the problem - sometimes an array has to
> Tomasz> request a release of the drive few times (as the array is
> Tomasz> busy) and single 'ping_monitor' call is not sufficient. As
> Tomasz> there is no way to query monitor progress, it forces us to
> Tomasz> retry a check several times before an error is returned.
>
> Tomasz> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> Tomasz> ---
> Tomasz> Manage.c | 38 +++++++++++++++++++++++++-------------
> Tomasz> 1 file changed, 25 insertions(+), 13 deletions(-)
>
> Tomasz> diff --git a/Manage.c b/Manage.c
> Tomasz> index e2e88b8..7f8eb88 100644
> Tomasz> --- a/Manage.c
> Tomasz> +++ b/Manage.c
> Tomasz> @@ -1125,19 +1125,31 @@ int Manage_remove(struct supertype *tst, int fd, struct mddev_dev *dv,
> Tomasz> */
> Tomasz> if (rdev == 0)
> Tomasz> ret = -1;
> Tomasz> - else
> Tomasz> - ret = sysfs_unique_holder(devnm, rdev);
> Tomasz> - if (ret == 0) {
> Tomasz> - pr_err("%s is not a member, cannot remove.\n",
> Tomasz> - dv->devname);
> Tomasz> - close(lfd);
> Tomasz> - return -1;
> Tomasz> - }
> Tomasz> - if (ret >= 2) {
> Tomasz> - pr_err("%s is still in use, cannot remove.\n",
> Tomasz> - dv->devname);
> Tomasz> - close(lfd);
> Tomasz> - return -1;
> Tomasz> + else {
> Tomasz> + /* The drive has already been set to 'faulty', however monitor might
> Tomasz> + * not have had time to process it and the drive might still have
> Tomasz> + * an entry in the 'holders' directory. Try a few times to avoid
> Tomasz> + * a false error */
> Tomasz> + int count = 20;
> Tomasz> + do {
> Tomasz> + ret = sysfs_unique_holder(devnm, rdev);
> Tomasz> + if (ret < 2)
> Tomasz> + break;
> Tomasz> + usleep(100000);
>
> Really, you're sleeping 10 seconds without telling the user? That
> seems to be a bit obnoxious. Logging something here would be good.
Hi,
Sorry just back from vacation and just started attacking the mountain of
email.
I agree with John here, please add some logging message. Also is 10
seconds really needed? It seems an awful lot per iteration.
Cheers,
Jes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Remove: container should wait for an array to release a drive
2016-07-18 20:55 ` Jes Sorensen
@ 2016-07-19 7:23 ` Tomasz Majchrzak
2016-07-19 13:15 ` John Stoffel
0 siblings, 1 reply; 6+ messages in thread
From: Tomasz Majchrzak @ 2016-07-19 7:23 UTC (permalink / raw)
To: Jes Sorensen
Cc: John Stoffel, linux-raid, aleksey.obitotskiy, pawel.baldysiak,
artur.paszkiewicz
On Mon, Jul 18, 2016 at 04:55:27PM -0400, Jes Sorensen wrote:
> "John Stoffel" <john@stoffel.org> writes:
> >>>>>> "Tomasz" == Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
> >
> > Tomasz> A 'faulty' drive is being removed from a container after it
> > Tomasz> has been released by an array, however there is a race
> > Tomasz> there. The drive is released asynchronously by a monitor but
> > Tomasz> sometimes it doesn't happen before container checks it. It
> > Tomasz> results in a container refusing to remove a drive as it still
> > Tomasz> seems to be a part of some array.
> >
> > Tomasz> It seems 'ping_monitor' could be a solution here to assure
> > Tomasz> monitor has had a chance to process the events, however it
> > Tomasz> doesn't resolve the problem - sometimes an array has to
> > Tomasz> request a release of the drive few times (as the array is
> > Tomasz> busy) and single 'ping_monitor' call is not sufficient. As
> > Tomasz> there is no way to query monitor progress, it forces us to
> > Tomasz> retry a check several times before an error is returned.
> >
> > Tomasz> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> > Tomasz> ---
> > Tomasz> Manage.c | 38 +++++++++++++++++++++++++-------------
> > Tomasz> 1 file changed, 25 insertions(+), 13 deletions(-)
> >
> > Tomasz> diff --git a/Manage.c b/Manage.c
> > Tomasz> index e2e88b8..7f8eb88 100644
> > Tomasz> --- a/Manage.c
> > Tomasz> +++ b/Manage.c
> > Tomasz> @@ -1125,19 +1125,31 @@ int Manage_remove(struct supertype *tst, int fd, struct mddev_dev *dv,
> > Tomasz> */
> > Tomasz> if (rdev == 0)
> > Tomasz> ret = -1;
> > Tomasz> - else
> > Tomasz> - ret = sysfs_unique_holder(devnm, rdev);
> > Tomasz> - if (ret == 0) {
> > Tomasz> - pr_err("%s is not a member, cannot remove.\n",
> > Tomasz> - dv->devname);
> > Tomasz> - close(lfd);
> > Tomasz> - return -1;
> > Tomasz> - }
> > Tomasz> - if (ret >= 2) {
> > Tomasz> - pr_err("%s is still in use, cannot remove.\n",
> > Tomasz> - dv->devname);
> > Tomasz> - close(lfd);
> > Tomasz> - return -1;
> > Tomasz> + else {
> > Tomasz> + /* The drive has already been set to 'faulty', however monitor might
> > Tomasz> + * not have had time to process it and the drive might still have
> > Tomasz> + * an entry in the 'holders' directory. Try a few times to avoid
> > Tomasz> + * a false error */
> > Tomasz> + int count = 20;
> > Tomasz> + do {
> > Tomasz> + ret = sysfs_unique_holder(devnm, rdev);
> > Tomasz> + if (ret < 2)
> > Tomasz> + break;
> > Tomasz> + usleep(100000);
> >
> > Really, you're sleeping 10 seconds without telling the user? That
> > seems to be a bit obnoxious. Logging something here would be good.
>
> Hi,
>
> Sorry just back from vacation and just started attacking the mountain of
> email.
>
> I agree with John here, please add some logging message. Also is 10
> seconds really needed? It seems an awful lot per iteration.
>
> Cheers,
> Jes
Well, actually it's 20 iteration 100ms each so up to 2 seconds. I have never
seen it taking more than 3 iterations, however I don't have a full knowledge how
long it can take md module to release an array. I just added 2 seconds as a
precaution, better wait a bit longer than leave an array in inconsistent state.
Is it fine?
Regards,
Tomek
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Remove: container should wait for an array to release a drive
2016-07-19 7:23 ` Tomasz Majchrzak
@ 2016-07-19 13:15 ` John Stoffel
2016-07-19 14:27 ` Jes Sorensen
0 siblings, 1 reply; 6+ messages in thread
From: John Stoffel @ 2016-07-19 13:15 UTC (permalink / raw)
To: Tomasz Majchrzak
Cc: Jes Sorensen, John Stoffel, linux-raid, aleksey.obitotskiy,
pawel.baldysiak, artur.paszkiewicz
>>>>> "Tomasz" == Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
Tomasz> On Mon, Jul 18, 2016 at 04:55:27PM -0400, Jes Sorensen wrote:
>> "John Stoffel" <john@stoffel.org> writes:
>> >>>>>> "Tomasz" == Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
>> >
>> > Tomasz> A 'faulty' drive is being removed from a container after it
>> > Tomasz> has been released by an array, however there is a race
>> > Tomasz> there. The drive is released asynchronously by a monitor but
>> > Tomasz> sometimes it doesn't happen before container checks it. It
>> > Tomasz> results in a container refusing to remove a drive as it still
>> > Tomasz> seems to be a part of some array.
>> >
>> > Tomasz> It seems 'ping_monitor' could be a solution here to assure
>> > Tomasz> monitor has had a chance to process the events, however it
>> > Tomasz> doesn't resolve the problem - sometimes an array has to
>> > Tomasz> request a release of the drive few times (as the array is
>> > Tomasz> busy) and single 'ping_monitor' call is not sufficient. As
>> > Tomasz> there is no way to query monitor progress, it forces us to
>> > Tomasz> retry a check several times before an error is returned.
>> >
>> > Tomasz> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
>> > Tomasz> ---
>> > Tomasz> Manage.c | 38 +++++++++++++++++++++++++-------------
>> > Tomasz> 1 file changed, 25 insertions(+), 13 deletions(-)
>> >
>> > Tomasz> diff --git a/Manage.c b/Manage.c
>> > Tomasz> index e2e88b8..7f8eb88 100644
>> > Tomasz> --- a/Manage.c
>> > Tomasz> +++ b/Manage.c
>> > Tomasz> @@ -1125,19 +1125,31 @@ int Manage_remove(struct supertype *tst, int fd, struct mddev_dev *dv,
>> > Tomasz> */
>> > Tomasz> if (rdev == 0)
>> > Tomasz> ret = -1;
>> > Tomasz> - else
>> > Tomasz> - ret = sysfs_unique_holder(devnm, rdev);
>> > Tomasz> - if (ret == 0) {
>> > Tomasz> - pr_err("%s is not a member, cannot remove.\n",
>> > Tomasz> - dv->devname);
>> > Tomasz> - close(lfd);
>> > Tomasz> - return -1;
>> > Tomasz> - }
>> > Tomasz> - if (ret >= 2) {
>> > Tomasz> - pr_err("%s is still in use, cannot remove.\n",
>> > Tomasz> - dv->devname);
>> > Tomasz> - close(lfd);
>> > Tomasz> - return -1;
>> > Tomasz> + else {
>> > Tomasz> + /* The drive has already been set to 'faulty', however monitor might
>> > Tomasz> + * not have had time to process it and the drive might still have
>> > Tomasz> + * an entry in the 'holders' directory. Try a few times to avoid
>> > Tomasz> + * a false error */
>> > Tomasz> + int count = 20;
>> > Tomasz> + do {
>> > Tomasz> + ret = sysfs_unique_holder(devnm, rdev);
>> > Tomasz> + if (ret < 2)
>> > Tomasz> + break;
>> > Tomasz> + usleep(100000);
>> >
>> > Really, you're sleeping 10 seconds without telling the user? That
>> > seems to be a bit obnoxious. Logging something here would be good.
>>
>> Hi,
>>
>> Sorry just back from vacation and just started attacking the mountain of
>> email.
>>
>> I agree with John here, please add some logging message. Also is 10
>> seconds really needed? It seems an awful lot per iteration.
>>
>> Cheers,
>> Jes
Tomasz> Well, actually it's 20 iteration 100ms each so up to 2
Tomasz> seconds. I have never seen it taking more than 3 iterations,
Tomasz> however I don't have a full knowledge how long it can take md
Tomasz> module to release an array. I just added 2 seconds as a
Tomasz> precaution, better wait a bit longer than leave an array in
Tomasz> inconsistent state. Is it fine?
Then maybe instead of the magic number 100000, you put in a define
which says the expected sleep time, or maybe even just a commment? I
can never keep the usleep number units straight in my head anyway.
:-/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Remove: container should wait for an array to release a drive
2016-07-19 13:15 ` John Stoffel
@ 2016-07-19 14:27 ` Jes Sorensen
0 siblings, 0 replies; 6+ messages in thread
From: Jes Sorensen @ 2016-07-19 14:27 UTC (permalink / raw)
To: John Stoffel
Cc: Tomasz Majchrzak, linux-raid, aleksey.obitotskiy,
pawel.baldysiak, artur.paszkiewicz
"John Stoffel" <john@stoffel.org> writes:
>>>>>> "Tomasz" == Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
>
> Tomasz> On Mon, Jul 18, 2016 at 04:55:27PM -0400, Jes Sorensen wrote:
>>> "John Stoffel" <john@stoffel.org> writes:
>>> > Really, you're sleeping 10 seconds without telling the user? That
>>> > seems to be a bit obnoxious. Logging something here would be good.
>>>
>>> Hi,
>>>
>>> Sorry just back from vacation and just started attacking the mountain of
>>> email.
>>>
>>> I agree with John here, please add some logging message. Also is 10
>>> seconds really needed? It seems an awful lot per iteration.
>>>
>>> Cheers,
>>> Jes
>
> Tomasz> Well, actually it's 20 iteration 100ms each so up to 2
> Tomasz> seconds. I have never seen it taking more than 3 iterations,
> Tomasz> however I don't have a full knowledge how long it can take md
> Tomasz> module to release an array. I just added 2 seconds as a
> Tomasz> precaution, better wait a bit longer than leave an array in
> Tomasz> inconsistent state. Is it fine?
>
> Then maybe instead of the magic number 100000, you put in a define
> which says the expected sleep time, or maybe even just a commment? I
> can never keep the usleep number units straight in my head anyway.
> :-/
Heh, made the same mistake here - I blame the jetlag and the crazy
weather here in NYC :)
If you can add a comment explaining it, that will satisfy my concerns.
Thanks,
Jes
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-07-19 14:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-18 12:11 [PATCH] Remove: container should wait for an array to release a drive Tomasz Majchrzak
2016-07-18 13:37 ` John Stoffel
2016-07-18 20:55 ` Jes Sorensen
2016-07-19 7:23 ` Tomasz Majchrzak
2016-07-19 13:15 ` John Stoffel
2016-07-19 14:27 ` Jes Sorensen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.