| Message ID | 20190402100742.8355-5-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix property bugs | expand |
On 2.04.19 г. 13:07 ч., 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()). > > 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 */ This comment brings no value. Either explain *why* it can be removed or remove it altogether. > + 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); (not related to this patch) but should the reservation be released after setxattr is called, even if it succeeds, since we need to hold it until the reservation is committed. E.g. if we have to inherit 2 props then in the first iteration of the loop we reserve space, we call btrfs_Setxattr with enough reserved space, then in the 2nd reservation we need another reservation on top of the previous one, no ? Otherwise the change per se looks good: Reviewed-by: Nikolay Borisov <nborisov@suse.com> > if (ret) > - goto out; > + return ret; > } > - ret = 0; > -out: > - return ret; > + > + return 0; > } > > int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans, >
On Tue, Apr 02, 2019 at 04:35:12PM +0300, Nikolay Borisov wrote: > On 2.04.19 г. 13:07 ч., 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()). > > > > 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 */ > > This comment brings no value. Either explain *why* it can be removed or > remove it altogether. > > > + 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); > > (not related to this patch) but should the reservation be released after > setxattr is called, even if it succeeds, since we need to hold it until > the reservation is committed. E.g. if we have to inherit 2 props then in > the first iteration of the loop we reserve space, we call btrfs_Setxattr > with enough reserved space, then in the 2nd reservation we need another > reservation on top of the previous one, no ? I think that's the problem Josef's patches were supposed to fix.
On Tue, Apr 02, 2019 at 06:07:41PM +0800, 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()). > > Signed-off-by: Anand Jain <anand.jain@oracle.com> Reviewed-by: David Sterba <dsterba@suse.com>
On Tue, Apr 02, 2019 at 04:35:12PM +0300, Nikolay Borisov wrote: > > @@ -367,20 +367,35 @@ static int inherit_props(struct btrfs_trans_handle *trans, > > if (!value) > > continue; > > > > + /* may be removed */ > > This comment brings no value. Either explain *why* it can be removed or > remove it altogether. Comment replaced by /* * This is not strictly necessary as the property should be * valid, but in case it isn't, don't propagate it futher. */ I'd keep it the because that's what the direct opencoding transformation would do. If we want to remove it, then the reasoning should be provided.
On 3/4/19 7:24 AM, David Sterba wrote: > On Tue, Apr 02, 2019 at 04:35:12PM +0300, Nikolay Borisov wrote: >>> @@ -367,20 +367,35 @@ static int inherit_props(struct btrfs_trans_handle *trans, >>> if (!value) >>> continue; >>> >>> + /* may be removed */ >> >> This comment brings no value. Either explain *why* it can be removed or >> remove it altogether. > > Comment replaced by > > /* > * This is not strictly necessary as the property should be > * valid, but in case it isn't, don't propagate it futher. > */ > > I'd keep it the because that's what the direct opencoding transformation would > do. If we want to remove it, then the reasoning should be provided. Agreed with the reasoning. 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(-)