Re: [PATCH 0/3 RESEND Rebased] readmirror feature

[Anand Jain <anand.jain@oracle.com>]

> On 24 Jul 2019, at 8:20 AM, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> 
> 
> 
> On 2019/6/26 下午4:33, Anand Jain wrote:
>> These patches are tested to be working fine.
>> 
>> 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 adds the devid as the readmirror policy.
>> 
>> The property is stored as an extented attributes of root inode
>> (BTRFS_FS_TREE_OBJECTID).
> 
> This doesn't look right to me.
> 
> As readmirror should work at chunk layer, putting it into root tree
> doesn't follow the layer separation of btrfs.
> 
> And furthermore, this breaks the XATTR schema. Normally we only have
> XATTR item after an INODE item, not a ROOT_ITEM.
> 
> Is the on-disk format already accepted or still under design stage?
> 

 I mentioned about the storage for this new property in the RFC patch, as I knew there will be some surprises.

 The advantage of using the XATTR on the ROOT_ITEM is there is no on-disk format update nor there is any new KEY, albeit it deviates from the traditional way of using the xattr. Also, this approach don’t need an ioctl, as things work using the existing get/set xattr interface.

 The other way I had in mind was to introduce a new Key in the dev-tree such as

    (BTRFS_READMIRROR_OBJECTID, BTRFS_PERSISTENT_ITEM_KEY, devid)

 Again the interface can be ioctl or the get/set xattr. If we have to use the xattr then irrespective which inode is used we would anyway store it in the dev-tree using the above key.

 This is still open for changes, the idea is to get a long lasting flexible design, so comments are welcome.

Thanks, Anand


> Thanks,
> Qu
> 
>> User provided devid list is validated against the fs_devices::dev_list.
>> 
>> For example:
>>   Usage:
>>     btrfs property set <mnt> readmirror devid<n>[,<m>...]
>>     btrfs property set <mnt> readmirror ""
>> 
>>   mkfs.btrfs -fq -draid1 -mraid1 /dev/sd[b-d] && mount /dev/sdb /btrfs
>>   btrfs prop set /btrfs readmirror devid1,2
>>   btrfs prop get /btrfs readmirror
>>    readmirror=devid1,2
>>   getfattr -n btrfs.readmirror --absolute-names /btrfs
>>    btrfs.readmirror="devid1,2"
>>   btrfs prop set /btrfs readmirror ""
>>   getfattr -n btrfs.readmirror --absolute-names /btrfs
>>    /btrfs: btrfs.readmirror: No such attribute
>>   btrfs prop get /btrfs readmirror
>> 
>> 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.
>> 
>> Anand Jain (3):
>>  btrfs: add inode pointer to prop_handler::validate()
>>  btrfs: add readmirror property framework
>>  btrfs: add readmirror devid property
>> 
>> fs/btrfs/props.c   | 120 +++++++++++++++++++++++++++++++++++++++++++--
>> fs/btrfs/props.h   |   4 +-
>> fs/btrfs/volumes.c |  25 +++++++++-
>> fs/btrfs/volumes.h |   8 +++
>> fs/btrfs/xattr.c   |   2 +-
>> 5 files changed, 150 insertions(+), 9 deletions(-)
>> 
>