Re: [PATCH v2 1/3] btrfs: add readmirror type framework

From: Anand Jain <anand.jain@oracle.com>
To: Steven Davies <btrfs-list@steev.me.uk>, David Sterba <dsterba@suse.com>
Cc: linux-btrfs@vger.kernel.org
Subject: Re: [PATCH v2 1/3] btrfs: add readmirror type framework
Date: Fri, 3 Jan 2020 18:28:24 +0800	[thread overview]
Message-ID: <e652f476-eef8-62d1-4a3d-b01dbce2677a@oracle.com> (raw)
In-Reply-To: <4f4a1dab-fd9b-a5b6-2109-d82fc222d208@steev.me.uk>

On 3/1/20 3:32 AM, Steven Davies wrote:
> On 02/01/2020 10:12, Anand Jain wrote:
>> As of now we use %pid method to read stripped mirrored data. So
>> application's process id determines the stripe id to be read. This type
>> of read IO 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 and the overall disk
>> bandwidth remains under utilized.
>>
>> So this patch introduces a framework where we could add more readmirror
>> policies, such as routing the IO based on device's wait-queue or manual
>> when we have a read-preferred device or a policy based on the target
>> storage caching.
> 
> I think the idea is good but that it would be cleaner if the tunable was 
> named read_policy rather than readmirror as it's more obvious that it 
> contains a policy tunable.

  Um. 'read_policy' sounds good, but I hope it is clear enough to
  indicate that we are talking about read for only mirrored-chunks.
  Will rename to read_policy.

> Do you envisage allowing more than one policy to be active for a 
> filesystem? If not, what about using the same structure as the CPU 
> frequency and block IO schedulers with the format
> 
> #cat /sys/block/sda/queue/scheduler
> noop [deadline] cfq
> 
> Such that btrfs would (eventually) have something like
> 
> #cat /sys/fs/btrfs/UUID/read_policy
> by_pid [user_defined_device] by_shortest_queue
> 

  And in case of user_defined_device, the device for the read shall be
  specified in

   cat /sys/fs/btrfs/UUID/devinfo/<devid>/read_preferred

   0 = unset, 1 = set.

   (devinfo patches are in the ML [1] open for comment)
   [1]
   [PATCH v3 4/4] btrfs: sysfs, add devid/dev_state kobject and attribute

> And the policy would be changed by echo'ing the new policy name to the 
> read_policy kobject.

  I like this approach, will change it to use this format.

Thanks, Anand

> Steve