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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html