| Message ID | 1549937931-15339-2-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc props.c cleanups | expand |
On Tue, Feb 12, 2019 at 10:18:49AM +0800, Anand Jain wrote: > btrfs_set_prop() is a redirect to __btrfs_set_prop() with the > transaction handler equal to NULL. And __btrfs_set_prop() inturn diectly > uses trans to do_setxattr() which when trans is NULL creates a transaction. That's right and I think that some of the callsites could actually pass the existing transaction to btrfs_set_prop instead of relying on the fallback behaviour. Also, the potential NULL as transaction handle is not a good thing from the API point of view and I'd like to reduce that to minimum (there are some justified cases). > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 3f9d7be30bf4..5a4ed2f66e09 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -284,7 +284,8 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg) > binode->flags &= ~BTRFS_INODE_COMPRESS; > binode->flags |= BTRFS_INODE_NOCOMPRESS; > > - ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); > + ret = btrfs_set_prop(NULL, inode, "btrfs.compression", NULL, 0, > + 0); > if (ret && ret != -ENODATA) > goto out_drop; > } else if (fsflags & FS_COMPR_FL) { > @@ -302,13 +303,14 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg) > if (!comp || comp[0] == 0) > comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB); > > - ret = btrfs_set_prop(inode, "btrfs.compression", > - comp, strlen(comp), 0); > + ret = btrfs_set_prop(NULL, inode, "btrfs.compression", comp, > + strlen(comp), 0); > if (ret) > goto out_drop; > > } else { > - ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); > + ret = btrfs_set_prop(NULL, inode, "btrfs.compression", NULL, 0, > + 0); > if (ret && ret != -ENODATA) > goto out_drop; > binode->flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS); When the if-else block ends, there's a new transaction started, this seems unnecessary. > diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c > index dc6140013ae8..4525a2a4d1cd 100644 > --- a/fs/btrfs/props.c > +++ b/fs/btrfs/props.c > @@ -85,12 +85,9 @@ static const struct hlist_head *find_prop_handlers_by_hash(const u64 hash) > return NULL; > } > > -static int __btrfs_set_prop(struct btrfs_trans_handle *trans, > - struct inode *inode, > - const char *name, > - const char *value, > - size_t value_len, > - int flags) > +int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, > + const char *name, const char *value, size_t value_len, > + int flags) > { > const struct prop_handler *handler; > int ret; > @@ -133,15 +130,6 @@ static int __btrfs_set_prop(struct btrfs_trans_handle *trans, > return 0; > } > > -int btrfs_set_prop(struct inode *inode, > - const char *name, > - const char *value, > - size_t value_len, > - int flags) > -{ > - return __btrfs_set_prop(NULL, inode, name, value, value_len, flags); I agree that one function would be better here, with defined semantics of 'trans'. There are more cleanups around the properties, also in the xattr handling functions. This patchset is a step in the right direction and I think the cleanups could be more extensive. The idea for the xattr function: - the VFS callbacks (like btrfs_xattr_handler_set_prop) will start the transaction themselves and pass the handle to the prop function - a xattr function that does not take a valid trans could be named like btrfs_setxattr_notrans and will start the transaction, ie. lifting that to the callers - there's a maze of the xattr callbacks so assertions are needed
On 2/21/19 3:00 AM, David Sterba wrote: > On Tue, Feb 12, 2019 at 10:18:49AM +0800, Anand Jain wrote: >> btrfs_set_prop() is a redirect to __btrfs_set_prop() with the >> transaction handler equal to NULL. And __btrfs_set_prop() inturn diectly >> uses trans to do_setxattr() which when trans is NULL creates a transaction. > > That's right and I think that some of the callsites could actually pass > the existing transaction to btrfs_set_prop instead of relying on the > fallback behaviour. > > Also, the potential NULL as transaction handle is not a good thing from > the API point of view and I'd like to reduce that to minimum (there are > some justified cases). > >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >> index 3f9d7be30bf4..5a4ed2f66e09 100644 >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c >> @@ -284,7 +284,8 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg) >> binode->flags &= ~BTRFS_INODE_COMPRESS; >> binode->flags |= BTRFS_INODE_NOCOMPRESS; >> >> - ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); >> + ret = btrfs_set_prop(NULL, inode, "btrfs.compression", NULL, 0, >> + 0); >> if (ret && ret != -ENODATA) >> goto out_drop; >> } else if (fsflags & FS_COMPR_FL) { >> @@ -302,13 +303,14 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg) >> if (!comp || comp[0] == 0) >> comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB); >> >> - ret = btrfs_set_prop(inode, "btrfs.compression", >> - comp, strlen(comp), 0); >> + ret = btrfs_set_prop(NULL, inode, "btrfs.compression", comp, >> + strlen(comp), 0); >> if (ret) >> goto out_drop; >> >> } else { >> - ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); >> + ret = btrfs_set_prop(NULL, inode, "btrfs.compression", NULL, 0, >> + 0); >> if (ret && ret != -ENODATA) >> goto out_drop; >> binode->flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS); > > When the if-else block ends, there's a new transaction started, this > seems unnecessary. > >> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c >> index dc6140013ae8..4525a2a4d1cd 100644 >> --- a/fs/btrfs/props.c >> +++ b/fs/btrfs/props.c >> @@ -85,12 +85,9 @@ static const struct hlist_head *find_prop_handlers_by_hash(const u64 hash) >> return NULL; >> } >> >> -static int __btrfs_set_prop(struct btrfs_trans_handle *trans, >> - struct inode *inode, >> - const char *name, >> - const char *value, >> - size_t value_len, >> - int flags) >> +int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, >> + const char *name, const char *value, size_t value_len, >> + int flags) >> { >> const struct prop_handler *handler; >> int ret; >> @@ -133,15 +130,6 @@ static int __btrfs_set_prop(struct btrfs_trans_handle *trans, >> return 0; >> } >> >> -int btrfs_set_prop(struct inode *inode, >> - const char *name, >> - const char *value, >> - size_t value_len, >> - int flags) >> -{ >> - return __btrfs_set_prop(NULL, inode, name, value, value_len, flags); > > I agree that one function would be better here, with defined semantics > of 'trans'. > > There are more cleanups around the properties, also in the xattr > handling functions. This patchset is a step in the right direction and I > think the cleanups could be more extensive. > > The idea for the xattr function: > > - the VFS callbacks (like btrfs_xattr_handler_set_prop) will start the > transaction themselves and pass the handle to the prop function These vfs callbacks must handle the update inode part as well, which btrfs_setxattr() skips if trans != NULL. ---- inode_inc_iversion(inode); inode->i_ctime = current_time(inode); set_bit(BTRFS_INODE_COPY_EVERYTHING, &BTRFS_I(inode)->runtime_flags); ret = btrfs_update_inode(trans, root, inode); ---- Will give a try. > - a xattr function that does not take a valid trans could be named like > btrfs_setxattr_notrans and will start the transaction, ie. lifting > that to the callers Yep. Also btrfs_set_props_notrans() and btrfs_set_acl_notrans(). > - there's a maze of the xattr callbacks so assertions are needed ok. Thanks, Anand
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 3f9d7be30bf4..5a4ed2f66e09 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -284,7 +284,8 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg) binode->flags &= ~BTRFS_INODE_COMPRESS; binode->flags |= BTRFS_INODE_NOCOMPRESS; - ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); + ret = btrfs_set_prop(NULL, inode, "btrfs.compression", NULL, 0, + 0); if (ret && ret != -ENODATA) goto out_drop; } else if (fsflags & FS_COMPR_FL) { @@ -302,13 +303,14 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg) if (!comp || comp[0] == 0) comp = btrfs_compress_type2str(BTRFS_COMPRESS_ZLIB); - ret = btrfs_set_prop(inode, "btrfs.compression", - comp, strlen(comp), 0); + ret = btrfs_set_prop(NULL, inode, "btrfs.compression", comp, + strlen(comp), 0); if (ret) goto out_drop; } else { - ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); + ret = btrfs_set_prop(NULL, inode, "btrfs.compression", NULL, 0, + 0); if (ret && ret != -ENODATA) goto out_drop; binode->flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS); diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c index dc6140013ae8..4525a2a4d1cd 100644 --- a/fs/btrfs/props.c +++ b/fs/btrfs/props.c @@ -85,12 +85,9 @@ static const struct hlist_head *find_prop_handlers_by_hash(const u64 hash) return NULL; } -static int __btrfs_set_prop(struct btrfs_trans_handle *trans, - struct inode *inode, - const char *name, - const char *value, - size_t value_len, - int flags) +int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, + const char *name, const char *value, size_t value_len, + int flags) { const struct prop_handler *handler; int ret; @@ -133,15 +130,6 @@ static int __btrfs_set_prop(struct btrfs_trans_handle *trans, return 0; } -int btrfs_set_prop(struct inode *inode, - const char *name, - const char *value, - size_t value_len, - int flags) -{ - return __btrfs_set_prop(NULL, inode, name, value, value_len, flags); -} - static int iterate_object_props(struct btrfs_root *root, struct btrfs_path *path, u64 objectid, @@ -313,8 +301,8 @@ static int inherit_props(struct btrfs_trans_handle *trans, num_bytes, BTRFS_RESERVE_NO_FLUSH); if (ret) goto out; - ret = __btrfs_set_prop(trans, inode, h->xattr_name, - value, strlen(value), 0); + ret = btrfs_set_prop(trans, inode, h->xattr_name, value, + strlen(value), 0); btrfs_block_rsv_release(fs_info, trans->block_rsv, num_bytes); if (ret) goto out; diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h index 618815b4f9d5..9dbdae47cf27 100644 --- a/fs/btrfs/props.h +++ b/fs/btrfs/props.h @@ -10,10 +10,8 @@ void __init btrfs_props_init(void); -int btrfs_set_prop(struct inode *inode, - const char *name, - const char *value, - size_t value_len, +int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, + const char *name, const char *value, size_t value_len, int flags); int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path); diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c index f141b45ce349..499bb79ba135 100644 --- a/fs/btrfs/xattr.c +++ b/fs/btrfs/xattr.c @@ -379,7 +379,7 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler, size_t size, int flags) { name = xattr_full_name(handler, name); - return btrfs_set_prop(inode, name, value, size, flags); + return btrfs_set_prop(NULL, inode, name, value, size, flags); } static const struct xattr_handler btrfs_security_xattr_handler = {
btrfs_set_prop() is a redirect to __btrfs_set_prop() with the transaction handler equal to NULL. And __btrfs_set_prop() inturn diectly uses trans to do_setxattr() which when trans is NULL creates a transaction. Instead rename __btrfs_set_prop() to btrfs_set_prop(), and update the caller with NULL argument. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v3: none v2: none fs/btrfs/ioctl.c | 10 ++++++---- fs/btrfs/props.c | 22 +++++----------------- fs/btrfs/props.h | 6 ++---- fs/btrfs/xattr.c | 2 +- 4 files changed, 14 insertions(+), 26 deletions(-)