* [PATCH 2/5] ceph: remove zero-size xattr
2014-02-11 5:30 [PATCH 1/5] ceph: properly handle XATTR_CREATE and XATTR_REPLACE Yan, Zheng
@ 2014-02-11 5:30 ` Yan, Zheng
2014-02-11 14:47 ` Alex Elder
2014-02-11 5:30 ` [PATCH 3/5] ceph: fix ceph_removexattr() Yan, Zheng
` (3 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Yan, Zheng @ 2014-02-11 5:30 UTC (permalink / raw)
To: ceph-devel; +Cc: sage, Yan, Zheng
Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
---
fs/ceph/xattr.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 28f9793..6ed0e5a 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -12,6 +12,9 @@
#define XATTR_CEPH_PREFIX "ceph."
#define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
+static int __remove_xattr(struct ceph_inode_info *ci,
+ struct ceph_inode_xattr *xattr);
+
/*
* List of handlers for synthetic system.* attributes. Other
* attributes are handled directly.
@@ -359,6 +362,12 @@ static int __set_xattr(struct ceph_inode_info *ci,
kfree(val);
return err;
}
+ if (!val_len) {
+ if (xattr)
+ __remove_xattr(ci, xattr);
+ kfree(name);
+ return 0;
+ }
}
if (!xattr) {
--
1.8.5.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/5] ceph: remove zero-size xattr
2014-02-11 5:30 ` [PATCH 2/5] ceph: remove zero-size xattr Yan, Zheng
@ 2014-02-11 14:47 ` Alex Elder
2014-02-11 15:10 ` Yan, Zheng
0 siblings, 1 reply; 19+ messages in thread
From: Alex Elder @ 2014-02-11 14:47 UTC (permalink / raw)
To: Yan, Zheng, ceph-devel; +Cc: sage
On 02/10/2014 11:30 PM, Yan, Zheng wrote:
> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
You really need to explain better under what circumstances
a zero-size xattr is getting removed.
But apparently it's only when you're updating an xattr
(not building it up from a blob from storage).
Why are you doing this? Why can't an xattr exist with
an empty value?
Looking at generic_setxattr() in "fs/xattr.c" we see:
if (size == 0)
value = ""; /* empty EA, do not remove */
The code you have below looks OK, but it seems that you
shouldn't be doing this.
Am I missing something?
-Alex
> ---
> fs/ceph/xattr.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index 28f9793..6ed0e5a 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -12,6 +12,9 @@
> #define XATTR_CEPH_PREFIX "ceph."
> #define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
>
> +static int __remove_xattr(struct ceph_inode_info *ci,
> + struct ceph_inode_xattr *xattr);
> +
> /*
> * List of handlers for synthetic system.* attributes. Other
> * attributes are handled directly.
> @@ -359,6 +362,12 @@ static int __set_xattr(struct ceph_inode_info *ci,
> kfree(val);
> return err;
> }
> + if (!val_len) {
> + if (xattr)
> + __remove_xattr(ci, xattr);
> + kfree(name);
> + return 0;
> + }
> }
>
> if (!xattr) {
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/5] ceph: remove zero-size xattr
2014-02-11 14:47 ` Alex Elder
@ 2014-02-11 15:10 ` Yan, Zheng
2014-02-11 17:25 ` Alex Elder
0 siblings, 1 reply; 19+ messages in thread
From: Yan, Zheng @ 2014-02-11 15:10 UTC (permalink / raw)
To: Alex Elder; +Cc: Yan, Zheng, ceph-devel, Sage Weil
On Tue, Feb 11, 2014 at 10:47 PM, Alex Elder <elder@ieee.org> wrote:
> On 02/10/2014 11:30 PM, Yan, Zheng wrote:
>> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
>
> You really need to explain better under what circumstances
> a zero-size xattr is getting removed.
>
> But apparently it's only when you're updating an xattr
> (not building it up from a blob from storage).
>
> Why are you doing this? Why can't an xattr exist with
> an empty value?
That is how other FS behave, at least for ext* and btrfs.
Regards
Yan, Zheng
>
> Looking at generic_setxattr() in "fs/xattr.c" we see:
> if (size == 0)
> value = ""; /* empty EA, do not remove */
>
> The code you have below looks OK, but it seems that you
> shouldn't be doing this.
>
> Am I missing something?
>
> -Alex
>
>> ---
>> fs/ceph/xattr.c | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
>> index 28f9793..6ed0e5a 100644
>> --- a/fs/ceph/xattr.c
>> +++ b/fs/ceph/xattr.c
>> @@ -12,6 +12,9 @@
>> #define XATTR_CEPH_PREFIX "ceph."
>> #define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
>>
>> +static int __remove_xattr(struct ceph_inode_info *ci,
>> + struct ceph_inode_xattr *xattr);
>> +
>> /*
>> * List of handlers for synthetic system.* attributes. Other
>> * attributes are handled directly.
>> @@ -359,6 +362,12 @@ static int __set_xattr(struct ceph_inode_info *ci,
>> kfree(val);
>> return err;
>> }
>> + if (!val_len) {
>> + if (xattr)
>> + __remove_xattr(ci, xattr);
>> + kfree(name);
>> + return 0;
>> + }
>> }
>>
>> if (!xattr) {
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/5] ceph: remove zero-size xattr
2014-02-11 15:10 ` Yan, Zheng
@ 2014-02-11 17:25 ` Alex Elder
2014-02-12 2:37 ` Yan, Zheng
0 siblings, 1 reply; 19+ messages in thread
From: Alex Elder @ 2014-02-11 17:25 UTC (permalink / raw)
To: Yan, Zheng; +Cc: Yan, Zheng, ceph-devel, Sage Weil
On 02/11/2014 09:10 AM, Yan, Zheng wrote:
> On Tue, Feb 11, 2014 at 10:47 PM, Alex Elder <elder@ieee.org> wrote:
>> On 02/10/2014 11:30 PM, Yan, Zheng wrote:
>>> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
>>
>> You really need to explain better under what circumstances
>> a zero-size xattr is getting removed.
>>
>> But apparently it's only when you're updating an xattr
>> (not building it up from a blob from storage).
>>
>> Why are you doing this? Why can't an xattr exist with
>> an empty value?
>
> That is how other FS behave, at least for ext* and btrfs.
I haven't tested this, I'm just glancing through code.
But it looks to me like a zero-length value is OK, but
a null value pointer means it should be deleted. Note
in btrfs_setxattr(), for example, the same bit of code
I referenced before:
if (size == 0)
value = ""; /* empty EA, do not remove */
And ext4 seems to delete for a null value, but handle
an xattr whose value *length* is zero. Same with XFS.
So again, I haven't verified through testing, but my
reading of the code (though rusty) still seems to show
that an attribute can have an empty (zero-size) value.
-Alex
> Regards
> Yan, Zheng
>
>>
>> Looking at generic_setxattr() in "fs/xattr.c" we see:
>> if (size == 0)
>> value = ""; /* empty EA, do not remove */
>>
>> The code you have below looks OK, but it seems that you
>> shouldn't be doing this.
>>
>> Am I missing something?
>>
>> -Alex
>>
>>> ---
>>> fs/ceph/xattr.c | 9 +++++++++
>>> 1 file changed, 9 insertions(+)
>>>
>>> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
>>> index 28f9793..6ed0e5a 100644
>>> --- a/fs/ceph/xattr.c
>>> +++ b/fs/ceph/xattr.c
>>> @@ -12,6 +12,9 @@
>>> #define XATTR_CEPH_PREFIX "ceph."
>>> #define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
>>>
>>> +static int __remove_xattr(struct ceph_inode_info *ci,
>>> + struct ceph_inode_xattr *xattr);
>>> +
>>> /*
>>> * List of handlers for synthetic system.* attributes. Other
>>> * attributes are handled directly.
>>> @@ -359,6 +362,12 @@ static int __set_xattr(struct ceph_inode_info *ci,
>>> kfree(val);
>>> return err;
>>> }
>>> + if (!val_len) {
>>> + if (xattr)
>>> + __remove_xattr(ci, xattr);
>>> + kfree(name);
>>> + return 0;
>>> + }
>>> }
>>>
>>> if (!xattr) {
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/5] ceph: remove zero-size xattr
2014-02-11 17:25 ` Alex Elder
@ 2014-02-12 2:37 ` Yan, Zheng
2014-02-12 2:43 ` Sage Weil
0 siblings, 1 reply; 19+ messages in thread
From: Yan, Zheng @ 2014-02-12 2:37 UTC (permalink / raw)
To: Alex Elder, Yan, Zheng; +Cc: ceph-devel, Sage Weil
On 02/12/2014 01:25 AM, Alex Elder wrote:
> On 02/11/2014 09:10 AM, Yan, Zheng wrote:
>> On Tue, Feb 11, 2014 at 10:47 PM, Alex Elder <elder@ieee.org> wrote:
>>> On 02/10/2014 11:30 PM, Yan, Zheng wrote:
>>>> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
>>>
>>> You really need to explain better under what circumstances
>>> a zero-size xattr is getting removed.
>>>
>>> But apparently it's only when you're updating an xattr
>>> (not building it up from a blob from storage).
>>>
>>> Why are you doing this? Why can't an xattr exist with
>>> an empty value?
>>
>> That is how other FS behave, at least for ext* and btrfs.
>
> I haven't tested this, I'm just glancing through code.
> But it looks to me like a zero-length value is OK, but
> a null value pointer means it should be deleted. Note
> in btrfs_setxattr(), for example, the same bit of code
> I referenced before:
>
> if (size == 0)
> value = ""; /* empty EA, do not remove */
>
> And ext4 seems to delete for a null value, but handle
> an xattr whose value *length* is zero. Same with XFS.
>
> So again, I haven't verified through testing, but my
> reading of the code (though rusty) still seems to show
> that an attribute can have an empty (zero-size) value.
You are right, Thanks. how about below patch?
---
From f11d5da84230e4993333a063019bad68c67b50d4 Mon Sep 17 00:00:00 2001
From: "Yan, Zheng" <zheng.z.yan@intel.com>
Date: Tue, 11 Feb 2014 13:04:19 +0800
Subject: [PATCH 2/5] ceph: remove xattr when null value is given to setxattr()
For the setxattr request, introduce a new flag CEPH_XATTR_REMOVE
to distinguish null value case from the zero-length value case.
Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
---
fs/ceph/xattr.c | 16 ++++++++++++++--
include/linux/ceph/ceph_fs.h | 5 +++--
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 28f9793..f6becf6 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -12,6 +12,9 @@
#define XATTR_CEPH_PREFIX "ceph."
#define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
+static int __remove_xattr(struct ceph_inode_info *ci,
+ struct ceph_inode_xattr *xattr);
+
/*
* List of handlers for synthetic system.* attributes. Other
* attributes are handled directly.
@@ -359,6 +362,12 @@ static int __set_xattr(struct ceph_inode_info *ci,
kfree(val);
return err;
}
+ if (update_xattr < 0) {
+ if (xattr)
+ __remove_xattr(ci, xattr);
+ kfree(name);
+ return 0;
+ }
}
if (!xattr) {
@@ -862,6 +871,9 @@ static int ceph_sync_setxattr(struct dentry *dentry, const char *name,
dout("setxattr value=%.*s\n", (int)size, value);
+ if (!value)
+ flags |= CEPH_XATTR_REMOVE;
+
/* do request */
req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_SETXATTR,
USE_AUTH_MDS);
@@ -965,8 +977,8 @@ retry:
goto retry;
}
- err = __set_xattr(ci, newname, name_len, newval,
- val_len, flags, 1, &xattr);
+ err = __set_xattr(ci, newname, name_len, newval, val_len,
+ flags, val ? 1 : -1, &xattr);
if (!err) {
dirty = __ceph_mark_dirty_caps(ci, CEPH_CAP_XATTR_EXCL);
diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h
index 2623cff..25bfb0e 100644
--- a/include/linux/ceph/ceph_fs.h
+++ b/include/linux/ceph/ceph_fs.h
@@ -373,8 +373,9 @@ extern const char *ceph_mds_op_name(int op);
/*
* Ceph setxattr request flags.
*/
-#define CEPH_XATTR_CREATE 1
-#define CEPH_XATTR_REPLACE 2
+#define CEPH_XATTR_CREATE (1 << 0)
+#define CEPH_XATTR_REPLACE (1 << 1)
+#define CEPH_XATTR_REMOVE (1 << 31)
union ceph_mds_request_args {
struct {
--
1.8.5.3
>
> -Alex
>
>> Regards
>> Yan, Zheng
>>
>>>
>>> Looking at generic_setxattr() in "fs/xattr.c" we see:
>>> if (size == 0)
>>> value = ""; /* empty EA, do not remove */
>>>
>>> The code you have below looks OK, but it seems that you
>>> shouldn't be doing this.
>>>
>>> Am I missing something?
>>>
>>> -Alex
>>>
>>>> ---
>>>> fs/ceph/xattr.c | 9 +++++++++
>>>> 1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
>>>> index 28f9793..6ed0e5a 100644
>>>> --- a/fs/ceph/xattr.c
>>>> +++ b/fs/ceph/xattr.c
>>>> @@ -12,6 +12,9 @@
>>>> #define XATTR_CEPH_PREFIX "ceph."
>>>> #define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
>>>>
>>>> +static int __remove_xattr(struct ceph_inode_info *ci,
>>>> + struct ceph_inode_xattr *xattr);
>>>> +
>>>> /*
>>>> * List of handlers for synthetic system.* attributes. Other
>>>> * attributes are handled directly.
>>>> @@ -359,6 +362,12 @@ static int __set_xattr(struct ceph_inode_info *ci,
>>>> kfree(val);
>>>> return err;
>>>> }
>>>> + if (!val_len) {
>>>> + if (xattr)
>>>> + __remove_xattr(ci, xattr);
>>>> + kfree(name);
>>>> + return 0;
>>>> + }
>>>> }
>>>>
>>>> if (!xattr) {
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/5] ceph: remove zero-size xattr
2014-02-12 2:37 ` Yan, Zheng
@ 2014-02-12 2:43 ` Sage Weil
2014-02-12 2:46 ` Yan, Zheng
0 siblings, 1 reply; 19+ messages in thread
From: Sage Weil @ 2014-02-12 2:43 UTC (permalink / raw)
To: Yan, Zheng; +Cc: Alex Elder, Yan, Zheng, ceph-devel
On Wed, 12 Feb 2014, Yan, Zheng wrote:
> On 02/12/2014 01:25 AM, Alex Elder wrote:
> > On 02/11/2014 09:10 AM, Yan, Zheng wrote:
> >> On Tue, Feb 11, 2014 at 10:47 PM, Alex Elder <elder@ieee.org> wrote:
> >>> On 02/10/2014 11:30 PM, Yan, Zheng wrote:
> >>>> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
> >>>
> >>> You really need to explain better under what circumstances
> >>> a zero-size xattr is getting removed.
> >>>
> >>> But apparently it's only when you're updating an xattr
> >>> (not building it up from a blob from storage).
> >>>
> >>> Why are you doing this? Why can't an xattr exist with
> >>> an empty value?
> >>
> >> That is how other FS behave, at least for ext* and btrfs.
> >
> > I haven't tested this, I'm just glancing through code.
> > But it looks to me like a zero-length value is OK, but
> > a null value pointer means it should be deleted. Note
> > in btrfs_setxattr(), for example, the same bit of code
> > I referenced before:
> >
> > if (size == 0)
> > value = ""; /* empty EA, do not remove */
> >
> > And ext4 seems to delete for a null value, but handle
> > an xattr whose value *length* is zero. Same with XFS.
> >
> > So again, I haven't verified through testing, but my
> > reading of the code (though rusty) still seems to show
> > that an attribute can have an empty (zero-size) value.
>
> You are right, Thanks. how about below patch?
>
> ---
> >From f11d5da84230e4993333a063019bad68c67b50d4 Mon Sep 17 00:00:00 2001
> From: "Yan, Zheng" <zheng.z.yan@intel.com>
> Date: Tue, 11 Feb 2014 13:04:19 +0800
> Subject: [PATCH 2/5] ceph: remove xattr when null value is given to setxattr()
>
> For the setxattr request, introduce a new flag CEPH_XATTR_REMOVE
> to distinguish null value case from the zero-length value case.
>
> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
> ---
> fs/ceph/xattr.c | 16 ++++++++++++++--
> include/linux/ceph/ceph_fs.h | 5 +++--
> 2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index 28f9793..f6becf6 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -12,6 +12,9 @@
> #define XATTR_CEPH_PREFIX "ceph."
> #define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
>
> +static int __remove_xattr(struct ceph_inode_info *ci,
> + struct ceph_inode_xattr *xattr);
> +
> /*
> * List of handlers for synthetic system.* attributes. Other
> * attributes are handled directly.
> @@ -359,6 +362,12 @@ static int __set_xattr(struct ceph_inode_info *ci,
> kfree(val);
> return err;
> }
> + if (update_xattr < 0) {
> + if (xattr)
> + __remove_xattr(ci, xattr);
> + kfree(name);
> + return 0;
> + }
> }
>
> if (!xattr) {
> @@ -862,6 +871,9 @@ static int ceph_sync_setxattr(struct dentry *dentry, const char *name,
>
> dout("setxattr value=%.*s\n", (int)size, value);
>
> + if (!value)
> + flags |= CEPH_XATTR_REMOVE;
> +
> /* do request */
> req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_SETXATTR,
How about just turning this into a REMOVEXATTR request in the !value case?
sage
> USE_AUTH_MDS);
> @@ -965,8 +977,8 @@ retry:
> goto retry;
> }
>
> - err = __set_xattr(ci, newname, name_len, newval,
> - val_len, flags, 1, &xattr);
> + err = __set_xattr(ci, newname, name_len, newval, val_len,
> + flags, val ? 1 : -1, &xattr);
>
> if (!err) {
> dirty = __ceph_mark_dirty_caps(ci, CEPH_CAP_XATTR_EXCL);
> diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h
> index 2623cff..25bfb0e 100644
> --- a/include/linux/ceph/ceph_fs.h
> +++ b/include/linux/ceph/ceph_fs.h
> @@ -373,8 +373,9 @@ extern const char *ceph_mds_op_name(int op);
> /*
> * Ceph setxattr request flags.
> */
> -#define CEPH_XATTR_CREATE 1
> -#define CEPH_XATTR_REPLACE 2
> +#define CEPH_XATTR_CREATE (1 << 0)
> +#define CEPH_XATTR_REPLACE (1 << 1)
> +#define CEPH_XATTR_REMOVE (1 << 31)
>
> union ceph_mds_request_args {
> struct {
> --
> 1.8.5.3
>
>
>
>
>
>
>
>
>
>
>
>
> >
> > -Alex
> >
> >> Regards
> >> Yan, Zheng
> >>
> >>>
> >>> Looking at generic_setxattr() in "fs/xattr.c" we see:
> >>> if (size == 0)
> >>> value = ""; /* empty EA, do not remove */
> >>>
> >>> The code you have below looks OK, but it seems that you
> >>> shouldn't be doing this.
> >>>
> >>> Am I missing something?
> >>>
> >>> -Alex
> >>>
> >>>> ---
> >>>> fs/ceph/xattr.c | 9 +++++++++
> >>>> 1 file changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> >>>> index 28f9793..6ed0e5a 100644
> >>>> --- a/fs/ceph/xattr.c
> >>>> +++ b/fs/ceph/xattr.c
> >>>> @@ -12,6 +12,9 @@
> >>>> #define XATTR_CEPH_PREFIX "ceph."
> >>>> #define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
> >>>>
> >>>> +static int __remove_xattr(struct ceph_inode_info *ci,
> >>>> + struct ceph_inode_xattr *xattr);
> >>>> +
> >>>> /*
> >>>> * List of handlers for synthetic system.* attributes. Other
> >>>> * attributes are handled directly.
> >>>> @@ -359,6 +362,12 @@ static int __set_xattr(struct ceph_inode_info *ci,
> >>>> kfree(val);
> >>>> return err;
> >>>> }
> >>>> + if (!val_len) {
> >>>> + if (xattr)
> >>>> + __remove_xattr(ci, xattr);
> >>>> + kfree(name);
> >>>> + return 0;
> >>>> + }
> >>>> }
> >>>>
> >>>> if (!xattr) {
> >>>>
> >>>
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> >>> the body of a message to majordomo@vger.kernel.org
> >>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/5] ceph: remove zero-size xattr
2014-02-12 2:43 ` Sage Weil
@ 2014-02-12 2:46 ` Yan, Zheng
0 siblings, 0 replies; 19+ messages in thread
From: Yan, Zheng @ 2014-02-12 2:46 UTC (permalink / raw)
To: Sage Weil; +Cc: Alex Elder, Yan, Zheng, ceph-devel
On 02/12/2014 10:43 AM, Sage Weil wrote:
> On Wed, 12 Feb 2014, Yan, Zheng wrote:
>> On 02/12/2014 01:25 AM, Alex Elder wrote:
>>> On 02/11/2014 09:10 AM, Yan, Zheng wrote:
>>>> On Tue, Feb 11, 2014 at 10:47 PM, Alex Elder <elder@ieee.org> wrote:
>>>>> On 02/10/2014 11:30 PM, Yan, Zheng wrote:
>>>>>> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
>>>>>
>>>>> You really need to explain better under what circumstances
>>>>> a zero-size xattr is getting removed.
>>>>>
>>>>> But apparently it's only when you're updating an xattr
>>>>> (not building it up from a blob from storage).
>>>>>
>>>>> Why are you doing this? Why can't an xattr exist with
>>>>> an empty value?
>>>>
>>>> That is how other FS behave, at least for ext* and btrfs.
>>>
>>> I haven't tested this, I'm just glancing through code.
>>> But it looks to me like a zero-length value is OK, but
>>> a null value pointer means it should be deleted. Note
>>> in btrfs_setxattr(), for example, the same bit of code
>>> I referenced before:
>>>
>>> if (size == 0)
>>> value = ""; /* empty EA, do not remove */
>>>
>>> And ext4 seems to delete for a null value, but handle
>>> an xattr whose value *length* is zero. Same with XFS.
>>>
>>> So again, I haven't verified through testing, but my
>>> reading of the code (though rusty) still seems to show
>>> that an attribute can have an empty (zero-size) value.
>>
>> You are right, Thanks. how about below patch?
>>
>> ---
>> >From f11d5da84230e4993333a063019bad68c67b50d4 Mon Sep 17 00:00:00 2001
>> From: "Yan, Zheng" <zheng.z.yan@intel.com>
>> Date: Tue, 11 Feb 2014 13:04:19 +0800
>> Subject: [PATCH 2/5] ceph: remove xattr when null value is given to setxattr()
>>
>> For the setxattr request, introduce a new flag CEPH_XATTR_REMOVE
>> to distinguish null value case from the zero-length value case.
>>
>> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
>> ---
>> fs/ceph/xattr.c | 16 ++++++++++++++--
>> include/linux/ceph/ceph_fs.h | 5 +++--
>> 2 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
>> index 28f9793..f6becf6 100644
>> --- a/fs/ceph/xattr.c
>> +++ b/fs/ceph/xattr.c
>> @@ -12,6 +12,9 @@
>> #define XATTR_CEPH_PREFIX "ceph."
>> #define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
>>
>> +static int __remove_xattr(struct ceph_inode_info *ci,
>> + struct ceph_inode_xattr *xattr);
>> +
>> /*
>> * List of handlers for synthetic system.* attributes. Other
>> * attributes are handled directly.
>> @@ -359,6 +362,12 @@ static int __set_xattr(struct ceph_inode_info *ci,
>> kfree(val);
>> return err;
>> }
>> + if (update_xattr < 0) {
>> + if (xattr)
>> + __remove_xattr(ci, xattr);
>> + kfree(name);
>> + return 0;
>> + }
>> }
>>
>> if (!xattr) {
>> @@ -862,6 +871,9 @@ static int ceph_sync_setxattr(struct dentry *dentry, const char *name,
>>
>> dout("setxattr value=%.*s\n", (int)size, value);
>>
>> + if (!value)
>> + flags |= CEPH_XATTR_REMOVE;
>> +
>> /* do request */
>> req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_SETXATTR,
>
> How about just turning this into a REMOVEXATTR request in the !value case?
>
REMOVEXATTR request doesn't respect the CEPH_XATTR_REPLACE flag.
Regards
Yan, Zheng
> sage
>
>> USE_AUTH_MDS);
>> @@ -965,8 +977,8 @@ retry:
>> goto retry;
>> }
>>
>> - err = __set_xattr(ci, newname, name_len, newval,
>> - val_len, flags, 1, &xattr);
>> + err = __set_xattr(ci, newname, name_len, newval, val_len,
>> + flags, val ? 1 : -1, &xattr);
>>
>> if (!err) {
>> dirty = __ceph_mark_dirty_caps(ci, CEPH_CAP_XATTR_EXCL);
>> diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h
>> index 2623cff..25bfb0e 100644
>> --- a/include/linux/ceph/ceph_fs.h
>> +++ b/include/linux/ceph/ceph_fs.h
>> @@ -373,8 +373,9 @@ extern const char *ceph_mds_op_name(int op);
>> /*
>> * Ceph setxattr request flags.
>> */
>> -#define CEPH_XATTR_CREATE 1
>> -#define CEPH_XATTR_REPLACE 2
>> +#define CEPH_XATTR_CREATE (1 << 0)
>> +#define CEPH_XATTR_REPLACE (1 << 1)
>> +#define CEPH_XATTR_REMOVE (1 << 31)
>>
>> union ceph_mds_request_args {
>> struct {
>> --
>> 1.8.5.3
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>>
>>> -Alex
>>>
>>>> Regards
>>>> Yan, Zheng
>>>>
>>>>>
>>>>> Looking at generic_setxattr() in "fs/xattr.c" we see:
>>>>> if (size == 0)
>>>>> value = ""; /* empty EA, do not remove */
>>>>>
>>>>> The code you have below looks OK, but it seems that you
>>>>> shouldn't be doing this.
>>>>>
>>>>> Am I missing something?
>>>>>
>>>>> -Alex
>>>>>
>>>>>> ---
>>>>>> fs/ceph/xattr.c | 9 +++++++++
>>>>>> 1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
>>>>>> index 28f9793..6ed0e5a 100644
>>>>>> --- a/fs/ceph/xattr.c
>>>>>> +++ b/fs/ceph/xattr.c
>>>>>> @@ -12,6 +12,9 @@
>>>>>> #define XATTR_CEPH_PREFIX "ceph."
>>>>>> #define XATTR_CEPH_PREFIX_LEN (sizeof (XATTR_CEPH_PREFIX) - 1)
>>>>>>
>>>>>> +static int __remove_xattr(struct ceph_inode_info *ci,
>>>>>> + struct ceph_inode_xattr *xattr);
>>>>>> +
>>>>>> /*
>>>>>> * List of handlers for synthetic system.* attributes. Other
>>>>>> * attributes are handled directly.
>>>>>> @@ -359,6 +362,12 @@ static int __set_xattr(struct ceph_inode_info *ci,
>>>>>> kfree(val);
>>>>>> return err;
>>>>>> }
>>>>>> + if (!val_len) {
>>>>>> + if (xattr)
>>>>>> + __remove_xattr(ci, xattr);
>>>>>> + kfree(name);
>>>>>> + return 0;
>>>>>> + }
>>>>>> }
>>>>>>
>>>>>> if (!xattr) {
>>>>>>
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/5] ceph: fix ceph_removexattr()
2014-02-11 5:30 [PATCH 1/5] ceph: properly handle XATTR_CREATE and XATTR_REPLACE Yan, Zheng
2014-02-11 5:30 ` [PATCH 2/5] ceph: remove zero-size xattr Yan, Zheng
@ 2014-02-11 5:30 ` Yan, Zheng
2014-02-11 14:50 ` Alex Elder
2014-02-11 5:30 ` [PATCH 4/5] ceph: add missing init_acl() for mkdir() and atomic_open() Yan, Zheng
` (2 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Yan, Zheng @ 2014-02-11 5:30 UTC (permalink / raw)
To: ceph-devel; +Cc: sage, Yan, Zheng
Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
---
fs/ceph/xattr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 6ed0e5a..79f9c12 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -463,7 +463,7 @@ static int __remove_xattr(struct ceph_inode_info *ci,
struct ceph_inode_xattr *xattr)
{
if (!xattr)
- return -EOPNOTSUPP;
+ return -ENODATA;
rb_erase(&xattr->node, &ci->i_xattrs.index);
--
1.8.5.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/5] ceph: fix ceph_removexattr()
2014-02-11 5:30 ` [PATCH 3/5] ceph: fix ceph_removexattr() Yan, Zheng
@ 2014-02-11 14:50 ` Alex Elder
0 siblings, 0 replies; 19+ messages in thread
From: Alex Elder @ 2014-02-11 14:50 UTC (permalink / raw)
To: Yan, Zheng, ceph-devel; +Cc: sage
On 02/10/2014 11:30 PM, Yan, Zheng wrote:
> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
You need a better explanation than "fix."
We have the ENODATA thing (versus ENOATTR), but again
what you have is fine.
> ---
> fs/ceph/xattr.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index 6ed0e5a..79f9c12 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -463,7 +463,7 @@ static int __remove_xattr(struct ceph_inode_info *ci,
> struct ceph_inode_xattr *xattr)
> {
> if (!xattr)
> - return -EOPNOTSUPP;
> + return -ENODATA;
It seems like this could be a void function, and the (one) caller
could check for null before making the call.
Either way though, this looks good.
Reviewed-by: Alex Elder <elder@linaro.org>
>
> rb_erase(&xattr->node, &ci->i_xattrs.index);
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/5] ceph: add missing init_acl() for mkdir() and atomic_open()
2014-02-11 5:30 [PATCH 1/5] ceph: properly handle XATTR_CREATE and XATTR_REPLACE Yan, Zheng
2014-02-11 5:30 ` [PATCH 2/5] ceph: remove zero-size xattr Yan, Zheng
2014-02-11 5:30 ` [PATCH 3/5] ceph: fix ceph_removexattr() Yan, Zheng
@ 2014-02-11 5:30 ` Yan, Zheng
2014-02-11 17:45 ` Alex Elder
2014-02-14 5:46 ` Guangliang Zhao
2014-02-11 5:30 ` [PATCH 5/5] ceph: fix ceph_set_acl() Yan, Zheng
2014-02-11 14:36 ` [PATCH 1/5] ceph: properly handle XATTR_CREATE and XATTR_REPLACE Alex Elder
4 siblings, 2 replies; 19+ messages in thread
From: Yan, Zheng @ 2014-02-11 5:30 UTC (permalink / raw)
To: ceph-devel; +Cc: sage, Yan, Zheng
Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
---
fs/ceph/dir.c | 9 +++++----
fs/ceph/file.c | 1 +
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 6da4df8..3bbd0eb 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -695,9 +695,8 @@ static int ceph_mknod(struct inode *dir, struct dentry *dentry,
ceph_mdsc_put_request(req);
if (!err)
- err = ceph_init_acl(dentry, dentry->d_inode, dir);
-
- if (err)
+ ceph_init_acl(dentry, dentry->d_inode, dir);
+ else
d_drop(dentry);
return err;
}
@@ -776,7 +775,9 @@ static int ceph_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
err = ceph_handle_notrace_create(dir, dentry);
ceph_mdsc_put_request(req);
out:
- if (err < 0)
+ if (!err)
+ ceph_init_acl(dentry, dentry->d_inode, dir);
+ else
d_drop(dentry);
return err;
}
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index dfd2ce3..09c7afe 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -286,6 +286,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
} else {
dout("atomic_open finish_open on dn %p\n", dn);
if (req->r_op == CEPH_MDS_OP_CREATE && req->r_reply_info.has_create_ino) {
+ ceph_init_acl(dentry, dentry->d_inode, dir);
*opened |= FILE_CREATED;
}
err = finish_open(file, dentry, ceph_open, opened);
--
1.8.5.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/5] ceph: add missing init_acl() for mkdir() and atomic_open()
2014-02-11 5:30 ` [PATCH 4/5] ceph: add missing init_acl() for mkdir() and atomic_open() Yan, Zheng
@ 2014-02-11 17:45 ` Alex Elder
2014-02-14 5:46 ` Guangliang Zhao
1 sibling, 0 replies; 19+ messages in thread
From: Alex Elder @ 2014-02-11 17:45 UTC (permalink / raw)
To: Yan, Zheng, ceph-devel; +Cc: sage
On 02/10/2014 11:30 PM, Yan, Zheng wrote:
This looks OK to me but you should get another
opinion, I haven't really given it as thorough a
review as I normally do.
Reviewed-by: Alex Elder <elder@linaro.org>
> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
> ---
> fs/ceph/dir.c | 9 +++++----
> fs/ceph/file.c | 1 +
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 6da4df8..3bbd0eb 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -695,9 +695,8 @@ static int ceph_mknod(struct inode *dir, struct dentry *dentry,
> ceph_mdsc_put_request(req);
>
> if (!err)
> - err = ceph_init_acl(dentry, dentry->d_inode, dir);
> -
> - if (err)
> + ceph_init_acl(dentry, dentry->d_inode, dir);
> + else
> d_drop(dentry);
> return err;
> }
> @@ -776,7 +775,9 @@ static int ceph_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
> err = ceph_handle_notrace_create(dir, dentry);
> ceph_mdsc_put_request(req);
> out:
> - if (err < 0)
> + if (!err)
> + ceph_init_acl(dentry, dentry->d_inode, dir);
> + else
Do symlinks have ACLs? (I don't know, I just notice that
ceph_symlink() doesn't call ceph_init_acl() on the symlink.)
> d_drop(dentry);
> return err;
> }
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index dfd2ce3..09c7afe 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -286,6 +286,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
> } else {
> dout("atomic_open finish_open on dn %p\n", dn);
> if (req->r_op == CEPH_MDS_OP_CREATE && req->r_reply_info.has_create_ino) {
> + ceph_init_acl(dentry, dentry->d_inode, dir);
> *opened |= FILE_CREATED;
This seems OK but I have to admit I don't know this path
through the code and I don't have time to really dig into
it right now.
> }
> err = finish_open(file, dentry, ceph_open, opened);
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/5] ceph: add missing init_acl() for mkdir() and atomic_open()
2014-02-11 5:30 ` [PATCH 4/5] ceph: add missing init_acl() for mkdir() and atomic_open() Yan, Zheng
2014-02-11 17:45 ` Alex Elder
@ 2014-02-14 5:46 ` Guangliang Zhao
2014-02-14 6:07 ` Yan, Zheng
2014-02-14 6:12 ` Yan, Zheng
1 sibling, 2 replies; 19+ messages in thread
From: Guangliang Zhao @ 2014-02-14 5:46 UTC (permalink / raw)
To: Yan, Zheng; +Cc: ceph-devel, sage
On Tue, Feb 11, 2014 at 01:30:11PM +0800, Yan Zheng wrote:
This looks good, but there maybe some hiccups, see below.
The symlink also need ceph_init_acl() as Alex mentioned, I have
add it in the patch.
> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
> ---
> fs/ceph/dir.c | 9 +++++----
> fs/ceph/file.c | 1 +
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 6da4df8..3bbd0eb 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -695,9 +695,8 @@ static int ceph_mknod(struct inode *dir, struct dentry *dentry,
> ceph_mdsc_put_request(req);
>
> if (!err)
> - err = ceph_init_acl(dentry, dentry->d_inode, dir);
> -
> - if (err)
> + ceph_init_acl(dentry, dentry->d_inode, dir);
> + else
> d_drop(dentry);
ceph_init_acl() may fail, we shoud take care of the return value.
> return err;
> }
> @@ -776,7 +775,9 @@ static int ceph_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
> err = ceph_handle_notrace_create(dir, dentry);
> ceph_mdsc_put_request(req);
> out:
> - if (err < 0)
> + if (!err)
> + ceph_init_acl(dentry, dentry->d_inode, dir);
> + else
> d_drop(dentry);
> return err;
> }
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index dfd2ce3..09c7afe 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -286,6 +286,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
> } else {
> dout("atomic_open finish_open on dn %p\n", dn);
> if (req->r_op == CEPH_MDS_OP_CREATE && req->r_reply_info.has_create_ino) {
> + ceph_init_acl(dentry, dentry->d_inode, dir);
Same to above.
> *opened |= FILE_CREATED;
> }
> err = finish_open(file, dentry, ceph_open, opened);
> --
> 1.8.5.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best regards,
Guangliang
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/5] ceph: add missing init_acl() for mkdir() and atomic_open()
2014-02-14 5:46 ` Guangliang Zhao
@ 2014-02-14 6:07 ` Yan, Zheng
2014-02-14 6:12 ` Yan, Zheng
1 sibling, 0 replies; 19+ messages in thread
From: Yan, Zheng @ 2014-02-14 6:07 UTC (permalink / raw)
To: ceph-devel, sage
On 02/14/2014 01:46 PM, Guangliang Zhao wrote:
> On Tue, Feb 11, 2014 at 01:30:11PM +0800, Yan Zheng wrote:
>
> This looks good, but there maybe some hiccups, see below.
>
> The symlink also need ceph_init_acl() as Alex mentioned, I have
> add it in the patch.
will update my patch.
>
>> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
>> ---
>> fs/ceph/dir.c | 9 +++++----
>> fs/ceph/file.c | 1 +
>> 2 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
>> index 6da4df8..3bbd0eb 100644
>> --- a/fs/ceph/dir.c
>> +++ b/fs/ceph/dir.c
>> @@ -695,9 +695,8 @@ static int ceph_mknod(struct inode *dir, struct dentry *dentry,
>> ceph_mdsc_put_request(req);
>>
>> if (!err)
>> - err = ceph_init_acl(dentry, dentry->d_inode, dir);
>> -
>> - if (err)
>> + ceph_init_acl(dentry, dentry->d_inode, dir);
>> + else
>> d_drop(dentry);
>
> ceph_init_acl() may fail, we shoud take care of the return value.
I remove the error handling intentionally because creating inode and initializing
ACL are not atomic in current implementation. A better approach is initializing the
ACL on the metadata server, but it requires more works.
Regards
Yan, Zheng
>
>> return err;
>> }
>> @@ -776,7 +775,9 @@ static int ceph_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
>> err = ceph_handle_notrace_create(dir, dentry);
>> ceph_mdsc_put_request(req);
>> out:
>> - if (err < 0)
>> + if (!err)
>> + ceph_init_acl(dentry, dentry->d_inode, dir);
>> + else
>> d_drop(dentry);
>> return err;
>> }
>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>> index dfd2ce3..09c7afe 100644
>> --- a/fs/ceph/file.c
>> +++ b/fs/ceph/file.c
>> @@ -286,6 +286,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
>> } else {
>> dout("atomic_open finish_open on dn %p\n", dn);
>> if (req->r_op == CEPH_MDS_OP_CREATE && req->r_reply_info.has_create_ino) {
>> + ceph_init_acl(dentry, dentry->d_inode, dir);
>
> Same to above.
>
>> *opened |= FILE_CREATED;
>> }
>> err = finish_open(file, dentry, ceph_open, opened);
>> --
>> 1.8.5.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/5] ceph: add missing init_acl() for mkdir() and atomic_open()
2014-02-14 5:46 ` Guangliang Zhao
2014-02-14 6:07 ` Yan, Zheng
@ 2014-02-14 6:12 ` Yan, Zheng
2014-02-14 13:08 ` Alex Elder
1 sibling, 1 reply; 19+ messages in thread
From: Yan, Zheng @ 2014-02-14 6:12 UTC (permalink / raw)
To: ceph-devel, sage
updated patch
---
From 9c517a5af5af472a0bee9c900b7df400b10f3098 Mon Sep 17 00:00:00 2001
From: "Yan, Zheng" <zheng.z.yan@intel.com>
Date: Tue, 11 Feb 2014 12:55:05 +0800
Subject: [PATCH 4/5] ceph: add missing init_acl() for mkdir() and
atomic_open()
Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
---
fs/ceph/dir.c | 13 ++++++++-----
fs/ceph/file.c | 1 +
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 6da4df8..2e3b30d 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -695,9 +695,8 @@ static int ceph_mknod(struct inode *dir, struct dentry *dentry,
ceph_mdsc_put_request(req);
if (!err)
- err = ceph_init_acl(dentry, dentry->d_inode, dir);
-
- if (err)
+ ceph_init_acl(dentry, dentry->d_inode, dir);
+ else
d_drop(dentry);
return err;
}
@@ -735,7 +734,9 @@ static int ceph_symlink(struct inode *dir, struct dentry *dentry,
if (!err && !req->r_reply_info.head->is_dentry)
err = ceph_handle_notrace_create(dir, dentry);
ceph_mdsc_put_request(req);
- if (err)
+ if (!err)
+ ceph_init_acl(dentry, dentry->d_inode, dir);
+ else
d_drop(dentry);
return err;
}
@@ -776,7 +777,9 @@ static int ceph_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
err = ceph_handle_notrace_create(dir, dentry);
ceph_mdsc_put_request(req);
out:
- if (err < 0)
+ if (!err)
+ ceph_init_acl(dentry, dentry->d_inode, dir);
+ else
d_drop(dentry);
return err;
}
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index dfd2ce3..09c7afe 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -286,6 +286,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
} else {
dout("atomic_open finish_open on dn %p\n", dn);
if (req->r_op == CEPH_MDS_OP_CREATE && req->r_reply_info.has_create_ino) {
+ ceph_init_acl(dentry, dentry->d_inode, dir);
*opened |= FILE_CREATED;
}
err = finish_open(file, dentry, ceph_open, opened);
--
1.8.5.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/5] ceph: add missing init_acl() for mkdir() and atomic_open()
2014-02-14 6:12 ` Yan, Zheng
@ 2014-02-14 13:08 ` Alex Elder
0 siblings, 0 replies; 19+ messages in thread
From: Alex Elder @ 2014-02-14 13:08 UTC (permalink / raw)
To: Yan, Zheng, ceph-devel, sage
On 02/14/2014 12:12 AM, Yan, Zheng wrote:
> updated patch
This looks OK, but I just finished reviewing a separate patch that
seems to do this... One or the other. Provided you resolve that:
Reviewed-by: Alex Elder <elder@linaro.org>
> ---
> From 9c517a5af5af472a0bee9c900b7df400b10f3098 Mon Sep 17 00:00:00 2001
> From: "Yan, Zheng" <zheng.z.yan@intel.com>
> Date: Tue, 11 Feb 2014 12:55:05 +0800
> Subject: [PATCH 4/5] ceph: add missing init_acl() for mkdir() and
> atomic_open()
>
> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
> ---
> fs/ceph/dir.c | 13 ++++++++-----
> fs/ceph/file.c | 1 +
> 2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> index 6da4df8..2e3b30d 100644
> --- a/fs/ceph/dir.c
> +++ b/fs/ceph/dir.c
> @@ -695,9 +695,8 @@ static int ceph_mknod(struct inode *dir, struct dentry *dentry,
> ceph_mdsc_put_request(req);
>
> if (!err)
> - err = ceph_init_acl(dentry, dentry->d_inode, dir);
> -
> - if (err)
> + ceph_init_acl(dentry, dentry->d_inode, dir);
> + else
> d_drop(dentry);
> return err;
> }
> @@ -735,7 +734,9 @@ static int ceph_symlink(struct inode *dir, struct dentry *dentry,
> if (!err && !req->r_reply_info.head->is_dentry)
> err = ceph_handle_notrace_create(dir, dentry);
> ceph_mdsc_put_request(req);
> - if (err)
> + if (!err)
> + ceph_init_acl(dentry, dentry->d_inode, dir);
> + else
> d_drop(dentry);
> return err;
> }
> @@ -776,7 +777,9 @@ static int ceph_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
> err = ceph_handle_notrace_create(dir, dentry);
> ceph_mdsc_put_request(req);
> out:
> - if (err < 0)
> + if (!err)
> + ceph_init_acl(dentry, dentry->d_inode, dir);
> + else
> d_drop(dentry);
> return err;
> }
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index dfd2ce3..09c7afe 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -286,6 +286,7 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
> } else {
> dout("atomic_open finish_open on dn %p\n", dn);
> if (req->r_op == CEPH_MDS_OP_CREATE && req->r_reply_info.has_create_ino) {
> + ceph_init_acl(dentry, dentry->d_inode, dir);
> *opened |= FILE_CREATED;
> }
> err = finish_open(file, dentry, ceph_open, opened);
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/5] ceph: fix ceph_set_acl()
2014-02-11 5:30 [PATCH 1/5] ceph: properly handle XATTR_CREATE and XATTR_REPLACE Yan, Zheng
` (2 preceding siblings ...)
2014-02-11 5:30 ` [PATCH 4/5] ceph: add missing init_acl() for mkdir() and atomic_open() Yan, Zheng
@ 2014-02-11 5:30 ` Yan, Zheng
2014-02-11 18:06 ` Alex Elder
2014-02-11 14:36 ` [PATCH 1/5] ceph: properly handle XATTR_CREATE and XATTR_REPLACE Alex Elder
4 siblings, 1 reply; 19+ messages in thread
From: Yan, Zheng @ 2014-02-11 5:30 UTC (permalink / raw)
To: ceph-devel; +Cc: sage, Yan, Zheng
If acl is equivalent to file mode permission bits, ceph_set_acl()
needs to remove any existing acl xattr. Use __ceph_setxattr() to
handle both setting and removing acl xattr cases, it doesn't return
-ENODATA when there is no acl xattr.
Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
---
fs/ceph/acl.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
index 4c2d452..accc9f2 100644
--- a/fs/ceph/acl.c
+++ b/fs/ceph/acl.c
@@ -160,11 +160,7 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
goto out_dput;
}
- if (value)
- ret = __ceph_setxattr(dentry, name, value, size, 0);
- else
- ret = __ceph_removexattr(dentry, name);
-
+ ret = __ceph_setxattr(dentry, name, value, size, 0);
if (ret) {
if (new_mode != old_mode) {
newattrs.ia_mode = old_mode;
--
1.8.5.3
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 5/5] ceph: fix ceph_set_acl()
2014-02-11 5:30 ` [PATCH 5/5] ceph: fix ceph_set_acl() Yan, Zheng
@ 2014-02-11 18:06 ` Alex Elder
0 siblings, 0 replies; 19+ messages in thread
From: Alex Elder @ 2014-02-11 18:06 UTC (permalink / raw)
To: Yan, Zheng, ceph-devel; +Cc: sage
On 02/10/2014 11:30 PM, Yan, Zheng wrote:
> If acl is equivalent to file mode permission bits, ceph_set_acl()
> needs to remove any existing acl xattr. Use __ceph_setxattr() to
> handle both setting and removing acl xattr cases, it doesn't return
> -ENODATA when there is no acl xattr.
This looks good, however (continuing with our other
discussion) I assume that __set_xattr() will use a
null value rather than 0 value_len to determine whether
to remove the xattr.
And I'll continue to suggest better descriptions. (I
realize English is not your native language but I
really think the description is important, to be
able to verify the code change matches what the
developer intended it to do.) The problem here is
that ceph_set_acl() could return -ENODATA if it
attempted to remove a non-existent ACL attribute,
and that shouldn't happen. (Right?)
Reviewed-by: Alex Elder <elder@linaro.org>
PS Note that ceph_set_acl() can be made static, and its
only caller, ceph_init_acl() has its return value
ignored in all cases. So in some respects this change
not matter much.
> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
> ---
> fs/ceph/acl.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
> index 4c2d452..accc9f2 100644
> --- a/fs/ceph/acl.c
> +++ b/fs/ceph/acl.c
> @@ -160,11 +160,7 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> goto out_dput;
> }
>
> - if (value)
> - ret = __ceph_setxattr(dentry, name, value, size, 0);
> - else
> - ret = __ceph_removexattr(dentry, name);
> -
> + ret = __ceph_setxattr(dentry, name, value, size, 0);
> if (ret) {
> if (new_mode != old_mode) {
> newattrs.ia_mode = old_mode;
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/5] ceph: properly handle XATTR_CREATE and XATTR_REPLACE
2014-02-11 5:30 [PATCH 1/5] ceph: properly handle XATTR_CREATE and XATTR_REPLACE Yan, Zheng
` (3 preceding siblings ...)
2014-02-11 5:30 ` [PATCH 5/5] ceph: fix ceph_set_acl() Yan, Zheng
@ 2014-02-11 14:36 ` Alex Elder
4 siblings, 0 replies; 19+ messages in thread
From: Alex Elder @ 2014-02-11 14:36 UTC (permalink / raw)
To: Yan, Zheng, ceph-devel; +Cc: sage
On 02/10/2014 11:30 PM, Yan, Zheng wrote:
> return -EEXIST if XATTR_CREATE is set and xattr alread exists.
> return -ENODATA if XATTR_REPLACE is set but xattr does not exist.
(I have a set of changes to this source file that I never
got in; I'll see if I can dig them up and post them for
review too, as long as the file is getting some attention.)
My man page says this second case should return ENOATTR,
and although that has the same numeric value as ENODATA,
that could someday change.
That being said, I see "fs/xattr.c uses ENODATA, so I
suppose what you've got is fine. (Maybe somebody should
fix the man page to resolve the discrepancy.)
I have one other comment at the end--basically that you're
fixing a bug in addition to what you've mentioned above.
Please at least mention that in your explanation.
Otherwise this looks good.
Reviewed-by: Alex Elder <elder@linaro.org>
>
> Signed-off-by: Yan, Zheng <zheng.z.yan@intel.com>
> ---
> fs/ceph/xattr.c | 38 ++++++++++++++++++++++++++------------
> 1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index 898b656..28f9793 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -319,8 +319,7 @@ static struct ceph_vxattr *ceph_match_vxattr(struct inode *inode,
> static int __set_xattr(struct ceph_inode_info *ci,
> const char *name, int name_len,
> const char *val, int val_len,
> - int dirty,
> - int should_free_name, int should_free_val,
> + int flags, int update_xattr,
> struct ceph_inode_xattr **newxattr)
> {
> struct rb_node **p;
> @@ -349,12 +348,25 @@ static int __set_xattr(struct ceph_inode_info *ci,
> xattr = NULL;
> }
>
> + if (update_xattr) {
> + int err = 0;
> + if (xattr && (flags & XATTR_CREATE))
> + err = -EEXIST;
> + else if (!xattr && (flags & XATTR_REPLACE))
> + err = -ENODATA;
> + if (err) {
> + kfree(name);
> + kfree(val);
> + return err;
> + }
> + }
> +
> if (!xattr) {
> new = 1;
> xattr = *newxattr;
> xattr->name = name;
> xattr->name_len = name_len;
> - xattr->should_free_name = should_free_name;
> + xattr->should_free_name = update_xattr;
>
> ci->i_xattrs.count++;
> dout("__set_xattr count=%d\n", ci->i_xattrs.count);
> @@ -364,7 +376,7 @@ static int __set_xattr(struct ceph_inode_info *ci,
> if (xattr->should_free_val)
> kfree((void *)xattr->val);
>
> - if (should_free_name) {
> + if (update_xattr) {
> kfree((void *)name);
> name = xattr->name;
> }
> @@ -379,8 +391,8 @@ static int __set_xattr(struct ceph_inode_info *ci,
> xattr->val = "";
>
> xattr->val_len = val_len;
> - xattr->dirty = dirty;
> - xattr->should_free_val = (val && should_free_val);
> + xattr->dirty = update_xattr;
> + xattr->should_free_val = (val && update_xattr);
>
> if (new) {
> rb_link_node(&xattr->node, parent, p);
> @@ -588,7 +600,7 @@ start:
> p += len;
>
> err = __set_xattr(ci, name, namelen, val, len,
> - 0, 0, 0, &xattrs[numattr]);
> + 0, 0, &xattrs[numattr]);
>
> if (err < 0)
> goto bad;
> @@ -892,7 +904,7 @@ int __ceph_setxattr(struct dentry *dentry, const char *name,
> struct ceph_inode_info *ci = ceph_inode(inode);
> int issued;
> int err;
> - int dirty;
> + int dirty = 0;
> int name_len = strlen(name);
> int val_len = size;
> char *newname = NULL;
> @@ -954,11 +966,13 @@ retry:
> }
>
> err = __set_xattr(ci, newname, name_len, newval,
> - val_len, 1, 1, 1, &xattr);
> + val_len, flags, 1, &xattr);
>
The following is a good change (only mark ceph inode dirty
if __set_xattr() succeeds), but it is a change of behavior
that should be called attention to in the explanation at the
top of this patch (and possibly separated into its own patch).
> - dirty = __ceph_mark_dirty_caps(ci, CEPH_CAP_XATTR_EXCL);
> - ci->i_xattrs.dirty = true;
> - inode->i_ctime = CURRENT_TIME;
> + if (!err) {
> + dirty = __ceph_mark_dirty_caps(ci, CEPH_CAP_XATTR_EXCL);
> + ci->i_xattrs.dirty = true;
> + inode->i_ctime = CURRENT_TIME;
> + }
>
> spin_unlock(&ci->i_ceph_lock);
> if (dirty)
>
^ permalink raw reply [flat|nested] 19+ messages in thread