Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions
Sorry to reply late. On Mon, 24 Sep 2012 18:47:42 +0200, David Sterba wrote: This is the most straightforward transformation I can think of. It doesn't result in an unnecessary BUG_ON, keeps churn to a minimum and agree with you. doesn't change the style of the balance ioctl. (If I were to check every filter argument that way, btrfs_balance_ioctl() would be very long and complicated.) I think the check in btrfs_balance_ioctl() is necessary, the reason is above. btrfs_balance_ioctl does not seem as the right place, it does the processing related to the state of balance (resume/cancel etc). Look at btrfs_balance() itself, it does lot more sanity checks of the parameters I think we should not put the check in btrfs_balance(), because the arguments are valid forever if they pass the check when they are input, if we put the check in btrfs_balance(), the check will be done every time we resume the balance. it is unnecessary. We can put the extra checks into helpers (and not only this one) if clarity and readability of the function becomes a concern. Agree. I will put this check into a helper in the next version of this patch. And I will make a separate patch to move the current check in btrfs_balance from btrfs_balance to the above helper after this patch is received. Thanks Miao -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions
On mon, 24 Sep 2012 21:42:42 +0300, Ilya Dryomov wrote: On Mon, Sep 24, 2012 at 06:47:42PM +0200, David Sterba wrote: On Mon, Sep 24, 2012 at 10:05:02AM +0800, Miao Xie wrote: Generally, we should check the value when it is input. If not, we might run our program with the wrong value, and it is possible to cause unknown problems. So I think the above chunk is necessary. The difference is that giving an invalid value will exit early (your version), while Ilya's version will clamp the 'usage' to the allowed range during processing. From usability POV, I'd prefer to let the command fail early with a verbose error eg. that the range is invalid. I think that's the job for progs, and that's where this and a few other checks are currently implemented. There is no possibility for unknown problems, that's exactly how it's been implemented before the cleanup. The purpose of balance filters (and the usage filter in particular) is to lower the number of chunks we have to relocate. If someone decides to use raw ioctls, and supplies us with the invalid value, we just cut it down to a 100 and proceed. usage=100 is the equivalent of not using the usage filter at all, so the raw ioctl user will get what he deserves. This is very similar to what we currently do with other filters. For example, soft can only be used with convert, and this is checked by progs. But, if somebody were to set a soft flag without setting convert we would simply ignore that soft. Same goes for drange and devid, invalid ranges, invalid devids, etc. Pulling all these checks into the kernel is questionable at best, and, if enough people insist on it, should be discussed separately. I think the usage is a special case that doesn't cause critical problem and are not used everywhere. But if there is a variant which is referenced at several places and the kernel would crash if it is invalid, in this case, we would add the check everywhere and make the code more complex and ugly if we don't check it when it is passed in. Besides that if we have done lots of work before the check, we must roll back when we find the variant is invalid, it wastes time. So I think doing the check and returning the error number as early as possible is a rule we should follow. Thanks Miao -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions
On Sun, Sep 23, 2012 at 05:54:18PM +0800, Miao Xie wrote: @@ -2347,7 +2335,8 @@ static int chunk_usage_filter(struct btrfs_fs_info *fs_info, u64 chunk_offset, cache = btrfs_lookup_block_group(fs_info, chunk_offset); chunk_used = btrfs_block_group_used(cache-item); - user_thresh = div_factor_fine(cache-key.offset, bargs-usage); + BUG_ON(bargs-usage 0 || bargs-usage 100); otherwise it reliably crashes here Sorry, I don't know why it will crash here if we input 0. I tried to input 0, and it worked well. My oversight, sorry. I think the only case we must take into account is the users might input the wrong value (100 or 0) on the old kernel, and it can be stored into the filesystem. If we mount this filesystem on the new kernel, some problems may happen. So better avoid a BUG_ON. -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions
On Mon, Sep 24, 2012 at 10:05:02AM +0800, Miao Xie wrote: Generally, we should check the value when it is input. If not, we might run our program with the wrong value, and it is possible to cause unknown problems. So I think the above chunk is necessary. The difference is that giving an invalid value will exit early (your version), while Ilya's version will clamp the 'usage' to the allowed range during processing. From usability POV, I'd prefer to let the command fail early with a verbose error eg. that the range is invalid. (diff is on top of the patch in question) This is the most straightforward transformation I can think of. It doesn't result in an unnecessary BUG_ON, keeps churn to a minimum and agree with you. doesn't change the style of the balance ioctl. (If I were to check every filter argument that way, btrfs_balance_ioctl() would be very long and complicated.) I think the check in btrfs_balance_ioctl() is necessary, the reason is above. btrfs_balance_ioctl does not seem as the right place, it does the processing related to the state of balance (resume/cancel etc). Look at btrfs_balance() itself, it does lot more sanity checks of the parameters. We can put the extra checks into helpers (and not only this one) if clarity and readability of the function becomes a concern. david -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions
On Mon, Sep 24, 2012 at 06:47:42PM +0200, David Sterba wrote: On Mon, Sep 24, 2012 at 10:05:02AM +0800, Miao Xie wrote: Generally, we should check the value when it is input. If not, we might run our program with the wrong value, and it is possible to cause unknown problems. So I think the above chunk is necessary. The difference is that giving an invalid value will exit early (your version), while Ilya's version will clamp the 'usage' to the allowed range during processing. From usability POV, I'd prefer to let the command fail early with a verbose error eg. that the range is invalid. I think that's the job for progs, and that's where this and a few other checks are currently implemented. There is no possibility for unknown problems, that's exactly how it's been implemented before the cleanup. The purpose of balance filters (and the usage filter in particular) is to lower the number of chunks we have to relocate. If someone decides to use raw ioctls, and supplies us with the invalid value, we just cut it down to a 100 and proceed. usage=100 is the equivalent of not using the usage filter at all, so the raw ioctl user will get what he deserves. This is very similar to what we currently do with other filters. For example, soft can only be used with convert, and this is checked by progs. But, if somebody were to set a soft flag without setting convert we would simply ignore that soft. Same goes for drange and devid, invalid ranges, invalid devids, etc. Pulling all these checks into the kernel is questionable at best, and, if enough people insist on it, should be discussed separately. Thanks, Ilya -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions
On Fri, 21 Sep 2012 17:24:44 +0200, David Sterba wrote:
On Fri, Sep 21, 2012 at 05:07:46PM +0800, Miao Xie wrote:
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3335,6 +3335,24 @@ static long btrfs_ioctl_balance(struct file *file,
void __user *arg)
goto do_balance;
}
+
+if ((bargs-data.flags BTRFS_BALANCE_ARGS_USAGE)
+(bargs-data.usage 0 || bargs-data.usage 100)) {
the 0 checks belong here
+ret = -EINVAL;
+goto out_bargs;
+}
+
+if ((bargs-meta.flags BTRFS_BALANCE_ARGS_USAGE)
+(bargs-meta.usage 0 || bargs-meta.usage 100)) {
+ret = -EINVAL;
+goto out_bargs;
+}
+
+if ((bargs-sys.flags BTRFS_BALANCE_ARGS_USAGE)
+(bargs-sys.usage 0 || bargs-sys.usage 100)) {
+ret = -EINVAL;
+goto out_bargs;
+}
} else {
bargs = NULL;
}
@@ -2347,7 +2335,8 @@ static int chunk_usage_filter(struct btrfs_fs_info
*fs_info, u64 chunk_offset,
cache = btrfs_lookup_block_group(fs_info, chunk_offset);
chunk_used = btrfs_block_group_used(cache-item);
-user_thresh = div_factor_fine(cache-key.offset, bargs-usage);
+BUG_ON(bargs-usage 0 || bargs-usage 100);
otherwise it reliably crashes here
Sorry, I don't know why it will crash here if we input 0. I tried to input 0,
and it worked well.
I think the only case we must take into account is the users might input the
wrong value (100 or 0)
on the old kernel, and it can be stored into the filesystem. If we mount this
filesystem
on the new kernel, some problems may happen.
Thanks
Miao
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions
On Fri, Sep 21, 2012 at 05:07:46PM +0800, Miao Xie wrote:
div_factor{_fine} has been implemented for two times, and these two functions
are very similar, so cleanup the reduplicate implement and drop the original
div_factor(), and then rename div_factor_fine() to div_factor(). So the
divisor
of the new div_factor() is 100, not 10.
And I move div_factor into a independent file named math.h because it is a
common math function, may be used by every composition of btrfs.
Because these functions are mostly used on the hot path, and we are sure
the parameters are right in the most cases, we don't add complex checks
for the parameters. But in the other place, we must check and make sure
the parameters are right. So besides the code cleanup, this patch also
add a check for the usage of the space balance, it is the only place that
we need add check to make sure the parameters of div_factor are right till
now.
Signed-off-by: Miao Xie mi...@cn.fujitsu.com
---
Changelog v2 - v3:
- drop the original div_factor and rename div_factor_fine to div_factor
- drop the check of the factor
Changelog v1 - v2:
- add missing check
---
fs/btrfs/extent-tree.c | 29 ++---
fs/btrfs/ioctl.c | 18 ++
fs/btrfs/math.h| 33 +
fs/btrfs/relocation.c |2 +-
fs/btrfs/transaction.c |2 +-
fs/btrfs/volumes.c | 30 +-
6 files changed, 64 insertions(+), 50 deletions(-)
create mode 100644 fs/btrfs/math.h
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a010234..bcb9ced 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -33,6 +33,7 @@
#include volumes.h
#include locking.h
#include free-space-cache.h
+#include math.h
#undef SCRAMBLE_DELAYED_REFS
@@ -648,24 +649,6 @@ void btrfs_clear_space_info_full(struct btrfs_fs_info
*info)
rcu_read_unlock();
}
-static u64 div_factor(u64 num, int factor)
-{
- if (factor == 10)
- return num;
- num *= factor;
- do_div(num, 10);
- return num;
-}
-
-static u64 div_factor_fine(u64 num, int factor)
-{
- if (factor == 100)
- return num;
- num *= factor;
- do_div(num, 100);
- return num;
-}
-
u64 btrfs_find_block_group(struct btrfs_root *root,
u64 search_start, u64 search_hint, int owner)
{
@@ -674,7 +657,7 @@ u64 btrfs_find_block_group(struct btrfs_root *root,
u64 last = max(search_hint, search_start);
u64 group_start = 0;
int full_search = 0;
- int factor = 9;
+ int factor = 90;
int wrapped = 0;
again:
while (1) {
@@ -708,7 +691,7 @@ again:
if (!full_search factor 10) {
last = search_start;
full_search = 1;
- factor = 10;
+ factor = 100;
goto again;
}
found:
@@ -3513,7 +3496,7 @@ static int should_alloc_chunk(struct btrfs_root *root,
if (force == CHUNK_ALLOC_LIMITED) {
thresh = btrfs_super_total_bytes(root-fs_info-super_copy);
thresh = max_t(u64, 64 * 1024 * 1024,
-div_factor_fine(thresh, 1));
+div_factor(thresh, 1));
if (num_bytes - num_allocated thresh)
return 1;
@@ -3521,12 +3504,12 @@ static int should_alloc_chunk(struct btrfs_root *root,
thresh = btrfs_super_total_bytes(root-fs_info-super_copy);
/* 256MB or 2% of the FS */
- thresh = max_t(u64, 256 * 1024 * 1024, div_factor_fine(thresh, 2));
+ thresh = max_t(u64, 256 * 1024 * 1024, div_factor(thresh, 2));
/* system chunks need a much small threshold */
if (sinfo-flags BTRFS_BLOCK_GROUP_SYSTEM)
thresh = 32 * 1024 * 1024;
- if (num_bytes thresh sinfo-bytes_used div_factor(num_bytes, 8))
+ if (num_bytes thresh sinfo-bytes_used div_factor(num_bytes, 80))
return 0;
return 1;
}
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 9384a2a..d8d53f7 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3335,6 +3335,24 @@ static long btrfs_ioctl_balance(struct file *file,
void __user *arg)
goto do_balance;
}
+
+ if ((bargs-data.flags BTRFS_BALANCE_ARGS_USAGE)
+ (bargs-data.usage 0 || bargs-data.usage 100)) {
+ ret = -EINVAL;
+ goto out_bargs;
+ }
+
+ if ((bargs-meta.flags BTRFS_BALANCE_ARGS_USAGE)
+ (bargs-meta.usage 0 || bargs-meta.usage 100)) {
+ ret = -EINVAL;
+ goto out_bargs;
+ }
+
+ if ((bargs-sys.flags BTRFS_BALANCE_ARGS_USAGE)
+ (bargs-sys.usage 0 || bargs-sys.usage 100)) {
+
Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions
On Sun, 23 Sep 2012 14:49:24 +0300, Ilya Dryomov wrote:
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 9384a2a..d8d53f7 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3335,6 +3335,24 @@ static long btrfs_ioctl_balance(struct file *file,
void __user *arg)
goto do_balance;
}
+
+if ((bargs-data.flags BTRFS_BALANCE_ARGS_USAGE)
+(bargs-data.usage 0 || bargs-data.usage 100)) {
+ret = -EINVAL;
+goto out_bargs;
+}
+
+if ((bargs-meta.flags BTRFS_BALANCE_ARGS_USAGE)
+(bargs-meta.usage 0 || bargs-meta.usage 100)) {
+ret = -EINVAL;
+goto out_bargs;
+}
+
+if ((bargs-sys.flags BTRFS_BALANCE_ARGS_USAGE)
+(bargs-sys.usage 0 || bargs-sys.usage 100)) {
+ret = -EINVAL;
+goto out_bargs;
+}
} else {
bargs = NULL;
}
Why not drop this hunk ...
Generally, we should check the value when it is input. If not, we might
run our program with the wrong value, and it is possible to cause unknown
problems. So I think the above chunk is necessary.
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6019fb2..ff86f91 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2334,8 +2334,13 @@ static int chunk_usage_filter(struct btrfs_fs_info
*fs_info, u64 chunk_offset,
cache = btrfs_lookup_block_group(fs_info, chunk_offset);
chunk_used = btrfs_block_group_used(cache-item);
- BUG_ON(bargs-usage 0 || bargs-usage 100);
- user_thresh = div_factor(cache-key.offset, bargs-usage);
+ if (bargs-usage == 0)
+ user_thresh = 0;
+ else if (bargs-usage = 100)
+ user_thresh = cache-key.offset;
+ else
+ user_thresh = div_factor(cache-key.offset, bargs-usage);
+
if (chunk_used user_thresh)
ret = 0;
(diff is on top of the patch in question)
This is the most straightforward transformation I can think of. It
doesn't result in an unnecessary BUG_ON, keeps churn to a minimum and
agree with you.
doesn't change the style of the balance ioctl. (If I were to check
every filter argument that way, btrfs_balance_ioctl() would be very long
and complicated.)
I think the check in btrfs_balance_ioctl() is necessary, the reason is above.
Thanks
Miao
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/2] Btrfs: cleanup duplicated division functions
On Fri, Sep 21, 2012 at 05:07:46PM +0800, Miao Xie wrote:
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3335,6 +3335,24 @@ static long btrfs_ioctl_balance(struct file *file,
void __user *arg)
goto do_balance;
}
+
+ if ((bargs-data.flags BTRFS_BALANCE_ARGS_USAGE)
+ (bargs-data.usage 0 || bargs-data.usage 100)) {
the 0 checks belong here
+ ret = -EINVAL;
+ goto out_bargs;
+ }
+
+ if ((bargs-meta.flags BTRFS_BALANCE_ARGS_USAGE)
+ (bargs-meta.usage 0 || bargs-meta.usage 100)) {
+ ret = -EINVAL;
+ goto out_bargs;
+ }
+
+ if ((bargs-sys.flags BTRFS_BALANCE_ARGS_USAGE)
+ (bargs-sys.usage 0 || bargs-sys.usage 100)) {
+ ret = -EINVAL;
+ goto out_bargs;
+ }
} else {
bargs = NULL;
}
@@ -2347,7 +2335,8 @@ static int chunk_usage_filter(struct btrfs_fs_info
*fs_info, u64 chunk_offset,
cache = btrfs_lookup_block_group(fs_info, chunk_offset);
chunk_used = btrfs_block_group_used(cache-item);
- user_thresh = div_factor_fine(cache-key.offset, bargs-usage);
+ BUG_ON(bargs-usage 0 || bargs-usage 100);
otherwise it reliably crashes here
+ user_thresh = div_factor(cache-key.offset, bargs-usage);
if (chunk_used user_thresh)
ret = 0;
other than that, patch looks good tome,
thanks,
david
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
