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

[Josef Bacik <josef@toxicpanda.com>]

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)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 17389a42a3ab..cc570078bf7d 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1450,8 +1450,10 @@ void btrfs_sysfs_remove_device(struct btrfs_device *device)
        devices_kobj = device->fs_info->fs_devices->devices_kobj;
        ASSERT(devices_kobj);
 
-       if (device->bdev)
+       if (device->bdev) {
                sysfs_remove_link(devices_kobj, bdev_kobj(device->bdev)->name);
+               sysfs_remove_link(&device->devid_kobj, bdev_kobj(device->bdev)->name);
+       }
 
        if (device->devid_kobj.state_initialized) {
                kobject_del(&device->devid_kobj);
@@ -1628,10 +1630,23 @@ int btrfs_sysfs_add_device(struct btrfs_device *device)
 
        nofs_flag = memalloc_nofs_save();
 
+       init_completion(&device->kobj_unregister);
+       ret = kobject_init_and_add(&device->devid_kobj, &devid_ktype,
+                                  devinfo_kobj, "%llu", device->devid);
+       if (ret) {
+               kobject_put(&device->devid_kobj);
+               btrfs_warn(device->fs_info,
+                          "devinfo init for devid %llu failed: %d",
+                          device->devid, ret);
+       }
+
        if (device->bdev) {
                struct kobject *disk_kobj = bdev_kobj(device->bdev);
 
                ret = sysfs_create_link(devices_kobj, disk_kobj, disk_kobj->name);
+               if (!ret)
+                       ret = sysfs_create_link(device->devid_kobj, disk_kobj,
+                                               disk_kobj->name);
                if (ret) {
                        btrfs_warn(device->fs_info,
                                "creating sysfs device link for devid %llu failed: %d",
@@ -1640,16 +1655,6 @@ int btrfs_sysfs_add_device(struct btrfs_device *device)
                }
        }
 
-       init_completion(&device->kobj_unregister);
-       ret = kobject_init_and_add(&device->devid_kobj, &devid_ktype,
-                                  devinfo_kobj, "%llu", device->devid);
-       if (ret) {
-               kobject_put(&device->devid_kobj);
-               btrfs_warn(device->fs_info,
-                          "devinfo init for devid %llu failed: %d",
-                          device->devid, ret);
-       }
-
 out:
        memalloc_nofs_restore(nofs_flag);
        return ret;