| Message ID | 20120923114924.GA1975@zambezi.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;