Message ID | d48a0e28d4ba516602297437b1f132f2a8efd5d2.1614028083.git.kreijack@inwind.it (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [1/4] btrfs: add ioctl BTRFS_IOC_DEV_PROPERTIES. | expand |
Hi Goffredo, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on kdave/for-next] [also build test WARNING on next-20210222] [cannot apply to v5.11] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Goffredo-Baroncelli/btrfs-add-ioctl-BTRFS_IOC_DEV_PROPERTIES/20210223-062001 base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next config: i386-randconfig-r011-20210222 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/62c95ccebf2c45bb8e91d379b454dd720734da34 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Goffredo-Baroncelli/btrfs-add-ioctl-BTRFS_IOC_DEV_PROPERTIES/20210223-062001 git checkout 62c95ccebf2c45bb8e91d379b454dd720734da34 # save the attached .config to linux build tree make W=1 ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): fs/btrfs/ioctl.c: In function 'btrfs_ioctl_dev_properties': >> fs/btrfs/ioctl.c:4923:1: warning: the frame size of 1036 bytes is larger than 1024 bytes [-Wframe-larger-than=] 4923 | } | ^ vim +4923 fs/btrfs/ioctl.c 4858 4859 static long btrfs_ioctl_dev_properties(struct file *file, 4860 void __user *argp) 4861 { 4862 struct inode *inode = file_inode(file); 4863 struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); 4864 struct btrfs_ioctl_dev_properties dev_props; 4865 struct btrfs_device *device; 4866 struct btrfs_root *root = fs_info->chunk_root; 4867 struct btrfs_trans_handle *trans; 4868 int ret; 4869 u64 prev_type; 4870 4871 if (!capable(CAP_SYS_ADMIN)) 4872 return -EPERM; 4873 4874 if (copy_from_user(&dev_props, argp, sizeof(dev_props))) 4875 return -EFAULT; 4876 4877 device = btrfs_find_device(fs_info->fs_devices, dev_props.devid, 4878 NULL, NULL); 4879 if (!device) { 4880 btrfs_info(fs_info, "change_dev_properties: unable to find device %llu", 4881 dev_props.devid); 4882 return -ENODEV; 4883 } 4884 4885 if (dev_props.properties & BTRFS_DEV_PROPERTY_READ) { 4886 u64 props = dev_props.properties; 4887 4888 memset(&dev_props, 0, sizeof(dev_props)); 4889 if (props & BTRFS_DEV_PROPERTY_TYPE) { 4890 dev_props.properties = BTRFS_DEV_PROPERTY_TYPE; 4891 dev_props.type = device->type; 4892 } 4893 if (copy_to_user(argp, &dev_props, sizeof(dev_props))) 4894 return -EFAULT; 4895 return 0; 4896 } 4897 4898 /* it is possible to set only BTRFS_DEV_PROPERTY_TYPE for now */ 4899 if (dev_props.properties & ~(BTRFS_DEV_PROPERTY_TYPE)) 4900 return -EPERM; 4901 4902 trans = btrfs_start_transaction(root, 1); 4903 if (IS_ERR(trans)) 4904 return PTR_ERR(trans); 4905 4906 prev_type = device->type; 4907 device->type = dev_props.type; 4908 ret = btrfs_update_device(trans, device); 4909 4910 if (ret < 0) { 4911 btrfs_abort_transaction(trans, ret); 4912 btrfs_end_transaction(trans); 4913 device->type = prev_type; 4914 return ret; 4915 } 4916 4917 ret = btrfs_commit_transaction(trans); 4918 if (ret < 0) 4919 device->type = prev_type; 4920 4921 return ret; 4922 > 4923 } 4924 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Mon, Feb 22, 2021 at 10:19:06PM +0100, Goffredo Baroncelli wrote: > From: Goffredo Baroncelli <kreijack@inwind.it> > > This ioctl is a base for returning / setting information from / to the > fields of the btrfs_dev_item object. Please don't add a new ioctl for properties, they're using the xattr as interface alrady.
On 2/23/21 2:53 PM, David Sterba wrote: > On Mon, Feb 22, 2021 at 10:19:06PM +0100, Goffredo Baroncelli wrote: >> From: Goffredo Baroncelli <kreijack@inwind.it> >> >> This ioctl is a base for returning / setting information from / to the >> fields of the btrfs_dev_item object. Hi David, > > Please don't add a new ioctl for properties, they're using the xattr as > interface alrady. > how it is supposed to work with a device ? I have to point out that the property is already exported in sysfs. However I read a comment in sysfs code that discourages to make a filesystem updating (open a new transaction) in the sysfs handler. Do you have any suggestion ? Thanks in advance BR Goffredo
I resend it because I made a little mess with the quotation On 2/23/21 6:59 PM, Goffredo Baroncelli wrote: > On 2/23/21 2:53 PM, David Sterba wrote: >> On Mon, Feb 22, 2021 at 10:19:06PM +0100, Goffredo Baroncelli wrote: >>> From: Goffredo Baroncelli <kreijack@inwind.it> >>> >>> This ioctl is a base for returning / setting information from / to the >>> fields of the btrfs_dev_item object. > Hi David, > >> >> Please don't add a new ioctl for properties, they're using the xattr as >> interface alrady. How it is supposed to work with a device ? I have to point out that the property is already exported in sysfs. However I read a comment in sysfs code that discourages to make a filesystem updating (open a new transaction) in the sysfs handler. Do you have any suggestion ? > > Thanks in advance > BR > Goffredo >
On 2/23/21 5:28 AM, kernel test robot wrote: > Hi Goffredo, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on kdave/for-next] > [also build test WARNING on next-20210222] > [cannot apply to v5.11] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Goffredo-Baroncelli/btrfs-add-ioctl-BTRFS_IOC_DEV_PROPERTIES/20210223-062001 > base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next > config: i386-randconfig-r011-20210222 (attached as .config) > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 > reproduce (this is a W=1 build): > # https://github.com/0day-ci/linux/commit/62c95ccebf2c45bb8e91d379b454dd720734da34 > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Goffredo-Baroncelli/btrfs-add-ioctl-BTRFS_IOC_DEV_PROPERTIES/20210223-062001 > git checkout 62c95ccebf2c45bb8e91d379b454dd720734da34 > # save the attached .config to linux build tree > make W=1 ARCH=i386 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > fs/btrfs/ioctl.c: In function 'btrfs_ioctl_dev_properties': >>> fs/btrfs/ioctl.c:4923:1: warning: the frame size of 1036 bytes is larger than 1024 bytes [-Wframe-larger-than=] > 4923 | } > | ^ > > > vim +4923 fs/btrfs/ioctl.c > > 4858 > 4859 static long btrfs_ioctl_dev_properties(struct file *file, > 4860 void __user *argp) > 4861 { > 4862 struct inode *inode = file_inode(file); > 4863 struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); > 4864 struct btrfs_ioctl_dev_properties dev_props; Right, now that btrfs_ioctl_dev_properties is large 1k, it is not nice to store it in the stack... > 4865 struct btrfs_device *device; > 4866 struct btrfs_root *root = fs_info->chunk_root; > 4867 struct btrfs_trans_handle *trans; > 4868 int ret; > 4869 u64 prev_type; > 4870 > 4871 if (!capable(CAP_SYS_ADMIN)) > 4872 return -EPERM; > 4873 > 4874 if (copy_from_user(&dev_props, argp, sizeof(dev_props))) > 4875 return -EFAULT; > 4876 > 4877 device = btrfs_find_device(fs_info->fs_devices, dev_props.devid, > 4878 NULL, NULL); > 4879 if (!device) { > 4880 btrfs_info(fs_info, "change_dev_properties: unable to find device %llu", > 4881 dev_props.devid); > 4882 return -ENODEV; > 4883 } > 4884 > 4885 if (dev_props.properties & BTRFS_DEV_PROPERTY_READ) { > 4886 u64 props = dev_props.properties; > 4887 > 4888 memset(&dev_props, 0, sizeof(dev_props)); > 4889 if (props & BTRFS_DEV_PROPERTY_TYPE) { > 4890 dev_props.properties = BTRFS_DEV_PROPERTY_TYPE; > 4891 dev_props.type = device->type; > 4892 } > 4893 if (copy_to_user(argp, &dev_props, sizeof(dev_props))) > 4894 return -EFAULT; > 4895 return 0; > 4896 } > 4897 > 4898 /* it is possible to set only BTRFS_DEV_PROPERTY_TYPE for now */ > 4899 if (dev_props.properties & ~(BTRFS_DEV_PROPERTY_TYPE)) > 4900 return -EPERM; > 4901 > 4902 trans = btrfs_start_transaction(root, 1); > 4903 if (IS_ERR(trans)) > 4904 return PTR_ERR(trans); > 4905 > 4906 prev_type = device->type; > 4907 device->type = dev_props.type; > 4908 ret = btrfs_update_device(trans, device); > 4909 > 4910 if (ret < 0) { > 4911 btrfs_abort_transaction(trans, ret); > 4912 btrfs_end_transaction(trans); > 4913 device->type = prev_type; > 4914 return ret; > 4915 } > 4916 > 4917 ret = btrfs_commit_transaction(trans); > 4918 if (ret < 0) > 4919 device->type = prev_type; > 4920 > 4921 return ret; > 4922 >> 4923 } > 4924 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org >
On 23/02/2021 21:53, David Sterba wrote: > On Mon, Feb 22, 2021 at 10:19:06PM +0100, Goffredo Baroncelli wrote: >> From: Goffredo Baroncelli <kreijack@inwind.it> >> >> This ioctl is a base for returning / setting information from / to the >> fields of the btrfs_dev_item object. > > Please don't add a new ioctl for properties, they're using the xattr as > interface alrady. > IMO a feature like this can be in memory only initially[1]. And later when this feature is stable, add its on-disk. [1] https://patchwork.kernel.org/project/linux-btrfs/patch/0ed770d6d5e37fc942f3034d917d2b38477d7d20.1613668002.git.anand.jain@oracle.com/ Thanks, Anand
On Wed, Feb 24, 2021 at 10:27:45AM +0800, Anand Jain wrote: > On 23/02/2021 21:53, David Sterba wrote: > > On Mon, Feb 22, 2021 at 10:19:06PM +0100, Goffredo Baroncelli wrote: > > > From: Goffredo Baroncelli <kreijack@inwind.it> > > > > > > This ioctl is a base for returning / setting information from / to the > > > fields of the btrfs_dev_item object. > > > > Please don't add a new ioctl for properties, they're using the xattr as > > interface alrady. > > > > IMO a feature like this can be in memory only initially[1]. And later > when this feature is stable, add its on-disk. The "metadata_only" and "data_only" settings need to be persistent for the feature to really work. It is very expensive to recover (need to balance metadata on a spinning disk) if the filesystem allocates a new chunk after mount but before userspace can reestablish the preferences. The whole point of metadata_only and data_only is that we never have to do that. > [1] https://patchwork.kernel.org/project/linux-btrfs/patch/0ed770d6d5e37fc942f3034d917d2b38477d7d20.1613668002.git.anand.jain@oracle.com/ > > > Thanks, Anand >
On Tue, Feb 23, 2021 at 02:53:30PM +0100, David Sterba wrote: > On Mon, Feb 22, 2021 at 10:19:06PM +0100, Goffredo Baroncelli wrote: > > From: Goffredo Baroncelli <kreijack@inwind.it> > > > > This ioctl is a base for returning / setting information from / to the > > fields of the btrfs_dev_item object. > > Please don't add a new ioctl for properties, they're using the xattr as > interface alrady. We had some earlier discussion about why xattrs on an inode are a bad idea for this feature: https://lore.kernel.org/linux-btrfs/20210123172118.GJ28049@hungrycats.org/ TL;DR they shouldn't be copied from one filesystem to another. Maybe it's better from a CLI UI point of view to put them under 'btrfs dev' instead of 'btrfs property'?
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index a8c60d46d19c..07898ee3a08d 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -4851,6 +4851,72 @@ static int btrfs_ioctl_set_features(struct file *file, void __user *arg) return ret; } +static long btrfs_ioctl_dev_properties(struct file *file, + void __user *argp) +{ + struct inode *inode = file_inode(file); + struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); + struct btrfs_ioctl_dev_properties dev_props; + struct btrfs_device *device; + struct btrfs_root *root = fs_info->chunk_root; + struct btrfs_trans_handle *trans; + int ret; + u64 prev_type; + + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + + if (copy_from_user(&dev_props, argp, sizeof(dev_props))) + return -EFAULT; + + device = btrfs_find_device(fs_info->fs_devices, dev_props.devid, + NULL, NULL); + if (!device) { + btrfs_info(fs_info, "change_dev_properties: unable to find device %llu", + dev_props.devid); + return -ENODEV; + } + + if (dev_props.properties & BTRFS_DEV_PROPERTY_READ) { + u64 props = dev_props.properties; + + memset(&dev_props, 0, sizeof(dev_props)); + if (props & BTRFS_DEV_PROPERTY_TYPE) { + dev_props.properties = BTRFS_DEV_PROPERTY_TYPE; + dev_props.type = device->type; + } + if (copy_to_user(argp, &dev_props, sizeof(dev_props))) + return -EFAULT; + return 0; + } + + /* it is possible to set only BTRFS_DEV_PROPERTY_TYPE for now */ + if (dev_props.properties & ~(BTRFS_DEV_PROPERTY_TYPE)) + return -EPERM; + + trans = btrfs_start_transaction(root, 1); + if (IS_ERR(trans)) + return PTR_ERR(trans); + + prev_type = device->type; + device->type = dev_props.type; + ret = btrfs_update_device(trans, device); + + if (ret < 0) { + btrfs_abort_transaction(trans, ret); + btrfs_end_transaction(trans); + device->type = prev_type; + return ret; + } + + ret = btrfs_commit_transaction(trans); + if (ret < 0) + device->type = prev_type; + + return ret; + +} + static int _btrfs_ioctl_send(struct file *file, void __user *argp, bool compat) { struct btrfs_ioctl_send_args *arg; @@ -5034,6 +5100,8 @@ long btrfs_ioctl(struct file *file, unsigned int return btrfs_ioctl_get_subvol_rootref(file, argp); case BTRFS_IOC_INO_LOOKUP_USER: return btrfs_ioctl_ino_lookup_user(file, argp); + case BTRFS_IOC_DEV_PROPERTIES: + return btrfs_ioctl_dev_properties(file, argp); } return -ENOTTY; diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index b8fab44394f5..0c649b444dcd 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -2809,7 +2809,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path return ret; } -static noinline int btrfs_update_device(struct btrfs_trans_handle *trans, +int btrfs_update_device(struct btrfs_trans_handle *trans, struct btrfs_device *device) { int ret; diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index d4c3e0dd32b8..0c07b8deecab 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -600,5 +600,7 @@ int btrfs_bg_type_to_factor(u64 flags); const char *btrfs_bg_type_to_raid_name(u64 flags); int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info); int btrfs_repair_one_zone(struct btrfs_fs_info *fs_info, u64 logical); +int btrfs_update_device(struct btrfs_trans_handle *trans, + struct btrfs_device *device); #endif diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h index 5df73001aad4..bab35d3f819c 100644 --- a/include/uapi/linux/btrfs.h +++ b/include/uapi/linux/btrfs.h @@ -860,6 +860,43 @@ struct btrfs_ioctl_get_subvol_rootref_args { __u8 align[7]; }; +#define BTRFS_DEV_PROPERTY_TYPE (1ULL << 0) +#define BTRFS_DEV_PROPERTY_DEV_GROUP (1ULL << 1) +#define BTRFS_DEV_PROPERTY_SEEK_SPEED (1ULL << 2) +#define BTRFS_DEV_PROPERTY_BANDWIDTH (1ULL << 3) +#define BTRFS_DEV_PROPERTY_READ (1ULL << 60) + +/* + * The ioctl BTRFS_IOC_DEV_PROPERTIES can read and write the device properties. + * + * The properties that the user want to write have to be set + * in the 'properties' field using the BTRFS_DEV_PROPERTY_xxxx constants. + * + * If the ioctl is used to read the device properties, the bit + * BTRFS_DEV_PROPERTY_READ has to be set in the 'properties' field. + * In this case the properties that the user want have to be set in the + * 'properties' field. The kernel doesn't return a property that was not + * required, however it may return a subset of the requested properties. + * The returned properties have the corrispondent BTRFS_DEV_PROPERTY_xxxx + * flag set in the 'properties' field. + * + * Up to 2020/05/11 the only properties that can be read/write is the 'type' + * one. + */ +struct btrfs_ioctl_dev_properties { + __u64 devid; + __u64 properties; + __u64 type; + __u32 dev_group; + __u8 seek_speed; + __u8 bandwidth; + + /* + * for future expansion, pad up to 1k + */ + __u8 reserved[1024-30]; +}; + /* Error codes as returned by the kernel */ enum btrfs_err_code { BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET = 1, @@ -988,5 +1025,7 @@ enum btrfs_err_code { struct btrfs_ioctl_ino_lookup_user_args) #define BTRFS_IOC_SNAP_DESTROY_V2 _IOW(BTRFS_IOCTL_MAGIC, 63, \ struct btrfs_ioctl_vol_args_v2) +#define BTRFS_IOC_DEV_PROPERTIES _IOW(BTRFS_IOCTL_MAGIC, 64, \ + struct btrfs_ioctl_dev_properties) #endif /* _UAPI_LINUX_BTRFS_H */