Re: [PATCH] btrfs: Remove unneeded cast to s64 for qgroup rfer state
On Mon, Aug 31, 2015 at 02:51:08PM +0800, Qu Wenruo wrote: > > > Alexandru Moise wrote on 2015/08/31 09:32 +0300: > >On Mon, Aug 31, 2015 at 09:44:49AM +0800, Qu Wenruo wrote: > >From the perspective of users, qgroup's referenced or exclusive > is negative,but user can not continue to write data! a workaround > way is to cast u64 to s64 when doing qgroup reservation > >>> > >>>I am unable to reproduce this problem without his modification. > >>>I could be wrong in reverting this, so I'm gonna CC Wang as well so > >>>he is aware of this patch. > >> > >>The cast is a workaround for a quite old qgroup bug, which will > >>cause excl/rfer overflow to minus. > >> > >>The remove of cast rfer/exel now is OK, as qgroup keeps maturing, > >>especially after 4.2-rc1 rfer/exel will keep sane under most case > >>(exception will be qgroup reassign and subvolume deletion, but will > >>not case minus value even under than case). > > > >rfer/exel and reserved are all of type unsigned int, how exactly would > >they overflow to minus? > > Due to qgroup bugs of course, > In old implement, btrfs_find_all_roots() will not always find the > correct roots. > > Causing quota to minus more bytes on existing qgroups. > > For example qg->rfer is 16K, btrfs_find_all_roots() think the qg > previously own a 32K extent but not now, and qgroup accounting > decides to decrease qg->rfer by 32K, now you get -16K, which is a > super huge number if used as u64. > > > > >> > >>But I'm not a fan to remove it now. > >>As qgroup still has a known huge bug for the qg->reserved part, we > >>are aware of it and working on it actively. > > > >Can you tell me more about this known huge bug and how you can > >reproduce it using the present implementation? > > > > Check the fstest patch I submitted: > https://patchwork.kernel.org/patch/7023301/ > > Btrfs qgroup has qgroup reserved space leak problem, and under some > case, it can also overflow to minus.(I don't have a minus > reproducer, but it already happened several times in my test > environment) > > That's what we are fixing now, trying to make it public before 4.3-rc1. > > Thanks, > Qu Thank you for the detailed explanation Qu, I will read more on your changes and perhaps learn a thing or two. All the respect, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] btrfs: Remove unneeded cast to s64 for qgroup rfer state
Alexandru Moise wrote on 2015/08/31 09:32 +0300: On Mon, Aug 31, 2015 at 09:44:49AM +0800, Qu Wenruo wrote: >From the perspective of users, qgroup's referenced or exclusive is negative,but user can not continue to write data! a workaround way is to cast u64 to s64 when doing qgroup reservation I am unable to reproduce this problem without his modification. I could be wrong in reverting this, so I'm gonna CC Wang as well so he is aware of this patch. The cast is a workaround for a quite old qgroup bug, which will cause excl/rfer overflow to minus. The remove of cast rfer/exel now is OK, as qgroup keeps maturing, especially after 4.2-rc1 rfer/exel will keep sane under most case (exception will be qgroup reassign and subvolume deletion, but will not case minus value even under than case). rfer/exel and reserved are all of type unsigned int, how exactly would they overflow to minus? Due to qgroup bugs of course, In old implement, btrfs_find_all_roots() will not always find the correct roots. Causing quota to minus more bytes on existing qgroups. For example qg->rfer is 16K, btrfs_find_all_roots() think the qg previously own a 32K extent but not now, and qgroup accounting decides to decrease qg->rfer by 32K, now you get -16K, which is a super huge number if used as u64. But I'm not a fan to remove it now. As qgroup still has a known huge bug for the qg->reserved part, we are aware of it and working on it actively. Can you tell me more about this known huge bug and how you can reproduce it using the present implementation? Check the fstest patch I submitted: https://patchwork.kernel.org/patch/7023301/ Btrfs qgroup has qgroup reserved space leak problem, and under some case, it can also overflow to minus.(I don't have a minus reproducer, but it already happened several times in my test environment) That's what we are fixing now, trying to make it public before 4.3-rc1. Thanks, Qu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] btrfs: Remove unneeded cast to s64 for qgroup rfer state
On Mon, Aug 31, 2015 at 09:44:49AM +0800, Qu Wenruo wrote: > >>>From the perspective of users, qgroup's referenced or exclusive > >>is negative,but user can not continue to write data! a workaround > >>way is to cast u64 to s64 when doing qgroup reservation > > > >I am unable to reproduce this problem without his modification. > >I could be wrong in reverting this, so I'm gonna CC Wang as well so > >he is aware of this patch. > > The cast is a workaround for a quite old qgroup bug, which will > cause excl/rfer overflow to minus. > > The remove of cast rfer/exel now is OK, as qgroup keeps maturing, > especially after 4.2-rc1 rfer/exel will keep sane under most case > (exception will be qgroup reassign and subvolume deletion, but will > not case minus value even under than case). rfer/exel and reserved are all of type unsigned int, how exactly would they overflow to minus? > > But I'm not a fan to remove it now. > As qgroup still has a known huge bug for the qg->reserved part, we > are aware of it and working on it actively. Can you tell me more about this known huge bug and how you can reproduce it using the present implementation? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] btrfs: Remove unneeded cast to s64 for qgroup rfer state
On Mon, Aug 31, 2015 at 09:44:49AM +0800, Qu Wenruo wrote: > >>>From the perspective of users, qgroup's referenced or exclusive > >>is negative,but user can not continue to write data! a workaround > >>way is to cast u64 to s64 when doing qgroup reservation > > > >I am unable to reproduce this problem without his modification. > >I could be wrong in reverting this, so I'm gonna CC Wang as well so > >he is aware of this patch. > > The cast is a workaround for a quite old qgroup bug, which will > cause excl/rfer overflow to minus. > > The remove of cast rfer/exel now is OK, as qgroup keeps maturing, > especially after 4.2-rc1 rfer/exel will keep sane under most case > (exception will be qgroup reassign and subvolume deletion, but will > not case minus value even under than case). rfer/exel and reserved are all of type unsigned int, how exactly would they overflow to minus? > > But I'm not a fan to remove it now. > As qgroup still has a known huge bug for the qg->reserved part, we > are aware of it and working on it actively. Can you tell me more about this known huge bug and how you can reproduce it using the present implementation? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] btrfs: Remove unneeded cast to s64 for qgroup rfer state
Alexandru Moise wrote on 2015/08/31 09:32 +0300: On Mon, Aug 31, 2015 at 09:44:49AM +0800, Qu Wenruo wrote: >From the perspective of users, qgroup's referenced or exclusive is negative,but user can not continue to write data! a workaround way is to cast u64 to s64 when doing qgroup reservation I am unable to reproduce this problem without his modification. I could be wrong in reverting this, so I'm gonna CC Wang as well so he is aware of this patch. The cast is a workaround for a quite old qgroup bug, which will cause excl/rfer overflow to minus. The remove of cast rfer/exel now is OK, as qgroup keeps maturing, especially after 4.2-rc1 rfer/exel will keep sane under most case (exception will be qgroup reassign and subvolume deletion, but will not case minus value even under than case). rfer/exel and reserved are all of type unsigned int, how exactly would they overflow to minus? Due to qgroup bugs of course, In old implement, btrfs_find_all_roots() will not always find the correct roots. Causing quota to minus more bytes on existing qgroups. For example qg->rfer is 16K, btrfs_find_all_roots() think the qg previously own a 32K extent but not now, and qgroup accounting decides to decrease qg->rfer by 32K, now you get -16K, which is a super huge number if used as u64. But I'm not a fan to remove it now. As qgroup still has a known huge bug for the qg->reserved part, we are aware of it and working on it actively. Can you tell me more about this known huge bug and how you can reproduce it using the present implementation? Check the fstest patch I submitted: https://patchwork.kernel.org/patch/7023301/ Btrfs qgroup has qgroup reserved space leak problem, and under some case, it can also overflow to minus.(I don't have a minus reproducer, but it already happened several times in my test environment) That's what we are fixing now, trying to make it public before 4.3-rc1. Thanks, Qu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] btrfs: Remove unneeded cast to s64 for qgroup rfer state
On Mon, Aug 31, 2015 at 02:51:08PM +0800, Qu Wenruo wrote: > > > Alexandru Moise wrote on 2015/08/31 09:32 +0300: > >On Mon, Aug 31, 2015 at 09:44:49AM +0800, Qu Wenruo wrote: > >From the perspective of users, qgroup's referenced or exclusive > is negative,but user can not continue to write data! a workaround > way is to cast u64 to s64 when doing qgroup reservation > >>> > >>>I am unable to reproduce this problem without his modification. > >>>I could be wrong in reverting this, so I'm gonna CC Wang as well so > >>>he is aware of this patch. > >> > >>The cast is a workaround for a quite old qgroup bug, which will > >>cause excl/rfer overflow to minus. > >> > >>The remove of cast rfer/exel now is OK, as qgroup keeps maturing, > >>especially after 4.2-rc1 rfer/exel will keep sane under most case > >>(exception will be qgroup reassign and subvolume deletion, but will > >>not case minus value even under than case). > > > >rfer/exel and reserved are all of type unsigned int, how exactly would > >they overflow to minus? > > Due to qgroup bugs of course, > In old implement, btrfs_find_all_roots() will not always find the > correct roots. > > Causing quota to minus more bytes on existing qgroups. > > For example qg->rfer is 16K, btrfs_find_all_roots() think the qg > previously own a 32K extent but not now, and qgroup accounting > decides to decrease qg->rfer by 32K, now you get -16K, which is a > super huge number if used as u64. > > > > >> > >>But I'm not a fan to remove it now. > >>As qgroup still has a known huge bug for the qg->reserved part, we > >>are aware of it and working on it actively. > > > >Can you tell me more about this known huge bug and how you can > >reproduce it using the present implementation? > > > > Check the fstest patch I submitted: > https://patchwork.kernel.org/patch/7023301/ > > Btrfs qgroup has qgroup reserved space leak problem, and under some > case, it can also overflow to minus.(I don't have a minus > reproducer, but it already happened several times in my test > environment) > > That's what we are fixing now, trying to make it public before 4.3-rc1. > > Thanks, > Qu Thank you for the detailed explanation Qu, I will read more on your changes and perhaps learn a thing or two. All the respect, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] btrfs: Remove unneeded cast to s64 for qgroup rfer state
Alexandru Moise wrote on 2015/08/29 11:45 +:
This patch reverts commit: b4fcd6be6bbd702ae1a6545c9b413681850a9814
Wang Shilong added those casts as a workaround for a bug reproduced
using the following steps:
Steps to reproduce:
mkfs.btrfs
mount
dd if=/dev/zero of=//data bs=1M count=10
sync
btrfs quota enable
btrfs qgroup create 0/5
btrfs qgroup limit 5M 0/5
rm -f //data
sync
btrfs qgroup show
dd if=/dev/zero of=data bs=1M count=1
>From the perspective of users, qgroup's referenced or exclusive
is negative,but user can not continue to write data! a workaround
way is to cast u64 to s64 when doing qgroup reservation
I am unable to reproduce this problem without his modification.
I could be wrong in reverting this, so I'm gonna CC Wang as well so
he is aware of this patch.
The cast is a workaround for a quite old qgroup bug, which will cause
excl/rfer overflow to minus.
The remove of cast rfer/exel now is OK, as qgroup keeps maturing,
especially after 4.2-rc1 rfer/exel will keep sane under most case
(exception will be qgroup reassign and subvolume deletion, but will not
case minus value even under than case).
But I'm not a fan to remove it now.
As qgroup still has a known huge bug for the qg->reserved part, we are
aware of it and working on it actively.
So for such cleanup, I'd prefer to do it when we rework the accounting
part of qgroup.
Thanks,
Qu
Signed-off-by: Alexandru Moise <00moses.alexande...@gmail.com>
---
fs/btrfs/qgroup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 8a82029..9c75e86 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2077,14 +2077,14 @@ int btrfs_qgroup_reserve(struct btrfs_root *root, u64
num_bytes)
qg = u64_to_ptr(unode->aux);
if ((qg->lim_flags & BTRFS_QGROUP_LIMIT_MAX_RFER) &&
- qg->reserved + (s64)qg->rfer + num_bytes >
+ qg->reserved + qg->rfer + num_bytes >
qg->max_rfer) {
ret = -EDQUOT;
goto out;
}
if ((qg->lim_flags & BTRFS_QGROUP_LIMIT_MAX_EXCL) &&
- qg->reserved + (s64)qg->excl + num_bytes >
+ qg->reserved + qg->excl + num_bytes >
qg->max_excl) {
ret = -EDQUOT;
goto out;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] btrfs: Remove unneeded cast to s64 for qgroup rfer state
Alexandru Moise wrote on 2015/08/29 11:45 +:
This patch reverts commit: b4fcd6be6bbd702ae1a6545c9b413681850a9814
Wang Shilong added those casts as a workaround for a bug reproduced
using the following steps:
Steps to reproduce:
mkfs.btrfs disk
mount disk mnt
dd if=/dev/zero of=/mnt/data bs=1M count=10
sync
btrfs quota enable mnt
btrfs qgroup create 0/5 mnt
btrfs qgroup limit 5M 0/5 mnt
rm -f /mnt/data
sync
btrfs qgroup show mnt
dd if=/dev/zero of=data bs=1M count=1
From the perspective of users, qgroup's referenced or exclusive
is negative,but user can not continue to write data! a workaround
way is to cast u64 to s64 when doing qgroup reservation
I am unable to reproduce this problem without his modification.
I could be wrong in reverting this, so I'm gonna CC Wang as well so
he is aware of this patch.
The cast is a workaround for a quite old qgroup bug, which will cause
excl/rfer overflow to minus.
The remove of cast rfer/exel now is OK, as qgroup keeps maturing,
especially after 4.2-rc1 rfer/exel will keep sane under most case
(exception will be qgroup reassign and subvolume deletion, but will not
case minus value even under than case).
But I'm not a fan to remove it now.
As qgroup still has a known huge bug for the qg-reserved part, we are
aware of it and working on it actively.
So for such cleanup, I'd prefer to do it when we rework the accounting
part of qgroup.
Thanks,
Qu
Signed-off-by: Alexandru Moise 00moses.alexande...@gmail.com
---
fs/btrfs/qgroup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 8a82029..9c75e86 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2077,14 +2077,14 @@ int btrfs_qgroup_reserve(struct btrfs_root *root, u64
num_bytes)
qg = u64_to_ptr(unode-aux);
if ((qg-lim_flags BTRFS_QGROUP_LIMIT_MAX_RFER)
- qg-reserved + (s64)qg-rfer + num_bytes
+ qg-reserved + qg-rfer + num_bytes
qg-max_rfer) {
ret = -EDQUOT;
goto out;
}
if ((qg-lim_flags BTRFS_QGROUP_LIMIT_MAX_EXCL)
- qg-reserved + (s64)qg-excl + num_bytes
+ qg-reserved + qg-excl + num_bytes
qg-max_excl) {
ret = -EDQUOT;
goto out;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] btrfs: Remove unneeded cast to s64 for qgroup rfer state
This patch reverts commit: b4fcd6be6bbd702ae1a6545c9b413681850a9814
Wang Shilong added those casts as a workaround for a bug reproduced
using the following steps:
Steps to reproduce:
> mkfs.btrfs
> mount
> dd if=/dev/zero of=//data bs=1M count=10
> sync
> btrfs quota enable
> btrfs qgroup create 0/5
> btrfs qgroup limit 5M 0/5
> rm -f //data
> sync
> btrfs qgroup show
> dd if=/dev/zero of=data bs=1M count=1
>
>>From the perspective of users, qgroup's referenced or exclusive
>is negative,but user can not continue to write data! a workaround
>way is to cast u64 to s64 when doing qgroup reservation
I am unable to reproduce this problem without his modification.
I could be wrong in reverting this, so I'm gonna CC Wang as well so
he is aware of this patch.
Signed-off-by: Alexandru Moise <00moses.alexande...@gmail.com>
---
fs/btrfs/qgroup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 8a82029..9c75e86 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2077,14 +2077,14 @@ int btrfs_qgroup_reserve(struct btrfs_root *root, u64
num_bytes)
qg = u64_to_ptr(unode->aux);
if ((qg->lim_flags & BTRFS_QGROUP_LIMIT_MAX_RFER) &&
- qg->reserved + (s64)qg->rfer + num_bytes >
+ qg->reserved + qg->rfer + num_bytes >
qg->max_rfer) {
ret = -EDQUOT;
goto out;
}
if ((qg->lim_flags & BTRFS_QGROUP_LIMIT_MAX_EXCL) &&
- qg->reserved + (s64)qg->excl + num_bytes >
+ qg->reserved + qg->excl + num_bytes >
qg->max_excl) {
ret = -EDQUOT;
goto out;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] btrfs: Remove unneeded cast to s64 for qgroup rfer state
This patch reverts commit: b4fcd6be6bbd702ae1a6545c9b413681850a9814
Wang Shilong added those casts as a workaround for a bug reproduced
using the following steps:
Steps to reproduce:
mkfs.btrfs disk
mount disk mnt
dd if=/dev/zero of=/mnt/data bs=1M count=10
sync
btrfs quota enable mnt
btrfs qgroup create 0/5 mnt
btrfs qgroup limit 5M 0/5 mnt
rm -f /mnt/data
sync
btrfs qgroup show mnt
dd if=/dev/zero of=data bs=1M count=1
From the perspective of users, qgroup's referenced or exclusive
is negative,but user can not continue to write data! a workaround
way is to cast u64 to s64 when doing qgroup reservation
I am unable to reproduce this problem without his modification.
I could be wrong in reverting this, so I'm gonna CC Wang as well so
he is aware of this patch.
Signed-off-by: Alexandru Moise 00moses.alexande...@gmail.com
---
fs/btrfs/qgroup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 8a82029..9c75e86 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2077,14 +2077,14 @@ int btrfs_qgroup_reserve(struct btrfs_root *root, u64
num_bytes)
qg = u64_to_ptr(unode-aux);
if ((qg-lim_flags BTRFS_QGROUP_LIMIT_MAX_RFER)
- qg-reserved + (s64)qg-rfer + num_bytes
+ qg-reserved + qg-rfer + num_bytes
qg-max_rfer) {
ret = -EDQUOT;
goto out;
}
if ((qg-lim_flags BTRFS_QGROUP_LIMIT_MAX_EXCL)
- qg-reserved + (s64)qg-excl + num_bytes
+ qg-reserved + qg-excl + num_bytes
qg-max_excl) {
ret = -EDQUOT;
goto out;
--
2.5.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
