Re: [PATCH 0/7][V11] btrfs: allocation_hint

[Josef Bacik <josef@toxicpanda.com>]

On Wed, Jan 26, 2022 at 09:32:07PM +0100, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli <kreijack@inwind.it>
> 
> Hi all,
> 
> This patches set was born after some discussion between me, Zygo and Josef.
> Some details can be found in https://github.com/btrfs/btrfs-todo/issues/19.
> 
> Some further information about a real use case can be found in
> https://lore.kernel.org/linux-btrfs/20210116002533.GE31381@hungrycats.org/
> 
> Recently Shafeeq told me that he is interested too, due to the performance gain.
> 
> In V8, revision I switched away from an ioctl API in favor of a sysfs API (
> see patch #2 and #3).
> 
> In V9, I renamed the sysfs interface from devinfo/type to devinfo/allocation_hint.
> Moreover I renamed dev_info->type to dev_info->flags.
> 
> In V10, I renamed the tag 'PREFERRED' from PREFERRED_X to X_PREFERRED; I added
> another devinfo property, called major_minor. For now it is unused;
> the plan is to use this in btrfs-progs to go from the block device to the filesystem.
> First client will be "btrfs prop get/set allocation_hint", but I see others possible
> clients.
> Finally I made some cleanup, and remove the mount option 'allocation_hint'
> 
> In V11 I added a new 'feature' file /sys/fs/btrfs/features/allocation_hint
> to help the detection of the allocation_hint feature.
> 

Sorry Goffredo I dropped the ball on this, lets try and get this shit nailed
down and done so we can get it merged.  The code overall looks good, I just have
two things I want changed

1. The sysfs file should use a string, not a magic number.  Think how
   /sys/block/<dev>/queue/scheduler works.  You echo "metadata_only" >
   allocation_hint, you cat allocation_hint and it says "none" or
   "metadata_only".  If you want to be fancy you can do exactly like
   queue/scheduler and show the list of options with [] around the selected
   option.

2. I hate the major_minor thing, can you do similar to what we do for devices/
   and symlink the correct device sysfs directory under devid?

Other than that the code is perfect.  I think these two changes make it solid
enough to merge.  Thanks,

Josef