| Message ID | 1550857192-10513-7-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc props.c cleanups | expand |
On Sat, Feb 23, 2019 at 01:39:48AM +0800, Anand Jain wrote: > In preparation to make trans argument of btrfs_setxattr() a mandatory, > start the transaction in btrfs_set_prop_trans. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > v4: born > fs/btrfs/props.c | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c > index e581da1bfbb6..f878ba3160f0 100644 > --- a/fs/btrfs/props.c > +++ b/fs/btrfs/props.c > @@ -4,6 +4,7 @@ > */ > > #include <linux/hashtable.h> > +#include <linux/iversion.h> > #include "props.h" > #include "btrfs_inode.h" > #include "transaction.h" > @@ -103,7 +104,25 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, > int btrfs_set_prop_trans(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); > + struct btrfs_root *root = BTRFS_I(inode)->root; > + struct btrfs_trans_handle *trans; > + int ret; > + > + trans = btrfs_start_transaction(root, 2); > + if (IS_ERR(trans)) > + return PTR_ERR(trans); > + > + ret = btrfs_set_prop(trans, inode, name, value, value_len, flags); > + > + if (!ret) { > + 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); > + ASSERT(!ret); This is not right. The previous code uses BUG_ON which is also not right, but does not silently continue if asserts are compiled out. Please add proper error handling here. > + } > + btrfs_end_transaction(trans); > + return ret; > } > > static int iterate_object_props(struct btrfs_root *root, > -- > 1.8.3.1
On 2/28/19 12:08 AM, David Sterba wrote: > On Sat, Feb 23, 2019 at 01:39:48AM +0800, Anand Jain wrote: >> In preparation to make trans argument of btrfs_setxattr() a mandatory, >> start the transaction in btrfs_set_prop_trans. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> v4: born >> fs/btrfs/props.c | 21 ++++++++++++++++++++- >> 1 file changed, 20 insertions(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c >> index e581da1bfbb6..f878ba3160f0 100644 >> --- a/fs/btrfs/props.c >> +++ b/fs/btrfs/props.c >> @@ -4,6 +4,7 @@ >> */ >> >> #include <linux/hashtable.h> >> +#include <linux/iversion.h> >> #include "props.h" >> #include "btrfs_inode.h" >> #include "transaction.h" >> @@ -103,7 +104,25 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, >> int btrfs_set_prop_trans(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); >> + struct btrfs_root *root = BTRFS_I(inode)->root; >> + struct btrfs_trans_handle *trans; >> + int ret; >> + >> + trans = btrfs_start_transaction(root, 2); >> + if (IS_ERR(trans)) >> + return PTR_ERR(trans); >> + >> + ret = btrfs_set_prop(trans, inode, name, value, value_len, flags); >> + >> + if (!ret) { >> + 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); >> + ASSERT(!ret); > > This is not right. The previous code uses BUG_ON which is also not > right, but does not silently continue if asserts are compiled out. > Please add proper error handling here. Error handling should save and undo of inode version, i_ctime and runtime_flags. Is a new patch for this OK? and here will use BUG_ON as in the original. Thanks, Anand >> + } >> + btrfs_end_transaction(trans); >> + return ret; >> } >> >> static int iterate_object_props(struct btrfs_root *root, >> -- >> 1.8.3.1
On Thu, Feb 28, 2019 at 06:36:40PM +0800, Anand Jain wrote: > >> + if (!ret) { > >> + 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); > >> + ASSERT(!ret); > > > > This is not right. The previous code uses BUG_ON which is also not > > right, but does not silently continue if asserts are compiled out. > > Please add proper error handling here. > > Error handling should save and undo of inode version, i_ctime and > runtime_flags. Is a new patch for this OK? and here will use BUG_ON > as in the original. Ok, use BUG_ON, it's effectively only a code copy. The error handling could be tricky here.
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c index e581da1bfbb6..f878ba3160f0 100644 --- a/fs/btrfs/props.c +++ b/fs/btrfs/props.c @@ -4,6 +4,7 @@ */ #include <linux/hashtable.h> +#include <linux/iversion.h> #include "props.h" #include "btrfs_inode.h" #include "transaction.h" @@ -103,7 +104,25 @@ static int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, int btrfs_set_prop_trans(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); + struct btrfs_root *root = BTRFS_I(inode)->root; + struct btrfs_trans_handle *trans; + int ret; + + trans = btrfs_start_transaction(root, 2); + if (IS_ERR(trans)) + return PTR_ERR(trans); + + ret = btrfs_set_prop(trans, inode, name, value, value_len, flags); + + if (!ret) { + 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); + ASSERT(!ret); + } + btrfs_end_transaction(trans); + return ret; } static int iterate_object_props(struct btrfs_root *root,
In preparation to make trans argument of btrfs_setxattr() a mandatory, start the transaction in btrfs_set_prop_trans. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v4: born fs/btrfs/props.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)