From: Steven Davies <btrfs-list@steev.me.uk>
To: Anand Jain <anand.jain@oracle.com>
Cc: linux-btrfs@vger.kernel.org
Subject: Re: [PATCH v2 2/3] btrfs: add read_mirror_policy parameter devid
Date: Tue, 22 Jan 2019 14:28:53 +0000 [thread overview]
Message-ID: <5e35fc5da89e1722caf261f89679d039@steev.me.uk> (raw)
In-Reply-To: <24ea40ca-5216-fbe4-2b4b-64da7d8a89fa@oracle.com>
On 2019-01-22 13:43, Anand Jain wrote:
> On 01/21/2019 07:56 PM, Steven Davies wrote:
>> On 2018-05-16 11:03, Anand Jain wrote:
>>> + break;
>>> + }
>>
>> I noticed that it's possible to pass this option multiple times at
>> mount, which sets multiple devices as read mirrors. While that doesn't
>> do anything harmful, the code below will only use the first device. It
>> may be worth at least mentioning this in documentation.
>
> There were few feedback if read_mirror_policy should be a mount
> option or a sysfs or a property. IMO property is better as it would
> be persistent. In sysfs and mount-option, the user or a config file
> has to remember. Will fix.
I agree. Would/could this be set at mkfs time? It would then be similar
to how mdadm's --write-mostly is set up.
>>> +
>>> ret = -EINVAL;
>>> goto out;
>>> case Opt_err:
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 64dba5c4cf33..3a9c3804ff17 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -5220,6 +5220,16 @@ static int find_live_mirror(struct
>>> btrfs_fs_info *fs_info,
>>> num_stripes = map->num_stripes;
>>>
>>> switch(fs_info->read_mirror_policy) {
>>> + case BTRFS_READ_MIRROR_BY_DEV:
>>> + preferred_mirror = first;
>>> + if (test_bit(BTRFS_DEV_STATE_READ_MIRROR,
>>> + &map->stripes[preferred_mirror].dev->dev_state))
>>> + break;
>>> + if (test_bit(BTRFS_DEV_STATE_READ_MIRROR,
>>> + &map->stripes[++preferred_mirror].dev->dev_state))
>>> <--[*]
>>> + break;
>>> + preferred_mirror = first;
>>
>> Why set preferred_mirror again? The only effect of re-setting it will
>> be to use the lowest devid (1) rather than the highest (2). Is there
>> any difference? Either way it should never happen, because the above
>> code traps it (except when the BTRFS_DEV_STATE_READ_MIRROR flag has
>> been changed as mentioned above).
>
> Code at [*] above does ++preferred_mirror. So the following
> preferred_mirror = first;
> is not redundant.
Yes, but preferred_mirror will be first + num_stripes; meaning that
without that line the last stripe is preferred and with it the first
stripe is preferred. It only changes the fallback case from reading from
devid 2 to devid 1, which barely matters. Perhaps it would be even
better to fall back to pid % num_stripes in this case anyway?
>> From Goffredo's comment[1] on Timofey's similar effect patch, if it
>> becomes possible to have more mirrors in a RAID1/RAID10 fs then this
>> code will need to be updated to test dev_state for more than two
>> devices. Would it be sensible to implement this as a for loop straight
>> away?
>
> Right. A loop is better, will add.
Thinking more about this, if it becomes possible to have more than two
devices in a RAID1 or part-RAID10 then do we also need to consider that
there may be more than one READ_MIRROR drive? e.g. slow+fast+fast drives
in a RAID1 with this approach would result in all chunks being read from
the first drive with READ_MIRROR set if both chunks are on the fast
drives. This is where Timofey's queue length patch in [1] would become
useful - in any case, that is an existing problem and probably deserving
of an entirely new patch.
>> [1]: https://patchwork.kernel.org/patch/10678531/#22315779
>
>
> Thanks,
> Anand.
Steve
next prev parent reply other threads:[~2019-01-22 14:28 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-16 10:03 [PATCH v2 0/3] btrfs: add read mirror policy Anand Jain
2018-05-16 10:03 ` [PATCH v2 1/3] btrfs: add mount option read_mirror_policy Anand Jain
2018-05-16 10:03 ` [PATCH v2 2/3] btrfs: add read_mirror_policy parameter devid Anand Jain
2019-01-21 11:56 ` Steven Davies
2019-01-22 13:43 ` Anand Jain
2019-01-22 14:28 ` Steven Davies [this message]
2018-05-16 10:03 ` [PATCH v2 3/3] btrfs: read_mirror_policy ability to reset Anand Jain
2018-05-16 22:35 ` [PATCH v2 0/3] btrfs: add read mirror policy David Sterba
2018-05-17 2:32 ` Anand Jain
2018-05-17 12:25 ` Austin S. Hemmelgarn
2018-05-17 14:46 ` Jeff Mahoney
2018-05-18 8:06 ` Anand Jain
2018-05-18 12:36 ` Austin S. Hemmelgarn
2018-05-17 14:46 ` Jeff Mahoney
2018-05-17 15:22 ` Austin S. Hemmelgarn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5e35fc5da89e1722caf261f89679d039@steev.me.uk \
--to=btrfs-list@steev.me.uk \
--cc=anand.jain@oracle.com \
--cc=linux-btrfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).