Re: [PATCH v8 2/2] btrfs: check device for critical errors and mark failed

[Anand Jain <anand.jain@oracle.com>]

On 10/14/2017 02:46 AM, Liu Bo wrote:
> On Sun, Oct 08, 2017 at 10:23:58PM +0800, Anand Jain wrote:
>>
>>
>> On 10/07/2017 07:56 AM, Liu Bo wrote:
>>> On Thu, Oct 05, 2017 at 09:56:59PM +0800, Anand Jain wrote:
>>>>
>>>>
>>>> On 10/05/2017 04:11 AM, Liu Bo wrote:
>>>>> On Tue, Oct 03, 2017 at 11:59:20PM +0800, Anand Jain wrote:
>>>>>> From: Anand Jain <Anand.Jain@oracle.com>
>>>>>>
>>>>>> Write and flush errors are critical errors, upon which the device fd
>>>>>> must be closed and marked as failed.
>>>>>>
>>>>>
>>>>> Can we defer the job of closing device to umount?
>>>>
>>>>    Originally I think it was designed to handle the bad disk
>>>>    same as missing device. as below [1]. This patch just does that.
>>
>>>>    But I am curious to know if there is any issue to close failed
>>>>    device ?
>>
>>     Anything on this ?
> 
> No big deal, it's just different with md's behavior, but surely it
> doesn't make a big difference whether we close device fd here or not.

  Ok.

>>
>>
>>>> [1]
>>>> -----
>>>> static int read_one_dev(struct btrfs_fs_info *fs_info,
>>>>                           struct extent_buffer *leaf,
>>>>                           struct btrfs_dev_item *dev_item)
>>>> ::
>>>>               /*
>>>>                * this happens when a device that was properly setup
>>>>                * in the device info lists suddenly goes bad.
>>>>                * device->bdev is NULL, and so we have to set
>>>>                * device->missing to one here
>>>>                */
>>>> ------
>>>>
>>>>    So here I misuse the device missing scenario (device->bdev = NULL)
>>>>    to the failed device scenario. (code comment mentioned it).
>>>>
>>>
>>> This is OK to me, we can unify these stuff later, for now, ->missing
>>> is to bypass this failed disk for r/w.
>>>
>>>>    Secondly as in the patch 1/2 commit log and also Austin mentioned it,
>>>>    if failed device close is deferred it will also defer the cleanup of
>>>>    the failed device at the block layer.
>>>>
>>>>    But yes block layer can still clean up when btrfs closes the
>>>>    device at the time of replace, also where in the long run, pulling
>>>>    out of the failed disk would (planned) trigger a udev notification
>>>>    into the btrfs sysfs interface and it can close the device.
>>>>
>>>>    Anything that I have missed ?
>>>>
>>>
>>> I'm more inclined to do cleanup by making udev call 'device remove',
>>> which is supposed to do all the validation checks you need to done
>>> here.
>>
>>     Validation check like what ?
> 
> OK, just realize that I was talking about what we should do upon
> device getting pulled out, in that case, validation check about raid
> profile is needed, such as can we keep the raid profile or do we need
> to degrade raid proflie, and 'device delete' can do that.

  OK.

> Although I agree with the fact that write/flush errors are critical
> ones,


> it's still too strict if it got only a few write/flush errors,
> given cable issues or timeout from some layers may also lead to those
> errors.  A compromised way is to have a threshold to give drives
> another chance.


  Fundamentally IO retries are property of the IO module to deal with it.
  It knows what type of IO error is worth a retry. If FS receives an IO
  has failed it should trust IO module has done its best effort. Adding
  additional retires at the FS layer is _not_ a good idea - it would
  affect the time to fail-over, which is very crucial for HA (high
  availability) configurations.

  Instead to deal with spurious IO error its planned to create an offline
  device state, as I wrote about it here [1].

    [1]
    https://www.spinics.net/lists/linux-btrfs/msg49643.html

  I removed offline part here to keep the changes specific to the trigger
  as in here, and it can be added later.


Thanks, Anand

> thanks,
> 
> -liubo
> 
>>
>> Thanks, Anand
>>
>>>>    Further, jfyi closing of failed device isn't related to the
>>>>    looping for device failure though.
>>>>
>>>>> We can go mark the device failed and skip it while doing read/write,
>>>>> and umount can do the cleanup work.
>>>>
>>>>    In servers a need of umount is considered as downtime, things
>>>>    shouldn't wait for umount to cleanup.
>>>>
>>>
>>> OK.
>>>
>>>>> That way we don't need a dedicated thread looping around to detect a
>>>>> rare situation.
>>>>
>>>>    I think its better to make it event based instead of thread looping,
>>>>    pls take a look..
>>>>
>>>>      [PATCH v8.1 2/2] btrfs: mark device failed for write and flush errors
>>>>
>>>
>>> Event based is good, for anything about failing disk, we would have to
>>> follow what md is doing eventually, i.e. let udev handle it.
>>>
>>> thanks,
>>> -liubo
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>