* [PATCH v3 0/3] Misc props.c cleanups
@ 2019-02-12 2:18 Anand Jain
2019-02-12 2:18 ` [PATCH v3 1/3] btrfs: kill __btrfs_set_prop() Anand Jain
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Anand Jain @ 2019-02-12 2:18 UTC (permalink / raw)
To: linux-btrfs
v3: Merge patch 2/5 and 3/5 as in v1.
Not included 1/5 in v1 as its already integrated in misc-next.
While adding the readmirror property found few cleanup things which
can be fixed. As these aren't part of upcoming readmirror property
I am sending these separately.
Anand Jain (3):
btrfs: kill __btrfs_set_prop()
btrfs: drop redundant forward declaration in props.c
btrfs: trivial, fix c coding style
fs/btrfs/ioctl.c | 10 +--
fs/btrfs/props.c | 201 +++++++++++++++++++++++++------------------------------
fs/btrfs/props.h | 6 +-
fs/btrfs/xattr.c | 2 +-
4 files changed, 99 insertions(+), 120 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/3] btrfs: kill __btrfs_set_prop()
2019-02-12 2:18 [PATCH v3 0/3] Misc props.c cleanups Anand Jain
@ 2019-02-12 2:18 ` Anand Jain
2019-02-20 19:00 ` David Sterba
2019-02-12 2:18 ` [PATCH v3 2/3] btrfs: drop redundant forward declaration in props.c Anand Jain
2019-02-12 2:18 ` [PATCH v3 3/3] btrfs: trivial, fix c coding style Anand Jain
2 siblings, 1 reply; 6+ messages in thread
From: Anand Jain @ 2019-02-12 2:18 UTC (permalink / raw)
To: linux-btrfs
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(-)
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 = {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/3] btrfs: drop redundant forward declaration in props.c
2019-02-12 2:18 [PATCH v3 0/3] Misc props.c cleanups Anand Jain
2019-02-12 2:18 ` [PATCH v3 1/3] btrfs: kill __btrfs_set_prop() Anand Jain
@ 2019-02-12 2:18 ` Anand Jain
2019-02-12 2:18 ` [PATCH v3 3/3] btrfs: trivial, fix c coding style Anand Jain
2 siblings, 0 replies; 6+ messages in thread
From: Anand Jain @ 2019-02-12 2:18 UTC (permalink / raw)
To: linux-btrfs
Drop forward declaration of the functions,
prop_compression_validate(), prop_compression_apply() and
prop_compression_extract(). By moving prop_handlers[], btrfs_props_init()
prop_compression_validate(), prop_compression_apply() and
prop_compression_extract() appropriately within the file. No functional
changes.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
---
v3: merge with 2/5 and 3/5 as in v1
v2: none
fs/btrfs/props.c | 163 +++++++++++++++++++++++++++----------------------------
1 file changed, 79 insertions(+), 84 deletions(-)
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 4525a2a4d1cd..77a03076b18e 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -23,36 +23,6 @@ struct prop_handler {
int inheritable;
};
-static int prop_compression_validate(const char *value, size_t len);
-static int prop_compression_apply(struct inode *inode,
- const char *value,
- size_t len);
-static const char *prop_compression_extract(struct inode *inode);
-
-static struct prop_handler prop_handlers[] = {
- {
- .xattr_name = XATTR_BTRFS_PREFIX "compression",
- .validate = prop_compression_validate,
- .apply = prop_compression_apply,
- .extract = prop_compression_extract,
- .inheritable = 1
- },
-};
-
-void __init btrfs_props_init(void)
-{
- int i;
-
- hash_init(prop_handlers_ht);
-
- for (i = 0; i < ARRAY_SIZE(prop_handlers); i++) {
- struct prop_handler *p = &prop_handlers[i];
- u64 h = btrfs_name_hash(p->xattr_name, strlen(p->xattr_name));
-
- hash_add(prop_handlers_ht, &p->node, h);
- }
-}
-
static const struct hlist_head *find_prop_handlers_by_hash(const u64 hash)
{
struct hlist_head *h;
@@ -271,6 +241,78 @@ int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path)
return ret;
}
+static int prop_compression_validate(const char *value, size_t len)
+{
+ if (!value)
+ return 0;
+
+ if (!strncmp("lzo", value, len))
+ return 0;
+ else if (!strncmp("zlib", value, len))
+ return 0;
+ else if (!strncmp("zstd", value, len))
+ return 0;
+
+ return -EINVAL;
+}
+
+static int prop_compression_apply(struct inode *inode, const char *value,
+ size_t len)
+{
+ struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
+ int type;
+
+ if (len == 0) {
+ BTRFS_I(inode)->flags |= BTRFS_INODE_NOCOMPRESS;
+ BTRFS_I(inode)->flags &= ~BTRFS_INODE_COMPRESS;
+ BTRFS_I(inode)->prop_compress = BTRFS_COMPRESS_NONE;
+
+ return 0;
+ }
+
+ if (!strncmp("lzo", value, 3)) {
+ type = BTRFS_COMPRESS_LZO;
+ btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
+ } else if (!strncmp("zlib", value, 4)) {
+ type = BTRFS_COMPRESS_ZLIB;
+ } else if (!strncmp("zstd", value, len)) {
+ type = BTRFS_COMPRESS_ZSTD;
+ btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
+ } else {
+ return -EINVAL;
+ }
+
+ BTRFS_I(inode)->flags &= ~BTRFS_INODE_NOCOMPRESS;
+ BTRFS_I(inode)->flags |= BTRFS_INODE_COMPRESS;
+ BTRFS_I(inode)->prop_compress = type;
+
+ return 0;
+}
+
+static const char *prop_compression_extract(struct inode *inode)
+{
+ switch (BTRFS_I(inode)->prop_compress) {
+ case BTRFS_COMPRESS_ZLIB:
+ case BTRFS_COMPRESS_LZO:
+ case BTRFS_COMPRESS_ZSTD:
+ return btrfs_compress_type2str(BTRFS_I(inode)->prop_compress);
+ default:
+ break;
+ }
+
+ return NULL;
+}
+
+static struct prop_handler prop_handlers[] = {
+ {
+ .xattr_name = XATTR_BTRFS_PREFIX "compression",
+ .validate = prop_compression_validate,
+ .apply = prop_compression_apply,
+ .extract = prop_compression_extract,
+ .inheritable = 1
+ },
+};
+
static int inherit_props(struct btrfs_trans_handle *trans,
struct inode *inode,
struct inode *parent)
@@ -352,64 +394,17 @@ int btrfs_subvol_inherit_props(struct btrfs_trans_handle *trans,
return ret;
}
-static int prop_compression_validate(const char *value, size_t len)
-{
- if (!strncmp("lzo", value, len))
- return 0;
- else if (!strncmp("zlib", value, len))
- return 0;
- else if (!strncmp("zstd", value, len))
- return 0;
-
- return -EINVAL;
-}
-
-static int prop_compression_apply(struct inode *inode,
- const char *value,
- size_t len)
+void __init btrfs_props_init(void)
{
- struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
- int type;
-
- if (len == 0) {
- BTRFS_I(inode)->flags |= BTRFS_INODE_NOCOMPRESS;
- BTRFS_I(inode)->flags &= ~BTRFS_INODE_COMPRESS;
- BTRFS_I(inode)->prop_compress = BTRFS_COMPRESS_NONE;
-
- return 0;
- }
-
- if (!strncmp("lzo", value, 3)) {
- type = BTRFS_COMPRESS_LZO;
- btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
- } else if (!strncmp("zlib", value, 4)) {
- type = BTRFS_COMPRESS_ZLIB;
- } else if (!strncmp("zstd", value, len)) {
- type = BTRFS_COMPRESS_ZSTD;
- btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
- } else {
- return -EINVAL;
- }
+ int i;
- BTRFS_I(inode)->flags &= ~BTRFS_INODE_NOCOMPRESS;
- BTRFS_I(inode)->flags |= BTRFS_INODE_COMPRESS;
- BTRFS_I(inode)->prop_compress = type;
+ hash_init(prop_handlers_ht);
- return 0;
-}
+ for (i = 0; i < ARRAY_SIZE(prop_handlers); i++) {
+ struct prop_handler *p = &prop_handlers[i];
+ u64 h = btrfs_name_hash(p->xattr_name, strlen(p->xattr_name));
-static const char *prop_compression_extract(struct inode *inode)
-{
- switch (BTRFS_I(inode)->prop_compress) {
- case BTRFS_COMPRESS_ZLIB:
- case BTRFS_COMPRESS_LZO:
- case BTRFS_COMPRESS_ZSTD:
- return btrfs_compress_type2str(BTRFS_I(inode)->prop_compress);
- default:
- break;
+ hash_add(prop_handlers_ht, &p->node, h);
}
-
- return NULL;
}
-
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 3/3] btrfs: trivial, fix c coding style
2019-02-12 2:18 [PATCH v3 0/3] Misc props.c cleanups Anand Jain
2019-02-12 2:18 ` [PATCH v3 1/3] btrfs: kill __btrfs_set_prop() Anand Jain
2019-02-12 2:18 ` [PATCH v3 2/3] btrfs: drop redundant forward declaration in props.c Anand Jain
@ 2019-02-12 2:18 ` Anand Jain
2 siblings, 0 replies; 6+ messages in thread
From: Anand Jain @ 2019-02-12 2:18 UTC (permalink / raw)
To: linux-btrfs
Maintain the lines extented upto 80 char where possible, and indent the
argument.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v3: changelog added.
fs/btrfs/props.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 77a03076b18e..3c15f19bfd2f 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -70,8 +70,8 @@ int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
return -EINVAL;
if (value_len == 0) {
- ret = btrfs_setxattr(trans, inode, handler->xattr_name,
- NULL, 0, flags);
+ ret = btrfs_setxattr(trans, inode, handler->xattr_name, NULL, 0,
+ flags);
if (ret)
return ret;
@@ -101,12 +101,10 @@ int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
}
static int iterate_object_props(struct btrfs_root *root,
- struct btrfs_path *path,
- u64 objectid,
+ struct btrfs_path *path, u64 objectid,
void (*iterator)(void *,
const struct prop_handler *,
- const char *,
- size_t),
+ const char *, size_t),
void *ctx)
{
int ret;
@@ -211,10 +209,8 @@ static int iterate_object_props(struct btrfs_root *root,
return ret;
}
-static void inode_prop_iterator(void *ctx,
- const struct prop_handler *handler,
- const char *value,
- size_t len)
+static void inode_prop_iterator(void *ctx, const struct prop_handler *handler,
+ const char *value, size_t len)
{
struct inode *inode = ctx;
struct btrfs_root *root = BTRFS_I(inode)->root;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/3] btrfs: kill __btrfs_set_prop()
2019-02-12 2:18 ` [PATCH v3 1/3] btrfs: kill __btrfs_set_prop() Anand Jain
@ 2019-02-20 19:00 ` David Sterba
2019-02-21 9:03 ` Anand Jain
0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2019-02-20 19:00 UTC (permalink / raw)
To: Anand Jain; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/3] btrfs: kill __btrfs_set_prop()
2019-02-20 19:00 ` David Sterba
@ 2019-02-21 9:03 ` Anand Jain
0 siblings, 0 replies; 6+ messages in thread
From: Anand Jain @ 2019-02-21 9:03 UTC (permalink / raw)
To: dsterba, linux-btrfs
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-21 9:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-12 2:18 [PATCH v3 0/3] Misc props.c cleanups Anand Jain
2019-02-12 2:18 ` [PATCH v3 1/3] btrfs: kill __btrfs_set_prop() Anand Jain
2019-02-20 19:00 ` David Sterba
2019-02-21 9:03 ` Anand Jain
2019-02-12 2:18 ` [PATCH v3 2/3] btrfs: drop redundant forward declaration in props.c Anand Jain
2019-02-12 2:18 ` [PATCH v3 3/3] btrfs: trivial, fix c coding style Anand Jain
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).