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

[Josef Bacik <josef@toxicpanda.com>]

On Wed, Mar 02, 2022 at 08:30:22PM +0100, Goffredo Baroncelli wrote:
> On 01/03/2022 22.43, Josef Bacik wrote:
> > On Tue, Mar 01, 2022 at 07:55:07PM +0100, Goffredo Baroncelli wrote:
> > > On 01/03/2022 16.07, Josef Bacik wrote:
> > > > On Mon, Feb 28, 2022 at 10:01:35PM +0100, Goffredo Baroncelli wrote:
> > > > > Hi Josef,
> > > > > 
> > > > > On 28/02/2022 18.04, Josef Bacik wrote:
> > > > > > On Wed, Jan 26, 2022 at 09:32:07PM +0100, Goffredo Baroncelli wrote:
> > > > > > > From: Goffredo Baroncelli <kreijack@inwind.it>
> > > > > > > 
> > > > > > > Hi all,
> > > > > > > 
> > > > > [...
> > > > > 
> > > > > > > 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.
> > > > > 
> > > > > Ok.
> > > > > > 
> > > > > > 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?
> > > > > > 
> > > > > Ok, do you have any suggestion for the name ? what about bdev ?
> > > > > 
> > > > 
> > > > You literally just add a link to the device kobj to the devid kobj.  If you look
> > > > at btrfs_sysfs_add_device(), you would do something like this (completely
> > > > untested and uncompiled)
> > > 
> > > I will give an eye to your code; thanks. However my question was more basic.
> > > 
> > > Now we have:
> > > 
> > > .../btrfs/<uuid>/devinfo/<dev-nr>/major_minor
> > > 
> > > with the link, as you suggested, I think that we will have:
> > > 
> > > .../btrfs/<uuid>/devinfo/<dev-nr>/bdev -> ../../../../devices/pci0000:00/0000:00:01.2/0000:01:00.1/ata6/host5/target5:0:0/5:0:0:0/block/sdg/sdg3
> > >                                    ^^^^
> > > 
> > > (notice 'bdev', which is the name that I asked)
> > > 
> > > looking at your patch, it seems to me that the link will be named like the device name:
> > > 
> > > .../btrfs/<uuid>/devinfo/<dev-nr>/sdg3 -> ../../../../devices/pci0000:00/0000:00:01.2/0000:01:00.1/ata6/host5/target5:0:0/5:0:0:0/block/sdg/sdg3
> > >                                    ^^^^
> > > 
> > > which is quite convoluted as approach, because the code has to find a name that matches a device (sdg3), instead to look for a fixed name (bdev).
> > > 
> > > Because I know that every people has a strong, valid (and above all different !) opinion about the name, I want to ask it before issue another patches set.
> > > For the record, I like 'bdev' only because I saw used (by bcache)
> > > 
> > > IMHO, the btrfs world had been simpler if devices/ sysfs directory was populated by the btrfs-dev-nr instead the device name
> > > 
> > 
> > Ahh ok I see, you make a good point.  I agree it would have been better to have
> > the dev nrs in devices and then links in there, but here we are.
> > 
> > I think for now drop this patch from this series, since it's another bike
> > shedding opportunity and I'd rather get the core functionality in.  Do what I
> > asked in #1 and drop this patch from this series, follow up with a different
> > series if you feel strongly enough about it and that way we can have that
> > discussion in that thread and not hold up your feature.  Thanks,
> still be a problem:
> - how go from the "dev name" (e.g. /dev/sdg3) to the sysfs field
>   (e.g. /sys/btrfs/<uuid>/devinfo/<devid>/allocation_hint) ?
> 
> For simple filesystem (e.g. 1 disk), it is trivial (and not useful); for more complex
> one (2, 3 disks) it is easy to make mistake.
> 
> btrfs-progs relies on major_minor; it is possible to used the BTRFS_IOC_DEV_INFO
> but it requires CAP_ADMIN....
> 

Well this just made me go look at the code and realize you don't require
CAP_ADMIN for the sysfs knob, which we're going to need.  So using
BTRFS_IOC_DEV_INFO shouldn't be a problem.  Thanks,

Josef