Re: [PATCH 2/2] btrfs: add read_mirror_policy parameter devid

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

On 02/01/2018 01:26 PM, Edmund Nadolski wrote:
> On 1/31/18 7:36 AM, Anand Jain wrote:
>>
>>
>> On 01/31/2018 09:42 PM, Nikolay Borisov wrote:
>>
>>
>>>>> So usually this should be functionality handled by the raid/san
>>>>> controller I guess, > but given that btrfs is playing the role of a
>>>>> controller here at what point are we drawing the line of not
>>>>> implementing block-level functionality into the filesystem ?
>>>>
>>>>    Don't worry this is not invading into the block layer. How
>>>>    can you even build this functionality in the block layer ?
>>>>    Block layer even won't know that disks are mirrored. RAID
>>>>    does or BTRFS in our case.
>>>>
>>>
>>> By block layer I guess I meant the storage driver of a particular raid
>>> card. Because what is currently happening is re-implementing
>>> functionality that will generally sit in the driver. So my question was
>>> more generic and high-level - at what point do we draw the line of
>>> implementing feature that are generally implemented in hardware devices
>>> (be it their drivers or firmware).
>>
>>   Not all HW configs use RAID capable HBAs. A server connected to a SATA
>>   JBOD using a SATA HBA without MD will relay on BTRFS to provide all the
>>   features and capabilities that otherwise would have provided by such a
>>   presumable HW config.
> 
> That does sort of sound like means implementing some portion of the
> HBA features/capabilities in the filesystem.
> 
> To me it seems this this could be workable at the fs level, provided it
> deals just with policies and remains hardware-neutral.

  Thanks. Ok.

> However most
> of the use cases appear to involve some hardware-dependent knowledge
> or assumptions. 

> What happens when someone sets this on a virtual disk,
> or say a (persistent) memory-backed block device? 

  Do you have any policy in particular ?

> Case #6 seems to
> open up some potential for unexpected interactions (which may be hard
> to reproduce, esp. in error/recovery scenarios).

  Yep. Even the #1 pid based (current default) which motivated
  me to provide the devid based policy.

> Case #2 takes a devid, but I notice btrfs_device::devid says, "the
> internal btrfs device id".  How does a user obtain that internal value
> so it can be set as a mount option?

   btrfs fi show -m

Thanks, Anand

> Thanks,
> Ed
> 
> 
>>>>>> ::
>>>>>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>>>>>> index 39ba59832f38..478623e6e074 100644
>>>>>>>> --- a/fs/btrfs/volumes.c
>>>>>>>> +++ b/fs/btrfs/volumes.c
>>>>>>>> @@ -5270,6 +5270,16 @@ static int find_live_mirror(struct
>>>>>>>> btrfs_fs_info *fs_info,
>>>>>>>>              num = map->num_stripes;
>>>>>>>>            switch(fs_info->read_mirror_policy) {
>>>>>>>> +    case BTRFS_READ_MIRROR_BY_DEV:
>>>>>>>> +        optimal = first;
>>>>>>>> +        if (test_bit(BTRFS_DEV_STATE_READ_MIRROR,
>>>>>>>> +                 &map->stripes[optimal].dev->dev_state))
>>>>>>>> +            break;
>>>>>>>> +        if (test_bit(BTRFS_DEV_STATE_READ_MIRROR,
>>>>>>>> +                 &map->stripes[++optimal].dev->dev_state))
>>>>>>>> +            break;
>>>>>>>> +        optimal = first;
>>>>>>>
>>>>>>> you set optimal 2 times, the second one seems redundant.
>>>>>>
>>>>>>     No actually. When both the disks containing the stripe does not
>>>>>>     have the BTRFS_DEV_STATE_READ_MIRROR, then I would just want to
>>>>>>     use first found stripe.
>>>>>
>>>>> Yes, and the fact that you've already set optimal = first right after
>>>>> BTRFS_READ_MIRROR_BY_DEV ensures that, no ? Why do you need to again
>>>>> set
>>>>> optimal right before the final break? What am I missing here?
>>>>
>>>>     Ah. I think you are missing ++optimal in the 2nd if.
>>>
>>> You are right, but I'd prefer you index the stripes array with 'optimal'
>>> and 'optimal + 1' and leave just a single assignment
>>
>>   Ok. Will improve that.
>>
>> Thanks, Anand
>>
>>
>>>>
>>>> Thanks, Anand
>>>>
>>> -- 
>>> 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
>>>
>> -- 
>> 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
>>
> 
> --
> 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
>