* [PATCH 0/1] btrfs: reuse existing pointers from btrfs_ioctl @ 2022-01-05 8:30 Sahil Kang 2022-01-05 8:30 ` [PATCH 1/1] " Sahil Kang 2022-01-05 9:30 ` [PATCH 0/1] " Qu Wenruo 0 siblings, 2 replies; 10+ messages in thread From: Sahil Kang @ 2022-01-05 8:30 UTC (permalink / raw) To: linux-btrfs This is a small cleanup that reuses some of the existing pointers and removes the duplicated dereferencing. There are other subfunctions that have similar/identical dereferencing as the enclosing btrfs_ioctl, but I'm not sure if changing those is welcomed since it would require changing their arg count. Right now, all of the sub ioctl functions have essentially the same signature. For example, reusing the pointers for btrfs_ioctl_set_fslabel would require passing in *fs_info and *root, in addition to its current args. Sahil Kang (1): btrfs: reuse existing pointers from btrfs_ioctl fs/btrfs/ioctl.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) -- Sahil ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] btrfs: reuse existing pointers from btrfs_ioctl 2022-01-05 8:30 [PATCH 0/1] btrfs: reuse existing pointers from btrfs_ioctl Sahil Kang @ 2022-01-05 8:30 ` Sahil Kang 2022-01-05 9:53 ` Qu Wenruo 2022-01-06 15:27 ` [PATCH 1/1] btrfs: reuse existing pointers " David Sterba 2022-01-05 9:30 ` [PATCH 0/1] " Qu Wenruo 1 sibling, 2 replies; 10+ messages in thread From: Sahil Kang @ 2022-01-05 8:30 UTC (permalink / raw) To: linux-btrfs btrfs_ioctl already contains pointers to the inode and btrfs_root structs, so we can pass them into the subfunctions instead of the toplevel struct file. Signed-off-by: Sahil Kang <sahil.kang@asilaycomputing.com> --- fs/btrfs/ioctl.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index edfecfe62b4b..26cade35eccd 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -414,10 +414,8 @@ void btrfs_exclop_finish(struct btrfs_fs_info *fs_info) sysfs_notify(&fs_info->fs_devices->fsid_kobj, NULL, "exclusive_operation"); } -static int btrfs_ioctl_getversion(struct file *file, int __user *arg) +static int btrfs_ioctl_getversion(struct inode *inode, int __user *arg) { - struct inode *inode = file_inode(file); - return put_user(inode->i_generation, arg); } @@ -2559,25 +2557,22 @@ static int btrfs_search_path_in_tree_user(struct user_namespace *mnt_userns, return ret; } -static noinline int btrfs_ioctl_ino_lookup(struct file *file, +static noinline int btrfs_ioctl_ino_lookup(struct btrfs_root *root, void __user *argp) { struct btrfs_ioctl_ino_lookup_args *args; - struct inode *inode; int ret = 0; args = memdup_user(argp, sizeof(*args)); if (IS_ERR(args)) return PTR_ERR(args); - inode = file_inode(file); - /* * Unprivileged query to obtain the containing subvolume root id. The * path is reset so it's consistent with btrfs_search_path_in_tree. */ if (args->treeid == 0) - args->treeid = BTRFS_I(inode)->root->root_key.objectid; + args->treeid = root->root_key.objectid; if (args->objectid == BTRFS_FIRST_FREE_OBJECTID) { args->name[0] = 0; @@ -2589,7 +2584,7 @@ static noinline int btrfs_ioctl_ino_lookup(struct file *file, goto out; } - ret = btrfs_search_path_in_tree(BTRFS_I(inode)->root->fs_info, + ret = btrfs_search_path_in_tree(root->fs_info, args->treeid, args->objectid, args->name); @@ -2765,7 +2760,8 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp) * Return ROOT_REF information of the subvolume containing this inode * except the subvolume name. */ -static int btrfs_ioctl_get_subvol_rootref(struct file *file, void __user *argp) +static int btrfs_ioctl_get_subvol_rootref(struct btrfs_root *subvol_root, + void __user *argp) { struct btrfs_ioctl_get_subvol_rootref_args *rootrefs; struct btrfs_root_ref *rref; @@ -2773,7 +2769,6 @@ static int btrfs_ioctl_get_subvol_rootref(struct file *file, void __user *argp) struct btrfs_path *path; struct btrfs_key key; struct extent_buffer *leaf; - struct inode *inode; u64 objectid; int slot; int ret; @@ -2789,9 +2784,8 @@ static int btrfs_ioctl_get_subvol_rootref(struct file *file, void __user *argp) return PTR_ERR(rootrefs); } - inode = file_inode(file); - root = BTRFS_I(inode)->root->fs_info->tree_root; - objectid = BTRFS_I(inode)->root->root_key.objectid; + root = subvol_root->fs_info->tree_root; + objectid = subvol_root->root_key.objectid; key.objectid = objectid; key.type = BTRFS_ROOT_REF_KEY; @@ -4873,7 +4867,7 @@ long btrfs_ioctl(struct file *file, unsigned int switch (cmd) { case FS_IOC_GETVERSION: - return btrfs_ioctl_getversion(file, argp); + return btrfs_ioctl_getversion(inode, argp); case FS_IOC_GETFSLABEL: return btrfs_ioctl_get_fslabel(fs_info, argp); case FS_IOC_SETFSLABEL: @@ -4921,7 +4915,7 @@ long btrfs_ioctl(struct file *file, unsigned int case BTRFS_IOC_TREE_SEARCH_V2: return btrfs_ioctl_tree_search_v2(file, argp); case BTRFS_IOC_INO_LOOKUP: - return btrfs_ioctl_ino_lookup(file, argp); + return btrfs_ioctl_ino_lookup(root, argp); case BTRFS_IOC_INO_PATHS: return btrfs_ioctl_ino_to_path(root, argp); case BTRFS_IOC_LOGICAL_INO: @@ -5000,7 +4994,7 @@ long btrfs_ioctl(struct file *file, unsigned int case BTRFS_IOC_GET_SUBVOL_INFO: return btrfs_ioctl_get_subvol_info(file, argp); case BTRFS_IOC_GET_SUBVOL_ROOTREF: - return btrfs_ioctl_get_subvol_rootref(file, argp); + return btrfs_ioctl_get_subvol_rootref(root, argp); case BTRFS_IOC_INO_LOOKUP_USER: return btrfs_ioctl_ino_lookup_user(file, argp); case FS_IOC_ENABLE_VERITY: -- Sahil ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] btrfs: reuse existing pointers from btrfs_ioctl 2022-01-05 8:30 ` [PATCH 1/1] " Sahil Kang @ 2022-01-05 9:53 ` Qu Wenruo 2022-01-06 15:03 ` David Sterba 2022-01-06 15:27 ` [PATCH 1/1] btrfs: reuse existing pointers " David Sterba 1 sibling, 1 reply; 10+ messages in thread From: Qu Wenruo @ 2022-01-05 9:53 UTC (permalink / raw) To: Sahil Kang, linux-btrfs On 2022/1/5 16:30, Sahil Kang wrote: > btrfs_ioctl already contains pointers to the inode and btrfs_root > structs, so we can pass them into the subfunctions instead of the > toplevel struct file. > > Signed-off-by: Sahil Kang <sahil.kang@asilaycomputing.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Just some small nitpicks inlined below. [...] > -static int btrfs_ioctl_get_subvol_rootref(struct file *file, void __user *argp) > +static int btrfs_ioctl_get_subvol_rootref(struct btrfs_root *subvol_root, > + void __user *argp) I guess you're using the @subvol_root naming is to avoid the conflicts in the existing @root defined locally. It may be a better idea to rename @root to @tree_root, or doesn't define it at all. Since the existing @root is only used in 3 locations. [...] > @@ -4873,7 +4867,7 @@ long btrfs_ioctl(struct file *file, unsigned int > > switch (cmd) { > case FS_IOC_GETVERSION: > - return btrfs_ioctl_getversion(file, argp); > + return btrfs_ioctl_getversion(inode, argp); > case FS_IOC_GETFSLABEL: > return btrfs_ioctl_get_fslabel(fs_info, argp); > case FS_IOC_SETFSLABEL: > @@ -4921,7 +4915,7 @@ long btrfs_ioctl(struct file *file, unsigned int > case BTRFS_IOC_TREE_SEARCH_V2: > return btrfs_ioctl_tree_search_v2(file, argp); > case BTRFS_IOC_INO_LOOKUP: > - return btrfs_ioctl_ino_lookup(file, argp); > + return btrfs_ioctl_ino_lookup(root, argp); > case BTRFS_IOC_INO_PATHS: > return btrfs_ioctl_ino_to_path(root, argp); > case BTRFS_IOC_LOGICAL_INO: > @@ -5000,7 +4994,7 @@ long btrfs_ioctl(struct file *file, unsigned int > case BTRFS_IOC_GET_SUBVOL_INFO: > return btrfs_ioctl_get_subvol_info(file, argp); > case BTRFS_IOC_GET_SUBVOL_ROOTREF: > - return btrfs_ioctl_get_subvol_rootref(file, argp); > + return btrfs_ioctl_get_subvol_rootref(root, argp); > case BTRFS_IOC_INO_LOOKUP_USER: > return btrfs_ioctl_ino_lookup_user(file, argp); > case FS_IOC_ENABLE_VERITY: There are some remaining ioctl sub-routines which you may want to further cleanup, as they don't really rely on @file to do write check (aka, RO operations): - btrfs_ioctl_subvol_getflags() - btrfs_ioctl_tree_search() - btrfs_ioctl_tree_search_v2() - btrfs_ioctl_send() - btrfs_ioctl_get_subvol_info() - btrfs_ioctl_ino_lookup_user() One suspicious ioctls, they should require write permission, but don't: - btrfs_ioctl_start_sync() If there is no running trans, it would bail out without doing anything, but I'm wondering if we should call mnt_want_write_file(). As this means, if we mount a subvolume read-only, but the fs still has some part mounted RW, we can start a sync using the read-only mounted part. Thanks, Qu ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] btrfs: reuse existing pointers from btrfs_ioctl 2022-01-05 9:53 ` Qu Wenruo @ 2022-01-06 15:03 ` David Sterba 2022-01-16 2:48 ` [PATCH 0/1] btrfs: reuse existing inode " Sahil Kang 0 siblings, 1 reply; 10+ messages in thread From: David Sterba @ 2022-01-06 15:03 UTC (permalink / raw) To: Qu Wenruo; +Cc: Sahil Kang, linux-btrfs On Wed, Jan 05, 2022 at 05:53:20PM +0800, Qu Wenruo wrote: > One suspicious ioctls, they should require write permission, but don't: > > - btrfs_ioctl_start_sync() > If there is no running trans, it would bail out without doing > anything, but I'm wondering if we should call mnt_want_write_file(). > > As this means, if we mount a subvolume read-only, but the fs still has > some part mounted RW, we can start a sync using the read-only mounted > part. We can possibly do the mnt_want_write but it should not propagate the errors, because calling sync on a fully read-only filesystem should not error out. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 0/1] btrfs: reuse existing inode from btrfs_ioctl 2022-01-06 15:03 ` David Sterba @ 2022-01-16 2:48 ` Sahil Kang 2022-01-16 2:48 ` [PATCH 1/1] " Sahil Kang 0 siblings, 1 reply; 10+ messages in thread From: Sahil Kang @ 2022-01-16 2:48 UTC (permalink / raw) To: linux-btrfs This is a follow up to misc-next/152adfc to incorporate Qu's review. Sahil Kang (1): btrfs: reuse existing inode from btrfs_ioctl fs/btrfs/ioctl.c | 43 ++++++++++++++++++------------------------- fs/btrfs/send.c | 4 ++-- fs/btrfs/send.h | 2 +- 3 files changed, 21 insertions(+), 28 deletions(-) -- Sahil ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] btrfs: reuse existing inode from btrfs_ioctl 2022-01-16 2:48 ` [PATCH 0/1] btrfs: reuse existing inode " Sahil Kang @ 2022-01-16 2:48 ` Sahil Kang 2022-01-17 13:39 ` David Sterba 0 siblings, 1 reply; 10+ messages in thread From: Sahil Kang @ 2022-01-16 2:48 UTC (permalink / raw) To: linux-btrfs btrfs_ioctl extracts inode from file so we can pass that into the subfunctions. Signed-off-by: Sahil Kang <sahil.kang@asilaycomputing.com> --- fs/btrfs/ioctl.c | 43 ++++++++++++++++++------------------------- fs/btrfs/send.c | 4 ++-- fs/btrfs/send.h | 2 +- 3 files changed, 21 insertions(+), 28 deletions(-) diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 26cae57b77aa..f202a9f18639 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -1941,10 +1941,9 @@ static noinline int btrfs_ioctl_snap_create_v2(struct file *file, return ret; } -static noinline int btrfs_ioctl_subvol_getflags(struct file *file, +static noinline int btrfs_ioctl_subvol_getflags(struct inode *inode, void __user *arg) { - struct inode *inode = file_inode(file); struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); struct btrfs_root *root = BTRFS_I(inode)->root; int ret = 0; @@ -2274,12 +2273,11 @@ static noinline int search_ioctl(struct inode *inode, return ret; } -static noinline int btrfs_ioctl_tree_search(struct file *file, - void __user *argp) +static noinline int btrfs_ioctl_tree_search(struct inode *inode, + void __user *argp) { struct btrfs_ioctl_search_args __user *uargs; struct btrfs_ioctl_search_key sk; - struct inode *inode; int ret; size_t buf_size; @@ -2293,7 +2291,6 @@ static noinline int btrfs_ioctl_tree_search(struct file *file, buf_size = sizeof(uargs->buf); - inode = file_inode(file); ret = search_ioctl(inode, &sk, &buf_size, uargs->buf); /* @@ -2308,12 +2305,11 @@ static noinline int btrfs_ioctl_tree_search(struct file *file, return ret; } -static noinline int btrfs_ioctl_tree_search_v2(struct file *file, +static noinline int btrfs_ioctl_tree_search_v2(struct inode *inode, void __user *argp) { struct btrfs_ioctl_search_args_v2 __user *uarg; struct btrfs_ioctl_search_args_v2 args; - struct inode *inode; int ret; size_t buf_size; const size_t buf_limit = SZ_16M; @@ -2332,7 +2328,6 @@ static noinline int btrfs_ioctl_tree_search_v2(struct file *file, if (buf_size > buf_limit) buf_size = buf_limit; - inode = file_inode(file); ret = search_ioctl(inode, &args.key, &buf_size, (char __user *)(&uarg->buf[0])); if (ret == 0 && copy_to_user(&uarg->key, &args.key, sizeof(args.key))) @@ -2666,7 +2661,7 @@ static int btrfs_ioctl_ino_lookup_user(struct file *file, void __user *argp) } /* Get the subvolume information in BTRFS_ROOT_ITEM and BTRFS_ROOT_BACKREF */ -static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp) +static int btrfs_ioctl_get_subvol_info(struct inode *inode, void __user *argp) { struct btrfs_ioctl_get_subvol_info_args *subvol_info; struct btrfs_fs_info *fs_info; @@ -2678,7 +2673,6 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp) struct extent_buffer *leaf; unsigned long item_off; unsigned long item_len; - struct inode *inode; int slot; int ret = 0; @@ -2692,7 +2686,6 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp) return -ENOMEM; } - inode = file_inode(file); fs_info = BTRFS_I(inode)->root->fs_info; /* Get root_item of inode's subvolume */ @@ -2786,12 +2779,11 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp) * Return ROOT_REF information of the subvolume containing this inode * except the subvolume name. */ -static int btrfs_ioctl_get_subvol_rootref(struct btrfs_root *subvol_root, +static int btrfs_ioctl_get_subvol_rootref(struct btrfs_root *root, void __user *argp) { struct btrfs_ioctl_get_subvol_rootref_args *rootrefs; struct btrfs_root_ref *rref; - struct btrfs_root *root; struct btrfs_path *path; struct btrfs_key key; struct extent_buffer *leaf; @@ -2810,14 +2802,13 @@ static int btrfs_ioctl_get_subvol_rootref(struct btrfs_root *subvol_root, return PTR_ERR(rootrefs); } - root = subvol_root->fs_info->tree_root; - objectid = subvol_root->root_key.objectid; - + objectid = root->root_key.objectid; key.objectid = objectid; key.type = BTRFS_ROOT_REF_KEY; key.offset = rootrefs->min_treeid; found = 0; + root = root->fs_info->tree_root; ret = btrfs_search_slot(NULL, root, &key, path, 0, 0); if (ret < 0) { goto out; @@ -4859,7 +4850,9 @@ static int btrfs_ioctl_set_features(struct file *file, void __user *arg) return ret; } -static int _btrfs_ioctl_send(struct file *file, void __user *argp, bool compat) +static int _btrfs_ioctl_send(struct inode *inode, + void __user *argp, + bool compat) { struct btrfs_ioctl_send_args *arg; int ret; @@ -4889,7 +4882,7 @@ static int _btrfs_ioctl_send(struct file *file, void __user *argp, bool compat) if (IS_ERR(arg)) return PTR_ERR(arg); } - ret = btrfs_ioctl_send(file, arg); + ret = btrfs_ioctl_send(inode, arg); kfree(arg); return ret; } @@ -4924,7 +4917,7 @@ long btrfs_ioctl(struct file *file, unsigned int case BTRFS_IOC_SNAP_DESTROY_V2: return btrfs_ioctl_snap_destroy(file, argp, true); case BTRFS_IOC_SUBVOL_GETFLAGS: - return btrfs_ioctl_subvol_getflags(file, argp); + return btrfs_ioctl_subvol_getflags(inode, argp); case BTRFS_IOC_SUBVOL_SETFLAGS: return btrfs_ioctl_subvol_setflags(file, argp); case BTRFS_IOC_DEFAULT_SUBVOL: @@ -4948,9 +4941,9 @@ long btrfs_ioctl(struct file *file, unsigned int case BTRFS_IOC_BALANCE: return btrfs_ioctl_balance(file, NULL); case BTRFS_IOC_TREE_SEARCH: - return btrfs_ioctl_tree_search(file, argp); + return btrfs_ioctl_tree_search(inode, argp); case BTRFS_IOC_TREE_SEARCH_V2: - return btrfs_ioctl_tree_search_v2(file, argp); + return btrfs_ioctl_tree_search_v2(inode, argp); case BTRFS_IOC_INO_LOOKUP: return btrfs_ioctl_ino_lookup(root, argp); case BTRFS_IOC_INO_PATHS: @@ -4999,10 +4992,10 @@ long btrfs_ioctl(struct file *file, unsigned int return btrfs_ioctl_set_received_subvol_32(file, argp); #endif case BTRFS_IOC_SEND: - return _btrfs_ioctl_send(file, argp, false); + return _btrfs_ioctl_send(inode, argp, false); #if defined(CONFIG_64BIT) && defined(CONFIG_COMPAT) case BTRFS_IOC_SEND_32: - return _btrfs_ioctl_send(file, argp, true); + return _btrfs_ioctl_send(inode, argp, true); #endif case BTRFS_IOC_GET_DEV_STATS: return btrfs_ioctl_get_dev_stats(fs_info, argp); @@ -5029,7 +5022,7 @@ long btrfs_ioctl(struct file *file, unsigned int case BTRFS_IOC_SET_FEATURES: return btrfs_ioctl_set_features(file, argp); case BTRFS_IOC_GET_SUBVOL_INFO: - return btrfs_ioctl_get_subvol_info(file, argp); + return btrfs_ioctl_get_subvol_info(inode, argp); case BTRFS_IOC_GET_SUBVOL_ROOTREF: return btrfs_ioctl_get_subvol_rootref(root, argp); case BTRFS_IOC_INO_LOOKUP_USER: diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c index d8ccb62aa7d2..ebf83c4b5e25 100644 --- a/fs/btrfs/send.c +++ b/fs/btrfs/send.c @@ -7473,10 +7473,10 @@ static void dedupe_in_progress_warn(const struct btrfs_root *root) root->root_key.objectid, root->dedupe_in_progress); } -long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg) +long btrfs_ioctl_send(struct inode *inode, struct btrfs_ioctl_send_args *arg) { int ret = 0; - struct btrfs_root *send_root = BTRFS_I(file_inode(mnt_file))->root; + struct btrfs_root *send_root = BTRFS_I(inode)->root; struct btrfs_fs_info *fs_info = send_root->fs_info; struct btrfs_root *clone_root; struct send_ctx *sctx = NULL; diff --git a/fs/btrfs/send.h b/fs/btrfs/send.h index 23bcefc84e49..08602fdd600a 100644 --- a/fs/btrfs/send.h +++ b/fs/btrfs/send.h @@ -126,7 +126,7 @@ enum { #define BTRFS_SEND_A_MAX (__BTRFS_SEND_A_MAX - 1) #ifdef __KERNEL__ -long btrfs_ioctl_send(struct file *mnt_file, struct btrfs_ioctl_send_args *arg); +long btrfs_ioctl_send(struct inode *inode, struct btrfs_ioctl_send_args *arg); #endif #endif -- Sahil ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] btrfs: reuse existing inode from btrfs_ioctl 2022-01-16 2:48 ` [PATCH 1/1] " Sahil Kang @ 2022-01-17 13:39 ` David Sterba 0 siblings, 0 replies; 10+ messages in thread From: David Sterba @ 2022-01-17 13:39 UTC (permalink / raw) To: Sahil Kang; +Cc: linux-btrfs On Sat, Jan 15, 2022 at 06:48:47PM -0800, Sahil Kang wrote: > btrfs_ioctl extracts inode from file so we can pass that into the > subfunctions. > > Signed-off-by: Sahil Kang <sahil.kang@asilaycomputing.com> Added to devel, thanks. > -static int _btrfs_ioctl_send(struct file *file, void __user *argp, bool compat) > +static int _btrfs_ioctl_send(struct inode *inode, > + void __user *argp, > + bool compat) Please keep the lines filled and don't split when not necessary. Fixed in the committed patch. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] btrfs: reuse existing pointers from btrfs_ioctl 2022-01-05 8:30 ` [PATCH 1/1] " Sahil Kang 2022-01-05 9:53 ` Qu Wenruo @ 2022-01-06 15:27 ` David Sterba 1 sibling, 0 replies; 10+ messages in thread From: David Sterba @ 2022-01-06 15:27 UTC (permalink / raw) To: Sahil Kang; +Cc: linux-btrfs On Wed, Jan 05, 2022 at 12:30:06AM -0800, Sahil Kang wrote: > btrfs_ioctl already contains pointers to the inode and btrfs_root > structs, so we can pass them into the subfunctions instead of the > toplevel struct file. > > Signed-off-by: Sahil Kang <sahil.kang@asilaycomputing.com> Added to misc-next, thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/1] btrfs: reuse existing pointers from btrfs_ioctl 2022-01-05 8:30 [PATCH 0/1] btrfs: reuse existing pointers from btrfs_ioctl Sahil Kang 2022-01-05 8:30 ` [PATCH 1/1] " Sahil Kang @ 2022-01-05 9:30 ` Qu Wenruo 2022-01-06 15:11 ` David Sterba 1 sibling, 1 reply; 10+ messages in thread From: Qu Wenruo @ 2022-01-05 9:30 UTC (permalink / raw) To: Sahil Kang, linux-btrfs On 2022/1/5 16:30, Sahil Kang wrote: > This is a small cleanup that reuses some of the existing pointers and > removes the duplicated dereferencing. Some quick glance indeed shows that we have pretty inconsistent arguments for a lot of ioctls, mostly switching between file/inode. So the cleanup looks good to me. But I also find that, there are ioctl sub-routines that requires file as it needs to call mnt_want_write_file(). I know I'm asking for too much, but maybe it's a good idea to separate those ioctl sub-routines into two types: - Those write ioctls which need @file to do mnt_want_write_file() check So they can use the existing @file argument as usual - Those read-only ioctls which don't need @file at all Then pass @inode/@root to those functions. (Personally speaking I prefer to pass @inode and let sub-routines to handle the root grabbing) Anyway, your patch looks like a good initial step. Will give more detailed review on the patch. Thanks, Qu > > There are other subfunctions that have similar/identical dereferencing > as the enclosing btrfs_ioctl, but I'm not sure if changing those is > welcomed since it would require changing their arg count. Right now, > all of the sub ioctl functions have essentially the same signature. > > For example, reusing the pointers for btrfs_ioctl_set_fslabel would > require passing in *fs_info and *root, in addition to its current > args. > > Sahil Kang (1): > btrfs: reuse existing pointers from btrfs_ioctl > > fs/btrfs/ioctl.c | 28 +++++++++++----------------- > 1 file changed, 11 insertions(+), 17 deletions(-) > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/1] btrfs: reuse existing pointers from btrfs_ioctl 2022-01-05 9:30 ` [PATCH 0/1] " Qu Wenruo @ 2022-01-06 15:11 ` David Sterba 0 siblings, 0 replies; 10+ messages in thread From: David Sterba @ 2022-01-06 15:11 UTC (permalink / raw) To: Qu Wenruo; +Cc: Sahil Kang, linux-btrfs On Wed, Jan 05, 2022 at 05:30:20PM +0800, Qu Wenruo wrote: > > > On 2022/1/5 16:30, Sahil Kang wrote: > > This is a small cleanup that reuses some of the existing pointers and > > removes the duplicated dereferencing. > > Some quick glance indeed shows that we have pretty inconsistent > arguments for a lot of ioctls, mostly switching between file/inode. > > So the cleanup looks good to me. Agreed. > But I also find that, there are ioctl sub-routines that requires file as > it needs to call mnt_want_write_file(). I've noticed there are some calls to file_mnt_user_ns that use the file. > I know I'm asking for too much, but maybe it's a good idea to separate > those ioctl sub-routines into two types: > > - Those write ioctls which need @file to do mnt_want_write_file() check > So they can use the existing @file argument as usual > > - Those read-only ioctls which don't need @file at all > Then pass @inode/@root to those functions. > (Personally speaking I prefer to pass @inode and let sub-routines to > handle the root grabbing) The main switch function btrfs_ioctl extracts all the pointers so I'd suggest to pass only the one that the ioctl callbacks need, ie. not an inode if it's just for the root pointer. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-01-17 13:40 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-05 8:30 [PATCH 0/1] btrfs: reuse existing pointers from btrfs_ioctl Sahil Kang 2022-01-05 8:30 ` [PATCH 1/1] " Sahil Kang 2022-01-05 9:53 ` Qu Wenruo 2022-01-06 15:03 ` David Sterba 2022-01-16 2:48 ` [PATCH 0/1] btrfs: reuse existing inode " Sahil Kang 2022-01-16 2:48 ` [PATCH 1/1] " Sahil Kang 2022-01-17 13:39 ` David Sterba 2022-01-06 15:27 ` [PATCH 1/1] btrfs: reuse existing pointers " David Sterba 2022-01-05 9:30 ` [PATCH 0/1] " Qu Wenruo 2022-01-06 15:11 ` David Sterba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).