| Message ID | 1552539906-29703-4-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix property bugs | expand |
On 14.03.19 г. 7:05 ч., Anand Jain wrote: > When an inode inherits property from its parent, we call btrfs_set_prop(). > btrfs_set_prop() does an elaborate checks, which is not required in the > context of inheriting a property. Instead just open-code only the required > items from btrfs_set_prop() and then call btrfs_setxattr() directly. So > now the only user of btrfs_set_prop() is gone, (except for the wraper > function btrfs_set_prop_trans()). Um, no: git grep -P '(?<!__)btrfs_set_prop' fs/btrfs/ fs/btrfs/ioctl.c: ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); fs/btrfs/ioctl.c: ret = btrfs_set_prop(inode, "btrfs.compression", fs/btrfs/ioctl.c: ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); fs/btrfs/props.c:int btrfs_set_prop(struct inode *inode, fs/btrfs/props.h:int btrfs_set_prop(struct inode *inode, fs/btrfs/xattr.c: return btrfs_set_prop(inode, name, value, size, flags); Rebase the patch on misc-next and remove the last sentence since it's factually wrong. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/props.c | 27 +++++++++++++++++++++------ > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c > index 72a06c4d3c70..fb84e76f3b1d 100644 > --- a/fs/btrfs/props.c > +++ b/fs/btrfs/props.c > @@ -367,20 +367,35 @@ static int inherit_props(struct btrfs_trans_handle *trans, > if (!value) > continue; > > + /* may be removed */ > + ret = h->validate(value, strlen(value)); > + if (ret) > + continue; > + > num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1); > ret = btrfs_block_rsv_add(root, trans->block_rsv, > num_bytes, BTRFS_RESERVE_NO_FLUSH); > if (ret) > - goto out; > - ret = btrfs_set_prop(trans, inode, h->xattr_name, value, What branch is this code based on, currently misc-next is using (and has been since 2014) __btrfs_set_prop yet you remove btrfs_set_prop? > + return ret; > + > + ret = btrfs_setxattr(trans, inode, h->xattr_name, value, > strlen(value), 0); > + if (!ret) { > + ret = h->apply(inode, value, strlen(value)); > + if (ret) > + btrfs_setxattr(trans, inode, h->xattr_name, > + NULL, 0, 0); > + else > + set_bit(BTRFS_INODE_HAS_PROPS, > + &BTRFS_I(inode)->runtime_flags); > + } > + > btrfs_block_rsv_release(fs_info, trans->block_rsv, num_bytes); > if (ret) > - goto out; > + return ret; > } > - ret = 0; > -out: > - return ret; > + > + return 0; > } > > int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans, >
On 3/14/19 4:57 PM, Nikolay Borisov wrote: > > > On 14.03.19 г. 7:05 ч., Anand Jain wrote: >> When an inode inherits property from its parent, we call btrfs_set_prop(). >> btrfs_set_prop() does an elaborate checks, which is not required in the >> context of inheriting a property. Instead just open-code only the required >> items from btrfs_set_prop() and then call btrfs_setxattr() directly. So >> now the only user of btrfs_set_prop() is gone, (except for the wraper >> function btrfs_set_prop_trans()). > > Um, no: > > git grep -P '(?<!__)btrfs_set_prop' fs/btrfs/ > fs/btrfs/ioctl.c: ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); > fs/btrfs/ioctl.c: ret = btrfs_set_prop(inode, "btrfs.compression", > fs/btrfs/ioctl.c: ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0); > fs/btrfs/props.c:int btrfs_set_prop(struct inode *inode, > fs/btrfs/props.h:int btrfs_set_prop(struct inode *inode, > fs/btrfs/xattr.c: return btrfs_set_prop(inode, name, value, size, flags); > > > Rebase the patch on misc-next and remove the last sentence > since it's factually wrong. Oh. This is based on misc-5.2. Oops I should have mentioned. Thanks. Anand
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c index 72a06c4d3c70..fb84e76f3b1d 100644 --- a/fs/btrfs/props.c +++ b/fs/btrfs/props.c @@ -367,20 +367,35 @@ static int inherit_props(struct btrfs_trans_handle *trans, if (!value) continue; + /* may be removed */ + ret = h->validate(value, strlen(value)); + if (ret) + continue; + num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1); ret = btrfs_block_rsv_add(root, trans->block_rsv, num_bytes, BTRFS_RESERVE_NO_FLUSH); if (ret) - goto out; - ret = btrfs_set_prop(trans, inode, h->xattr_name, value, + return ret; + + ret = btrfs_setxattr(trans, inode, h->xattr_name, value, strlen(value), 0); + if (!ret) { + ret = h->apply(inode, value, strlen(value)); + if (ret) + btrfs_setxattr(trans, inode, h->xattr_name, + NULL, 0, 0); + else + set_bit(BTRFS_INODE_HAS_PROPS, + &BTRFS_I(inode)->runtime_flags); + } + btrfs_block_rsv_release(fs_info, trans->block_rsv, num_bytes); if (ret) - goto out; + return ret; } - ret = 0; -out: - return ret; + + return 0; } int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans,
When an inode inherits property from its parent, we call btrfs_set_prop(). btrfs_set_prop() does an elaborate checks, which is not required in the context of inheriting a property. Instead just open-code only the required items from btrfs_set_prop() and then call btrfs_setxattr() directly. So now the only user of btrfs_set_prop() is gone, (except for the wraper function btrfs_set_prop_trans()). Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/props.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)