On 31.01.2018 15:38, Anand Jain wrote:
>
>
> On 01/31/2018 05:54 PM, Nikolay Borisov wrote:
>>
>>
>> On 31.01.2018 11:28, Anand Jain wrote:
>>>
>>>
>>> On 01/31/2018 04:38 PM, Nikolay Borisov wrote:
>>>>
>>>>
>>>> On 30.01.2018 08:30, Anand Jain wrote:
>>>>> Adds the mount option:
>>>>> mount -o read_mirror_policy=<devid>
>>>>>
>>>>> To set the devid of the device which should be used for read. That
>>>>> means all the normal reads will go to that particular device only.
>>>>>
>>>>> This also helps testing and gives a better control for the test
>>>>> scripts including mount context reads.
>>>>
>>>> Some code comments below. OTOH, does such policy really make sense,
>>>> what
>>>> happens if the selected device fails, will the other mirror be retried?
>>>
>>> Everything as usual, read_mirror_policy=devid just lets the user to
>>> specify his read optimized disk, so that we don't depend on the pid
>>> to pick a stripe mirrored disk, and instead we would pick as suggested
>>> by the user, and if that disk fails then we go back to the other
>>> mirror
>>> which may not be the read optimized disk as we have no other choice.
>>>
>>>> If the answer to the previous question is positive then why do we
>>>> really
>>>> care which device is going to be tried first?
>>>
>>> It matters.
>>> - If you are reading from both disks alternatively, then it
>>> duplicates the LUN cache on the storage.
>>> - Some disks are read-optimized and using that for reading and going
>>> back to the other disk only when this disk fails provides a better
>>> overall read performance.
>>
>> 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).
>>> ::
>>>>> 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
>
> Thanks, Anand
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
