On Thu, Sep 12, 2019 at 03:41:42PM +0800, Anand Jain wrote:
>
>
> Thanks for the comments. More below.
>
> On 12/9/19 3:16 AM, Josef Bacik wrote:
> > On Wed, Sep 11, 2019 at 03:13:21PM -0400, Eli V wrote:
> > > On Wed, Sep 11, 2019 at 2:46 PM Josef Bacik <jo...@toxicpanda.com> wrote:
> > > >
> > > > On Mon, Aug 26, 2019 at 05:04:36PM +0800, Anand Jain wrote:
> > > > > Function call chain __btrfs_map_block()->find_live_mirror() uses
> > > > > thread pid to determine the %mirror_num when the mirror_num=0.
> > > > >
> > > > > This patch introduces a framework so that we can add policies to
> > > > > determine
> > > > > the %mirror_num. And also adds the devid as the readmirror policy.
> > > > >
> > > > > The new property is stored as an item in the device tree as show
> > > > > below.
> > > > > (BTRFS_READMIRROR_OBJECTID, BTRFS_PERSISTENT_ITEM_KEY, devid)
> > > > >
> > > > > To be able to set and get this new property also introduces new ioctls
> > > > > BTRFS_IOC_GET_READMIRROR and BTRFS_IOC_SET_READMIRROR. The ioctl
> > > > > argument
> > > > > is defined as
> > > > > struct btrfs_ioctl_readmirror_args {
> > > > > __u64 type; /* RW */
> > > > > __u64 device_bitmap; /* RW */
> > > > > }
> > > > >
> > > > > An usage example as follows:
> > > > > btrfs property set /btrfs readmirror devid:1,3
> > > > > btrfs property get /btrfs readmirror
> > > > > readmirror devid:1 3
> > > > > btrfs property set /btrfs readmirror ""
> > > > > btrfs property get /btrfs readmirror
> > > > > readmirror default
> > > > >
> > > > > This patchset has been tested completely, however marked as RFC for
> > > > > the
> > > > > following reasons and comments on them (or any other) are appreciated
> > > > > as
> > > > > usual.
> > > > > . The new objectid is defined as
> > > > > #define BTRFS_READMIRROR_OBJECTID -1ULL
> > > > > Need consent we are fine to use this value, and with this value it
> > > > > shall be placed just before the DEV_STATS_OBJECTID item which is
> > > > > more
> > > > > frequently used only during the device errors.
> > > > >
> > > > > . I am using a u64 bitmap to represent the devices id, so the max
> > > > > device
> > > > > id that we could represent is 63, its a kind of limitation which
> > > > > should
> > > > > be addressed before integration, I wonder if there is any
> > > > > suggestion?
> > > > > Kindly note that, multiple ioctls with each time representing a
> > > > > set of
> > > > > device(s) is not a choice because we need to make sure the
> > > > > readmirror
> > > > > changes happens in a commit transaction.
> > > > >
> > > > > v1->RFC v2:
> > > > > . Property is stored as a dev-tree item instead of root inode
> > > > > extended
> > > > > attribute.
> > > > > . Rename BTRFS_DEV_STATE_READ_OPRIMIZED to
> > > > > BTRFS_DEV_STATE_READ_PREFERRED.
> > > > > . Changed format specifier from devid1,2,3.. to devid:1,2,3..
> > > > >
> > > > > RFC->v1:
> > > > > Drops pid as one of the readmirror policy choices and as usual
> > > > > remains
> > > > > as default. And when the devid is reset the readmirror policy
> > > > > falls back
> > > > > to pid.
> > > > > Drops the mount -o readmirror idea, it can be added at a later
> > > > > point of
> > > > > time.
> > > > > Property now accepts more than 1 devid as readmirror device. As
> > > > > shown
> > > > > in the example above.
> > > > >
> > > >
> > > > This is a lot of infrastructure
>
> Ok. Any idea on a better implementation?
> How about extended attribute approach? v1 patches proposed
> it, but it abused the extended attribute as commented here [1]
> and v2 got changed to an item-key.
>
> [1]
> https://lore.kernel.org/linux-btrfs/be68e6ea-00bc-b750-25e1-9c584b993...@gmx.com/
>
That's a NAK on the prop interface. This is a fs wide policy, not a
directory/inode policy.
>
> > > > to just change which mirror we read to based on
> > > > some arbitrary user policy. I assume this is to solve the case where
> > > > you have
> > > > slow and fast disks, so you can always read from the fast disk? And
> > > > then it's
> > > > only used in RAID1, so the very narrow usecase of having a RAID1 setup
> > > > with a
> > > > SSD and a normal disk? I'm not seeing a point to this much code for one
> > > > particular obscure setup. Thanks,
> > > >
> > > > Josef
> > >
> > > Not commenting on the code itself, but as a user I see this SSD RAID1
> > > acceleration as a future much have feature. It's only obscure at the
> > > moment because we don't have code to take advantage of it. But on
> > > large btrfs filesystems with hundreds of GB of metadata, like I have
> > > for backups, the usability of the filesystem is dramatically improved
> > > having the metadata on an SSD( though currently only half of the time
> > > due to the even/odd pid distribution.)
> >
> > But that's different from a mirror. 100% it would be nice to say "put my
> > metadata on the ssd, data elsewhere". That's not what this patch is about,
> > this
> > patch is specifically about changing which drive we choose in a mirrored
> > setup,
> > which is super unlikely to mirror a SSD with a slow drive, cause it's just
> > going
> > to be slow no matter what. Sure we could make it so reads always go to the
> > SSD,
> > but we can accomplish that by just adding a check for nonrotational in the
> > code,
> > and then we don't have to encode all this nonsense in the file system.
> > Thanks,
>
> I wrote about the readmirror policy framework here[2],
> I forgot to link it here, sorry about that, my mistake.
>
> [2]
>
> https://lore.kernel.org/linux-btrfs/1552989624-29577-1-git-send-email-anand.j...@oracle.com/
>
> Readmirror policy is for raid1, raid10 and future N way mirror.
> Yes for now its only for raid1.
>
> Here the idea is to create a framework so that readmirror policy
> can be configured as needed. And nonrotational can be one such policy.
>
> The example of hard-coded nonrotational policy does not work in case
> of ssd and a remote iscsi ssd, OR in case of local ssd and a NVME block
> device, as all these are still nonrotational devices. So hard-coded
> policy is not a good idea. If we have to hardcode then there is Q-depth
> based readmirror routing is better (patch in the ML), but that is
> not good enough, because some configs wants it based on the disk-LBA
> so that SAN storage target cache is balanced and not duplicated.
> So in short it must be a configurable policy.
>
Again, if you are mixing disk types you likely always want non-rotational, but
still mixing different speed devices in a mirror setup is just asking for weird
latency problems. I don't think solving this use case is necessary. If you mix
ssd + network device in a serious production setup then you probably should be
fired cause you don't know what you are doing. Having the generic
"nonrotational gets priority" is going to cover 99% of the actual use cases that
make sense.
The SAN usecase I can sort of see, but again I don't feel like it's a problem we
need to solve with on-disk format. Add a priority to sysfs so you can change it
with udev or something on the fly. Thanks,
Josef
