| Message ID | 20220409043432.1244773-1-cccheng@synology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: do not allow compression on nocow files | expand |
On Sat, Apr 9, 2022 at 10:14 AM Chung-Chiang Cheng <cccheng@synology.com> wrote: > > Compression and nocow are mutually exclusive. Besides ioctl, there is > another way to enable/disable/reset compression directly via xattr. The > following steps will result in a invalid combination. > > $ touch bar > $ lsattr bar > ---------------C-- bar The example should show how the NOCOW attribute ended up in the file. Either the filesystem mounted with -o nodatacow or a chattr +C was done on the file bar before the lsattr command. > $ setfattr -n btrfs.compression -v zstd bar > $ lsattr bar > --------c------C-- bar > > Reported-by: Jayce Lin <jaycelin@synology.com> > Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> The changelog should mention what happens if we end up both with compression and nodatacow set on a file. Will all future writes be compressed and nodatacow have no effect? Or will compression have no effect and we nodatacow overrides it? Or something else? Some corruption, some crash, etc? > --- > fs/btrfs/props.c | 20 ++++++++++++-------- > fs/btrfs/props.h | 3 ++- > fs/btrfs/xattr.c | 2 +- > 3 files changed, 15 insertions(+), 10 deletions(-) > > diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c > index 1a6d2d5b4b33..fde2bf069b03 100644 > --- a/fs/btrfs/props.c > +++ b/fs/btrfs/props.c > @@ -17,7 +17,7 @@ static DEFINE_HASHTABLE(prop_handlers_ht, BTRFS_PROP_HANDLERS_HT_BITS); > struct prop_handler { > struct hlist_node node; > const char *xattr_name; > - int (*validate)(const char *value, size_t len); > + int (*validate)(struct inode *inode, const char *value, size_t len); > int (*apply)(struct inode *inode, const char *value, size_t len); > const char *(*extract)(struct inode *inode); > int inheritable; > @@ -55,7 +55,8 @@ find_prop_handler(const char *name, > return NULL; > } > > -int btrfs_validate_prop(const char *name, const char *value, size_t value_len) > +int btrfs_validate_prop(struct inode *inode, const char *name, The inode can be made const. The validation handler should never need to change anything in the inode. Also, we can use struct btrfs_inode * instead of struct inode (preferred). > + const char *value, size_t value_len) > { > const struct prop_handler *handler; > > @@ -69,7 +70,7 @@ int btrfs_validate_prop(const char *name, const char *value, size_t value_len) > if (value_len == 0) > return 0; > > - return handler->validate(value, value_len); > + return handler->validate(inode, value, value_len); > } > > int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, > @@ -252,18 +253,21 @@ 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) > +static int prop_compression_validate(struct inode *inode, const char *value, size_t len) Same here, about const and using struct btrfs_inode * instead. > { > if (!value) > return 0; > > - if (btrfs_compress_is_valid_type(value, len)) > - return 0; > - > if ((len == 2 && strncmp("no", value, 2) == 0) || > (len == 4 && strncmp("none", value, 4) == 0)) > return 0; > > + if (!inode || BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW) How can inode ever be NULL? Unless I missed something, it can't be NULL. We should also get a test case for fstests, to help prevent regressions in the future. Otherwise it looks fine. Thanks. > + return -EINVAL; > + > + if (btrfs_compress_is_valid_type(value, len)) > + return 0; > + > return -EINVAL; > } > > @@ -364,7 +368,7 @@ static int inherit_props(struct btrfs_trans_handle *trans, > * This is not strictly necessary as the property should be > * valid, but in case it isn't, don't propagate it further. > */ > - ret = h->validate(value, strlen(value)); > + ret = h->validate(inode, value, strlen(value)); > if (ret) > continue; > > diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h > index 40b2c65b518c..0504c03177e3 100644 > --- a/fs/btrfs/props.h > +++ b/fs/btrfs/props.h > @@ -13,7 +13,8 @@ void __init btrfs_props_init(void); > 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_validate_prop(const char *name, const char *value, size_t value_len); > +int btrfs_validate_prop(struct inode *inode, const char *name, > + const char *value, size_t value_len); > > 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 99abf41b89b9..9aceae07a02b 100644 > --- a/fs/btrfs/xattr.c > +++ b/fs/btrfs/xattr.c > @@ -403,7 +403,7 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler, > struct btrfs_root *root = BTRFS_I(inode)->root; > > name = xattr_full_name(handler, name); > - ret = btrfs_validate_prop(name, value, size); > + ret = btrfs_validate_prop(inode, name, value, size); > if (ret) > return ret; > > -- > 2.34.1 >
On Sat, Apr 09, 2022 at 12:34:32PM +0800, Chung-Chiang Cheng wrote: > -static int prop_compression_validate(const char *value, size_t len) > +static int prop_compression_validate(struct inode *inode, const char *value, size_t len) > { > if (!value) > return 0; > > - if (btrfs_compress_is_valid_type(value, len)) > - return 0; > - > if ((len == 2 && strncmp("no", value, 2) == 0) || > (len == 4 && strncmp("none", value, 4) == 0)) > return 0; > > + if (!inode || BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW) > + return -EINVAL; I think the nodatacow check should be the first one, before the validation of value for "no" or "none", it's logically the same as the btrfs_compress_is_valid_type. Also, should there be a check for NODATASUM? The checks in the property should do the same as the normal compression code, see eg. inode_can_compress. It's an internal helper so it would be cleaner to have it exported and reuse here instead of duplicatin the conditions. For a minimal fix I'd be OK with the code duplication as this would need to go to stable, so "fixes before cleanups".
> > + if (!inode || BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW) > > How can inode ever be NULL? > Unless I missed something, it can't be NULL. > > We should also get a test case for fstests, to help prevent > regressions in the future. > > Otherwise it looks fine. Thanks for your comments. I will send a v2 patch and a fstest item later. And as David mentioned, inode_can_compress() can be reused to reduce duplication. Thanks.
On Mon, Apr 11, 2022 at 10:27 PM David Sterba <dsterba@suse.cz> wrote: > > On Sat, Apr 09, 2022 at 12:34:32PM +0800, Chung-Chiang Cheng wrote: > > -static int prop_compression_validate(const char *value, size_t len) > > +static int prop_compression_validate(struct inode *inode, const char *value, size_t len) > > { > > if (!value) > > return 0; > > > > - if (btrfs_compress_is_valid_type(value, len)) > > - return 0; > > - > > if ((len == 2 && strncmp("no", value, 2) == 0) || > > (len == 4 && strncmp("none", value, 4) == 0)) > > return 0; > > > > + if (!inode || BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW) > > + return -EINVAL; > > I think the nodatacow check should be the first one, before the > validation of value for "no" or "none", it's logically the same as the > btrfs_compress_is_valid_type. if this check is located before the validation of value for no/none, the following operation isn't allowed. is it ok? although it makes no sense, it doesn't produce any invalid combination. $ touch bar $ chattr +C bar $ lsattr bar ---------------C-- bar $ setfattr -n btrfs.compression -v no bar setfattr: bar: Invalid argument Thanks.
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c index 1a6d2d5b4b33..fde2bf069b03 100644 --- a/fs/btrfs/props.c +++ b/fs/btrfs/props.c @@ -17,7 +17,7 @@ static DEFINE_HASHTABLE(prop_handlers_ht, BTRFS_PROP_HANDLERS_HT_BITS); struct prop_handler { struct hlist_node node; const char *xattr_name; - int (*validate)(const char *value, size_t len); + int (*validate)(struct inode *inode, const char *value, size_t len); int (*apply)(struct inode *inode, const char *value, size_t len); const char *(*extract)(struct inode *inode); int inheritable; @@ -55,7 +55,8 @@ find_prop_handler(const char *name, return NULL; } -int btrfs_validate_prop(const char *name, const char *value, size_t value_len) +int btrfs_validate_prop(struct inode *inode, const char *name, + const char *value, size_t value_len) { const struct prop_handler *handler; @@ -69,7 +70,7 @@ int btrfs_validate_prop(const char *name, const char *value, size_t value_len) if (value_len == 0) return 0; - return handler->validate(value, value_len); + return handler->validate(inode, value, value_len); } int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode, @@ -252,18 +253,21 @@ 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) +static int prop_compression_validate(struct inode *inode, const char *value, size_t len) { if (!value) return 0; - if (btrfs_compress_is_valid_type(value, len)) - return 0; - if ((len == 2 && strncmp("no", value, 2) == 0) || (len == 4 && strncmp("none", value, 4) == 0)) return 0; + if (!inode || BTRFS_I(inode)->flags & BTRFS_INODE_NODATACOW) + return -EINVAL; + + if (btrfs_compress_is_valid_type(value, len)) + return 0; + return -EINVAL; } @@ -364,7 +368,7 @@ static int inherit_props(struct btrfs_trans_handle *trans, * This is not strictly necessary as the property should be * valid, but in case it isn't, don't propagate it further. */ - ret = h->validate(value, strlen(value)); + ret = h->validate(inode, value, strlen(value)); if (ret) continue; diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h index 40b2c65b518c..0504c03177e3 100644 --- a/fs/btrfs/props.h +++ b/fs/btrfs/props.h @@ -13,7 +13,8 @@ void __init btrfs_props_init(void); 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_validate_prop(const char *name, const char *value, size_t value_len); +int btrfs_validate_prop(struct inode *inode, const char *name, + const char *value, size_t value_len); 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 99abf41b89b9..9aceae07a02b 100644 --- a/fs/btrfs/xattr.c +++ b/fs/btrfs/xattr.c @@ -403,7 +403,7 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler, struct btrfs_root *root = BTRFS_I(inode)->root; name = xattr_full_name(handler, name); - ret = btrfs_validate_prop(name, value, size); + ret = btrfs_validate_prop(inode, name, value, size); if (ret) return ret;
Compression and nocow are mutually exclusive. Besides ioctl, there is another way to enable/disable/reset compression directly via xattr. The following steps will result in a invalid combination. $ touch bar $ lsattr bar ---------------C-- bar $ setfattr -n btrfs.compression -v zstd bar $ lsattr bar --------c------C-- bar Reported-by: Jayce Lin <jaycelin@synology.com> Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> --- fs/btrfs/props.c | 20 ++++++++++++-------- fs/btrfs/props.h | 3 ++- fs/btrfs/xattr.c | 2 +- 3 files changed, 15 insertions(+), 10 deletions(-)