[v4,17/18] btrfs: dedup: add a property handler for online dedup
diff mbox

Message ID 1452751054-2365-18-git-send-email-quwenruo@cn.fujitsu.com
State New
Headers show

Commit Message

Qu Wenruo Jan. 14, 2016, 5:57 a.m. UTC
From: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>

We use btrfs extended attribute "btrfs.dedup" to record per-file online
dedup status, so add a dedup property handler.

Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
---
 fs/btrfs/props.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Comments

Filipe Manana Jan. 14, 2016, 9:56 a.m. UTC | #1
On Thu, Jan 14, 2016 at 5:57 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
> From: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>
> We use btrfs extended attribute "btrfs.dedup" to record per-file online
> dedup status, so add a dedup property handler.
>
> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
> ---
>  fs/btrfs/props.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index f9e6023..ae8b76d 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -41,6 +41,10 @@ static int prop_compression_apply(struct inode *inode,
>                                   size_t len);
>  static const char *prop_compression_extract(struct inode *inode);
>
> +static int prop_dedup_validate(const char *value, size_t len);
> +static int prop_dedup_apply(struct inode *inode, const char *value, size_t len);
> +static const char *prop_dedup_extract(struct inode *inode);
> +
>  static struct prop_handler prop_handlers[] = {
>         {
>                 .xattr_name = XATTR_BTRFS_PREFIX "compression",
> @@ -49,6 +53,13 @@ static struct prop_handler prop_handlers[] = {
>                 .extract = prop_compression_extract,
>                 .inheritable = 1
>         },
> +       {
> +               .xattr_name = XATTR_BTRFS_PREFIX "dedup",
> +               .validate = prop_dedup_validate,
> +               .apply = prop_dedup_apply,
> +               .extract = prop_dedup_extract,
> +               .inheritable = 1
> +       },
>  };
>
>  void __init btrfs_props_init(void)
> @@ -425,4 +436,33 @@ static const char *prop_compression_extract(struct inode *inode)
>         return NULL;
>  }
>
> +static int prop_dedup_validate(const char *value, size_t len)
> +{
> +       if (!strncmp("disable", value, len))
> +               return 0;
> +
> +       return -EINVAL;
> +}
> +
> +static int prop_dedup_apply(struct inode *inode, const char *value, size_t len)
> +{
> +       if (len == 0) {
> +               BTRFS_I(inode)->flags &= ~BTRFS_INODE_NODEDUP;
> +               return 0;
> +       }
> +
> +       if (!strncmp("disable", value, len)) {
> +               BTRFS_I(inode)->flags |= BTRFS_INODE_NODEDUP;
> +               return 0;
> +       }
> +
> +       return -EINVAL;
> +}
> +
> +static const char *prop_dedup_extract(struct inode *inode)
> +{
> +       if (BTRFS_I(inode)->flags | BTRFS_INODE_NODEDUP)
> +               return "disable";

| -> &

How about writing test cases (for xfstests)? Not only for the property
but for the whole dedup feature, it would avoid such simple
mistakes...
Take a look at the good example of xfs development. For example when
all the recent patches for their reflink implementation was posted
(and before getting merged), a comprehensive set of test cases for
xfstests was also posted...

>
> +       return NULL;
> +}
> --
> 2.7.0
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong Jan. 14, 2016, 7:04 p.m. UTC | #2
On Thu, Jan 14, 2016 at 09:56:12AM +0000, Filipe Manana wrote:
> On Thu, Jan 14, 2016 at 5:57 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
> > From: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
> >
> > We use btrfs extended attribute "btrfs.dedup" to record per-file online
> > dedup status, so add a dedup property handler.
> >
> > Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
> > ---
> >  fs/btrfs/props.c | 40 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> > index f9e6023..ae8b76d 100644
> > --- a/fs/btrfs/props.c
> > +++ b/fs/btrfs/props.c
> > @@ -41,6 +41,10 @@ static int prop_compression_apply(struct inode *inode,
> >                                   size_t len);
> >  static const char *prop_compression_extract(struct inode *inode);
> >
> > +static int prop_dedup_validate(const char *value, size_t len);
> > +static int prop_dedup_apply(struct inode *inode, const char *value, size_t len);
> > +static const char *prop_dedup_extract(struct inode *inode);
> > +
> >  static struct prop_handler prop_handlers[] = {
> >         {
> >                 .xattr_name = XATTR_BTRFS_PREFIX "compression",
> > @@ -49,6 +53,13 @@ static struct prop_handler prop_handlers[] = {
> >                 .extract = prop_compression_extract,
> >                 .inheritable = 1
> >         },
> > +       {
> > +               .xattr_name = XATTR_BTRFS_PREFIX "dedup",
> > +               .validate = prop_dedup_validate,
> > +               .apply = prop_dedup_apply,
> > +               .extract = prop_dedup_extract,
> > +               .inheritable = 1
> > +       },
> >  };
> >
> >  void __init btrfs_props_init(void)
> > @@ -425,4 +436,33 @@ static const char *prop_compression_extract(struct inode *inode)
> >         return NULL;
> >  }
> >
> > +static int prop_dedup_validate(const char *value, size_t len)
> > +{
> > +       if (!strncmp("disable", value, len))
> > +               return 0;
> > +
> > +       return -EINVAL;
> > +}
> > +
> > +static int prop_dedup_apply(struct inode *inode, const char *value, size_t len)
> > +{
> > +       if (len == 0) {
> > +               BTRFS_I(inode)->flags &= ~BTRFS_INODE_NODEDUP;
> > +               return 0;
> > +       }
> > +
> > +       if (!strncmp("disable", value, len)) {
> > +               BTRFS_I(inode)->flags |= BTRFS_INODE_NODEDUP;
> > +               return 0;
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> > +
> > +static const char *prop_dedup_extract(struct inode *inode)
> > +{
> > +       if (BTRFS_I(inode)->flags | BTRFS_INODE_NODEDUP)
> > +               return "disable";
> 
> | -> &
> 
> How about writing test cases (for xfstests)? Not only for the property
> but for the whole dedup feature, it would avoid such simple
> mistakes...
> Take a look at the good example of xfs development. For example when
> all the recent patches for their reflink implementation was posted
> (and before getting merged), a comprehensive set of test cases for
> xfstests was also posted...

Seconded. ;)

(More reflink xfstests are coming, too...)

--D

> 
> >
> > +       return NULL;
> > +}
> > --
> > 2.7.0
> >
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> -- 
> Filipe David Manana,
> 
> "Reasonable men adapt themselves to the world.
>  Unreasonable men adapt the world to themselves.
>  That's why all progress depends on unreasonable men."
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 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 linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Qu Wenruo Jan. 15, 2016, 1:37 a.m. UTC | #3
Filipe Manana wrote on 2016/01/14 09:56 +0000:
> On Thu, Jan 14, 2016 at 5:57 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>> From: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>>
>> We use btrfs extended attribute "btrfs.dedup" to record per-file online
>> dedup status, so add a dedup property handler.
>>
>> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>> ---
>>   fs/btrfs/props.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>
>> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
>> index f9e6023..ae8b76d 100644
>> --- a/fs/btrfs/props.c
>> +++ b/fs/btrfs/props.c
>> @@ -41,6 +41,10 @@ static int prop_compression_apply(struct inode *inode,
>>                                    size_t len);
>>   static const char *prop_compression_extract(struct inode *inode);
>>
>> +static int prop_dedup_validate(const char *value, size_t len);
>> +static int prop_dedup_apply(struct inode *inode, const char *value, size_t len);
>> +static const char *prop_dedup_extract(struct inode *inode);
>> +
>>   static struct prop_handler prop_handlers[] = {
>>          {
>>                  .xattr_name = XATTR_BTRFS_PREFIX "compression",
>> @@ -49,6 +53,13 @@ static struct prop_handler prop_handlers[] = {
>>                  .extract = prop_compression_extract,
>>                  .inheritable = 1
>>          },
>> +       {
>> +               .xattr_name = XATTR_BTRFS_PREFIX "dedup",
>> +               .validate = prop_dedup_validate,
>> +               .apply = prop_dedup_apply,
>> +               .extract = prop_dedup_extract,
>> +               .inheritable = 1
>> +       },
>>   };
>>
>>   void __init btrfs_props_init(void)
>> @@ -425,4 +436,33 @@ static const char *prop_compression_extract(struct inode *inode)
>>          return NULL;
>>   }
>>
>> +static int prop_dedup_validate(const char *value, size_t len)
>> +{
>> +       if (!strncmp("disable", value, len))
>> +               return 0;
>> +
>> +       return -EINVAL;
>> +}
>> +
>> +static int prop_dedup_apply(struct inode *inode, const char *value, size_t len)
>> +{
>> +       if (len == 0) {
>> +               BTRFS_I(inode)->flags &= ~BTRFS_INODE_NODEDUP;
>> +               return 0;
>> +       }
>> +
>> +       if (!strncmp("disable", value, len)) {
>> +               BTRFS_I(inode)->flags |= BTRFS_INODE_NODEDUP;
>> +               return 0;
>> +       }
>> +
>> +       return -EINVAL;
>> +}
>> +
>> +static const char *prop_dedup_extract(struct inode *inode)
>> +{
>> +       if (BTRFS_I(inode)->flags | BTRFS_INODE_NODEDUP)
>> +               return "disable";
>
> | -> &
>
> How about writing test cases (for xfstests)? Not only for the property
> but for the whole dedup feature, it would avoid such simple
> mistakes...

Yes, it's already in our schedule and we have some internal simple test 
case.

But the problem is,
Some behavior/format is not completely determined yet
Especially for on-disk format (ondisk backend only), and ioctl interface.

So I'm a little concerned about submit them too early before we made 
final decision on the ioctl interface.

> Take a look at the good example of xfs development. For example when
> all the recent patches for their reflink implementation was posted
> (and before getting merged), a comprehensive set of test cases for
> xfstests was also posted...

Yes, test driven development is very nice, but there is also problem, 
especially for btrfs, like btrfs/047 which needs btrfs send 
--stream-version support.

But unfortunately, there is not --stream-version support in upsteam 
btrfs-progs and the test case never get executed on upsteam btrfs-progs.

Personally speaking, xfs development can only happen in that way because 
xfstest maintainer is also the maintainer of xfs.
As Dave knows every aspect of xfs and can determine which feature will 
be merged.
But that's not true for btrfs, so he can merge wrong patch and cause 
inconsistent with what btrfs really supports between test cases.

Thanks,
Qu

>
>>
>> +       return NULL;
>> +}
>> --
>> 2.7.0
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 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 linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Filipe Manana Jan. 15, 2016, 9:19 a.m. UTC | #4
On Fri, Jan 15, 2016 at 1:37 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>
>
> Filipe Manana wrote on 2016/01/14 09:56 +0000:
>>
>> On Thu, Jan 14, 2016 at 5:57 AM, Qu Wenruo <quwenruo@cn.fujitsu.com>
>> wrote:
>>>
>>> From: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>>>
>>> We use btrfs extended attribute "btrfs.dedup" to record per-file online
>>> dedup status, so add a dedup property handler.
>>>
>>> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>>> ---
>>>   fs/btrfs/props.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 40 insertions(+)
>>>
>>> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
>>> index f9e6023..ae8b76d 100644
>>> --- a/fs/btrfs/props.c
>>> +++ b/fs/btrfs/props.c
>>> @@ -41,6 +41,10 @@ static int prop_compression_apply(struct inode *inode,
>>>                                    size_t len);
>>>   static const char *prop_compression_extract(struct inode *inode);
>>>
>>> +static int prop_dedup_validate(const char *value, size_t len);
>>> +static int prop_dedup_apply(struct inode *inode, const char *value,
>>> size_t len);
>>> +static const char *prop_dedup_extract(struct inode *inode);
>>> +
>>>   static struct prop_handler prop_handlers[] = {
>>>          {
>>>                  .xattr_name = XATTR_BTRFS_PREFIX "compression",
>>> @@ -49,6 +53,13 @@ static struct prop_handler prop_handlers[] = {
>>>                  .extract = prop_compression_extract,
>>>                  .inheritable = 1
>>>          },
>>> +       {
>>> +               .xattr_name = XATTR_BTRFS_PREFIX "dedup",
>>> +               .validate = prop_dedup_validate,
>>> +               .apply = prop_dedup_apply,
>>> +               .extract = prop_dedup_extract,
>>> +               .inheritable = 1
>>> +       },
>>>   };
>>>
>>>   void __init btrfs_props_init(void)
>>> @@ -425,4 +436,33 @@ static const char *prop_compression_extract(struct
>>> inode *inode)
>>>          return NULL;
>>>   }
>>>
>>> +static int prop_dedup_validate(const char *value, size_t len)
>>> +{
>>> +       if (!strncmp("disable", value, len))
>>> +               return 0;
>>> +
>>> +       return -EINVAL;
>>> +}
>>> +
>>> +static int prop_dedup_apply(struct inode *inode, const char *value,
>>> size_t len)
>>> +{
>>> +       if (len == 0) {
>>> +               BTRFS_I(inode)->flags &= ~BTRFS_INODE_NODEDUP;
>>> +               return 0;
>>> +       }
>>> +
>>> +       if (!strncmp("disable", value, len)) {
>>> +               BTRFS_I(inode)->flags |= BTRFS_INODE_NODEDUP;
>>> +               return 0;
>>> +       }
>>> +
>>> +       return -EINVAL;
>>> +}
>>> +
>>> +static const char *prop_dedup_extract(struct inode *inode)
>>> +{
>>> +       if (BTRFS_I(inode)->flags | BTRFS_INODE_NODEDUP)
>>> +               return "disable";
>>
>>
>> | -> &
>>
>> How about writing test cases (for xfstests)? Not only for the property
>> but for the whole dedup feature, it would avoid such simple
>> mistakes...
>
>
> Yes, it's already in our schedule and we have some internal simple test
> case.
>
> But the problem is,
> Some behavior/format is not completely determined yet
> Especially for on-disk format (ondisk backend only), and ioctl interface.

How does someone guesses that? You could add an RFC tag to the patches...

>
> So I'm a little concerned about submit them too early before we made final
> decision on the ioctl interface.

What's the problem? You can submit such tests only to the btrfs list
for now and tag them as RFC and mention that.

>
>> Take a look at the good example of xfs development. For example when
>> all the recent patches for their reflink implementation was posted
>> (and before getting merged), a comprehensive set of test cases for
>> xfstests was also posted...
>
>
> Yes, test driven development is very nice, but there is also problem,
> especially for btrfs, like btrfs/047 which needs btrfs send --stream-version
> support.

Yes, this one was a special case in that around 2 years ago it seemed
the functionality was going to be merged, but it ended up not being
merged due to reasons not known to me (no reasons pointed in the
mailing list nor in private).

Better have unused tests instead of features without tests (and many
btrfs specific features had no tests or almost nothing not that long
ago).

>
> But unfortunately, there is not --stream-version support in upsteam
> btrfs-progs and the test case never get executed on upsteam btrfs-progs.
>
> Personally speaking, xfs development can only happen in that way because
> xfstest maintainer is also the maintainer of xfs.
> As Dave knows every aspect of xfs and can determine which feature will be
> merged.

He can also accept patches to remove tests or change them...
Shouldn't be an excuse to not share tests along with patches for new
features, as noted above.

> But that's not true for btrfs, so he can merge wrong patch and cause
> inconsistent with what btrfs really supports between test cases.
>
> Thanks,
> Qu
>
>
>>
>>>
>>> +       return NULL;
>>> +}
>>> --
>>> 2.7.0
>>>
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>>
>
>
Qu Wenruo Jan. 15, 2016, 9:33 a.m. UTC | #5
Filipe Manana wrote on 2016/01/15 09:19 +0000:
> On Fri, Jan 15, 2016 at 1:37 AM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>>
>>
>> Filipe Manana wrote on 2016/01/14 09:56 +0000:
>>>
>>> On Thu, Jan 14, 2016 at 5:57 AM, Qu Wenruo <quwenruo@cn.fujitsu.com>
>>> wrote:
>>>>
>>>> From: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>>>>
>>>> We use btrfs extended attribute "btrfs.dedup" to record per-file online
>>>> dedup status, so add a dedup property handler.
>>>>
>>>> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>>>> ---
>>>>    fs/btrfs/props.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 40 insertions(+)
>>>>
>>>> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
>>>> index f9e6023..ae8b76d 100644
>>>> --- a/fs/btrfs/props.c
>>>> +++ b/fs/btrfs/props.c
>>>> @@ -41,6 +41,10 @@ static int prop_compression_apply(struct inode *inode,
>>>>                                     size_t len);
>>>>    static const char *prop_compression_extract(struct inode *inode);
>>>>
>>>> +static int prop_dedup_validate(const char *value, size_t len);
>>>> +static int prop_dedup_apply(struct inode *inode, const char *value,
>>>> size_t len);
>>>> +static const char *prop_dedup_extract(struct inode *inode);
>>>> +
>>>>    static struct prop_handler prop_handlers[] = {
>>>>           {
>>>>                   .xattr_name = XATTR_BTRFS_PREFIX "compression",
>>>> @@ -49,6 +53,13 @@ static struct prop_handler prop_handlers[] = {
>>>>                   .extract = prop_compression_extract,
>>>>                   .inheritable = 1
>>>>           },
>>>> +       {
>>>> +               .xattr_name = XATTR_BTRFS_PREFIX "dedup",
>>>> +               .validate = prop_dedup_validate,
>>>> +               .apply = prop_dedup_apply,
>>>> +               .extract = prop_dedup_extract,
>>>> +               .inheritable = 1
>>>> +       },
>>>>    };
>>>>
>>>>    void __init btrfs_props_init(void)
>>>> @@ -425,4 +436,33 @@ static const char *prop_compression_extract(struct
>>>> inode *inode)
>>>>           return NULL;
>>>>    }
>>>>
>>>> +static int prop_dedup_validate(const char *value, size_t len)
>>>> +{
>>>> +       if (!strncmp("disable", value, len))
>>>> +               return 0;
>>>> +
>>>> +       return -EINVAL;
>>>> +}
>>>> +
>>>> +static int prop_dedup_apply(struct inode *inode, const char *value,
>>>> size_t len)
>>>> +{
>>>> +       if (len == 0) {
>>>> +               BTRFS_I(inode)->flags &= ~BTRFS_INODE_NODEDUP;
>>>> +               return 0;
>>>> +       }
>>>> +
>>>> +       if (!strncmp("disable", value, len)) {
>>>> +               BTRFS_I(inode)->flags |= BTRFS_INODE_NODEDUP;
>>>> +               return 0;
>>>> +       }
>>>> +
>>>> +       return -EINVAL;
>>>> +}
>>>> +
>>>> +static const char *prop_dedup_extract(struct inode *inode)
>>>> +{
>>>> +       if (BTRFS_I(inode)->flags | BTRFS_INODE_NODEDUP)
>>>> +               return "disable";
>>>
>>>
>>> | -> &
>>>
>>> How about writing test cases (for xfstests)? Not only for the property
>>> but for the whole dedup feature, it would avoid such simple
>>> mistakes...
>>
>>
>> Yes, it's already in our schedule and we have some internal simple test
>> case.
>>
>> But the problem is,
>> Some behavior/format is not completely determined yet
>> Especially for on-disk format (ondisk backend only), and ioctl interface.
>
> How does someone guesses that? You could add an RFC tag to the patches...
>
>>
>> So I'm a little concerned about submit them too early before we made final
>> decision on the ioctl interface.
>
> What's the problem? You can submit such tests only to the btrfs list
> for now and tag them as RFC and mention that.

Wow, I never though xfstest patches can be submitted to btrfs maillist 
only!!

What a wonderful idea for RFC features like dedup!

I'll soon add such test cases.

Great thanks for such advice!
Qu

>
>>
>>> Take a look at the good example of xfs development. For example when
>>> all the recent patches for their reflink implementation was posted
>>> (and before getting merged), a comprehensive set of test cases for
>>> xfstests was also posted...
>>
>>
>> Yes, test driven development is very nice, but there is also problem,
>> especially for btrfs, like btrfs/047 which needs btrfs send --stream-version
>> support.
>
> Yes, this one was a special case in that around 2 years ago it seemed
> the functionality was going to be merged, but it ended up not being
> merged due to reasons not known to me (no reasons pointed in the
> mailing list nor in private).
>
> Better have unused tests instead of features without tests (and many
> btrfs specific features had no tests or almost nothing not that long
> ago).
>
>>
>> But unfortunately, there is not --stream-version support in upsteam
>> btrfs-progs and the test case never get executed on upsteam btrfs-progs.
>>
>> Personally speaking, xfs development can only happen in that way because
>> xfstest maintainer is also the maintainer of xfs.
>> As Dave knows every aspect of xfs and can determine which feature will be
>> merged.
>
> He can also accept patches to remove tests or change them...
> Shouldn't be an excuse to not share tests along with patches for new
> features, as noted above.
>
>> But that's not true for btrfs, so he can merge wrong patch and cause
>> inconsistent with what btrfs really supports between test cases.
>>
>> Thanks,
>> Qu
>>
>>
>>>
>>>>
>>>> +       return NULL;
>>>> +}
>>>> --
>>>> 2.7.0
>>>>
>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" 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 linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Duncan Jan. 15, 2016, 12:36 p.m. UTC | #6
Filipe Manana posted on Fri, 15 Jan 2016 09:19:28 +0000 as excerpted:

>> Yes, test driven development is very nice, but there is also problem,
>> especially for btrfs, like btrfs/047 which needs btrfs send
>> --stream-version support.
> 
> Yes, this one was a special case in that around 2 years ago it seemed
> the functionality was going to be merged, but it ended up not being
> merged due to reasons not known to me (no reasons pointed in the mailing
> list nor in private).

AFAIK/IIRC, the reason as I understood it is that ideally Chris Mason 
wants to do just one more stream version bump, which means being /very/ 
sure it includes anything that has come up as needed in intervening 
development.  There's a few things agreed to be missing in the current 
stream version, yes, but it's usable as-is in the near term, and before 
that bump is done, everything else related needs to be in a mature enough 
development state that we can be reasonably sure nothing else is going to 
come up as still missing, at least for a couple years after the stream 
version bump.

And without a second stream version to try, a --stream-version option 
isn't particularly testable, so it was judged to be too early to add it 
as an option.
Filipe Manana Jan. 15, 2016, 3:22 p.m. UTC | #7
On Fri, Jan 15, 2016 at 12:36 PM, Duncan <1i5t5.duncan@cox.net> wrote:
> Filipe Manana posted on Fri, 15 Jan 2016 09:19:28 +0000 as excerpted:
>
>>> Yes, test driven development is very nice, but there is also problem,
>>> especially for btrfs, like btrfs/047 which needs btrfs send
>>> --stream-version support.
>>
>> Yes, this one was a special case in that around 2 years ago it seemed
>> the functionality was going to be merged, but it ended up not being
>> merged due to reasons not known to me (no reasons pointed in the mailing
>> list nor in private).
>
> AFAIK/IIRC, the reason as I understood it is that ideally Chris Mason
> wants to do just one more stream version bump, which means being /very/
> sure it includes anything that has come up as needed in intervening
> development.  There's a few things agreed to be missing in the current
> stream version

Either you have had access to information I didn't had (Chris didn't
ever commented on
it, but Josef, David and Mark had) or you missed part of past discussions in the
mailing list (or perhaps my mail accounts didn't got anything).
Yes there were several new things discussed for a new version (and lot
of it listed in some
wiki page iirc), some of which I implemented and the remaining I
didn't, but left the necessary
changes in the stream format to avoid bumping the stream version
again. And that's where things
settled, without any more comments (neither on the mailing list nor
anywhere I could read,
something that happens often unfortunately).

>, yes, but it's usable as-is in the near term, and before
> that bump is done, everything else related needs to be in a mature enough
> development state that we can be reasonably sure nothing else is going to
> come up as still missing, at least for a couple years after the stream
> version bump.

Yes, send has had (and still has) several problems, but that shouldn't
prevent it from
getting more new useful commands indefinitely. Just like every part of
btrfs still has
bugs (you can see it either from the endless user reports or patches
coming in in every
release) with several degrees of severity.

>
> And without a second stream version to try, a --stream-version option
> isn't particularly testable, so it was judged to be too early to add it
> as an option.
>
> --
> Duncan - List replies preferred.   No HTML msgs.
> "Every nonfree program has a lord, a master --
> and if you use the program, he is your master."  Richard Stallman
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Duncan Jan. 16, 2016, 2:53 a.m. UTC | #8
Filipe Manana posted on Fri, 15 Jan 2016 15:22:39 +0000 as excerpted:

> On Fri, Jan 15, 2016 at 12:36 PM, Duncan <1i5t5.duncan@cox.net> wrote:
>> Filipe Manana posted on Fri, 15 Jan 2016 09:19:28 +0000 as excerpted:
>>
>>>> Yes, test driven development is very nice, but there is also problem,
>>>> especially for btrfs, like btrfs/047 which needs btrfs send
>>>> --stream-version support.
>>>
>>> Yes, this one was a special case in that around 2 years ago it seemed
>>> the functionality was going to be merged, but it ended up not being
>>> merged due to reasons not known to me (no reasons pointed in the
>>> mailing list nor in private).
>>
>> AFAIK/IIRC, the reason as I understood it is that ideally Chris Mason
>> wants to do just one more stream version bump, which means being /very/
>> sure it includes anything that has come up as needed in intervening
>> development.  There's a few things agreed to be missing in the current
>> stream version
> 
> Either you have had access to information I didn't had (Chris didn't
> ever commented on it, but Josef, David and Mark had) or you missed part
> of past discussions in the mailing list (or perhaps my mail accounts
> didn't got anything).
> Yes there were several new things discussed for a new version (and lot
> of it listed in some wiki page iirc), some of which I implemented and
> the remaining I didn't, but left the necessary changes in the stream
> format to avoid bumping the stream version again. And that's where
> things settled, without any more comments (neither on the mailing list
> nor anywhere I could read,
> something that happens often unfortunately).

So AFAIK/IIRC was incorrect, as I thought I had seen CMason comment on it 
but perhaps it was the others, and I wasn't aware that you had 
implemented all the proposed stream format changes, even if not all the 
code to actually make use of them.  But then it just... dropped.

Regardless of the details from back then, however, btrfs is arguably in a 
far more stable and mature state now than it was two years ago, and it 
may well be time to actually do that stream format bump now.

Tho "now" obviously means 4.6 at the earliest as it'll surely take a bit 
to dust off and reevaluate the patches, putting it beyond the current 4.5 
commit window.

Thanks.

Patch
diff mbox

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index f9e6023..ae8b76d 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -41,6 +41,10 @@  static int prop_compression_apply(struct inode *inode,
 				  size_t len);
 static const char *prop_compression_extract(struct inode *inode);
 
+static int prop_dedup_validate(const char *value, size_t len);
+static int prop_dedup_apply(struct inode *inode, const char *value, size_t len);
+static const char *prop_dedup_extract(struct inode *inode);
+
 static struct prop_handler prop_handlers[] = {
 	{
 		.xattr_name = XATTR_BTRFS_PREFIX "compression",
@@ -49,6 +53,13 @@  static struct prop_handler prop_handlers[] = {
 		.extract = prop_compression_extract,
 		.inheritable = 1
 	},
+	{
+		.xattr_name = XATTR_BTRFS_PREFIX "dedup",
+		.validate = prop_dedup_validate,
+		.apply = prop_dedup_apply,
+		.extract = prop_dedup_extract,
+		.inheritable = 1
+	},
 };
 
 void __init btrfs_props_init(void)
@@ -425,4 +436,33 @@  static const char *prop_compression_extract(struct inode *inode)
 	return NULL;
 }
 
+static int prop_dedup_validate(const char *value, size_t len)
+{
+	if (!strncmp("disable", value, len))
+		return 0;
+
+	return -EINVAL;
+}
+
+static int prop_dedup_apply(struct inode *inode, const char *value, size_t len)
+{
+	if (len == 0) {
+		BTRFS_I(inode)->flags &= ~BTRFS_INODE_NODEDUP;
+		return 0;
+	}
+
+	if (!strncmp("disable", value, len)) {
+		BTRFS_I(inode)->flags |= BTRFS_INODE_NODEDUP;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static const char *prop_dedup_extract(struct inode *inode)
+{
+	if (BTRFS_I(inode)->flags | BTRFS_INODE_NODEDUP)
+		return "disable";
 
+	return NULL;
+}