| Message ID | 1586173871-5559-1-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | readmirror feature (sysfs and in-memory only approach; with new read_policy device) | expand |
David, I am not sure if this will be integrated in 5.8 and worth the time to rebase. Kindly suggest. -Anand On 6/4/20 7:51 pm, Anand Jain wrote: > v7: > Fix switch's fall through warning. Changle logs updates where necessary. > > v6: > Patch 4/5 - If there is no change in device's read prefer then don't log > Patch 4/5 - Add pid to the logs > Patch 5/5 - If there isn't read preferred device in the chunk don't reset > read policy to default, instead just use stripe 0. As this is in > the read path it avoids going through the device list to find > read preferred device. So inline to this drop to check if there > is read preferred device before setting read policy to device. > > __ Original email: __ > > v5: > Worked on review comments as received in its previous version. > Please refer to individual patches for the specific changes. > Introduces the new read_policy 'device'. > > v4: > Rename readmirror attribute to read_policy. Drop separate kobj for > readmirror instead create read_policy attribute in fsid kobj. > merge v2:2/3 and v2:3/3 into v4:2/2. Patch titles have changed. > > v3: > v2: > Mainly fixes the fs_devices::readmirror declaration type from atomic_t > to u8. (Thanks Josef). > > v1: > As of now we use only %pid method to read stripped mirrored data. So > application's process id determines the stripe id to be read. This type > of routing typically helps in a system with many small independent > applications tying to read random data. On the other hand the %pid > based read IO distribution policy is inefficient if there is a single > application trying to read large data as because the overall disk > bandwidth would remains under utilized. > > One type of readmirror policy isn't good enough and other choices are > routing the IO based on device's waitqueue or manual when we have a > read-preferred device or a readmirror policy based on the target storage > caching. So this patch-set introduces a framework where we could add more > readmirror policies. > > This policy is a filesystem wide policy as of now, and though the > readmirror policy at the subvolume level is a novel approach as it > provides maximum flexibility in the data center, but as of now its not > practical to implement such a granularity as you can't really ensure > reflinked extents will be read from the stripe of its desire and so > there will be more limitations and it can be assessed separately. > > The approach in this patch-set is sys interface with in-memory policy. > And does not add any new readmirror type in this set, which can be add > once we are ok with the framework. Also the default policy remains %pid. > > Previous works: > ---------------------------------------------------------------------- > There were few RFCs [1] before, mainly to figure out storage > (or in memory only) for the readmirror policy and the interface needed. > > [1] > https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg86368.html > > https://lore.kernel.org/linux-btrfs/20190826090438.7044-1-anand.jain@oracle.com/ > > https://lore.kernel.org/linux-btrfs/5fcf9c23-89b5-b167-1f80-a0f4ac107d0b@oracle.com/ > > https://patchwork.kernel.org/cover/10859213/ > > Mount -o: > In the first trial it was attempted to use the mount -o option to carry > the readmirror policy, this is good for debugging which can make sure > even the mount thread metadata tree blocks are read from the disk desired. > It was very effective in testing radi1/raid10 write-holes. > > Extended attribute: > As extended attribute is associated with the inode, to implement this > there is bit of extended attribute abuse or else makes it mandatory to > mount the rootid 5. Its messy unless readmirror policy is applied at the > subvol level which is not possible as of now. > > An item type: > The proposed patch was to create an item to hold the readmirror policy, > it makes sense when compared to the abusive extended attribute approach > but introduces a new item and so no backward compatibility. > ----------------------------------------------------------------------- > > Anand Jain (5): > btrfs: add btrfs_strmatch helper > btrfs: create read policy framework > btrfs: create read policy sysfs attribute, pid > btrfs: introduce new device-state read_preferred > btrfs: introduce new read_policy device > > fs/btrfs/sysfs.c | 128 +++++++++++++++++++++++++++++++++++++++++++++ > fs/btrfs/volumes.c | 39 +++++++++++++- > fs/btrfs/volumes.h | 16 ++++++ > 3 files changed, 182 insertions(+), 1 deletion(-) >
On Thu, Apr 30, 2020 at 05:02:27PM +0800, Anand Jain wrote: > I am not sure if this will be integrated in 5.8 and worth the time to > rebase. Kindly suggest. The preparatory work is ok, but the actual mirror selection policy addresses a usecase that I think is not the one most users are interested in. Devices of vastly different performance capabilities like rotational disks vs nvme vs ssd vs network block devices in one filesystem are not something commonly found. What we really need is a saner balancing mechanism than pid-based, that is also going to be used any time there are more devices from the same speed class for the fast devices too. So, no the patchset is not on track for a merge without the improved default balancing. The preferred device for reads can be one of the policies, I understand the usecase and have not problem with that although wouldn't probably have use for it.
On 16/5/20 3:58 am, David Sterba wrote: > On Thu, Apr 30, 2020 at 05:02:27PM +0800, Anand Jain wrote: >> I am not sure if this will be integrated in 5.8 and worth the time to >> rebase. Kindly suggest. > > The preparatory work is ok, but the actual mirror selection policy > addresses a usecase that I think is not the one most users are > interested in. Devices of vastly different performance capabilities like > rotational disks vs nvme vs ssd vs network block devices in one > filesystem are not something commonly found. > > What we really need is a saner balancing mechanism than pid-based, that > is also going to be used any time there are more devices from the same > speed class for the fast devices too. > There are two things here, the read_policy framework in the preparatory patches and a new balancing or read_policy, device. > So, no the patchset is not on track for a merge without the improved > default balancing. It can be worked on top of the preparatory read_policy framework? This patchset does not change any default read_policy (or balancing) which is pid as of now. Working on a default read_policy/balancing was out of the scope of this patchset. > The preferred device for reads can be one of the > policies, I understand the usecase and have not problem with that > although wouldn't probably have use for it. For us, read_policy:device helps to reproduce raid1 data corruption https://patchwork.kernel.org/patch/11475417/ And xfstests btrfs/14[0-3] can be improved so that the reads directly go the device of the choice, instead of waiting for the odd/even pid. Common configuration won't need this, advance configurations assembled with heterogeneous devices where read performance is more critical than write will find read_policy:device useful. Thanks, Anand
On Tue, May 19, 2020 at 06:02:32PM +0800, Anand Jain wrote: > On 16/5/20 3:58 am, David Sterba wrote: > > On Thu, Apr 30, 2020 at 05:02:27PM +0800, Anand Jain wrote: > >> I am not sure if this will be integrated in 5.8 and worth the time to > >> rebase. Kindly suggest. > > > > The preparatory work is ok, but the actual mirror selection policy > > addresses a usecase that I think is not the one most users are > > interested in. Devices of vastly different performance capabilities like > > rotational disks vs nvme vs ssd vs network block devices in one > > filesystem are not something commonly found. > > > > What we really need is a saner balancing mechanism than pid-based, that > > is also going to be used any time there are more devices from the same > > speed class for the fast devices too. > > There are two things here, the read_policy framework in the preparatory > patches and a new balancing or read_policy, device. > > > So, no the patchset is not on track for a merge without the improved > > default balancing. > > It can be worked on top of the preparatory read_policy framework? Yes. > This patchset does not change any default read_policy (or balancing) > which is pid as of now. Working on a default read_policy/balancing > was out of the scope of this patchset. > > > The preferred device for reads can be one of the > > policies, I understand the usecase and have not problem with that > > although wouldn't probably have use for it. > > For us, read_policy:device helps to reproduce raid1 data corruption > https://patchwork.kernel.org/patch/11475417/ > And xfstests btrfs/14[0-3] can be improved so that the reads directly > go the device of the choice, instead of waiting for the odd/even pid. > > Common configuration won't need this, advance configurations assembled > with heterogeneous devices where read performance is more critical than > write will find read_policy:device useful. Yes that's the usecase and the possibility to make more targeted tests is also good, but that still means the feature is half-baked and missing the main part. If it was out of scope, ok fair, but I don't want to merge it at that state. It would be embarassing to announce mirror selection followed by "ah no it's useless for anything than this special usecase".
On 19/05/2020 11:02, Anand Jain wrote: > > > On 16/5/20 3:58 am, David Sterba wrote: >> On Thu, Apr 30, 2020 at 05:02:27PM +0800, Anand Jain wrote: >>> I am not sure if this will be integrated in 5.8 and worth the time to >>> rebase. Kindly suggest. >> >> The preparatory work is ok, but the actual mirror selection policy >> addresses a usecase that I think is not the one most users are >> interested in. Devices of vastly different performance capabilities like >> rotational disks vs nvme vs ssd vs network block devices in one >> filesystem are not something commonly found. >> >> What we really need is a saner balancing mechanism than pid-based, that >> is also going to be used any time there are more devices from the same >> speed class for the fast devices too. I can't find documentation about how mdadm chooses which mirror to read from (in the absence of --write-mostly) but everything suggests it uses different mirrors for different processes which sounds the same as or very much like %pid. Are you thinking along the lines of checking I/O queue length or average seek/read time when choosing the device? > There are two things here, the read_policy framework in the preparatory > patches and a new balancing or read_policy, device. > >> So, no the patchset is not on track for a merge without the improved >> default balancing. > > It can be worked on top of the preparatory read_policy framework? > This patchset does not change any default read_policy (or balancing) > which is pid as of now. Working on a default read_policy/balancing > was out of the scope of this patchset. > >> The preferred device for reads can be one of the >> policies, I understand the usecase and have not problem with that >> although wouldn't probably have use for it. > > For us, read_policy:device helps to reproduce raid1 data corruption > https://patchwork.kernel.org/patch/11475417/ > And xfstests btrfs/14[0-3] can be improved so that the reads directly > go the device of the choice, instead of waiting for the odd/even pid. > > Common configuration won't need this, advance configurations assembled > with heterogeneous devices where read performance is more critical than > write will find read_policy:device useful. +1 for merging this as-is; I'm one of the users with heterogeneous devices (SATA SSD and NVMe SSD) and it would bring a feature similar to mdadm's --write-mostly. I've seen a few other requests for this feature on other discussion fora as well.
On 22/5/20 9:46 pm, David Sterba wrote: > On Tue, May 19, 2020 at 06:02:32PM +0800, Anand Jain wrote: >> On 16/5/20 3:58 am, David Sterba wrote: >>> On Thu, Apr 30, 2020 at 05:02:27PM +0800, Anand Jain wrote: >>>> I am not sure if this will be integrated in 5.8 and worth the time to >>>> rebase. Kindly suggest. >>> >>> The preparatory work is ok, but the actual mirror selection policy >>> addresses a usecase that I think is not the one most users are >>> interested in. Devices of vastly different performance capabilities like >>> rotational disks vs nvme vs ssd vs network block devices in one >>> filesystem are not something commonly found. >>> >>> What we really need is a saner balancing mechanism than pid-based, that >>> is also going to be used any time there are more devices from the same >>> speed class for the fast devices too. >> >> There are two things here, the read_policy framework in the preparatory >> patches and a new balancing or read_policy, device. >> >>> So, no the patchset is not on track for a merge without the improved >>> default balancing. >> >> It can be worked on top of the preparatory read_policy framework? > > Yes. > >> This patchset does not change any default read_policy (or balancing) >> which is pid as of now. Working on a default read_policy/balancing >> was out of the scope of this patchset. >> >>> The preferred device for reads can be one of the >>> policies, I understand the usecase and have not problem with that >>> although wouldn't probably have use for it. >> >> For us, read_policy:device helps to reproduce raid1 data corruption >> https://patchwork.kernel.org/patch/11475417/ >> And xfstests btrfs/14[0-3] can be improved so that the reads directly >> go the device of the choice, instead of waiting for the odd/even pid. >> >> Common configuration won't need this, advance configurations assembled >> with heterogeneous devices where read performance is more critical than >> write will find read_policy:device useful. > > Yes that's the usecase and the possibility to make more targeted tests > is also good, but that still means the feature is half-baked and missing > the main part. If it was out of scope, ok fair, but I don't want to > merge it at that state. It would be embarassing to announce mirror > selection followed by "ah no it's useless for anything than this special > usecase". I didn't realize the need for default policy is prioritized before this patch set. Potential default read policy is interesting, looking into it. Thanks, Anand
On Tue, May 26, 2020 at 03:23:08PM +0800, Anand Jain wrote: > > Yes that's the usecase and the possibility to make more targeted tests > > is also good, but that still means the feature is half-baked and missing > > the main part. If it was out of scope, ok fair, but I don't want to > > merge it at that state. It would be embarassing to announce mirror > > selection followed by "ah no it's useless for anything than this special > > usecase". > > I didn't realize the need for default policy is prioritized before this > patch set. The updated default policy has been asked for for a long time so this is what makes it important. > Potential default read policy is interesting, looking into it. Thanks.