Re: [PATCH RFC] md/raid1: fix deadlock between freeze_array() and wait_barrier().

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 5330 bytes --]

On Tue, Jul 19 2016, Alexander Lyakas wrote:

> Hello Neil,
> Thank you for your response.
>
> On Fri, Jul 15, 2016 at 2:18 AM, NeilBrown <neilb@suse.com> wrote:
>> On Thu, Jul 14 2016, Alexander Lyakas wrote:
>>
>>>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>>>> index 32e282f4c83c..c528102b80b6 100644
>>>> --- a/drivers/md/raid10.c
>>>> +++ b/drivers/md/raid10.c
>>>> @@ -1288,7 +1288,7 @@ read_again:
>>>>                                 sectors_handled;
>>>>                         goto read_again;
>>>>                 } else
>>>> -                       generic_make_request(read_bio);
>>>> +                       bio_list_add_head(&current->bio_list, read_bio);
>>>>                 return;
>>>>         }
>>>
>>> Unfortunately, this patch doesn't work. It is super elegant, and seems
>>> like it really should work. But the problem is that the "rdev", to
>>> which we want to send the READ bio, might also be a remapping device
>>> (dm-linear, for example). This device will create its own remapped-bio
>>> and will call generic_make_request(), which will stick the bio onto
>>> current->bio_list TAIL:(:(:( So we are back at square one. This patch
>>> would work if *all* the remapping drivers in the stack were doing
>>> bio_list_add_head() instead of generic_make_request()  :(:(:(
>>>
>>> It seems the real fix should be that generic_make_request() would use
>>> bio_list_add_head(), but as pointed in
>>> http://www.spinics.net/lists/raid/msg52756.html, there are some
>>> concerns about changing the order of remapped bios.
>>>
>>
>> While those concerns are valid, they are about hypothetical performance
>> issues rather than observed deadlock issues.  So I wouldn't be too
>> worried about them.
> I am thinking of a hypothetical driver that splits say a 12Kb WRITE
> into 3x4kb WRITEs, and submits them in a proper order, hoping they
> will get to the disk in the same order, and the disk will work
> sequentially. But now we are deliberately hindering this. But I see
> that people much smarter than me are in this discussion, so I will
> leave it to them:)

Sure, that is the concern and ideally we would keep things in order.
But the elevator should re-order things in most cases so it shouldn't
matter too much.
For upstream, we obviously aim for best possible.  For stable backports,
we sometimes accept non-ideal code when the change is less intrusive.

If we do backport something to -stable, I would do it "properly" using
something very similar to the upstream version.  You don't seem to want
that for your code so I'm suggesting options that, while not 100% ideal,
should suit your needs - and obviously you will test your use cases.

>
>> However I think you said that you didn't want to touch core code at all
>> (maybe I misunderstood) so that wouldn't help you anyway.
> Yes, this is correct. Recompiling the kernel is a bit of a pain for
> us. We were smart enough to configure the md_mod as loadable module,
> so at least now I can patch MD code easily:)
>
>>
>> One option would be to punt the request requests to raidXd:
>>
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index 40b35be34f8d..f795e27b2124 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -1229,7 +1229,7 @@ read_again:
>>                                 sectors_handled;
>>                         goto read_again;
>>                 } else
>> -                       generic_make_request(read_bio);
>> +                       reschedule_retry(r1_bio);
>>                 return;
>>         }
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index 32e282f4c83c..eec38443075b 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -1288,7 +1288,7 @@ read_again:
>>                                 sectors_handled;
>>                         goto read_again;
>>                 } else
>> -                       generic_make_request(read_bio);
>> +                       reschedule_retry(r10_bio);
>>                 return;
>>         }
> This is more or less what my rudimentary patch is doing, except it is
> doing it only when we really need to wait for the barrier.
>
>>
>>
>> That might hurt performance, you would need to measure.
>> The other approach would be to revert the patch that caused the problem.
>> e.g.
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index 40b35be34f8d..062bb86e5fd8 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -884,7 +884,8 @@ static bool need_to_wait_for_sync(struct r1conf *conf, struct bio *bio)
>>                         wait = false;
>>                 else
>>                         wait = true;
>> -       }
>> +       } else if (conf->barrier)
>> +               wait = true;
>>
>>         return wait;
>>  }
>>
>>
> I am not sure how this patch helps. You added another condition, and
> now READs will also wait in some cases. But still if array_frozen is
> set, everybody will wait unconditionally, which is the root cause for
> the deadlock I think.

Maybe you're right.  I was thinking that array_frozen only causes
problems if there are read requests in the generic_make_request queue,
and this change would keep them out.  But I might have dropped a ball
somewhere.


>
> I see that there will be no magic solution for this problem:(
>

Not really.


NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]